Re: [PATCH v4 2/6] hwmon: pwm-fan: Simplify enable/disable check

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

On Mon, May 23, 2022 at 01:05:09PM +0200, Alexander Stein wrote:
> Instead of comparing the current to the new pwm duty to decide whether to
> enable the PWM, use a dedicated flag. Also apply the new PWM duty in any
> case. This is a preparation to enable/disable the regulator dynamically.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/hwmon/pwm-fan.c | 37 +++++++++++++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 831878daffe6..96b10d422828 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -29,10 +29,13 @@ struct pwm_fan_tach {
>  };
>  
>  struct pwm_fan_ctx {
> +	struct device *dev;
> +
>  	struct mutex lock;
>  	struct pwm_device *pwm;
>  	struct pwm_state pwm_state;
>  	struct regulator *reg_en;
> +	bool enabled;
>  
>  	int tach_count;
>  	struct pwm_fan_tach *tachs;
> @@ -85,14 +88,21 @@ static void sample_timer(struct timer_list *t)
>  static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
>  {
>  	struct pwm_state *state = &ctx->pwm_state;
> -	unsigned long period;
>  	int ret;
>  
> -	period = state->period;
> -	state->duty_cycle = DIV_ROUND_UP(ctx->pwm_value * (period - 1), MAX_PWM);
> +	if (ctx->enabled)
> +		return 0;
> +
>  	state->enabled = true;
>  	ret = pwm_apply_state(ctx->pwm, state);
> +	if (ret) {
> +		dev_err(ctx->dev, "failed to enable PWM\n");
> +		goto err;

There is no reason for this goto. Just return directly.

> +	}
>  
> +	ctx->enabled = true;
> +
> +err:
>  	return ret;
>  }
>  
> @@ -100,26 +110,36 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
>  {
>  	struct pwm_state *state = &ctx->pwm_state;
>  
> +	if (!ctx->enabled)
> +		return 0;
> +

ctx->enabled will initially be false. How is it known that pwm is
disabled when the driver is loaded ? At the very least that warrants
an explanation.

>  	state->enabled = false;
>  	state->duty_cycle = 0;
>  	pwm_apply_state(ctx->pwm, state);

This code is a bit inconsistent with pwm_fan_power_on(). Why check for
error there, but not here ?

>  
> +	ctx->enabled = false;
> +
>  	return 0;
>  }
>  
>  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  {
> +	struct pwm_state *state = &ctx->pwm_state;
> +	unsigned long period;
>  	int ret = 0;
>  
>  	mutex_lock(&ctx->lock);
> -	if (ctx->pwm_value == pwm)
> -		goto exit_set_pwm_err;
>  
> -	if (pwm > 0)
> +	if (pwm > 0) {
> +		period = state->period;
> +		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +		ret = pwm_apply_state(ctx->pwm, state);
> +		if (ret)
> +			goto exit_set_pwm_err;
>  		ret = pwm_fan_power_on(ctx);
> -	else
> +	} else {
>  		ret = pwm_fan_power_off(ctx);
> -
> +	}
>  	if (!ret)
>  		ctx->pwm_value = pwm;
>  
> @@ -326,6 +346,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  
>  	mutex_init(&ctx->lock);
>  
> +	ctx->dev = &pdev->dev;
>  	ctx->pwm = devm_of_pwm_get(dev, dev->of_node, NULL);
>  	if (IS_ERR(ctx->pwm))
>  		return dev_err_probe(dev, PTR_ERR(ctx->pwm), "Could not get PWM\n");