Il giorno lun 24 ago 2026 alle ore 10:11 Sascha Hauer <[email protected]> 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 | >
