Re: [RFC PATCH v2] regulator: pwm-regulator: Make assumptions about disabled PWM consistent

[Uwe Kleine-König]

Hello Martin,

On Sun, Jun 23, 2024 at 11:09:08PM +0200, Martin Blumenstingl wrote:
> On Sun, Jun 23, 2024 at 11:32 AM Uwe Kleine-König
> <u.kleine-koenig@xxxxxxxxxxxx> wrote:
> > On Sat, Jun 22, 2024 at 09:15:04PM +0200, Martin Blumenstingl wrote:
> > >       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)

In general you cannot know how a disabled PWM behaves. The objective of
.enabled = false is to save power and don't care about output. The
typical implementations yield the inactive level, but there are
exceptions that are hard to fix -- if at all. These include High-Z and
the active level. So if the pwm-regulator relies on the PWM emitting the
inactive level, .enabled = false is wrong. I guess in general you don't
know and .enabled = true + .duty_cycle = 0 is the right thing?

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

I'm unsure. I can tell the effect of .enabled = false, but I don't know
if this effect is ok for the pwm-regulator. I also don't know if
disabling and reenabling the regulator is expected to keep the voltage.
Who can tell? I'd hope Mark?

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature