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@xxxxxxxxxxxxxxx>
> > ---
> >
> > 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)
