Re: Re: [PATCH v4 3/6] hwmon: pwm-fan: Add dedicated power switch function

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

Hello Guenter,

thanks for your review.

Am Dienstag, 30. August 2022, 15:50:13 CEST schrieb Guenter Roeck:
> On Mon, May 23, 2022 at 01:05:10PM +0200, Alexander Stein wrote:
> > This handles enabling/disabling the regulator in a single function, while
> > keeping the enables/disabled balanced. This is a preparation when
> > regulator is switched from different code paths.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  drivers/hwmon/pwm-fan.c | 52 +++++++++++++++++++++++++++++------------
> >  1 file changed, 37 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> > index 96b10d422828..04af24268963 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -35,6 +35,7 @@ struct pwm_fan_ctx {
> > 
> >  	struct pwm_device *pwm;
> >  	struct pwm_state pwm_state;
> >  	struct regulator *reg_en;
> > 
> > +	bool regulator_enabled;
> > 
> >  	bool enabled;
> >  	
> >  	int tach_count;
> > 
> > @@ -85,6 +86,29 @@ static void sample_timer(struct timer_list *t)
> > 
> >  	mod_timer(&ctx->rpm_timer, jiffies + HZ);
> >  
> >  }
> > 
> > +static int pwm_fan_switch_power(struct pwm_fan_ctx *ctx, bool on)
> > +{
> > +	int ret = 0;
> > +
> > +	if (!ctx->reg_en)
> > +		return ret;
> > +
> > +	if (ctx->regulator_enabled && on) {
> > +		ret = 0;
> 
> ret is already 0 here.
> 
> > +	} else if (!ctx->regulator_enabled && on) {
> > +		ret = regulator_enable(ctx->reg_en);
> > +		if (ret == 0)
> > +			ctx->regulator_enabled = true;
> > +	} else if (ctx->regulator_enabled && !on) {
> > +		ret = regulator_disable(ctx->reg_en);
> > +		if (ret == 0)
> > +			ctx->regulator_enabled = false;
> > +	} else if (!ctx->regulator_enabled && !on) {
> > +		ret = 0;
> 
> ret is already 0 here.

Ok, I'll remove both branches, setting ret to 0 again. I just wanted to keep 
the branches for all possibilities, but no heard feelings here.

> > +	}
> > +	return ret;
> > +}
> > +
> > 
> >  static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
> >  {
> >  
> >  	struct pwm_state *state = &ctx->pwm_state;
> > 
> > @@ -316,7 +340,9 @@ static int pwm_fan_of_get_cooling_data(struct device
> > *dev,> 
> >  static void pwm_fan_regulator_disable(void *data)
> >  {
> > 
> > -	regulator_disable(data);
> > +	struct pwm_fan_ctx *ctx = data;
> > +
> > +	pwm_fan_switch_power(ctx, false);
> 
> You can directly pass 'data' as argument here; there is no need
> for the extra variable.

Nice, thanks. Will do so.

Best regards,
Alexander

> >  }
> >  
> >  static void pwm_fan_pwm_disable(void *__ctx)
> > 
> > @@ -360,13 +386,13 @@ static int pwm_fan_probe(struct platform_device
> > *pdev)> 
> >  		ctx->reg_en = NULL;
> >  	
> >  	} else {
> > 
> > -		ret = regulator_enable(ctx->reg_en);
> > +		ret = pwm_fan_switch_power(ctx, true);
> > 
> >  		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);
> > +					       ctx);
> > 
> >  		if (ret)
> >  		
> >  			return ret;
> >  	
> >  	}
> > 
> > @@ -512,12 +538,10 @@ static int pwm_fan_disable(struct device *dev)
> > 
> >  			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;
> > -		}
> > +	ret = pwm_fan_switch_power(ctx, false);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to disable fan supply: %d\n", ret);
> > +		return ret;
> > 
> >  	}
> >  	
> >  	return 0;
> > 
> > @@ -539,12 +563,10 @@ static int pwm_fan_resume(struct device *dev)
> > 
> >  	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
> >  	int ret;
> > 
> > -	if (ctx->reg_en) {
> > -		ret = regulator_enable(ctx->reg_en);
> > -		if (ret) {
> > -			dev_err(dev, "Failed to enable fan supply: 
%d\n", ret);
> > -			return ret;
> > -		}
> > +	ret = pwm_fan_switch_power(ctx, true);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable fan supply: %d\n", ret);
> > +		return ret;
> > 
> >  	}
> >  	
> >  	if (ctx->pwm_value == 0)