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

Guenter Roeck
2021-09-16T20:18:02Z

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
>