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

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hello Guenter,

thanks for your feedback.

Am Dienstag, 30. August 2022, 15:43:38 CEST schrieb Guenter Roeck:
> 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.

Sure, will do so.

> > +	}
> > 
> > +	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.

I'm not sure what you are concerned about. The PWM is enabled in probe 
unconditionally, calling __set_pwm(ctx, MAX_PWM).

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

You are right, make sense to check in both functions.

Thanks and best regards
Alexander

> > +	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");