Re: [PATCH hwmon-next v2 3/3] hwmon: (mlxreg-fan) Extend driver to support multiply cooling devices

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Sep 16, 2021 at 10:47:19PM +0300, Vadim Pasternak wrote:
> Add support for additional cooling devices in order to support the
> systems, which can be equipped with up-to four PWM controllers.
> 
> Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
> ---
> v0->v2:
>  Comments pointed out by Guenter:
>  - Drop call to thermal_cooling_device_unregister() in error flow,
>    devices registered by devm_thermal_of_cooling_device_register()
>    should be cleaned automatically.
> ---
>  drivers/hwmon/mlxreg-fan.c | 76 +++++++++++++++++++++++++-------------
>  1 file changed, 50 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c
> index 1a146cc4b0fd..566bee333c3c 100644
> --- a/drivers/hwmon/mlxreg-fan.c
> +++ b/drivers/hwmon/mlxreg-fan.c
> @@ -63,6 +63,8 @@
>  					 MLXREG_FAN_MAX_DUTY,		\
>  					 MLXREG_FAN_MAX_STATE))
>  
> +struct mlxreg_fan;
> +
>  /*
>   * struct mlxreg_fan_tacho - tachometer data (internal use):
>   *
> @@ -81,12 +83,18 @@ struct mlxreg_fan_tacho {
>  /*
>   * struct mlxreg_fan_pwm - PWM data (internal use):
>   *
> + * @fan: private data;
>   * @connected: indicates if PWM is connected;
>   * @reg: register offset;
> + * @cooling: cooling device levels;
> + * @cdev: cooling device;
>   */
>  struct mlxreg_fan_pwm {
> +	struct mlxreg_fan *fan;
>  	bool connected;
>  	u32 reg;
> +	u8 cooling_levels[MLXREG_FAN_MAX_STATE + 1];
> +	struct thermal_cooling_device *cdev;
>  };
>  
>  /*
> @@ -99,8 +107,6 @@ struct mlxreg_fan_pwm {
>   * @tachos_per_drwr - number of tachometers per drawer;
>   * @samples: minimum allowed samples per pulse;
>   * @divider: divider value for tachometer RPM calculation;
> - * @cooling: cooling device levels;
> - * @cdev: cooling device;
>   */
>  struct mlxreg_fan {
>  	struct device *dev;
> @@ -111,8 +117,6 @@ struct mlxreg_fan {
>  	int tachos_per_drwr;
>  	int samples;
>  	int divider;
> -	u8 cooling_levels[MLXREG_FAN_MAX_STATE + 1];
> -	struct thermal_cooling_device *cdev;
>  };
>  
>  static int
> @@ -305,11 +309,12 @@ static int mlxreg_fan_get_cur_state(struct thermal_cooling_device *cdev,
>  				    unsigned long *state)
>  
>  {
> -	struct mlxreg_fan *fan = cdev->devdata;
> +	struct mlxreg_fan_pwm *pwm = cdev->devdata;
> +	struct mlxreg_fan *fan = pwm->fan;
>  	u32 regval;
>  	int err;
>  
> -	err = regmap_read(fan->regmap, fan->pwm[0].reg, &regval);
> +	err = regmap_read(fan->regmap, pwm->reg, &regval);
>  	if (err) {
>  		dev_err(fan->dev, "Failed to query PWM duty\n");
>  		return err;
> @@ -324,7 +329,8 @@ static int mlxreg_fan_set_cur_state(struct thermal_cooling_device *cdev,
>  				    unsigned long state)
>  
>  {
> -	struct mlxreg_fan *fan = cdev->devdata;
> +	struct mlxreg_fan_pwm *pwm = cdev->devdata;
> +	struct mlxreg_fan *fan = pwm->fan;
>  	unsigned long cur_state;
>  	int i, config = 0;
>  	u32 regval;
> @@ -348,11 +354,11 @@ static int mlxreg_fan_set_cur_state(struct thermal_cooling_device *cdev,
>  		config = 1;
>  		state -= MLXREG_FAN_MAX_STATE;
>  		for (i = 0; i < state; i++)
> -			fan->cooling_levels[i] = state;
> +			pwm->cooling_levels[i] = state;
>  		for (i = state; i <= MLXREG_FAN_MAX_STATE; i++)
> -			fan->cooling_levels[i] = i;
> +			pwm->cooling_levels[i] = i;
>  
> -		err = regmap_read(fan->regmap, fan->pwm[0].reg, &regval);
> +		err = regmap_read(fan->regmap, pwm->reg, &regval);
>  		if (err) {
>  			dev_err(fan->dev, "Failed to query PWM duty\n");
>  			return err;
> @@ -369,8 +375,8 @@ static int mlxreg_fan_set_cur_state(struct thermal_cooling_device *cdev,
>  		return -EINVAL;
>  
>  	/* Normalize the state to the valid speed range. */
> -	state = fan->cooling_levels[state];
> -	err = regmap_write(fan->regmap, fan->pwm[0].reg,
> +	state = pwm->cooling_levels[state];
> +	err = regmap_write(fan->regmap, pwm->reg,
>  			   MLXREG_FAN_PWM_STATE2DUTY(state));
>  	if (err) {
>  		dev_err(fan->dev, "Failed to write PWM duty\n");
> @@ -541,13 +547,37 @@ static int mlxreg_fan_config(struct mlxreg_fan *fan,
>  		fan->tachos_per_drwr = tacho_avail / drwr_avail;
>  	}
>  
> -	/* Init cooling levels per PWM state. */
> -	for (i = 0; i < MLXREG_FAN_SPEED_MIN_LEVEL; i++)
> -		fan->cooling_levels[i] = MLXREG_FAN_SPEED_MIN_LEVEL;
> -	for (i = MLXREG_FAN_SPEED_MIN_LEVEL; i <= MLXREG_FAN_MAX_STATE; i++)
> -		fan->cooling_levels[i] = i;
> +	return 0;
> +}
> +
> +static int mlxreg_fan_cooling_config(struct device *dev, struct mlxreg_fan *fan)
> +{
> +	int i, j, err;
> +
> +	for (i = 0; i <= MLXREG_FAN_MAX_PWM; i++) {
> +		struct mlxreg_fan_pwm *pwm = &fan->pwm[i];
> +
> +		if (!pwm->connected)
> +			continue;
> +		pwm->fan = fan;
> +		pwm->cdev = devm_thermal_of_cooling_device_register(dev, NULL, "mlxreg_fan", pwm,
> +								    &mlxreg_fan_cooling_ops);
> +		if (IS_ERR(pwm->cdev)) {
> +			dev_err(dev, "Failed to register cooling device\n");
> +			err = PTR_ERR(pwm->cdev);
> +			goto devm_thermal_of_cooling_device_register_fail;

Unnecessary goto.

			return PTR_ERR(pwm->cdev);

> +		}
> +
> +		/* Init cooling levels per PWM state. */
> +		for (j = 0; j < MLXREG_FAN_SPEED_MIN_LEVEL; j++)
> +			pwm->cooling_levels[j] = MLXREG_FAN_SPEED_MIN_LEVEL;
> +		for (j = MLXREG_FAN_SPEED_MIN_LEVEL; j <= MLXREG_FAN_MAX_STATE; j++)
> +			pwm->cooling_levels[j] = j;
> +	}
>  
>  	return 0;
> +devm_thermal_of_cooling_device_register_fail:
> +	return err;
>  }
>  
>  static int mlxreg_fan_probe(struct platform_device *pdev)
> @@ -584,16 +614,10 @@ static int mlxreg_fan_probe(struct platform_device *pdev)
>  		return PTR_ERR(hwm);
>  	}
>  
> -	if (IS_REACHABLE(CONFIG_THERMAL)) {
> -		fan->cdev = devm_thermal_of_cooling_device_register(dev,
> -			NULL, "mlxreg_fan", fan, &mlxreg_fan_cooling_ops);
> -		if (IS_ERR(fan->cdev)) {
> -			dev_err(dev, "Failed to register cooling device\n");
> -			return PTR_ERR(fan->cdev);
> -		}
> -	}
> +	if (IS_REACHABLE(CONFIG_THERMAL))
> +		err = mlxreg_fan_cooling_config(dev, fan);
>  
> -	return 0;
> +	return err;
>  }
>  
>  static struct platform_driver mlxreg_fan_driver = {
> -- 
> 2.20.1
>