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

[Luca Lauro <famlauro93l@gmail.com>]

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.

>
> --
> 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 |
>