Re: [PATCH v2 1/1] hwmon: pwm-fan: dynamically switch regulator

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

On Wed, May 04, 2022 at 02:45:51PM +0200, Alexander Stein wrote:
> From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> 
> A pwm value equal to zero is meant to switch off the pwm
> hence also switching off the fan. Currently the optional
> regulator is always on. When using this driver on boards
> with an inverted pwm signal polarity this can cause running
> the fan at maximum speed when setting pwm to zero.
> 

The appropriate solution in this case would be to tell the
software that the pwm is inverted. Turning off the regulator
in that situation is a bad idea since setting the pwm value to
1 would set it to almost full speed. That does not really make
sense.

Guenter

> The proposed changes switch the regulator off, when PWM is
> currently enabled but pwm is requested to set to zero
> and switch der regulator on, when PWM is currently disabled
> but pwm shall be set to a no zero value.
> 
> Add __set_pwm_and_regulator and rewrite __set_pwm to
> handle regulator switching for the following conditions:
> 
> - probe: pwm duty -> max, pwm state is unkwown: use __set_pwm
>   and enable regulator separately to keep the devm action
> - off: new pwm duty is zero, pwm currently enabled: disable
>   regulator
> - on: new pwm duty is non zero, pwm currently disabled: enable
>   regulator
> 
> Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in v2:
> * Added my own missing S-o-b
> 
>  drivers/hwmon/pwm-fan.c | 144 ++++++++++++++++++++++++++--------------
>  1 file changed, 93 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index f12b9a28a232..b47d59fbe836 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -97,18 +97,50 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  	unsigned long period;
>  	int ret = 0;
>  	struct pwm_state *state = &ctx->pwm_state;
> +	/* track changes of reg_en for error handling */
> +	enum regulator_change {
> +		untouched,
> +		enabled,
> +		disabled,
> +	} reg_change = untouched;
>  
>  	mutex_lock(&ctx->lock);
> +
>  	if (ctx->pwm_value == pwm)
>  		goto exit_set_pwm_err;
>  
> +	if (ctx->reg_en) {
> +		if (pwm && !state->enabled) {
> +			reg_change = enabled;
> +			ret = regulator_enable(ctx->reg_en);
> +		} else if (!pwm && state->enabled) {
> +			reg_change = disabled;
> +			ret = regulator_disable(ctx->reg_en);
> +		}
> +		if (ret)
> +			goto exit_set_pwm_err;
> +	}
> +
>  	period = state->period;
>  	state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
>  	state->enabled = pwm ? true : false;
>  
>  	ret = pwm_apply_state(ctx->pwm, state);
> -	if (!ret)
> +	if (!ret) {
>  		ctx->pwm_value = pwm;
> +	} else if (reg_change != untouched) {
> +		/*
> +		 * revert regulator changes to keep consistency between
> +		 * pwm and regulator
> +		 */
> +		int err;
> +
> +		if (reg_change == enabled)
> +			err = regulator_disable(ctx->reg_en);
> +		else if (reg_change == disabled)
> +			err = regulator_enable(ctx->reg_en);
> +	}
> +
>  exit_set_pwm_err:
>  	mutex_unlock(&ctx->lock);
>  	return ret;
> @@ -280,18 +312,50 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
>  	return 0;
>  }
>  
> -static void pwm_fan_regulator_disable(void *data)
> +/*
> + * disable fan and regulator
> + * if cleanup is true, disable pwm regardless of regulator disable result
> + * this makes the function dual use for unloading driver and suspend
> + */
> +
> +static int __pwm_fan_disable_or_cleanup(struct pwm_fan_ctx *ctx, bool cleanup)
>  {
> -	regulator_disable(data);
> +	int ret;
> +
> +	if (ctx->pwm_value) {
> +		/* keep ctx->pwm_state unmodified for pwm_fan_resume() */
> +		struct pwm_state state = ctx->pwm_state;
> +
> +		/* regulator is only enabled if pwm_value is not zero */
> +		if (ctx->pwm_value && ctx->reg_en) {
> +			ret = regulator_disable(ctx->reg_en);
> +			if (ret) {
> +				pr_err("Failed to disable fan supply: %d\n", ret);
> +				if (!cleanup)
> +					return ret;
> +			}
> +		}
> +
> +		state.duty_cycle = 0;
> +		state.enabled = false;
> +		ret = pwm_apply_state(ctx->pwm, &state);
> +	}
> +
> +	return ret;
>  }
>  
> -static void pwm_fan_pwm_disable(void *__ctx)
> +static void pwm_fan_cleanup(void *__ctx)
>  {
>  	struct pwm_fan_ctx *ctx = __ctx;
> -
> -	ctx->pwm_state.enabled = false;
> -	pwm_apply_state(ctx->pwm, &ctx->pwm_state);
>  	del_timer_sync(&ctx->rpm_timer);
> +	__pwm_fan_disable_or_cleanup(ctx, true);
> +}
> +
> +static int pwm_fan_disable(void *__ctx)
> +{
> +	struct pwm_fan_ctx *ctx = __ctx;
> +
> +	return __pwm_fan_disable_or_cleanup(ctx, false);
>  }
>  
>  static int pwm_fan_probe(struct platform_device *pdev)
> @@ -324,19 +388,14 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  			return PTR_ERR(ctx->reg_en);
>  
>  		ctx->reg_en = NULL;
> -	} else {
> -		ret = regulator_enable(ctx->reg_en);
> -		if (ret) {
> -			dev_err(dev, "Failed to enable fan supply: %d\n", ret);
> -			return ret;
> -		}
> -		ret = devm_add_action_or_reset(dev, pwm_fan_regulator_disable,
> -					       ctx->reg_en);
> -		if (ret)
> -			return ret;
>  	}
>  
>  	pwm_init_state(ctx->pwm, &ctx->pwm_state);
> +	/*
> +	 * Ensure the PWM is switched on (including the regulator),
> +	 * independently from any previous PWM state
> +	 */
> +	ctx->pwm_state.enabled = false;
>  
>  	/*
>  	 * __set_pwm assumes that MAX_PWM * (period - 1) fits into an unsigned
> @@ -348,14 +407,17 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	/* Set duty cycle to maximum allowed and enable PWM output */
> +	/*
> +	 * Set duty cycle to maximum allowed and enable PWM output as well as
> +	 * the regulator. In case of error nothing is changed
> +	 */
>  	ret = __set_pwm(ctx, MAX_PWM);
>  	if (ret) {
>  		dev_err(dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;
>  	}
>  	timer_setup(&ctx->rpm_timer, sample_timer, 0);
> -	ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx);
> +	ret = devm_add_action_or_reset(dev, pwm_fan_cleanup, ctx);
>  	if (ret)
>  		return ret;
>  
> @@ -461,42 +523,22 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int pwm_fan_disable(struct device *dev)
> -{
> -	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> -	int ret;
> -
> -	if (ctx->pwm_value) {
> -		/* keep ctx->pwm_state unmodified for pwm_fan_resume() */
> -		struct pwm_state state = ctx->pwm_state;
> -
> -		state.duty_cycle = 0;
> -		state.enabled = false;
> -		ret = pwm_apply_state(ctx->pwm, &state);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
> -	if (ctx->reg_en) {
> -		ret = regulator_disable(ctx->reg_en);
> -		if (ret) {
> -			dev_err(dev, "Failed to disable fan supply: %d\n", ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static void pwm_fan_shutdown(struct platform_device *pdev)
>  {
> -	pwm_fan_disable(&pdev->dev);
> +	struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev);
> +
> +	pwm_fan_cleanup(ctx);
>  }
>  
>  #ifdef CONFIG_PM_SLEEP
>  static int pwm_fan_suspend(struct device *dev)
>  {
> -	return pwm_fan_disable(dev);
> +	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = pwm_fan_disable(ctx);
> +
> +	return ret;
>  }
>  
>  static int pwm_fan_resume(struct device *dev)
> @@ -504,6 +546,9 @@ static int pwm_fan_resume(struct device *dev)
>  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
>  	int ret;
>  
> +	if (ctx->pwm_value == 0)
> +		return 0;
> +
>  	if (ctx->reg_en) {
>  		ret = regulator_enable(ctx->reg_en);
>  		if (ret) {
> @@ -512,9 +557,6 @@ static int pwm_fan_resume(struct device *dev)
>  		}
>  	}
>  
> -	if (ctx->pwm_value == 0)
> -		return 0;
> -
>  	return pwm_apply_state(ctx->pwm, &ctx->pwm_state);
>  }
>  #endif
> -- 
> 2.25.1
>