Re: [PATCH v4 05/14] drivers: fan: add fan subsystem, core API and G76x fan controller driver

["Sascha Hauer" <s.hauer@pengutronix.de>]

On 2026-08-31 13:53, Luca Lauro wrote:
> Il giorno lun 24 ago 2026 alle ore 10:11 Sascha Hauer
> <s.hauer@pengutronix.de> ha scritto:
> >
> > Hi Luca,
> >
> > On 2026-08-13 17:26, Luca Lauro via B4 Relay wrote:
> > > +
> > > +struct fan_ops {
> > > +     int (*get_fan_startv)(struct device *dev, char *buf);
> > > +     int (*set_fan_startv)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_gear_multiplier)(struct device *dev, char *buf);
> > > +     int (*set_gear_multiplier)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_fan_ppr)(struct device *dev, char *buf);
> > > +     int (*set_fan_ppr)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_pwm_polarity)(struct device *dev, char *buf);
> > > +     int (*set_pwm_polarity)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_clk_freq)(struct device *dev, char *buf);
> > > +     int (*set_clk_freq)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_clk_div)(struct device *dev, char *buf);
> > > +     int (*set_clk_div)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_control_mode)(struct device *dev, char *buf);
> > > +     int (*set_control_mode)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_output_mode)(struct device *dev, char *buf);
> > > +     int (*set_output_mode)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_ooc_detection)(struct device *dev, char *buf);
> > > +     int (*set_ooc_detection)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_failure_detection)(struct device *dev, char *buf);
> > > +     int (*set_failure_detection)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_failure_state)(struct device *dev, char *buf);
> > > +     int (*get_ooc_state)(struct device *dev, char *buf);
> > > +
> > > +     int (*get_fan_speed)(struct device *dev, char *buf);
> > > +     int (*set_fan_speed)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_fan_rpm)(struct device *dev, char *buf);
> > > +     int (*set_fan_rpm)(struct device *dev, unsigned long val);
> > > +
> > > +     int (*get_fan_level)(struct device *dev, char *buf);
> > > +     int (*set_fan_level)(struct device *dev, unsigned long val);
> >
> > Converting the integer value to a string shouldn't be delegated to the
> > drivers. When the fan level can be expressed as unsigned long, then
> > get_fan_level() should take a unsigned long * as argument as well.
> >
> > Also the user facing interface you could use device parameters which
> > makes the fan command almost go away.
> >
> > Reworking the parameters above along the lines:
> >
> >         dev_add_param_uint32(&fan->dev, "rpm", fan_rpm_set, fan_rpm_get, &fan->rpm, "%u", fan);
> >
> > Will give you scriptable access to the parameters without an additional
> > command.
> 
> Thanks for the feedback.
> 
> Just to give some context:: the fan subsystem (fan.c / fan.h) is meant to
> be the hardware abstraction layer for the various fan controller
> hardware-specific
> drivers adapted from the Linux kernel.
> The “fan” command was only intended as a thin user-facing frontend on top
> of that HAL, similar in spirit to how barebox exposes unified interfaces
> for GPIO, LEDs, etc.
> 
> I agree that device parameters provide a more compact interface in barebox,
> and they already cover most of the use cases that the command was meant to
> address.
> 
> Before I rework the series, I would like to understand your preference for
> the user-facing layer: should the fan subsystem rely entirely on device
> parameters, or do you see value in keeping a small generic command on top
> of the HAL? I can follow either direction; just let me know which one fits
> better into barebox.

A command might be useful for more complex operations that can't be
expressed by device parameters, but for the bulk of the operations which
1:1 map onto a variable I would prefer device parameters over a command.

Note that using device parameters also allows you to provide defaults
via the environment.

But generally I question the ABI you have chosen. My first search didn't
reveal it because there is no g76x in the Kernel, but then I found the g762
driver which has a fairly complete device tree binding:

        g762@3e {
                compatible = "gmt,g762";
                reg = <0x3e>;
                clocks = <&g762_clk>;
                fan_gear_mode = <0>;
                fan_startv = <1>;
                pwm_polarity = <0>;
        };

The above things should just be configurable via device tree and not
even adjustable from the command line, as there's no point in doing so.

Also this fan subsytems looks as if it is written specifically for the
chip you are trying to handle, not as a generic subsystem.

I think you should have a closer look at how Linux does it, this might
give you some more guidance how this thing could be implemented.

Another thing: Please separate the subsystem patch from the driver(s),
it makes it easier to review the border between both.

I think you should send the fan stuff as an extra series, otherwise it
becomes the blocker for this series which as a whole already looks ok.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |