Message-ID: <CAFBinCANKBcttSEhc_0-+D0G3fO0CV67R41y-C7xEwhAXtA+LA@mail.gmail.com>
Date: Sun, 23 Jun 2024 23:09:08 +0200
From: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	broonie@...nel.org, lgirdwood@...il.com
Subject: Re: [RFC PATCH v2] regulator: pwm-regulator: Make assumptions about
 disabled PWM consistent

Hello Uwe,

On Sun, Jun 23, 2024 at 11:32 AM Uwe Kleine-König
<u.kleine-koenig@...libre.com> wrote:
>
> Hello Martin,
>
> On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> > Generally it's not known how a disabled PWM behaves. However if the
> > bootloader hands over a disabled PWM that drives a regulator and it's
> > claimed the regulator is enabled, then the most likely assumption is
> > that the PWM emits the inactive level. This is represented by duty_cycle
> > = 0 even for .polarity == PWM_POLARITY_INVERSED.
>
> I'd write: "This is represented by duty_cycle = 0 independent of the
> polarity."
That makes things easier - I'll apply this, thank you!

> > Change the implementation to always use duty_cycle = 0, regardless of
> > the polarity. Also update the code so it keeps a copy of the pwm_state
> > around. Both of these changes result in the fact that the logic from
> > pwm_regulator_init_boot_on() is much less complex and can be simplified
> > to a point where the whole function is not needed anymore.
>
> In my (German) ear the following sounds a bit nicer:
>
>         Both of these changes reduce the complexity of
>         pwm_regulator_init_boot_on() to a point where the whole function
>         is not needed anymore.
Sounds fine to my (German) ear as well - thanks!

> > Suggested-by: Uwe Kleine-König <u.kleine-koenig@...libre.com>
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
> > ---
> > Changes from v1 [0] (which was sent by Uwe):
> > - keep the struct pwm_state around to prevent a regression on Meson8b
> >   Odroid-C1 boards where just calling pwm_enable() without adjusting
> >   the duty_cycle to 0 would hang the board
> > - I'm actively looking for input on the commit description and
> >   suggestions whether/why Fixes tags should be applied
>
> Apart of the nitpicking above, I like the commit description.
>
> Regarding a Fixes tag: I'm unsure if without this patch, the
> pwm-regulator driver is broken for your Odroid-C1 board. It's not,
> right?
> I think I wouldn't add a Fixes tag and just consider this patch a
> cleanup then.
My Odroid-C1 works fine with and without this patch.
Only the patch that you found previously which you wanted to improve
is required (and reverting it breaks boot).

[...]
> > -     ret = pwm_apply_might_sleep(drvdata->pwm, &pstate);
> > -     if (ret) {
> > -             dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
> > +     ret = pwm_regulator_apply_state(rdev, &pstate);
> > +     if (ret)
> >               return ret;
> > -     }
>
> If you drop the local variable pstate and just do
>
>         pwm_set_relative_duty_cycle(drvdata->pwm_state,
>                                 drvdata->duty_cycle_table[selector].dutycycle, 100);
>
> you might get a mismatch between actual configuration and
> drvdata->pwm_state if pwm_regulator_apply_state() fails, but I think
> that doesn't matter and simplifies the code a bit. (Drop the assignment
> in pwm_regulator_apply_state() then.)
If you're fine with this potential mismatch (I am - I just was unsure
about potential side-effects) then you're right: this is an
improvement!

> >       drvdata->state = selector;
> >
> > @@ -115,17 +129,26 @@ static int pwm_regulator_list_voltage(struct regulator_dev *rdev,
> >  static int pwm_regulator_enable(struct regulator_dev *dev)
> >  {
> >       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > +     struct pwm_state pstate = drvdata->pwm_state;
> >
> >       gpiod_set_value_cansleep(drvdata->enb_gpio, 1);
> >
> > -     return pwm_enable(drvdata->pwm);
> > +     pstate.enabled = true;
> > +
> > +     return pwm_regulator_apply_state(dev, &pstate);
> >  }
> >
> >  static int pwm_regulator_disable(struct regulator_dev *dev)
> >  {
> >       struct pwm_regulator_data *drvdata = rdev_get_drvdata(dev);
> > +     struct pwm_state pstate = drvdata->pwm_state;
> > +     int ret;
> > +
> > +     pstate.enabled = false;
> >
> > -     pwm_disable(drvdata->pwm);
> > +     ret = pwm_regulator_apply_state(dev, &pstate);
> > +     if (ret)
> > +             return ret;
>
> With that part I'm a bit unhappy. You don't know what the pwm does when
> disabled (it might yield a 100% relative duty cycle). So the safe bet is
> to use .enabled=true, .duty_cycle=0 here.
>
> The only code that "knows" if it's safe to disable the PWM is the
> lowlevel pwm driver.
Here I don't know the regulator framework enough. Let's make two assumptions:
1. the optimization you suggest is implemented (I'm not against it,
it's just different from what pwm_disable() does)
2. regulator core does not expect the set voltage to change in a
.disable() callback

In that case disabling the PWM output is fine. Since we're now
updating the cached pwm_state with duty_cycle = 0 the next time the
regulator core calls the .enable() callback (without calling
.set_voltage() between disabling and enabling) we end up enabling the
PWM output with duty_cycle = 0 (and thus likely changing the voltage
output).
I see three options here:
- my assumption about the regulator core is incorrect, then your
optimization works just fine
- we only write enabled = false to the cached pwm_state but not duty_cycle = 0
- we drop the suggested optimization here (and maybe let PWM core handle this)

What do you think?


Best regards,
Martin

Powered by blists - more mailing lists