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

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

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.

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