Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Sven Van Asbroeck]

On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> Implements .get_state to read-out the current hardware state.
>

I am not convinced that we actually need this.

Looking at the pwm core, .get_state() is only called right after .request(),
to initialize the cached value of the state. The core then uses the cached
value throughout, it'll never read out the h/w again, until the next .request().

In our case, we know that the state right after request is always disabled,
because:
- we disable all pwm channels on probe (in PATCH v5 4/7)
- .free() disables the pwm channel

Conclusion: .get_state() will always return "pwm disabled", so why do we
bother reading out the h/w?

Of course, if we choose to leave the pwm enabled after .free(), then
.get_state() can even be left out! Do we want that? Genuine question, I do
not know the answer.

> The hardware readout may return slightly different values than those
> that were set in apply due to the limited range of possible prescale and
> counter register values.
>
> Also note that although the datasheet mentions 200 Hz as default
> frequency when using the internal 25 MHz oscillator, the calculated
> period from the default prescaler register setting of 30 is 5079040ns.
>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 1b5b5fb93b43..b3398963c0ff 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -331,6 +331,46 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>         return 0;
>  }
>
> +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +                                 struct pwm_state *state)
> +{
> +       struct pca9685 *pca = to_pca(chip);
> +       unsigned long long duty;
> +       unsigned int val;
> +
> +       /* Calculate (chip-wide) period from prescale value */
> +       regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> +       state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> +                       (val + 1);
> +
> +       /* The (per-channel) polarity is fixed */
> +       state->polarity = PWM_POLARITY_NORMAL;
> +
> +       if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> +               /*
> +                * The "all LEDs" channel does not support HW readout
> +                * Return 0 and disabled for backwards compatibility
> +                */
> +               state->duty_cycle = 0;
> +               state->enabled = false;
> +               return;
> +       }
> +
> +       duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> +
> +       state->enabled = !!duty;
> +       if (!state->enabled) {
> +               state->duty_cycle = 0;
> +               return;
> +       } else if (duty == PCA9685_COUNTER_RANGE) {
> +               state->duty_cycle = state->period;
> +               return;
> +       }
> +
> +       duty *= state->period;
> +       state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> +}
> +
>  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>         struct pca9685 *pca = to_pca(chip);
> @@ -353,6 +393,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>
>  static const struct pwm_ops pca9685_pwm_ops = {
>         .apply = pca9685_pwm_apply,
> +       .get_state = pca9685_pwm_get_state,
>         .request = pca9685_pwm_request,
>         .free = pca9685_pwm_free,
>         .owner = THIS_MODULE,
> --
> 2.29.2
>

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote:
> On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > Implements .get_state to read-out the current hardware state.
> >
> 
> I am not convinced that we actually need this.
> 
> Looking at the pwm core, .get_state() is only called right after .request(),
> to initialize the cached value of the state. The core then uses the cached
> value throughout, it'll never read out the h/w again, until the next .request().
> 
> In our case, we know that the state right after request is always disabled,
> because:
> - we disable all pwm channels on probe (in PATCH v5 4/7)
> - .free() disables the pwm channel
> 
> Conclusion: .get_state() will always return "pwm disabled", so why do we
> bother reading out the h/w?

If there are no plans for the PWM core to call .get_state more often in
the future, we could just read out the period and return 0 duty and
disabled.

Thierry, Uwe, what's your take on this?

> Of course, if we choose to leave the pwm enabled after .free(), then
> .get_state() can even be left out! Do we want that? Genuine question, I do
> not know the answer.

I do not think we should leave it enabled after free. It is less
complicated if we know that unrequested channels are not in use.

> 
> > The hardware readout may return slightly different values than those
> > that were set in apply due to the limited range of possible prescale and
> > counter register values.
> >
> > Also note that although the datasheet mentions 200 Hz as default
> > frequency when using the internal 25 MHz oscillator, the calculated
> > period from the default prescaler register setting of 30 is 5079040ns.
> >
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 1b5b5fb93b43..b3398963c0ff 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -331,6 +331,46 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >         return 0;
> >  }
> >
> > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                                 struct pwm_state *state)
> > +{
> > +       struct pca9685 *pca = to_pca(chip);
> > +       unsigned long long duty;
> > +       unsigned int val;
> > +
> > +       /* Calculate (chip-wide) period from prescale value */
> > +       regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > +       state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > +                       (val + 1);
> > +
> > +       /* The (per-channel) polarity is fixed */
> > +       state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +       if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> > +               /*
> > +                * The "all LEDs" channel does not support HW readout
> > +                * Return 0 and disabled for backwards compatibility
> > +                */
> > +               state->duty_cycle = 0;
> > +               state->enabled = false;
> > +               return;
> > +       }
> > +
> > +       duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > +
> > +       state->enabled = !!duty;
> > +       if (!state->enabled) {
> > +               state->duty_cycle = 0;
> > +               return;
> > +       } else if (duty == PCA9685_COUNTER_RANGE) {
> > +               state->duty_cycle = state->period;
> > +               return;
> > +       }
> > +
> > +       duty *= state->period;
> > +       state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> > +}
> > +
> >  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >         struct pca9685 *pca = to_pca(chip);
> > @@ -353,6 +393,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >
> >  static const struct pwm_ops pca9685_pwm_ops = {
> >         .apply = pca9685_pwm_apply,
> > +       .get_state = pca9685_pwm_get_state,
> >         .request = pca9685_pwm_request,
> >         .free = pca9685_pwm_free,
> >         .owner = THIS_MODULE,
> > --
> > 2.29.2
> >

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Sven Van Asbroeck]

On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
> >
> > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > bother reading out the h/w?
>
> If there are no plans for the PWM core to call .get_state more often in
> the future, we could just read out the period and return 0 duty and
> disabled.

I'm not sure why we should even read out the period?
When a channel is disabled, the period is not externally visible,
therefore it's meaningless ?

As far as I can tell, we can use this for .get_state():
memset(&pwm->state, 0, sizeof(pwm_state));

>
> Thierry, Uwe, what's your take on this?
>
> > Of course, if we choose to leave the pwm enabled after .free(), then
> > .get_state() can even be left out! Do we want that? Genuine question, I do
> > not know the answer.
>
> I do not think we should leave it enabled after free. It is less
> complicated if we know that unrequested channels are not in use.
>

Good point, I agree with you.

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi everyone,

happy new year, hope you are all well!

On Thu, Dec 17, 2020 at 12:52:42PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> > >
> > > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > > bother reading out the h/w?
> >
> > If there are no plans for the PWM core to call .get_state more often in
> > the future, we could just read out the period and return 0 duty and
> > disabled.
> 
> I'm not sure why we should even read out the period?
> When a channel is disabled, the period is not externally visible,
> therefore it's meaningless ?
> 
> As far as I can tell, we can use this for .get_state():
> memset(&pwm->state, 0, sizeof(pwm_state));
> 
> >
> > Thierry, Uwe, what's your take on this?

I will continue working on this series in the upcoming weeks.
Feedback on the .get_state issue would be greatly appreciated.

To summarize:
Is it OK for a driver to expect the chip->ops->get_state function to be
only called from the place in pwm core it is currently called from?
(Namely in pwm_device_request after chip->ops->request)

If yes, we could always return a 0 duty cycle and disabled state,
because this is the state we left it in after .probe (and .free).

However, if in the future, the pwm core adds additional calls to
chip->ops->get_state in other places, this could lead to problems.

--

Another point is the period: Sven suggested we do not read out the
period at all, as the PWM is disabled anyway (see above).
Is this acceptable?

And, if we never return anything but 0 in .get_state, should it be
implemented at all?

> >
> > > Of course, if we choose to leave the pwm enabled after .free(), then
> > > .get_state() can even be left out! Do we want that? Genuine question, I do
> > > not know the answer.
> >
> > I do not think we should leave it enabled after free. It is less
> > complicated if we know that unrequested channels are not in use.
> >
> 
> Good point, I agree with you.

Thanks and best regards,
Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Sven Van Asbroeck]

On Sun, Jan 3, 2021 at 12:04 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> I will continue working on this series in the upcoming weeks.
> Feedback on the .get_state issue would be greatly appreciated.

In absence of specifications, I tend to keep things as simple as possible.

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2092 bytes --]

Hello,

On Thu, Dec 17, 2020 at 06:43:04PM +0100, Clemens Gruber wrote:
> On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote:
> > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > >
> > > Implements .get_state to read-out the current hardware state.
> > >
> > 
> > I am not convinced that we actually need this.
> > 
> > Looking at the pwm core, .get_state() is only called right after .request(),
> > to initialize the cached value of the state. The core then uses the cached
> > value throughout, it'll never read out the h/w again, until the next .request().
> > 
> > In our case, we know that the state right after request is always disabled,
> > because:
> > - we disable all pwm channels on probe (in PATCH v5 4/7)
> > - .free() disables the pwm channel
> > 
> > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > bother reading out the h/w?
> 
> If there are no plans for the PWM core to call .get_state more often in
> the future, we could just read out the period and return 0 duty and
> disabled.
> 
> Thierry, Uwe, what's your take on this?

I have some plans here. In the past I tried to implement them (see
commit 01ccf903edd65f6421612321648fa5a7f4b7cb10), but this failed
(commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3).

> > Of course, if we choose to leave the pwm enabled after .free(), then
> > .get_state() can even be left out! Do we want that? Genuine question, I do
> > not know the answer.
> 
> I do not think we should leave it enabled after free. It is less
> complicated if we know that unrequested channels are not in use.

My position here is: A consumer should disable a PWM before calling
pwm_put. The driver should however not enforce this and so should not
modify the hardware state in .free().

Also .probe should not change the PWM configuration.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 938 bytes --]

On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> Another point is the period: Sven suggested we do not read out the
> period at all, as the PWM is disabled anyway (see above).
> Is this acceptable?

In my eyes consumers should consider the period value as "don't care" if
the PWM is off. But this doesn't match reality (and maybe also it
doesn't match Thierry's opinion). See for example the
drivers/video/backlight/pwm_bl.c driver which uses the idiom:

	pwm_get_state(mypwm, &state);
	state.enabled = true;
	pwm_apply_state(pb->pwm, &state);

which breaks if .period is invalid. (OK, this isn't totally accurate
because currently the .get_state callback has only little to do with
pwm_get_state(), but you get the point.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi,

On Mon, Jan 11, 2021 at 09:35:32PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Dec 17, 2020 at 06:43:04PM +0100, Clemens Gruber wrote:
> > On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote:
> > > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> > > <clemens.gruber@pqgruber.com> wrote:
> > > >
> > > > Implements .get_state to read-out the current hardware state.
> > > >
> > > 
> > > I am not convinced that we actually need this.
> > > 
> > > Looking at the pwm core, .get_state() is only called right after .request(),
> > > to initialize the cached value of the state. The core then uses the cached
> > > value throughout, it'll never read out the h/w again, until the next .request().
> > > 
> > > In our case, we know that the state right after request is always disabled,
> > > because:
> > > - we disable all pwm channels on probe (in PATCH v5 4/7)
> > > - .free() disables the pwm channel
> > > 
> > > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > > bother reading out the h/w?
> > 
> > If there are no plans for the PWM core to call .get_state more often in
> > the future, we could just read out the period and return 0 duty and
> > disabled.
> > 
> > Thierry, Uwe, what's your take on this?
> 
> I have some plans here. In the past I tried to implement them (see
> commit 01ccf903edd65f6421612321648fa5a7f4b7cb10), but this failed
> (commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3).
> 
> > > Of course, if we choose to leave the pwm enabled after .free(), then
> > > .get_state() can even be left out! Do we want that? Genuine question, I do
> > > not know the answer.
> > 
> > I do not think we should leave it enabled after free. It is less
> > complicated if we know that unrequested channels are not in use.
> 
> My position here is: A consumer should disable a PWM before calling
> pwm_put. The driver should however not enforce this and so should not
> modify the hardware state in .free().
> 
> Also .probe should not change the PWM configuration.

I see. This would also allow PWMs initialized in the bootloader (e.g.
backlights) to stay on between the bootloader and Linux and avoid
flickering.

If no one objects, I would then no longer reset period and duty cycles
in the driver (and for our projects, reset them in the bootloader code
to avoid leaving PWMs on after a kernel panic and watchdog reset, etc.)

And if there is no pre-known state of the registers, we actually need
the .get_state function fully implemented.

Thanks,
Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

Hello Clemens,

On Thu, Jan 14, 2021 at 06:16:22PM +0100, Clemens Gruber wrote:
> On Mon, Jan 11, 2021 at 09:35:32PM +0100, Uwe Kleine-König wrote:
> > My position here is: A consumer should disable a PWM before calling
> > pwm_put. The driver should however not enforce this and so should not
> > modify the hardware state in .free().
> > 
> > Also .probe should not change the PWM configuration.
> 
> I see. This would also allow PWMs initialized in the bootloader (e.g.
> backlights) to stay on between the bootloader and Linux and avoid
> flickering.
> 
> If no one objects, I would then no longer reset period and duty cycles
> in the driver (and for our projects, reset them in the bootloader code
> to avoid leaving PWMs on after a kernel panic and watchdog reset, etc.)
> 
> And if there is no pre-known state of the registers, we actually need
> the .get_state function fully implemented.

This sounds right.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi Sven,

On Fri, Jan 29, 2021 at 08:42:13AM -0500, Sven Van Asbroeck wrote:
> On Mon, Jan 11, 2021 at 3:35 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > My position here is: A consumer should disable a PWM before calling
> > pwm_put. The driver should however not enforce this and so should not
> > modify the hardware state in .free().
> >
> > Also .probe should not change the PWM configuration.
> 
> I agree that this is the most user-friendly behaviour.
> 
> The problem however with the pca9685 is that it has many degrees of
> freedom: there are many possible register values which produce the same
> physical chip outputs.
> 
> This could lead to a situation where, if .probe() does not reset the register
> values, subsequent writes may lead to different outputs than expected.
> 
> One possible solution is to write .get_state() so that it always reads the
> correct state, even if "unconventional" register settings are present, i.e.
> those written by an outside entity, e.g. a bootloader. Then write that
> state back using driver conventions.
> 
> This may be trickier than it sounds - after all we've learnt that the pca9685
> looks simple on the surface, but is actually quite challenging to get right.
> 
> Clemens, Uwe, what do you think?

Ok, so you suggest we extend our get_state logic to deal with cases
like the following:
If neither full OFF nor full ON is set && ON == OFF, we should probably
set the full OFF bit to disable the PWM and log a warning message?
(e.g. "invalid register setting detected: pwm disabled" ?)
If the ON registers are set and the nxp,staggered-outputs property is
not, I'd calculate (off - on) & 4095, set the OFF register to that value
and clear the ON register.

And then call our get_state in .probe, followed by a write of the
resulting / fixed-up state?

This would definitely solve the problem of invalid/unconventional values
set by the bootloader and avoid inconsistencies.
Sounds good to me!

If Thierry and Uwe have no objections, I can send out a new round of
patches in the upcoming weeks.

My current goal is to get the changes into 5.13.

Thanks,
Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Sven Van Asbroeck]

Hi Clemens,

On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> Ok, so you suggest we extend our get_state logic to deal with cases
> like the following:

Kind of. We can't control how other actors (bootloaders etc) program the
chip. As far as I know, there are many, many different register settings that
result in the same physical chip outputs. So if .probe() wants to preserve the
existing chip settings, .get_state() has to be able to deal with every possible
setting. Even invalid ones.

In addition, .apply() cannot make any assumptions as to which bits are
already set/cleared on the chip. Including preserved, invalid settings.

This might get quite complex.

However if we reset the chip in .probe() to a known state (a normalized state,
in the mathematical sense), then both .get_state() and .apply() become
much simpler. because they only need to deal with known, normalized states.

In short, it's a tradeoff between code complexity, and user friendliness/
features.

Sven

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi Sven,

On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens,
> 
> On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > Ok, so you suggest we extend our get_state logic to deal with cases
> > like the following:
> 
> Kind of. We can't control how other actors (bootloaders etc) program the
> chip. As far as I know, there are many, many different register settings that
> result in the same physical chip outputs. So if .probe() wants to preserve the
> existing chip settings, .get_state() has to be able to deal with every possible
> setting. Even invalid ones.

Is the driver really responsible for bootloaders that program the chip
with invalid values?
The chip comes out of PoR with sane default values. If the bootloader of
a user messes them up, isn't that a bootloader problem instead of a
Linux kernel driver problem?

> In addition, .apply() cannot make any assumptions as to which bits are
> already set/cleared on the chip. Including preserved, invalid settings.
> 
> This might get quite complex.
> 
> However if we reset the chip in .probe() to a known state (a normalized state,
> in the mathematical sense), then both .get_state() and .apply() become
> much simpler. because they only need to deal with known, normalized states.

Yes, I agree. This would however make it impossible to do a flicker-free
transition from bootloader to kernel, but that's not really a usecase I
have so I can live without it.

Another point in favor of resetting is that the driver already does it.
Removing the reset of the OFF register may break some boards who rely on
that behaviour.
My version only extended the reset to include the ON register.

> 
> In short, it's a tradeoff between code complexity, and user friendliness/
> features.
> 
> Sven

Thierry, Uwe, what's your take on this?

Thierry: Would you accept it if we continue to reset the registers in
.probe?

Thanks,
Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Sven Van Asbroeck]

Hi Clemens,

On Fri, Jan 29, 2021 at 3:37 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?

No, but it's responsible for correcting invalid values. Otherwise the driver
doesn't work.

> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?

Invalid values are only half the problem. The other half is that two valid
values might produce the same output, e.g.:

LEN_ON = 409, LED_OFF = 1228 and
LED_ON = 419, LED_OFF = 1238
produce the same result. you can't see the difference between the two
when scoping the channel. there are probably more ways to do this,
some might surprise us. It's a tricky chip.

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Sven Van Asbroeck]

Hi Clemens,

On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> LEN_ON = 409, LED_OFF = 1228 and
> LED_ON = 419, LED_OFF = 1238
> produce the same result. you can't see the difference between the two
> when scoping the channel. there are probably more ways to do this,
> some might surprise us. It's a tricky chip.

Please ignore this example, it's bogus. In my defence, it's a Friday
afternoon here :)

But consider the following: imagine the bootloader has enabled a few
pwm channels, and the driver's .probe() has left them on/unchanged.
Then the user enables another pwm channel, and tries to change the
period/prescaler. How would pca9685_may_change_prescaler() know
if changing the prescaler is allowed?

And the following: imagine the bootloader has enabled a few
pwm channels, and the driver's .probe() has left them on/unchanged.
After .probe(), the runtime_pm will immediately put the chip to sleep,
because it's unaware that some channels are alive.

I'm sure I'm overlooking a few complications here. probe not changing
the existing configuration, will add a lot of complexity to the driver.
I'm not saying this is necessarily bad, just a tradeoff. Or, a management
decision.

Sven

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi Sven, Thierry, Uwe,

On Fri, Jan 29, 2021 at 05:16:51PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens,
> 
> On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> >
> > LEN_ON = 409, LED_OFF = 1228 and
> > LED_ON = 419, LED_OFF = 1238
> > produce the same result. you can't see the difference between the two
> > when scoping the channel. there are probably more ways to do this,
> > some might surprise us. It's a tricky chip.
> 
> Please ignore this example, it's bogus. In my defence, it's a Friday
> afternoon here :)

Happens to the best of us :)

> 
> But consider the following: imagine the bootloader has enabled a few
> pwm channels, and the driver's .probe() has left them on/unchanged.
> Then the user enables another pwm channel, and tries to change the
> period/prescaler. How would pca9685_may_change_prescaler() know
> if changing the prescaler is allowed?
> 
> And the following: imagine the bootloader has enabled a few
> pwm channels, and the driver's .probe() has left them on/unchanged.
> After .probe(), the runtime_pm will immediately put the chip to sleep,
> because it's unaware that some channels are alive.

(We could read out the state in .probe. If a pwm is already enabled by
the bootloader, then the user can't change the period. Also, the chip
would not be put to sleep.

The user then can export channels and see if they are enabled. If he
wants to change the period, he needs to find the one enabled by the
bootloader and change the period there, before he requests more.
If the bootloader enabled more than one, then he has to disable all but
one to change the period.

Or did I miss something?)

> 
> I'm sure I'm overlooking a few complications here. probe not changing
> the existing configuration, will add a lot of complexity to the driver.
> I'm not saying this is necessarily bad, just a tradeoff. Or, a management
> decision.

But I agree that it is simpler if we keep the resets in probe. It would
also avoid a potentially breaking change for users that do not reset
their pca9685 chips in their bootloader code.
There might be users out there that depend on the driver to reset the
OFF registers in .probe.

If Thierry agrees / allows it, I can keep the resets for now.

Removing the resets could then be left as something to discuss further
in the future and something that belongs in a separate patch series?

Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi all,

On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> Hi Sven,
> 
> On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> > 
> > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > >
> > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > like the following:
> > 
> > Kind of. We can't control how other actors (bootloaders etc) program the
> > chip. As far as I know, there are many, many different register settings that
> > result in the same physical chip outputs. So if .probe() wants to preserve the
> > existing chip settings, .get_state() has to be able to deal with every possible
> > setting. Even invalid ones.
> 
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?
> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?
> 
> > In addition, .apply() cannot make any assumptions as to which bits are
> > already set/cleared on the chip. Including preserved, invalid settings.
> > 
> > This might get quite complex.
> > 
> > However if we reset the chip in .probe() to a known state (a normalized state,
> > in the mathematical sense), then both .get_state() and .apply() become
> > much simpler. because they only need to deal with known, normalized states.
> 
> Yes, I agree. This would however make it impossible to do a flicker-free
> transition from bootloader to kernel, but that's not really a usecase I
> have so I can live without it.
> 
> Another point in favor of resetting is that the driver already does it.
> Removing the reset of the OFF register may break some boards who rely on
> that behaviour.
> My version only extended the reset to include the ON register.
> 
> > 
> > In short, it's a tradeoff between code complexity, and user friendliness/
> > features.
> > 
> > Sven
> 
> Thierry, Uwe, what's your take on this?
> 
> Thierry: Would you accept it if we continue to reset the registers in
> .probe?
> 
> Thanks,
> Clemens

I realize that it is a difficult time at the moment, but it is a little
bit frustrating not getting any response from the maintainer.

I think the best way forward is to just keep the register resets in
probe as they are. If this is to be changed, I think it should be done
in a separate patchset and by someone who has a usecase requiring it.

Best regards,
Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

Hello,

On Mon, Feb 01, 2021 at 06:24:02PM +0100, Clemens Gruber wrote:
> Hi Sven, Thierry, Uwe,
> 
> On Fri, Jan 29, 2021 at 05:16:51PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> > 
> > On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> > >
> > > LEN_ON = 409, LED_OFF = 1228 and
> > > LED_ON = 419, LED_OFF = 1238
> > > produce the same result. you can't see the difference between the two
> > > when scoping the channel. there are probably more ways to do this,
> > > some might surprise us. It's a tricky chip.
> > 
> > Please ignore this example, it's bogus. In my defence, it's a Friday
> > afternoon here :)
> 
> Happens to the best of us :)
> 
> > 
> > But consider the following: imagine the bootloader has enabled a few
> > pwm channels, and the driver's .probe() has left them on/unchanged.
> > Then the user enables another pwm channel, and tries to change the
> > period/prescaler. How would pca9685_may_change_prescaler() know
> > if changing the prescaler is allowed?
> > 
> > And the following: imagine the bootloader has enabled a few
> > pwm channels, and the driver's .probe() has left them on/unchanged.
> > After .probe(), the runtime_pm will immediately put the chip to sleep,
> > because it's unaware that some channels are alive.
> 
> (We could read out the state in .probe. If a pwm is already enabled by
> the bootloader, then the user can't change the period. Also, the chip
> would not be put to sleep.
> 
> The user then can export channels and see if they are enabled. If he
> wants to change the period, he needs to find the one enabled by the
> bootloader and change the period there, before he requests more.
> If the bootloader enabled more than one, then he has to disable all but
> one to change the period.
> 
> Or did I miss something?)
> 
> > 
> > I'm sure I'm overlooking a few complications here. probe not changing
> > the existing configuration, will add a lot of complexity to the driver.
> > I'm not saying this is necessarily bad, just a tradeoff. Or, a management
> > decision.
> 
> But I agree that it is simpler if we keep the resets in probe. It would
> also avoid a potentially breaking change for users that do not reset
> their pca9685 chips in their bootloader code.

I would prefer to drop the reset. If the bootloader left with an invalid
state, this is active for sure until the PWM driver is loaded. If you
don't reset, the time is extended (usually) until the consumer comes
along and corrects the setting. So the downside of not resetting is
quite limited, but if you disable the PWM in .probe() the effect can be
worse. And consistency dictates to not reset.

> Removing the resets could then be left as something to discuss further
> in the future and something that belongs in a separate patch series?

That would be fine for me, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi Uwe,

On Mon, Mar 01, 2021 at 10:52:48PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Feb 01, 2021 at 06:24:02PM +0100, Clemens Gruber wrote:
> > Hi Sven, Thierry, Uwe,
> > 
> > On Fri, Jan 29, 2021 at 05:16:51PM -0500, Sven Van Asbroeck wrote:
> > > Hi Clemens,
> > > 
> > > On Fri, Jan 29, 2021 at 4:24 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> > > >
> > > > LEN_ON = 409, LED_OFF = 1228 and
> > > > LED_ON = 419, LED_OFF = 1238
> > > > produce the same result. you can't see the difference between the two
> > > > when scoping the channel. there are probably more ways to do this,
> > > > some might surprise us. It's a tricky chip.
> > > 
> > > Please ignore this example, it's bogus. In my defence, it's a Friday
> > > afternoon here :)
> > 
> > Happens to the best of us :)
> > 
> > > 
> > > But consider the following: imagine the bootloader has enabled a few
> > > pwm channels, and the driver's .probe() has left them on/unchanged.
> > > Then the user enables another pwm channel, and tries to change the
> > > period/prescaler. How would pca9685_may_change_prescaler() know
> > > if changing the prescaler is allowed?
> > > 
> > > And the following: imagine the bootloader has enabled a few
> > > pwm channels, and the driver's .probe() has left them on/unchanged.
> > > After .probe(), the runtime_pm will immediately put the chip to sleep,
> > > because it's unaware that some channels are alive.
> > 
> > (We could read out the state in .probe. If a pwm is already enabled by
> > the bootloader, then the user can't change the period. Also, the chip
> > would not be put to sleep.
> > 
> > The user then can export channels and see if they are enabled. If he
> > wants to change the period, he needs to find the one enabled by the
> > bootloader and change the period there, before he requests more.
> > If the bootloader enabled more than one, then he has to disable all but
> > one to change the period.
> > 
> > Or did I miss something?)
> > 
> > > 
> > > I'm sure I'm overlooking a few complications here. probe not changing
> > > the existing configuration, will add a lot of complexity to the driver.
> > > I'm not saying this is necessarily bad, just a tradeoff. Or, a management
> > > decision.
> > 
> > But I agree that it is simpler if we keep the resets in probe. It would
> > also avoid a potentially breaking change for users that do not reset
> > their pca9685 chips in their bootloader code.
> 
> I would prefer to drop the reset. If the bootloader left with an invalid
> state, this is active for sure until the PWM driver is loaded. If you
> don't reset, the time is extended (usually) until the consumer comes
> along and corrects the setting. So the downside of not resetting is
> quite limited, but if you disable the PWM in .probe() the effect can be
> worse. And consistency dictates to not reset.
> 
> > Removing the resets could then be left as something to discuss further
> > in the future and something that belongs in a separate patch series?
> 
> That would be fine for me, too.

Great, then I will prepare a new series next week.

Thanks,
Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2812 bytes --]

On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> Hi everyone,
> 
> happy new year, hope you are all well!
> 
> On Thu, Dec 17, 2020 at 12:52:42PM -0500, Sven Van Asbroeck wrote:
> > On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > > >
> > > > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > > > bother reading out the h/w?
> > >
> > > If there are no plans for the PWM core to call .get_state more often in
> > > the future, we could just read out the period and return 0 duty and
> > > disabled.
> > 
> > I'm not sure why we should even read out the period?
> > When a channel is disabled, the period is not externally visible,
> > therefore it's meaningless ?
> > 
> > As far as I can tell, we can use this for .get_state():
> > memset(&pwm->state, 0, sizeof(pwm_state));
> > 
> > >
> > > Thierry, Uwe, what's your take on this?
> 
> I will continue working on this series in the upcoming weeks.
> Feedback on the .get_state issue would be greatly appreciated.
> 
> To summarize:
> Is it OK for a driver to expect the chip->ops->get_state function to be
> only called from the place in pwm core it is currently called from?
> (Namely in pwm_device_request after chip->ops->request)
> 
> If yes, we could always return a 0 duty cycle and disabled state,
> because this is the state we left it in after .probe (and .free).
> 
> However, if in the future, the pwm core adds additional calls to
> chip->ops->get_state in other places, this could lead to problems.

It's not safe in general to assume that this function will be called
only at one specific point. If you implement the function, then it
should do what it says (i.e. read the current hardware state), and not
bother about when it might be called, or guess at the state that the PWM
might be in.

If you can't implement hardware readout, then that's fine (there are
some devices for which no physical way exists to read out the current
hardware state), but it doesn't sound like that's the problem here.

> Another point is the period: Sven suggested we do not read out the
> period at all, as the PWM is disabled anyway (see above).
> Is this acceptable?

No, if the PWM has separate bits for "enable" and "period", then they
should be read separately. The hardware state isn't about representing
what the currently configured output is, it's a representation of what
the current settings of the PWM channel are.

> And, if we never return anything but 0 in .get_state, should it be
> implemented at all?

Yes, not implementing .get_state() at all is better than lying. If you
always return an all-zeros state, you're inevitably going to return the
wrong result at some point in time.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > Another point is the period: Sven suggested we do not read out the
> > period at all, as the PWM is disabled anyway (see above).
> > Is this acceptable?
> 
> In my eyes consumers should consider the period value as "don't care" if
> the PWM is off. But this doesn't match reality (and maybe also it
> doesn't match Thierry's opinion). See for example the
> drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> 
> 	pwm_get_state(mypwm, &state);
> 	state.enabled = true;
> 	pwm_apply_state(pb->pwm, &state);
> 
> which breaks if .period is invalid. (OK, this isn't totally accurate
> because currently the .get_state callback has only little to do with
> pwm_get_state(), but you get the point.)

The idea behind atomic states in the PWM API is to provide accurate
snapshots of a PWM channel's settings. It's not a representation of
the PWM channel's physical output, although in some cases they may
be the same.

However, there's no 1:1 correspondence between those two. For example,
when looking purely at the physical output of a PWM it is in most cases
not possible to make the distinction between these two states:

    - duty: 0
      period: 100

    - duty: 0
      period: 200

Because the output will be a constant 0 (or 1, depending on polarity).

However, given that we want a snapshot of the currently configured
settings, we can't simply assume that there's a 1:1 correspondence and
then use shortcuts to simplify the hardware state representation because
it isn't going to be accurate.

It is entirely expected that consumers will be able to use an existing
atomic state, update it and then apply it without necessarily having to
recompute the whole state. So what pwm-backlight is doing is expressly
allowed (and in fact was one specific feature that we wanted to have in
the atomic API).

Similarly it's a valid use-case to do something like this:

	/* set duty cycle to 50% */
	pwm_get_state(pwm, &state);
	state.duty_cycle = state.period / 2;
	pwm_apply_state(pwm, &state);

which allows a consumer to do simple modifications without actually
knowing what period has been configured. Some consumers just don't care
about the period or don't even have a clue about what a good value would
be (again, pwm-backlight would be one example). For some PWMs it may
also not be possible to modify the period and if there's no explicit
reason to do so, why should consumers be forced to even bother?

All of that's out the window if we start taking shortcuts. If the PWM
provider reads out the hardware state's PWM as "don't care", how is the
consumer going to know what value to use? Yes, they can use things like
pwm_get_args() to get the configuration from DT, but then what's the
purpose of using states in the first place if consumers have to do all
the tracking manually anyway?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2952 bytes --]

On Mon, Jan 11, 2021 at 09:35:32PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Thu, Dec 17, 2020 at 06:43:04PM +0100, Clemens Gruber wrote:
> > On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote:
> > > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> > > <clemens.gruber@pqgruber.com> wrote:
> > > >
> > > > Implements .get_state to read-out the current hardware state.
> > > >
> > > 
> > > I am not convinced that we actually need this.
> > > 
> > > Looking at the pwm core, .get_state() is only called right after .request(),
> > > to initialize the cached value of the state. The core then uses the cached
> > > value throughout, it'll never read out the h/w again, until the next .request().
> > > 
> > > In our case, we know that the state right after request is always disabled,
> > > because:
> > > - we disable all pwm channels on probe (in PATCH v5 4/7)
> > > - .free() disables the pwm channel
> > > 
> > > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > > bother reading out the h/w?
> > 
> > If there are no plans for the PWM core to call .get_state more often in
> > the future, we could just read out the period and return 0 duty and
> > disabled.
> > 
> > Thierry, Uwe, what's your take on this?
> 
> I have some plans here. In the past I tried to implement them (see
> commit 01ccf903edd65f6421612321648fa5a7f4b7cb10), but this failed
> (commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3).
> 
> > > Of course, if we choose to leave the pwm enabled after .free(), then
> > > .get_state() can even be left out! Do we want that? Genuine question, I do
> > > not know the answer.
> > 
> > I do not think we should leave it enabled after free. It is less
> > complicated if we know that unrequested channels are not in use.
> 
> My position here is: A consumer should disable a PWM before calling
> pwm_put. The driver should however not enforce this and so should not
> modify the hardware state in .free().

There had been discussions in the past about at least letting the PWM
core warn about any PWMs that had been left enabled after pwm_put(). I
still think that's worthwhile to do because it is consistent with how
the rest of the kernel works (i.e. drivers are supposed to leave devices
in a quiescent state when they relinquish control), and consumers are
ultimately responsible for making sure they've cleaned up their
resources.

Most PWM drivers do a variant of this and assert a reset, disable clocks
and/or release runtime PM references when removing the PWM chip on
->remove(), but that happens at a different time than pwm_put(), so I
think it makes sense to nudge consumers in the right direction and WARN
when they've left a PWM enabled when calling pwm_put().

If we do that, it shouldn't take very long for any violations to get
fixed and then we don't have to enforce this in PWM drivers or the core.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3404 bytes --]

On Thu, Jan 14, 2021 at 06:16:22PM +0100, Clemens Gruber wrote:
> Hi,
> 
> On Mon, Jan 11, 2021 at 09:35:32PM +0100, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Thu, Dec 17, 2020 at 06:43:04PM +0100, Clemens Gruber wrote:
> > > On Wed, Dec 16, 2020 at 11:00:59PM -0500, Sven Van Asbroeck wrote:
> > > > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> > > > <clemens.gruber@pqgruber.com> wrote:
> > > > >
> > > > > Implements .get_state to read-out the current hardware state.
> > > > >
> > > > 
> > > > I am not convinced that we actually need this.
> > > > 
> > > > Looking at the pwm core, .get_state() is only called right after .request(),
> > > > to initialize the cached value of the state. The core then uses the cached
> > > > value throughout, it'll never read out the h/w again, until the next .request().
> > > > 
> > > > In our case, we know that the state right after request is always disabled,
> > > > because:
> > > > - we disable all pwm channels on probe (in PATCH v5 4/7)
> > > > - .free() disables the pwm channel
> > > > 
> > > > Conclusion: .get_state() will always return "pwm disabled", so why do we
> > > > bother reading out the h/w?
> > > 
> > > If there are no plans for the PWM core to call .get_state more often in
> > > the future, we could just read out the period and return 0 duty and
> > > disabled.
> > > 
> > > Thierry, Uwe, what's your take on this?
> > 
> > I have some plans here. In the past I tried to implement them (see
> > commit 01ccf903edd65f6421612321648fa5a7f4b7cb10), but this failed
> > (commit 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3).
> > 
> > > > Of course, if we choose to leave the pwm enabled after .free(), then
> > > > .get_state() can even be left out! Do we want that? Genuine question, I do
> > > > not know the answer.
> > > 
> > > I do not think we should leave it enabled after free. It is less
> > > complicated if we know that unrequested channels are not in use.
> > 
> > My position here is: A consumer should disable a PWM before calling
> > pwm_put. The driver should however not enforce this and so should not
> > modify the hardware state in .free().
> > 
> > Also .probe should not change the PWM configuration.
> 
> I see. This would also allow PWMs initialized in the bootloader (e.g.
> backlights) to stay on between the bootloader and Linux and avoid
> flickering.

Yes, that's precisely one of the reasons why we introduced the atomic
API. One of the use-cases that led to the current design was that the
kernel pwm-regulator on some platforms was causing devices to crash
because the driver would reprogram the PWM and cause a glitch on the
power supply for the CPUs.

So it's crucial in some cases that the PWM driver don't touch the
hardware state in ->probe(). If some drivers currently do so, that's
something we should eventually change, but given that there haven't been
any complaints yet, it likely means nothing breaks because of this, so
we do have the luxury of not having to rush things.

> If no one objects, I would then no longer reset period and duty cycles
> in the driver (and for our projects, reset them in the bootloader code
> to avoid leaving PWMs on after a kernel panic and watchdog reset, etc.)

This isn't strictly necessary, but it's obviously something that's up to
board designers/maintainers to decide.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]

On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens,
> 
> On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > Ok, so you suggest we extend our get_state logic to deal with cases
> > like the following:
> 
> Kind of. We can't control how other actors (bootloaders etc) program the
> chip. As far as I know, there are many, many different register settings that
> result in the same physical chip outputs. So if .probe() wants to preserve the
> existing chip settings, .get_state() has to be able to deal with every possible
> setting. Even invalid ones.

I said earlier that the PWM state is a snapshot of the current hardware
settings and that's not entirely accurate because it isn't actually a
complete representation of the hardware state. It's merely a
representation of the PWM software state that's currently applied to the
hardware.

This is simpler from an API point of view than completely representing
the actual hardware state, but it's also sufficient for most use-cases
because we don't care about the exact programming as long as it yields
the result represented by the atomic state. Although it's still vitally
important that the amount of state that we have is accurately
represented (i.e. duty-cycle/period values must not be collapsed to 0
when the PWM is off), otherwise the API isn't usable.

One good thing that comes from this simplification is that it gives us
a bit more flexibility in hardware readout because you can collapse a
large amount of variation into the couple of values that we have. So if
your bootloaders program weird values, you can canonicalize them as long
as they still yield the same result.

So roughly what should be guaranteed from an atomic API point of view is
that doing the following is glitch-free and doesn't cause a change in
the physical PWM signal:

    chip->ops->get_state(chip, pwm, &state);
    pwm_apply_state(pwm, &state);

Ideally we'd even be able to, though we don't do it at present, to
optimize that out as a no-op by comparing the new state with the current
state and just not doing anything if they are equal.

And just to clarify: glitch-free above means: to the extent possible. In
some cases it might not be possible to set PWM hardware state in a
completely glitch-free way. If so, there's not a lot we can do and it's
better to do something even if it's not ideal. The rationale behind this
is that nobody will select a chip that doesn't meet requirements to
perform a given task, so it's highly unlikely that a chip that glitches
during transitions will ever be used in a setup where it's required not
to glitch. We should obviously always do our best to keep glitches to a
minimum, but software can't change hardware...

> In addition, .apply() cannot make any assumptions as to which bits are
> already set/cleared on the chip. Including preserved, invalid settings.
> 
> This might get quite complex.
> 
> However if we reset the chip in .probe() to a known state (a normalized state,
> in the mathematical sense), then both .get_state() and .apply() become
> much simpler. because they only need to deal with known, normalized states.

As was mentioned before, this does restrict the usability of the driver.
In some cases you really want to avoid resetting the chip. But I'm also
okay with leaving this as-is because it's the status quo.

So what I'd propose is to take this forward and keep the reset during
probe for now and then follow up with a separate, simple patch that
removes the reset. That way we can easily back it out, or revert it, if
it causes any breakage, but it won't hold up this series.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3242 bytes --]

On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> Hi Sven,
> 
> On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens,
> > 
> > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > >
> > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > like the following:
> > 
> > Kind of. We can't control how other actors (bootloaders etc) program the
> > chip. As far as I know, there are many, many different register settings that
> > result in the same physical chip outputs. So if .probe() wants to preserve the
> > existing chip settings, .get_state() has to be able to deal with every possible
> > setting. Even invalid ones.
> 
> Is the driver really responsible for bootloaders that program the chip
> with invalid values?
> The chip comes out of PoR with sane default values. If the bootloader of
> a user messes them up, isn't that a bootloader problem instead of a
> Linux kernel driver problem?

It is ultimately a problem of the bootloader and where possible the
bootloader should be fixed. However, fixing bootloaders sometimes isn't
possible, or impractical, so the kernel has to be able to deal with
hardware that's been badly programmed by the bootloader. Within reason,
of course. Sometimes this can't be done in any other way than forcing a
hard reset of the chip, but it should always be a last resort.

> > In addition, .apply() cannot make any assumptions as to which bits are
> > already set/cleared on the chip. Including preserved, invalid settings.
> > 
> > This might get quite complex.
> > 
> > However if we reset the chip in .probe() to a known state (a normalized state,
> > in the mathematical sense), then both .get_state() and .apply() become
> > much simpler. because they only need to deal with known, normalized states.
> 
> Yes, I agree. This would however make it impossible to do a flicker-free
> transition from bootloader to kernel, but that's not really a usecase I
> have so I can live without it.
> 
> Another point in favor of resetting is that the driver already does it.
> Removing the reset of the OFF register may break some boards who rely on
> that behaviour.
> My version only extended the reset to include the ON register.
> 
> > 
> > In short, it's a tradeoff between code complexity, and user friendliness/
> > features.
> > 
> > Sven
> 
> Thierry, Uwe, what's your take on this?
> 
> Thierry: Would you accept it if we continue to reset the registers in
> .probe?

Yes, I think it's fine to continue to reset the registers since that's
basically what the driver already does. It'd be great if you could
follow up with a patch that removes the reset and leaves the hardware in
whatever state the bootloader has set up. Then we can take that patch
for a ride and see if there are any complains about it breaking. If
there are we can always try to fix them, but as a last resort we can
also revert, which then may be something we have to live with. But I
think we should at least try to make this consistent with how other
drivers do this so that people don't stumble over this particular
driver's behaviour.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

Hello Andy,

On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> On Monday, March 22, 2021, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > Thierry: Would you accept it if we continue to reset the registers in
> > > .probe?
> >
> > Yes, I think it's fine to continue to reset the registers since that's
> > basically what the driver already does. It'd be great if you could
> > follow up with a patch that removes the reset and leaves the hardware in
> > whatever state the bootloader has set up. Then we can take that patch
> > for a ride and see if there are any complains about it breaking. If
> > there are we can always try to fix them, but as a last resort we can
> > also revert, which then may be something we have to live with. But I
> > think we should at least try to make this consistent with how other
> > drivers do this so that people don't stumble over this particular
> > driver's
> 
> I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> boot loader left device completely untouched and device either in wrong
> state because if failed reset (saw this on PCA9555 which has a
> corresponding errata), or simply we have done a warm reset of the system.
> So, we also have to understand how to properly exit.

I don't think that not resetting is a real problem. My argumentation
goes as follows:

When the PWM driver is loaded and the PWM configuration is invalid, it
was already invalid for the time between power up (or warm start) and
PWM driver load time. Then it doesn't really hurt to keep the PWM
in this invalid state for a little moment longer until the consumer of
the PWM becomes active.

Together with the use cases where not resetting is the right thing to
do, I'm convinced not resetting is the better strategy.

> Another point, CCF has a bit “is critical”, and u guess PWM may get the
> same and make the all assumptions much easier.

So I think complicating the PWM framework for this isn't the right thing
to do.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Andy Shevchenko]

On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> > On Monday, March 22, 2021, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > > Thierry: Would you accept it if we continue to reset the registers in
> > > > .probe?
> > >
> > > Yes, I think it's fine to continue to reset the registers since that's
> > > basically what the driver already does. It'd be great if you could
> > > follow up with a patch that removes the reset and leaves the hardware in
> > > whatever state the bootloader has set up. Then we can take that patch
> > > for a ride and see if there are any complains about it breaking. If
> > > there are we can always try to fix them, but as a last resort we can
> > > also revert, which then may be something we have to live with. But I
> > > think we should at least try to make this consistent with how other
> > > drivers do this so that people don't stumble over this particular
> > > driver's
> >
> > I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> > boot loader left device completely untouched and device either in wrong
> > state because if failed reset (saw this on PCA9555 which has a
> > corresponding errata), or simply we have done a warm reset of the system.
> > So, we also have to understand how to properly exit.
>
> I don't think that not resetting is a real problem. My argumentation
> goes as follows:
>
> When the PWM driver is loaded and the PWM configuration is invalid, it
> was already invalid for the time between power up (or warm start) and
> PWM driver load time. Then it doesn't really hurt to keep the PWM
> in this invalid state for a little moment longer until the consumer of
> the PWM becomes active.

But this won't work in the cases when we have a chip with a shared
settings for period and/or duty cycle. You will never have a user come
due to -EBUSY.

> Together with the use cases where not resetting is the right thing to
> do, I'm convinced not resetting is the better strategy.

I'm on the opposite side, although I know the cases that resetting
maybe harmful (to some extent).
We definitely need a hint if we may or may not touch 1 or more
channels of a given PWM.

> > Another point, CCF has a bit “is critical”, and u guess PWM may get the
> > same and make the all assumptions much easier.
>
> So I think complicating the PWM framework for this isn't the right thing
> to do.

Again, I'm on opposite side here b/c see above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2488 bytes --]

On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> > > On Monday, March 22, 2021, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > > > Thierry: Would you accept it if we continue to reset the registers in
> > > > > .probe?
> > > >
> > > > Yes, I think it's fine to continue to reset the registers since that's
> > > > basically what the driver already does. It'd be great if you could
> > > > follow up with a patch that removes the reset and leaves the hardware in
> > > > whatever state the bootloader has set up. Then we can take that patch
> > > > for a ride and see if there are any complains about it breaking. If
> > > > there are we can always try to fix them, but as a last resort we can
> > > > also revert, which then may be something we have to live with. But I
> > > > think we should at least try to make this consistent with how other
> > > > drivers do this so that people don't stumble over this particular
> > > > driver's
> > >
> > > I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> > > boot loader left device completely untouched and device either in wrong
> > > state because if failed reset (saw this on PCA9555 which has a
> > > corresponding errata), or simply we have done a warm reset of the system.
> > > So, we also have to understand how to properly exit.
> >
> > I don't think that not resetting is a real problem. My argumentation
> > goes as follows:
> >
> > When the PWM driver is loaded and the PWM configuration is invalid, it
> > was already invalid for the time between power up (or warm start) and
> > PWM driver load time. Then it doesn't really hurt to keep the PWM
> > in this invalid state for a little moment longer until the consumer of
> > the PWM becomes active.
> 
> But this won't work in the cases when we have a chip with a shared
> settings for period and/or duty cycle. You will never have a user come
> due to -EBUSY.

That's wrong, the first consumer to enable the PWM (in software) is
supposed to be able to change the settings.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Andy Shevchenko]

On Mon, Mar 22, 2021 at 1:48 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote:
> > On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Mon, Mar 22, 2021 at 11:38:40AM +0200, Andy Shevchenko wrote:
> > > > On Monday, March 22, 2021, Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > > > > > Thierry: Would you accept it if we continue to reset the registers in
> > > > > > .probe?
> > > > >
> > > > > Yes, I think it's fine to continue to reset the registers since that's
> > > > > basically what the driver already does. It'd be great if you could
> > > > > follow up with a patch that removes the reset and leaves the hardware in
> > > > > whatever state the bootloader has set up. Then we can take that patch
> > > > > for a ride and see if there are any complains about it breaking. If
> > > > > there are we can always try to fix them, but as a last resort we can
> > > > > also revert, which then may be something we have to live with. But I
> > > > > think we should at least try to make this consistent with how other
> > > > > drivers do this so that people don't stumble over this particular
> > > > > driver's
> > > >
> > > > I guess we may miss (a PCB / silicon design flaw or warm boot case) when
> > > > boot loader left device completely untouched and device either in wrong
> > > > state because if failed reset (saw this on PCA9555 which has a
> > > > corresponding errata), or simply we have done a warm reset of the system.
> > > > So, we also have to understand how to properly exit.
> > >
> > > I don't think that not resetting is a real problem. My argumentation
> > > goes as follows:
> > >
> > > When the PWM driver is loaded and the PWM configuration is invalid, it
> > > was already invalid for the time between power up (or warm start) and
> > > PWM driver load time. Then it doesn't really hurt to keep the PWM
> > > in this invalid state for a little moment longer until the consumer of
> > > the PWM becomes active.
> >
> > But this won't work in the cases when we have a chip with a shared
> > settings for period and/or duty cycle. You will never have a user come
> > due to -EBUSY.
>
> That's wrong, the first consumer to enable the PWM (in software) is
> supposed to be able to change the settings.

If it's a critical PWM, how can you be allowed to do that?
And if so, what is the difference between resetting the device in this
case? You may consider it as a change to the settings by the first
consumer.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1901 bytes --]

Hello,

On Mon, Mar 22, 2021 at 02:15:08PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 22, 2021 at 1:48 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Mar 22, 2021 at 01:40:57PM +0200, Andy Shevchenko wrote:
> > > On Mon, Mar 22, 2021 at 1:22 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > When the PWM driver is loaded and the PWM configuration is invalid, it
> > > > was already invalid for the time between power up (or warm start) and
> > > > PWM driver load time. Then it doesn't really hurt to keep the PWM
> > > > in this invalid state for a little moment longer until the consumer of
> > > > the PWM becomes active.
> > >
> > > But this won't work in the cases when we have a chip with a shared
> > > settings for period and/or duty cycle. You will never have a user come
> > > due to -EBUSY.
> >
> > That's wrong, the first consumer to enable the PWM (in software) is
> > supposed to be able to change the settings.
> 
> If it's a critical PWM, how can you be allowed to do that?

You seem to have a tight concept of a critical PWM. I don't, so I have
problems following you. What is your picture about what is to be
allowed/denied for a critical PWM?

> And if so, what is the difference between resetting the device in this
> case?

The difference is that we have a consumer that knows what to do with the
PWM then.

> You may consider it as a change to the settings by the first
> consumer.

.. but without knowing if the first consumer is a backlight driver or a
motor control it's hard to know if disabling the PWM is OK. So I like
the concept of not doing anything until a process comes along that knows
better.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Hi Thierry,

On Mon, Mar 22, 2021 at 10:19:22AM +0100, Thierry Reding wrote:
> On Fri, Jan 29, 2021 at 09:37:47PM +0100, Clemens Gruber wrote:
> > Hi Sven,
> > 
> > On Fri, Jan 29, 2021 at 01:05:14PM -0500, Sven Van Asbroeck wrote:
> > > Hi Clemens,
> > > 
> > > On Fri, Jan 29, 2021 at 11:31 AM Clemens Gruber
> > > <clemens.gruber@pqgruber.com> wrote:
> > > >
> > > > Ok, so you suggest we extend our get_state logic to deal with cases
> > > > like the following:
> > > 
> > > Kind of. We can't control how other actors (bootloaders etc) program the
> > > chip. As far as I know, there are many, many different register settings that
> > > result in the same physical chip outputs. So if .probe() wants to preserve the
> > > existing chip settings, .get_state() has to be able to deal with every possible
> > > setting. Even invalid ones.
> > 
> > Is the driver really responsible for bootloaders that program the chip
> > with invalid values?
> > The chip comes out of PoR with sane default values. If the bootloader of
> > a user messes them up, isn't that a bootloader problem instead of a
> > Linux kernel driver problem?
> 
> It is ultimately a problem of the bootloader and where possible the
> bootloader should be fixed. However, fixing bootloaders sometimes isn't
> possible, or impractical, so the kernel has to be able to deal with
> hardware that's been badly programmed by the bootloader. Within reason,
> of course. Sometimes this can't be done in any other way than forcing a
> hard reset of the chip, but it should always be a last resort.
> 
> > > In addition, .apply() cannot make any assumptions as to which bits are
> > > already set/cleared on the chip. Including preserved, invalid settings.
> > > 
> > > This might get quite complex.
> > > 
> > > However if we reset the chip in .probe() to a known state (a normalized state,
> > > in the mathematical sense), then both .get_state() and .apply() become
> > > much simpler. because they only need to deal with known, normalized states.
> > 
> > Yes, I agree. This would however make it impossible to do a flicker-free
> > transition from bootloader to kernel, but that's not really a usecase I
> > have so I can live without it.
> > 
> > Another point in favor of resetting is that the driver already does it.
> > Removing the reset of the OFF register may break some boards who rely on
> > that behaviour.
> > My version only extended the reset to include the ON register.
> > 
> > > 
> > > In short, it's a tradeoff between code complexity, and user friendliness/
> > > features.
> > > 
> > > Sven
> > 
> > Thierry, Uwe, what's your take on this?
> > 
> > Thierry: Would you accept it if we continue to reset the registers in
> > .probe?
> 
> Yes, I think it's fine to continue to reset the registers since that's
> basically what the driver already does. It'd be great if you could
> follow up with a patch that removes the reset and leaves the hardware in
> whatever state the bootloader has set up. Then we can take that patch
> for a ride and see if there are any complains about it breaking. If
> there are we can always try to fix them, but as a last resort we can
> also revert, which then may be something we have to live with. But I
> think we should at least try to make this consistent with how other
> drivers do this so that people don't stumble over this particular
> driver's behaviour.

Thanks for your input!

Sounds good to me. I am currently preparing a new revision of the
series. As soon as that is reviewed and good to go, I will look into
removing the resets.

Clemens

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 7228 bytes --]

Hello Thierry,

On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > Another point is the period: Sven suggested we do not read out the
> > > period at all, as the PWM is disabled anyway (see above).
> > > Is this acceptable?
> > 
> > In my eyes consumers should consider the period value as "don't care" if
> > the PWM is off. But this doesn't match reality (and maybe also it
> > doesn't match Thierry's opinion). See for example the
> > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > 
> > 	pwm_get_state(mypwm, &state);
> > 	state.enabled = true;
> > 	pwm_apply_state(pb->pwm, &state);
> > 
> > which breaks if .period is invalid. (OK, this isn't totally accurate
> > because currently the .get_state callback has only little to do with
> > pwm_get_state(), but you get the point.)
> 
> The idea behind atomic states in the PWM API is to provide accurate
> snapshots of a PWM channel's settings. It's not a representation of
> the PWM channel's physical output, although in some cases they may
> be the same.

I think the pwm_state returned by .get_state() should be an accurate
representation of the PWM channel's physical output.

> However, there's no 1:1 correspondence between those two. For example,
> when looking purely at the physical output of a PWM it is in most cases
> not possible to make the distinction between these two states:
> 
>     - duty: 0
>       period: 100
> 
>     - duty: 0
>       period: 200
> 
> Because the output will be a constant 0 (or 1, depending on polarity).

I agree that there isn't in all cases a unique pwm_state that formalizes
the current output. That's because with .enabled=false the settings
.duty_cycle and .period hardly matter for the output and when
.duty_cycle = 0 or = .period the actual period also (mostly) doesn't
matter.

> However, given that we want a snapshot of the currently configured
> settings, we can't simply assume that there's a 1:1 correspondence and
> then use shortcuts to simplify the hardware state representation because
> it isn't going to be accurate.

When we assume that pwm_get_state returns the state of the hardware,
relying on these values might be too optimistic. Consider a hardware
(e.g.  pwm-cros-ec) that has no disable switch. Disabling is modeled by
.duty_cycle = 0. So after:

	pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled = false })

doing:

	pwm_get_state(pwm, &mystate);
	mystate.enabled = true;
	pwm_apply_state(pwm, &mystate);

the PWM stays configured with .duty_cycle = 0.

There are also more delicate surprises. Consider a PWM that can
implement all duty_cycles and periods with a resolution of 30 ns up to
the consumers preferred period of 2000 ns and uses the usual round-down
strategy. Consider the consumer wants to repeatedly switch between 75%
and 50% relative duty cycle. 

When relying on .get_state, the first configuration is likely to still
be 1500/2000. .get_state() then returns

	.duty_cycle = 1500
	.period = 1980

and then to implement the 50% relative duty the resulting request is

	.duty_cycle = 990
	.period = 1980

which can be implemented exactly. When then switching back to 75% the
request is

	.duty_cycle = 1485
	.period = 1980

yielding a period of 1470 ns. So this is a different setting than on the
first go to implement 75% because the rounding errors accumulate.

The IMHO only sane way out is that the consumer should always request
1500/2000 and 1000/2000.

So I think calculating the state from the previously implemented state
has too many surprises and should not be the recommended way.
Consumers should just request what they want and provide this state
without relying on .get_state(). And then the needs to cache the
duty_cycle for a disabled PWM or reading the period for a disabled PWM
just vanish in thin air.

> It is entirely expected that consumers will be able to use an existing
> atomic state, update it and then apply it without necessarily having to
> recompute the whole state. So what pwm-backlight is doing is expressly
> allowed (and in fact was one specific feature that we wanted to have in
> the atomic API).

Who is "we"?

> Similarly it's a valid use-case to do something like this:
> 
> 	/* set duty cycle to 50% */
> 	pwm_get_state(pwm, &state);
> 	state.duty_cycle = state.period / 2;
> 	pwm_apply_state(pwm, &state);

The cost to continue doing that is that the consumer has to cache the
state. Then the idiom looks as follows:

	state = &driver_data->state;
	state->duty_cycle = state->period / 2;
	pwm_apply_state(pwm, state);

which 

 - allows to drop caching in the PWM layer (which is good as it
   simplifies the PWM framework and saves some memory for consumers that
   don't need caching)

 - doesn't accumulate rounding errors

 - needs less PWM API calls

Also I think keeping the PWM configuration in the consumer instead of
the PWM is the right place, but I assume that's subjective. I don't
oppose to caching the previously applied state for consumers, but IMHO
we should differentiate between the currently configured state and the
previously applied state as there are semantic differences between these
two.

Note that even if we somehow normalize the state returned by a driver's
.get_state callback (e.g. by setting .duty_cycle = 0 for disabled PWMs)
this still matches your expectation that "consumers will be able to use
an existing atomic state, update it and then apply without necessarily
having to recompute the whole state". The critical part is just that
consumers should not start with a pwm_state returned by .get_state() but
from the previously requested state.

> which allows a consumer to do simple modifications without actually
> knowing what period has been configured. Some consumers just don't care
> about the period or don't even have a clue about what a good value would
> be (again, pwm-backlight would be one example). For some PWMs it may
> also not be possible to modify the period and if there's no explicit
> reason to do so, why should consumers be forced to even bother?
> 
> All of that's out the window if we start taking shortcuts. If the PWM
> provider reads out the hardware state's PWM as "don't care", how is the
> consumer going to know what value to use? Yes, they can use things like
> pwm_get_args() to get the configuration from DT, but then what's the
> purpose of using states in the first place if consumers have to do all
> the tracking manually anyway?

With my approach the consumers always have to explicitly request a
complete setting, but I'd consider this an advantage that makes the
mechanisms clearer. Other than that I don't see any disadvantages and
the PWM framework can stop pretending things that don't match (the
hardware's) reality.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11292 bytes --]

On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > Another point is the period: Sven suggested we do not read out the
> > > > period at all, as the PWM is disabled anyway (see above).
> > > > Is this acceptable?
> > > 
> > > In my eyes consumers should consider the period value as "don't care" if
> > > the PWM is off. But this doesn't match reality (and maybe also it
> > > doesn't match Thierry's opinion). See for example the
> > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > 
> > > 	pwm_get_state(mypwm, &state);
> > > 	state.enabled = true;
> > > 	pwm_apply_state(pb->pwm, &state);
> > > 
> > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > because currently the .get_state callback has only little to do with
> > > pwm_get_state(), but you get the point.)
> > 
> > The idea behind atomic states in the PWM API is to provide accurate
> > snapshots of a PWM channel's settings. It's not a representation of
> > the PWM channel's physical output, although in some cases they may
> > be the same.
> 
> I think the pwm_state returned by .get_state() should be an accurate
> representation of the PWM channel's physical output.

Yeah, like I said, in most cases that will be true. However, as
mentioned below, if there's no 1:1 correspondence between the settings
and what's actually coming out of the PWM, this isn't always possible.
But yes, it should always be as accurate as possible.

> > However, there's no 1:1 correspondence between those two. For example,
> > when looking purely at the physical output of a PWM it is in most cases
> > not possible to make the distinction between these two states:
> > 
> >     - duty: 0
> >       period: 100
> > 
> >     - duty: 0
> >       period: 200
> > 
> > Because the output will be a constant 0 (or 1, depending on polarity).
> 
> I agree that there isn't in all cases a unique pwm_state that formalizes
> the current output. That's because with .enabled=false the settings
> .duty_cycle and .period hardly matter for the output and when
> .duty_cycle = 0 or = .period the actual period also (mostly) doesn't
> matter.
> 
> > However, given that we want a snapshot of the currently configured
> > settings, we can't simply assume that there's a 1:1 correspondence and
> > then use shortcuts to simplify the hardware state representation because
> > it isn't going to be accurate.
> 
> When we assume that pwm_get_state returns the state of the hardware,
> relying on these values might be too optimistic. Consider a hardware
> (e.g.  pwm-cros-ec) that has no disable switch. Disabling is modeled by
> .duty_cycle = 0. So after:
> 
> 	pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled = false })
> 
> doing:
> 
> 	pwm_get_state(pwm, &mystate);
> 	mystate.enabled = true;
> 	pwm_apply_state(pwm, &mystate);
> 
> the PWM stays configured with .duty_cycle = 0.

Uhm... no, that's not what should be happening. pwm_get_state() copies
the PWM channel's internal software state. It doesn't actually go and
read the hardware state. We only ever read the hardware state when the
PWM is requested. After that there should be no reason to ever read back
the hardware state again because the consumer owns the state and that
state must not change unless explicitly requested by the owner.

So in the above case, mystate.duty_cycle should be 500 after that call
to pwm_get_state(). The fact that the hardware can't explicitly disable
a PWM and needs to emulate that by setting duty-cycle = 0 is a driver
implementation detail and shouldn't leak to the consumer.

> There are also more delicate surprises. Consider a PWM that can
> implement all duty_cycles and periods with a resolution of 30 ns up to
> the consumers preferred period of 2000 ns and uses the usual round-down
> strategy. Consider the consumer wants to repeatedly switch between 75%
> and 50% relative duty cycle. 
> 
> When relying on .get_state, the first configuration is likely to still
> be 1500/2000. .get_state() then returns
> 
> 	.duty_cycle = 1500
> 	.period = 1980
> 
> and then to implement the 50% relative duty the resulting request is
> 
> 	.duty_cycle = 990
> 	.period = 1980
> 
> which can be implemented exactly. When then switching back to 75% the
> request is
> 
> 	.duty_cycle = 1485
> 	.period = 1980
> 
> yielding a period of 1470 ns. So this is a different setting than on the
> first go to implement 75% because the rounding errors accumulate.
> 
> The IMHO only sane way out is that the consumer should always request
> 1500/2000 and 1000/2000.
> 
> So I think calculating the state from the previously implemented state
> has too many surprises and should not be the recommended way.
> Consumers should just request what they want and provide this state
> without relying on .get_state(). And then the needs to cache the
> duty_cycle for a disabled PWM or reading the period for a disabled PWM
> just vanish in thin air.

Again, note that ->get_state() and pwm_get_state() are two different
things. The naming is perhaps a bit unfortunate...

But also, the above should never happen because drivers are not supposed
to modify the software state and the core will never use ->get_state()
to read back the hardware state. Basically that means that
pwm_get_state(), followed by pwm_apply_state() called on the same PWM
and the same state object should be an idempotent operation.

Well, it's possible for a driver to have to modify the software state to
more accurately reflect what has been configured to hardware. So the
pwm_get_state()/pwm_apply_state()/pwm_get_state() sequence may give you
a different state from the initial state. However it must not be to a
degree that would make a subsequent pwm_apply_state() change the values
again.

So I guess the idempotency rule really is more like this: the following
sequence must always yield the same PWM signal:

	pwm_apply_state(pwm, &state);
	pwm_get_state(pwm, &state);
	pwm_apply_state(pwm, &state);

> > It is entirely expected that consumers will be able to use an existing
> > atomic state, update it and then apply it without necessarily having to
> > recompute the whole state. So what pwm-backlight is doing is expressly
> > allowed (and in fact was one specific feature that we wanted to have in
> > the atomic API).
> 
> Who is "we"?

Myself and whoever else was involved back at the time when we designed
the atomic API.

> > Similarly it's a valid use-case to do something like this:
> > 
> > 	/* set duty cycle to 50% */
> > 	pwm_get_state(pwm, &state);
> > 	state.duty_cycle = state.period / 2;
> > 	pwm_apply_state(pwm, &state);
> 
> The cost to continue doing that is that the consumer has to cache the
> state. Then the idiom looks as follows:
> 
> 	state = &driver_data->state;
> 	state->duty_cycle = state->period / 2;
> 	pwm_apply_state(pwm, state);

Sorry but no. Consumers have no business reaching into driver-data and
operating on the internal state objects.

> which 
> 
>  - allows to drop caching in the PWM layer (which is good as it
>    simplifies the PWM framework and saves some memory for consumers that
>    don't need caching)

What exactly is complicated in the PWM framework that would need to be
simplified. This is really all quite trivial.

>  - doesn't accumulate rounding errors

See above, if rounding errors accumulate then something is wrong. This
shouldn't be happening.

Now, the above idempotency rule does not rule out rounding that can
occur due to consumer error. So consumers need to be aware that some
hardware state may not be applied exactly as specified. Reusing too
much of the state may introduce these rounding errors. So yes, if you
want to toggle between 50% and 75% duty cycles, consumers should spell
that out explicitly, perhaps by caching the PWM args and reusing period
from that, for example.

>  - needs less PWM API calls
> 
> Also I think keeping the PWM configuration in the consumer instead of
> the PWM is the right place, but I assume that's subjective. I don't
> oppose to caching the previously applied state for consumers, but IMHO
> we should differentiate between the currently configured state and the
> previously applied state as there are semantic differences between these
> two.
> 
> Note that even if we somehow normalize the state returned by a driver's
> .get_state callback (e.g. by setting .duty_cycle = 0 for disabled PWMs)
> this still matches your expectation that "consumers will be able to use
> an existing atomic state, update it and then apply without necessarily
> having to recompute the whole state". The critical part is just that
> consumers should not start with a pwm_state returned by .get_state() but
> from the previously requested state.

Again, see above. pwm_get_state() does not use ->get_state().

> > which allows a consumer to do simple modifications without actually
> > knowing what period has been configured. Some consumers just don't care
> > about the period or don't even have a clue about what a good value would
> > be (again, pwm-backlight would be one example). For some PWMs it may
> > also not be possible to modify the period and if there's no explicit
> > reason to do so, why should consumers be forced to even bother?
> > 
> > All of that's out the window if we start taking shortcuts. If the PWM
> > provider reads out the hardware state's PWM as "don't care", how is the
> > consumer going to know what value to use? Yes, they can use things like
> > pwm_get_args() to get the configuration from DT, but then what's the
> > purpose of using states in the first place if consumers have to do all
> > the tracking manually anyway?
> 
> With my approach the consumers always have to explicitly request a
> complete setting, but I'd consider this an advantage that makes the
> mechanisms clearer. Other than that I don't see any disadvantages and
> the PWM framework can stop pretending things that don't match (the
> hardware's) reality.

That's really no different from what's currently happening. Consumers
always request a full state to be applied. The only difference is that
some of the values might be cached. But again, that's really a good
thing. Why should consumers need to have their own copy of PWM state
if all the want to do is toggle a PWM on and off?

And this is all very subjective. I don't think requiring consumers to
keep their own cached copy of the state is going to make things clearer
at all. If anything it complicates things for consumers. For example if
we ever want to extend PWM state with an additional field, we would end
up having to modify every single consumer to make sure it copies the
whole state. If we deal with the state in the core like we do right now
we only need to update the core (and perhaps some consumers that really
care about the new field).

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 16835 bytes --]

Hello Thierry,

On Wed, Mar 31, 2021 at 05:52:45PM +0200, Thierry Reding wrote:
> On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> > On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > > Another point is the period: Sven suggested we do not read out the
> > > > > period at all, as the PWM is disabled anyway (see above).
> > > > > Is this acceptable?
> > > > 
> > > > In my eyes consumers should consider the period value as "don't care" if
> > > > the PWM is off. But this doesn't match reality (and maybe also it
> > > > doesn't match Thierry's opinion). See for example the
> > > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > > 
> > > > 	pwm_get_state(mypwm, &state);
> > > > 	state.enabled = true;
> > > > 	pwm_apply_state(pb->pwm, &state);
> > > > 
> > > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > > because currently the .get_state callback has only little to do with
> > > > pwm_get_state(), but you get the point.)
> > > 
> > > The idea behind atomic states in the PWM API is to provide accurate
> > > snapshots of a PWM channel's settings. It's not a representation of
> > > the PWM channel's physical output, although in some cases they may
> > > be the same.
> > 
> > I think the pwm_state returned by .get_state() should be an accurate
> > representation of the PWM channel's physical output.
> 
> Yeah, like I said, in most cases that will be true. However, as
> mentioned below, if there's no 1:1 correspondence between the settings
> and what's actually coming out of the PWM, this isn't always possible.
> But yes, it should always be as accurate as possible.

It should always be true, not only in most cases. For any given PWM
output you can provide a pwm_state that describes this output (unless
you'd need a value bigger than u64 to describe it or a higher precision
as pwm_state only uses integer values). So it's a 1:n relation: You
should always be able to provide a pwm_state and in some cases there are
more than one such states (and you should still provide one of them).

> > > However, given that we want a snapshot of the currently configured
> > > settings, we can't simply assume that there's a 1:1 correspondence and
> > > then use shortcuts to simplify the hardware state representation because
> > > it isn't going to be accurate.
> > 
> > When we assume that pwm_get_state returns the state of the hardware,
> > relying on these values might be too optimistic. Consider a hardware
> > (e.g.  pwm-cros-ec) that has no disable switch. Disabling is modeled by
> > .duty_cycle = 0. So after:
> > 
> > 	pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled = false })
> > 
> > doing:
> > 
> > 	pwm_get_state(pwm, &mystate);
> > 	mystate.enabled = true;
> > 	pwm_apply_state(pwm, &mystate);
> > 
> > the PWM stays configured with .duty_cycle = 0.
> 
> Uhm... no, that's not what should be happening.

Agreed, it shouldn't be happening. Note the paragraph started with "When
we assume that pwm_get_state returns the state of the hardware" and with
that my statement is correct. And so the conclusion is that calculations
by the consumer should always be done based on requested states, not on
actually implemented settings.

> pwm_get_state() copies the PWM channel's internal software state. It
> doesn't actually go and read the hardware state. We only ever read the
> hardware state when the PWM is requested. After that there should be
> no reason to ever read back the hardware state again because the
> consumer owns the state and that state must not change unless
> explicitly requested by the owner.

I have problems to follow your reasoning. What is the semantic of "PWM
channel's internal software state"? What is "currently configured
setting"?

There are two different possible semantics for pwm_get_state():

 a) The state that was passed to pwm_apply_state() before.
 b) What is actually configured in hardware.

Currently the implemented semantic is a). (Apart from the very beginning
when there wasn't anything applied before.) And there is no way for a
consumer to get b). If you only ever want to yield a) there is indeed
no need to read the state from hardware.

> So in the above case, mystate.duty_cycle should be 500 after that call
> to pwm_get_state(). The fact that the hardware can't explicitly disable
> a PWM and needs to emulate that by setting duty-cycle = 0 is a driver
> implementation detail and shouldn't leak to the consumer.
> 
> > [...]
> >
> > So I think calculating the state from the previously implemented state
> > has too many surprises and should not be the recommended way.
> > Consumers should just request what they want and provide this state
> > without relying on .get_state(). And then the needs to cache the
> > duty_cycle for a disabled PWM or reading the period for a disabled PWM
> > just vanish in thin air.
> 
> Again, note that ->get_state() and pwm_get_state() are two different
> things.

I'm aware and interpret your statement here as confirmation that the
function that consumers base their calculations on should always return
the state that was requested before.

> The naming is perhaps a bit unfortunate...

What about renaming pwm_get_state() then, to pwm_get_last_applied_state()?

> But also, the above should never happen because drivers are not supposed
> to modify the software state and the core will never use ->get_state()
> to read back the hardware state.

Agreed. So .get_state() is only ever called at driver load time. (Well,
there is the PWM_DEBUG stuff, but that doesn't leak to the consumer.)

Then I think low level drivers should be allowed to make use of this
fact and drop all caching of data between .apply() and .get_state(), for
example for pwm-cros-ec:

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index c1c337969e4e..fb865f39da9f 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -38,26 +38,6 @@ static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
 	return container_of(c, struct cros_ec_pwm_device, chip);
 }
 
-static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct cros_ec_pwm *channel;
-
-	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
-	if (!channel)
-		return -ENOMEM;
-
-	pwm_set_chip_data(pwm, channel);
-
-	return 0;
-}
-
-static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
-
-	kfree(channel);
-}
-
 static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
 {
 	struct {
@@ -116,7 +96,6 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			     const struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
-	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
 	u16 duty_cycle;
 	int ret;
 
@@ -134,8 +113,6 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (ret < 0)
 		return ret;
 
-	channel->duty_cycle = state->duty_cycle;
-
 	return 0;
 }
 
@@ -143,7 +120,6 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				  struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
-	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
 	int ret;
 
 	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
@@ -154,20 +130,7 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	state->enabled = (ret > 0);
 	state->period = EC_PWM_MAX_DUTY;
-
-	/*
-	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
-	 * the cached duty cycle is not zero, used the cached duty cycle. This
-	 * ensures that the configured duty cycle is kept across a disable and
-	 * enable operation and avoids potentially confusing consumers.
-	 *
-	 * For the case of the initial hardware readout, channel->duty_cycle
-	 * will be 0 and the actual duty cycle read from the EC is used.
-	 */
-	if (ret == 0 && channel->duty_cycle > 0)
-		state->duty_cycle = channel->duty_cycle;
-	else
-		state->duty_cycle = ret;
+	state->duty_cycle = ret;
 }
 
 static struct pwm_device *

> Basically that means that pwm_get_state(), followed by
> pwm_apply_state() called on the same PWM and the same state object
> should be an idempotent operation.

Agreed.

> Well, it's possible for a driver to have to modify the software state to
> more accurately reflect what has been configured to hardware. So the
> pwm_get_state()/pwm_apply_state()/pwm_get_state() sequence may give you
> a different state from the initial state. However it must not be to a
> degree that would make a subsequent pwm_apply_state() change the values
> again.

Now you lost me. If pwm_get_state() has semantic a) (i.e. return the
pwm_state that was passed before to pwm_apply_state()) there is no
deviation necessary or possible. So I don't see how the state could ever
change in the pwm_get_state()/pwm_apply_state()/pwm_get_state()
sequence. Please explain in more detail.

> So I guess the idempotency rule really is more like this: the following
> sequence must always yield the same PWM signal:
> 
> 	pwm_apply_state(pwm, &state);
> 	pwm_get_state(pwm, &state);
> 	pwm_apply_state(pwm, &state);

This is just another idempotency rule.  So there isn't "the" idempotency
rule, in my eyes both should apply. That is:

 1) ("your" idempotency rule) Applying a state returned by pwm_get_state()
    should not modify the output. (Note: True for both semantics a) and b))
 2) ("my" idempotency rule) When a state that was returned by
    .get_state() (i.e. semantic b) only) is applied, .get_state() should
    return the same state afterwards.

> > > It is entirely expected that consumers will be able to use an existing
> > > atomic state, update it and then apply it without necessarily having to
> > > recompute the whole state. So what pwm-backlight is doing is expressly
> > > allowed (and in fact was one specific feature that we wanted to have in
> > > the atomic API).
> > >
> > > Similarly it's a valid use-case to do something like this:
> > > 
> > > 	/* set duty cycle to 50% */
> > > 	pwm_get_state(pwm, &state);
> > > 	state.duty_cycle = state.period / 2;
> > > 	pwm_apply_state(pwm, &state);
> > 
> > The cost to continue doing that is that the consumer has to cache the
> > state. Then the idiom looks as follows:
> > 
> > 	state = &driver_data->state;
> > 	state->duty_cycle = state->period / 2;
> > 	pwm_apply_state(pwm, state);
> 
> Sorry but no. Consumers have no business reaching into driver-data and
> operating on the internal state objects.

I wouldn't call a struct pwm_state driver-data. This is the way to
communicate between driver and consumer and so also with your idiom the
consumer has to use it. But ok, we can continue caching the last
requested pwm_state.

> > which 
> > 
> >  - allows to drop caching in the PWM layer (which is good as it
> >    simplifies the PWM framework and saves some memory for consumers that
> >    don't need caching)
> 
> What exactly is complicated in the PWM framework that would need to be
> simplified. This is really all quite trivial.

Even dropping trivial stuff is nice in my eyes.

> >  - doesn't accumulate rounding errors
> 
> See above, if rounding errors accumulate then something is wrong. This
> shouldn't be happening.
> 
> Now, the above idempotency rule does not rule out rounding that can
> occur due to consumer error.
> 
> So consumers need to be aware that some
> hardware state may not be applied exactly as specified. Reusing too
> much of the state may introduce these rounding errors.

Yes, if you start not to return the last requested state and consumers
make calculations based on that, you indeed get rounding errors. 

> So yes, if you want to toggle between 50% and 75% duty cycles,
> consumers should spell that out explicitly, perhaps by caching the PWM
> args and reusing period from that, for example.

You really confuse me. The obvious way to cache the PWM args is using a
pwm_state, isn't it? So you're saying exactly what I proposed?!

> >  - needs less PWM API calls
> > 
> > Also I think keeping the PWM configuration in the consumer instead of
> > the PWM is the right place, but I assume that's subjective. I don't
> > oppose to caching the previously applied state for consumers, but IMHO
> > we should differentiate between the currently configured state and the
> > previously applied state as there are semantic differences between these
> > two.
> > 
> > Note that even if we somehow normalize the state returned by a driver's
> > .get_state callback (e.g. by setting .duty_cycle = 0 for disabled PWMs)
> > this still matches your expectation that "consumers will be able to use
> > an existing atomic state, update it and then apply without necessarily
> > having to recompute the whole state". The critical part is just that
> > consumers should not start with a pwm_state returned by .get_state() but
> > from the previously requested state.
> 
> Again, see above. pwm_get_state() does not use ->get_state().

Indeed and because of that normalizing the return value of .get_state()
doesn't hurt consumers as (today) they don't get their hands on the
returned value.

> > > which allows a consumer to do simple modifications without actually
> > > knowing what period has been configured. Some consumers just don't care
> > > about the period or don't even have a clue about what a good value would
> > > be (again, pwm-backlight would be one example). For some PWMs it may
> > > also not be possible to modify the period and if there's no explicit
> > > reason to do so, why should consumers be forced to even bother?
> > > 
> > > All of that's out the window if we start taking shortcuts. If the PWM
> > > provider reads out the hardware state's PWM as "don't care", how is the
> > > consumer going to know what value to use? Yes, they can use things like
> > > pwm_get_args() to get the configuration from DT, but then what's the
> > > purpose of using states in the first place if consumers have to do all
> > > the tracking manually anyway?
> > 
> > With my approach the consumers always have to explicitly request a
> > complete setting, but I'd consider this an advantage that makes the
> > mechanisms clearer. Other than that I don't see any disadvantages and
> > the PWM framework can stop pretending things that don't match (the
> > hardware's) reality.
> 
> That's really no different from what's currently happening. Consumers
> always request a full state to be applied. The only difference is that
> some of the values might be cached. But again, that's really a good
> thing. Why should consumers need to have their own copy of PWM state
> if all the want to do is toggle a PWM on and off?

They might trade memory for speed.

	/* set duty cycle to 50% */
	pwm_get_state(pwm, &state);
	state.duty_cycle = state.period / 2;
	pwm_apply_state(pwm, &state);

allows to use a stack variable (and so doesn't occupy memory most of the
time). But you need to initialize if while you could just reuse the
pwm_state that was passed to pwm_apply_state before (without the need to
initialize).

> And this is all very subjective. I don't think requiring consumers to
> keep their own cached copy of the state is going to make things clearer
> at all. If anything it complicates things for consumers. For example if
> we ever want to extend PWM state with an additional field, we would end
> up having to modify every single consumer to make sure it copies the
> whole state. If we deal with the state in the core like we do right now
> we only need to update the core (and perhaps some consumers that really
> care about the new field).

The initial way to get the hands on a valid pwm_state are not any
different for the two idioms. And after that the only difference is
where the pwm_state is cached (in the PWM framework for you and directly
in the consumer for me). Both approaches then modify incrementally and
if we introduce a new member to pwm_state it affects both idioms in the
same way.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 18216 bytes --]

On Tue, Apr 06, 2021 at 08:33:57AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Mar 31, 2021 at 05:52:45PM +0200, Thierry Reding wrote:
> > On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > > > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > > > Another point is the period: Sven suggested we do not read out the
> > > > > > period at all, as the PWM is disabled anyway (see above).
> > > > > > Is this acceptable?
> > > > > 
> > > > > In my eyes consumers should consider the period value as "don't care" if
> > > > > the PWM is off. But this doesn't match reality (and maybe also it
> > > > > doesn't match Thierry's opinion). See for example the
> > > > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > > > 
> > > > > 	pwm_get_state(mypwm, &state);
> > > > > 	state.enabled = true;
> > > > > 	pwm_apply_state(pb->pwm, &state);
> > > > > 
> > > > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > > > because currently the .get_state callback has only little to do with
> > > > > pwm_get_state(), but you get the point.)
> > > > 
> > > > The idea behind atomic states in the PWM API is to provide accurate
> > > > snapshots of a PWM channel's settings. It's not a representation of
> > > > the PWM channel's physical output, although in some cases they may
> > > > be the same.
> > > 
> > > I think the pwm_state returned by .get_state() should be an accurate
> > > representation of the PWM channel's physical output.
> > 
> > Yeah, like I said, in most cases that will be true. However, as
> > mentioned below, if there's no 1:1 correspondence between the settings
> > and what's actually coming out of the PWM, this isn't always possible.
> > But yes, it should always be as accurate as possible.
> 
> It should always be true, not only in most cases. For any given PWM
> output you can provide a pwm_state that describes this output (unless
> you'd need a value bigger than u64 to describe it or a higher precision
> as pwm_state only uses integer values). So it's a 1:n relation: You
> should always be able to provide a pwm_state and in some cases there are
> more than one such states (and you should still provide one of them).

My point is that the correspondence between the pwm_state and what's
programmed to hardware can't always be read back to unambiguously give
the same result. As you mentioned there are these cases where the PWM
channel doesn't have a separate enable bit, in which case it must be
emulated by setting the duty cycle to 0.

In such cases it doesn't make sense to always rely on ->get_state() to
read back the logical state because it just can't. How is it supposed to
know from reading that 0 duty cycle whether that means the PWM is on or
off? So for the initial read-out we can't do any better so we have to
pick one. The easiest would probably be to just assume PWM disabled when
duty cycle is 0 and in most cases that will be just fine as the
resulting PWM will be the same regardless of which of the two options we
pick.

However, what I'm also saying is that once the consumer has called
pwm_apply_state(), there's no reason to read back the value from
hardware and potentially loose information about the state that isn't
contained in hardware. If the consumer has applied this state:

	{
		.period = 100,
		.duty_cycle = 0,
		.polarity = PWM_POLARITY_NORMAL,
		.enabled = true,
	}

then why would we want to have it call ->get_state() and turn this into:

	{
		.period = 100,
		.duty_cycle = 0,
		.polarity = PWM_POLARITY_NORMAL,
		.enabled = false,
	}

? If pwm_apply_state() has properly applied the first state there's no
reason for the consumer to continue using either its own cache or the
PWM framework's cache (via pwm_get_state()) and just toggle the bits as
necessary.

> > > > However, given that we want a snapshot of the currently configured
> > > > settings, we can't simply assume that there's a 1:1 correspondence and
> > > > then use shortcuts to simplify the hardware state representation because
> > > > it isn't going to be accurate.
> > > 
> > > When we assume that pwm_get_state returns the state of the hardware,
> > > relying on these values might be too optimistic. Consider a hardware
> > > (e.g.  pwm-cros-ec) that has no disable switch. Disabling is modeled by
> > > .duty_cycle = 0. So after:
> > > 
> > > 	pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled = false })
> > > 
> > > doing:
> > > 
> > > 	pwm_get_state(pwm, &mystate);
> > > 	mystate.enabled = true;
> > > 	pwm_apply_state(pwm, &mystate);
> > > 
> > > the PWM stays configured with .duty_cycle = 0.
> > 
> > Uhm... no, that's not what should be happening.
> 
> Agreed, it shouldn't be happening. Note the paragraph started with "When
> we assume that pwm_get_state returns the state of the hardware" and with
> that my statement is correct. And so the conclusion is that calculations
> by the consumer should always be done based on requested states, not on
> actually implemented settings.
> 
> > pwm_get_state() copies the PWM channel's internal software state. It
> > doesn't actually go and read the hardware state. We only ever read the
> > hardware state when the PWM is requested. After that there should be
> > no reason to ever read back the hardware state again because the
> > consumer owns the state and that state must not change unless
> > explicitly requested by the owner.
> 
> I have problems to follow your reasoning. What is the semantic of "PWM
> channel's internal software state"? What is "currently configured
> setting"?
> 
> There are two different possible semantics for pwm_get_state():
> 
>  a) The state that was passed to pwm_apply_state() before.
>  b) What is actually configured in hardware.
> 
> Currently the implemented semantic is a). (Apart from the very beginning
> when there wasn't anything applied before.) And there is no way for a
> consumer to get b). If you only ever want to yield a) there is indeed
> no need to read the state from hardware.

b) should only ever be necessary the first time a PWM is used. We
currently do that at request time because then we always guarantee that
the PWM state is up to date for every new consumer.

From a consumer point of view the difference between a) and b) shouldn't
matter. b) is the driver-specific representation of a), taking into
account any of the device's restrictions. So in order to be truly
agnostic of the underlying PWM controller, consumers should only ever
work with a).

Note that there's technically also:

	c) the driver-specific representation of what was passed to
	   pwm_apply_state()

The difference to a) being that it may have adjusted values for period
and duty cycle depending on any scaler restrictions and such. There's
also a difference to b) in that we can have information that the
hardware is not able to contain (such as the difference between duty
cycle = 0 and "off" by the presence of that extra "enabled" field).

> > So in the above case, mystate.duty_cycle should be 500 after that call
> > to pwm_get_state(). The fact that the hardware can't explicitly disable
> > a PWM and needs to emulate that by setting duty-cycle = 0 is a driver
> > implementation detail and shouldn't leak to the consumer.
> > 
> > > [...]
> > >
> > > So I think calculating the state from the previously implemented state
> > > has too many surprises and should not be the recommended way.
> > > Consumers should just request what they want and provide this state
> > > without relying on .get_state(). And then the needs to cache the
> > > duty_cycle for a disabled PWM or reading the period for a disabled PWM
> > > just vanish in thin air.
> > 
> > Again, note that ->get_state() and pwm_get_state() are two different
> > things.
> 
> I'm aware and interpret your statement here as confirmation that the
> function that consumers base their calculations on should always return
> the state that was requested before.

Yes.

> > The naming is perhaps a bit unfortunate...
> 
> What about renaming pwm_get_state() then, to pwm_get_last_applied_state()?

I replied to that patch earlier and I don't think it's worth the churn
and it just makes the API more cumbersome to use. If there's any
confusion we can clarify with better kerneldoc.

> > But also, the above should never happen because drivers are not supposed
> > to modify the software state and the core will never use ->get_state()
> > to read back the hardware state.
> 
> Agreed. So .get_state() is only ever called at driver load time. (Well,
> there is the PWM_DEBUG stuff, but that doesn't leak to the consumer.)

No that's not correct. ->get_state() can also be called when the
consumer changes.

> Then I think low level drivers should be allowed to make use of this
> fact and drop all caching of data between .apply() and .get_state(), for
> example for pwm-cros-ec:

No, I don't think there's a need to remove that code. It's entirely
reasonable to keep this extra information if it helps to more accurately
reflect the hardware state.

Thierry

> 
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index c1c337969e4e..fb865f39da9f 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -38,26 +38,6 @@ static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
>  	return container_of(c, struct cros_ec_pwm_device, chip);
>  }
>  
> -static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct cros_ec_pwm *channel;
> -
> -	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> -	if (!channel)
> -		return -ENOMEM;
> -
> -	pwm_set_chip_data(pwm, channel);
> -
> -	return 0;
> -}
> -
> -static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> -
> -	kfree(channel);
> -}
> -
>  static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
>  {
>  	struct {
> @@ -116,7 +96,6 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     const struct pwm_state *state)
>  {
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> -	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
>  	u16 duty_cycle;
>  	int ret;
>  
> @@ -134,8 +113,6 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (ret < 0)
>  		return ret;
>  
> -	channel->duty_cycle = state->duty_cycle;
> -
>  	return 0;
>  }
>  
> @@ -143,7 +120,6 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  				  struct pwm_state *state)
>  {
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> -	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
>  	int ret;
>  
>  	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> @@ -154,20 +130,7 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	state->enabled = (ret > 0);
>  	state->period = EC_PWM_MAX_DUTY;
> -
> -	/*
> -	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
> -	 * the cached duty cycle is not zero, used the cached duty cycle. This
> -	 * ensures that the configured duty cycle is kept across a disable and
> -	 * enable operation and avoids potentially confusing consumers.
> -	 *
> -	 * For the case of the initial hardware readout, channel->duty_cycle
> -	 * will be 0 and the actual duty cycle read from the EC is used.
> -	 */
> -	if (ret == 0 && channel->duty_cycle > 0)
> -		state->duty_cycle = channel->duty_cycle;
> -	else
> -		state->duty_cycle = ret;
> +	state->duty_cycle = ret;
>  }
>  
>  static struct pwm_device *
> 
> > Basically that means that pwm_get_state(), followed by
> > pwm_apply_state() called on the same PWM and the same state object
> > should be an idempotent operation.
> 
> Agreed.
> 
> > Well, it's possible for a driver to have to modify the software state to
> > more accurately reflect what has been configured to hardware. So the
> > pwm_get_state()/pwm_apply_state()/pwm_get_state() sequence may give you
> > a different state from the initial state. However it must not be to a
> > degree that would make a subsequent pwm_apply_state() change the values
> > again.
> 
> Now you lost me. If pwm_get_state() has semantic a) (i.e. return the
> pwm_state that was passed before to pwm_apply_state()) there is no
> deviation necessary or possible. So I don't see how the state could ever
> change in the pwm_get_state()/pwm_apply_state()/pwm_get_state()
> sequence. Please explain in more detail.

I don't think this is still a bit of a grey area, though admittedly I'm
not sure if there are any drivers that currently do this. I recall that
there had been discussions at some point whether it was a good idea to
let drivers update pwm->state to reflect the state that was actually
programmed. If so, it'd mean that pwm_get_state() could yield a state
that is slightly different from a) (this is basically the c) case that I
described above). However, it represents the exact same settings that
would have been applied with the state that was originally specified.

To pick up the idempotency rule from below, this is what would happen:

	1. pwm_apply_state(pwm, &state);
	2. pwm_get_state(pwm, &state);
	3. pwm_apply_state(pwm, &state);

The driver may have to adjust parameters slightly when applying the
state passed in during step 1. If it chooses to update the internal
state, then the state returned in step 2 may differ from the state
passed in during step 1. However, since it's the driver-specific
representation of the original state, when applying it again in step 3,
the PWM output should be exactly the same as after step 1.

> > So I guess the idempotency rule really is more like this: the following
> > sequence must always yield the same PWM signal:
> > 
> > 	pwm_apply_state(pwm, &state);
> > 	pwm_get_state(pwm, &state);
> > 	pwm_apply_state(pwm, &state);
> 
> This is just another idempotency rule.  So there isn't "the" idempotency
> rule, in my eyes both should apply. That is:
> 
>  1) ("your" idempotency rule) Applying a state returned by pwm_get_state()
>     should not modify the output. (Note: True for both semantics a) and b))
>  2) ("my" idempotency rule) When a state that was returned by
>     .get_state() (i.e. semantic b) only) is applied, .get_state() should
>     return the same state afterwards.

That's exactly what I described above as the first idempotency rule. And
yes, I agree that both of those rules should hold true.

> > > > It is entirely expected that consumers will be able to use an existing
> > > > atomic state, update it and then apply it without necessarily having to
> > > > recompute the whole state. So what pwm-backlight is doing is expressly
> > > > allowed (and in fact was one specific feature that we wanted to have in
> > > > the atomic API).
> > > >
> > > > Similarly it's a valid use-case to do something like this:
> > > > 
> > > > 	/* set duty cycle to 50% */
> > > > 	pwm_get_state(pwm, &state);
> > > > 	state.duty_cycle = state.period / 2;
> > > > 	pwm_apply_state(pwm, &state);
> > > 
> > > The cost to continue doing that is that the consumer has to cache the
> > > state. Then the idiom looks as follows:
> > > 
> > > 	state = &driver_data->state;
> > > 	state->duty_cycle = state->period / 2;
> > > 	pwm_apply_state(pwm, state);
> > 
> > Sorry but no. Consumers have no business reaching into driver-data and
> > operating on the internal state objects.
> 
> I wouldn't call a struct pwm_state driver-data. This is the way to
> communicate between driver and consumer and so also with your idiom the
> consumer has to use it. But ok, we can continue caching the last
> requested pwm_state.

struct pwm_state is not data at all, it's a definition. But once you
embed a struct pwm_state into a driver-specific structure and you
instantiate that data, then the embedded struct pwm_state also becomes
driver-specific data. Your example showed that the consumer was reaching
into driver-specific data to obtain the state and that's what I objected
to.

> > > which 
> > > 
> > >  - allows to drop caching in the PWM layer (which is good as it
> > >    simplifies the PWM framework and saves some memory for consumers that
> > >    don't need caching)
> > 
> > What exactly is complicated in the PWM framework that would need to be
> > simplified. This is really all quite trivial.
> 
> Even dropping trivial stuff is nice in my eyes.
> 
> > >  - doesn't accumulate rounding errors
> > 
> > See above, if rounding errors accumulate then something is wrong. This
> > shouldn't be happening.
> > 
> > Now, the above idempotency rule does not rule out rounding that can
> > occur due to consumer error.
> > 
> > So consumers need to be aware that some
> > hardware state may not be applied exactly as specified. Reusing too
> > much of the state may introduce these rounding errors.
> 
> Yes, if you start not to return the last requested state and consumers
> make calculations based on that, you indeed get rounding errors. 
> 
> > So yes, if you want to toggle between 50% and 75% duty cycles,
> > consumers should spell that out explicitly, perhaps by caching the PWM
> > args and reusing period from that, for example.
> 
> You really confuse me. The obvious way to cache the PWM args is using a
> pwm_state, isn't it? So you're saying exactly what I proposed?!

See the "So yes" confirmation at the beginning of that sentence? Looks
like I did agree with what you were proposing, although, admittedly, I'm
having trouble finding in the backlog what exactly that proposal was.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 20184 bytes --]

Hello Thierry,

On Tue, Apr 06, 2021 at 03:47:15PM +0200, Thierry Reding wrote:
> On Tue, Apr 06, 2021 at 08:33:57AM +0200, Uwe Kleine-König wrote:
> > On Wed, Mar 31, 2021 at 05:52:45PM +0200, Thierry Reding wrote:
> > > On Wed, Mar 31, 2021 at 12:25:43PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Mar 22, 2021 at 09:34:21AM +0100, Thierry Reding wrote:
> > > > > On Mon, Jan 11, 2021 at 09:43:50PM +0100, Uwe Kleine-König wrote:
> > > > > > On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> > > > > > > Another point is the period: Sven suggested we do not read out the
> > > > > > > period at all, as the PWM is disabled anyway (see above).
> > > > > > > Is this acceptable?
> > > > > > 
> > > > > > In my eyes consumers should consider the period value as "don't care" if
> > > > > > the PWM is off. But this doesn't match reality (and maybe also it
> > > > > > doesn't match Thierry's opinion). See for example the
> > > > > > drivers/video/backlight/pwm_bl.c driver which uses the idiom:
> > > > > > 
> > > > > > 	pwm_get_state(mypwm, &state);
> > > > > > 	state.enabled = true;
> > > > > > 	pwm_apply_state(pb->pwm, &state);
> > > > > > 
> > > > > > which breaks if .period is invalid. (OK, this isn't totally accurate
> > > > > > because currently the .get_state callback has only little to do with
> > > > > > pwm_get_state(), but you get the point.)
> > > > > 
> > > > > The idea behind atomic states in the PWM API is to provide accurate
> > > > > snapshots of a PWM channel's settings. It's not a representation of
> > > > > the PWM channel's physical output, although in some cases they may
> > > > > be the same.
> > > > 
> > > > I think the pwm_state returned by .get_state() should be an accurate
> > > > representation of the PWM channel's physical output.
> > > 
> > > Yeah, like I said, in most cases that will be true. However, as
> > > mentioned below, if there's no 1:1 correspondence between the settings
> > > and what's actually coming out of the PWM, this isn't always possible.
> > > But yes, it should always be as accurate as possible.
> > 
> > It should always be true, not only in most cases. For any given PWM
> > output you can provide a pwm_state that describes this output (unless
> > you'd need a value bigger than u64 to describe it or a higher precision
> > as pwm_state only uses integer values). So it's a 1:n relation: You
> > should always be able to provide a pwm_state and in some cases there are
> > more than one such states (and you should still provide one of them).
> 
> My point is that the correspondence between the pwm_state and what's
> programmed to hardware can't always be read back to unambiguously give
> the same result. As you mentioned there are these cases where the PWM
> channel doesn't have a separate enable bit, in which case it must be
> emulated by setting the duty cycle to 0.

OK, I think we agree here.

> In such cases it doesn't make sense to always rely on ->get_state() to
> read back the logical state because it just can't.

If with "logical state" you mean the state that was just requested to be
programmed, then I agree here, too.

> How is it supposed to know from reading that 0 duty cycle whether that
> means the PWM is on or off? So for the initial read-out we can't do
> any better so we have to pick one. The easiest would probably be to
> just assume PWM disabled when duty cycle is 0 and in most cases that
> will be just fine as the resulting PWM will be the same regardless of
> which of the two options we pick.

If the current period is completed before a new setting is applied
returning .enabled = true always is probably the more accurate view. But
details.

> However, what I'm also saying is that once the consumer has called
> pwm_apply_state(), there's no reason to read back the value from
> hardware and potentially loose information about the state that isn't
> contained in hardware.

Yes, as long as the consumer is only interested in their requested
values and not what was actually implemented, that's true.

> If the consumer has applied this state:
> 
> 	{
> 		.period = 100,
> 		.duty_cycle = 0,
> 		.polarity = PWM_POLARITY_NORMAL,
> 		.enabled = true,
> 	}
> 
> then why would we want to have it call ->get_state() and turn this into:
> 
> 	{
> 		.period = 100,
> 		.duty_cycle = 0,
> 		.polarity = PWM_POLARITY_NORMAL,
> 		.enabled = false,
> 	}
> ?

This case is probably indeed uninteresting because the two pwm_states
(nearly) describe the same thing. But the consumer might be interested
if

	{
		.period = 100,
		.duty_cycle = 50,
		.polarity = PWM_POLARITY_NORMAL,
		.enabled = true
	}

is implemented by the hardware as

	{
		.period = 90,
		.duty_cycle = 30,
		.polarity = PWM_POLARITY_NORMAL,
		.enabled = true
	}

because it can only do multiples of 30 ns.

> If pwm_apply_state() has properly applied the first state there's no
> reason for the consumer to continue using either its own cache or the
> PWM framework's cache (via pwm_get_state()) and just toggle the bits as
> necessary.

I fail to understand what you want to say here.

> > > > > However, given that we want a snapshot of the currently configured
> > > > > settings, we can't simply assume that there's a 1:1 correspondence and
> > > > > then use shortcuts to simplify the hardware state representation because
> > > > > it isn't going to be accurate.
> > > > 
> > > > When we assume that pwm_get_state returns the state of the hardware,
> > > > relying on these values might be too optimistic. Consider a hardware
> > > > (e.g.  pwm-cros-ec) that has no disable switch. Disabling is modeled by
> > > > .duty_cycle = 0. So after:
> > > > 
> > > > 	pwm_apply_state(mypwm, { .period = 1000, .duty_cycle = 500, .enabled = false })
> > > > 
> > > > doing:
> > > > 
> > > > 	pwm_get_state(pwm, &mystate);
> > > > 	mystate.enabled = true;
> > > > 	pwm_apply_state(pwm, &mystate);
> > > > 
> > > > the PWM stays configured with .duty_cycle = 0.
> > > 
> > > Uhm... no, that's not what should be happening.
> > 
> > Agreed, it shouldn't be happening. Note the paragraph started with "When
> > we assume that pwm_get_state returns the state of the hardware" and with
> > that my statement is correct. And so the conclusion is that calculations
> > by the consumer should always be done based on requested states, not on
> > actually implemented settings.
> > 
> > > pwm_get_state() copies the PWM channel's internal software state. It
> > > doesn't actually go and read the hardware state. We only ever read the
> > > hardware state when the PWM is requested. After that there should be
> > > no reason to ever read back the hardware state again because the
> > > consumer owns the state and that state must not change unless
> > > explicitly requested by the owner.
> > 
> > I have problems to follow your reasoning. What is the semantic of "PWM
> > channel's internal software state"? What is "currently configured
> > setting"?
> > 
> > There are two different possible semantics for pwm_get_state():
> > 
> >  a) The state that was passed to pwm_apply_state() before.
> >  b) What is actually configured in hardware.
> > 
> > Currently the implemented semantic is a). (Apart from the very beginning
> > when there wasn't anything applied before.) And there is no way for a
> > consumer to get b). If you only ever want to yield a) there is indeed
> > no need to read the state from hardware.
> 
> b) should only ever be necessary the first time a PWM is used. We
> currently do that at request time because then we always guarantee that
> the PWM state is up to date for every new consumer.
> 
> From a consumer point of view the difference between a) and b) shouldn't
> matter.

I think there are cases as above where a consumer might want to know b),
but AFAIK there is no such consumer in the kernel tree. A motor
controller might be such an example.

> b) is the driver-specific representation of a), taking into
> account any of the device's restrictions. So in order to be truly
> agnostic of the underlying PWM controller, consumers should only ever
> work with a).

I don't agree completely here. If a motor control driver wants a 50%
relative duty cycle but the PWM can only provide 47% or 52% it might be
picky about which of the two it prefers. Or consider the case we had
recently here on the list about the pwm-ir-tx.

> Note that there's technically also:
> 
> 	c) the driver-specific representation of what was passed to
> 	   pwm_apply_state()
> 
> The difference to a) being that it may have adjusted values for period
> and duty cycle depending on any scaler restrictions and such. There's
> also a difference to b) in that we can have information that the
> hardware is not able to contain (such as the difference between duty
> cycle = 0 and "off" by the presence of that extra "enabled" field).

If the driver adapted the state (but it doesn't, see commit
71523d1812aca61e32e742e87ec064e3d8c615e1) I'd want that it then yields
the exact same state that .get_state also returns in this situation,
i.e. b). Everything else doesn't make sense to me.

> > > So in the above case, mystate.duty_cycle should be 500 after that call
> > > to pwm_get_state(). The fact that the hardware can't explicitly disable
> > > a PWM and needs to emulate that by setting duty-cycle = 0 is a driver
> > > implementation detail and shouldn't leak to the consumer.
> > > 
> > > > [...]
> > > >
> > > > So I think calculating the state from the previously implemented state
> > > > has too many surprises and should not be the recommended way.
> > > > Consumers should just request what they want and provide this state
> > > > without relying on .get_state(). And then the needs to cache the
> > > > duty_cycle for a disabled PWM or reading the period for a disabled PWM
> > > > just vanish in thin air.
> > > 
> > > Again, note that ->get_state() and pwm_get_state() are two different
> > > things.
> > 
> > I'm aware and interpret your statement here as confirmation that the
> > function that consumers base their calculations on should always return
> > the state that was requested before.
> 
> Yes.
> 
> > > The naming is perhaps a bit unfortunate...
> > 
> > What about renaming pwm_get_state() then, to pwm_get_last_applied_state()?
> 
> I replied to that patch earlier and I don't think it's worth the churn
> and it just makes the API more cumbersome to use. If there's any
> confusion we can clarify with better kerneldoc.

That's one of the remaining points where we don't agree.
 
> > > But also, the above should never happen because drivers are not supposed
> > > to modify the software state and the core will never use ->get_state()
> > > to read back the hardware state.
> > 
> > Agreed. So .get_state() is only ever called at driver load time. (Well,
> > there is the PWM_DEBUG stuff, but that doesn't leak to the consumer.)
> 
> No that's not correct. ->get_state() can also be called when the
> consumer changes.

IMHO that's an inconsistency in the framework because (assuming no other
consumer makes use of the hardware in the break) pwm_get_state() returns
something different than pwm_put(); pwm_get(); pwm_get_state() even
though the hardware wasn't touched.
 
> > Then I think low level drivers should be allowed to make use of this
> > fact and drop all caching of data between .apply() and .get_state(), for
> > example for pwm-cros-ec:
> 
> No, I don't think there's a need to remove that code. It's entirely
> reasonable to keep this extra information if it helps to more accurately
> reflect the hardware state.

But it doesn't. It just reflects a software property, and to increase the
inconsistency the driver cached duty cycle isn't reset by pwm_put();
pwm_get();. In my eyes it's a workaround in the wrong layer if really a
consumer depends on getting back the last set duty_cycle for a disabled
PWM. The drivers should be as simple as possible while still providing
the full capabilities of the device. And caching a certain value that
doesn't matter for the operation of the hardware doesn't belong there.

Iff there are really consumers that depend on such a behaviour, then
(and only then) the framework should do something like:

	pwm_get_state_hw(pwm):
	    state = chip->ops->get_state()

	    if state.enabled == false:
	        # The duty_cycle doesn't matter for the semantic of the
		# returned state. So for convenience set it to the last
		# requested duty_cycle
		state.duty_cycle = pwm->state.duty_cycle

But my preference is that no user of pwm_get_state_hw() should take the
duty_cycle of a disabled PWM into account for any decision.

> > > Basically that means that pwm_get_state(), followed by
> > > pwm_apply_state() called on the same PWM and the same state object
> > > should be an idempotent operation.
> > 
> > Agreed.
> > 
> > > Well, it's possible for a driver to have to modify the software state to
> > > more accurately reflect what has been configured to hardware. So the
> > > pwm_get_state()/pwm_apply_state()/pwm_get_state() sequence may give you
> > > a different state from the initial state. However it must not be to a
> > > degree that would make a subsequent pwm_apply_state() change the values
> > > again.
> > 
> > Now you lost me. If pwm_get_state() has semantic a) (i.e. return the
> > pwm_state that was passed before to pwm_apply_state()) there is no
> > deviation necessary or possible. So I don't see how the state could ever
> > change in the pwm_get_state()/pwm_apply_state()/pwm_get_state()
> > sequence. Please explain in more detail.
> 
> I don't think this is still a bit of a grey area, though admittedly I'm
> not sure if there are any drivers that currently do this. I recall that
> there had been discussions at some point whether it was a good idea to
> let drivers update pwm->state to reflect the state that was actually
> programmed. If so, it'd mean that pwm_get_state() could yield a state
> that is slightly different from a) (this is basically the c) case that I
> described above). However, it represents the exact same settings that
> would have been applied with the state that was originally specified.

That doesn't make sense to me. So you think a driver should feed back
that it implemented .enabled = false using .duty_cycle = 0, but not that
the requested .enabled = true; .period = 100; was implemented with
.period = 90?

> > > So I guess the idempotency rule really is more like this: the following
> > > sequence must always yield the same PWM signal:
> > > 
> > > 	pwm_apply_state(pwm, &state);
> > > 	pwm_get_state(pwm, &state);
> > > 	pwm_apply_state(pwm, &state);
> > 
> > This is just another idempotency rule.  So there isn't "the" idempotency
> > rule, in my eyes both should apply. That is:
> > 
> >  1) ("your" idempotency rule) Applying a state returned by pwm_get_state()
> >     should not modify the output. (Note: True for both semantics a) and b))
> >  2) ("my" idempotency rule) When a state that was returned by
> >     .get_state() (i.e. semantic b) only) is applied, .get_state() should
> >     return the same state afterwards.
> 
> That's exactly what I described above as the first idempotency rule. And
> yes, I agree that both of those rules should hold true.

\o/

> > > > > It is entirely expected that consumers will be able to use an existing
> > > > > atomic state, update it and then apply it without necessarily having to
> > > > > recompute the whole state. So what pwm-backlight is doing is expressly
> > > > > allowed (and in fact was one specific feature that we wanted to have in
> > > > > the atomic API).
> > > > >
> > > > > Similarly it's a valid use-case to do something like this:
> > > > > 
> > > > > 	/* set duty cycle to 50% */
> > > > > 	pwm_get_state(pwm, &state);
> > > > > 	state.duty_cycle = state.period / 2;
> > > > > 	pwm_apply_state(pwm, &state);
> > > > 
> > > > The cost to continue doing that is that the consumer has to cache the
> > > > state. Then the idiom looks as follows:
> > > > 
> > > > 	state = &driver_data->state;
> > > > 	state->duty_cycle = state->period / 2;
> > > > 	pwm_apply_state(pwm, state);
> > > 
> > > Sorry but no. Consumers have no business reaching into driver-data and
> > > operating on the internal state objects.

Maybe you understood from my example that driver_data is the driver data
of the PWM lowlevel driver (or the PWM framework)? That would explain
your reaction, but I meant consumer driver data, i.e.
&driver_data->state is the pwm_state cached by the consumer.

> > I wouldn't call a struct pwm_state driver-data. This is the way to
> > communicate between driver and consumer and so also with your idiom the
> > consumer has to use it. But ok, we can continue caching the last
> > requested pwm_state.
> 
> struct pwm_state is not data at all, it's a definition.

You mean that struct pwm_state is (only) the format of some data, right?

> But once you embed a struct pwm_state into a driver-specific structure
> and you instantiate that data, then the embedded struct pwm_state also
> becomes driver-specific data. Your example showed that the consumer
> was reaching into driver-specific data to obtain the state and that's
> what I objected to.

There is a misunderstanding here somewhere.

I'll try to describe how I understood your approach:

  To program the PWM for the first time, the consumer does:

  	struct pwm_state mystate;

	...

	pwm_init_state(ddata->pwm, &mystate);

	mystate.duty_cycle = mystate.period / 2;
	mystate.enabled = true;

	pwm_apply_state(ddata->pwm, &mystate);

  and for further updates it does:

  	struct pwm_state mystate;

	pwm_get_state(ddata->pwm, &mystate);

	pwm_set_relative_duty_cycle(&mystate, intensity, 100);

	pwm_apply_state(ddata->pwm, &mystate);

With pwm_get_state() returning the state that was applied before, this
is semantically identically to my approach:

  To program the PWM for the first time, the consumer does:

	pwm_init_state(ddata->pwm, &ddata->pwmstate);

	ddata->pwmstate.duty_cycle = ddata->pwmstate.period / 2;
	ddata->pwmstate.enabled = true;

	pwm_apply_state(ddata->pwm, ddata->pwmstate);

  and for further updates it does:

	pwm_set_relative_duty_cycle(ddata->pwmstate, intensity, 100);

	pwm_apply_state(ddata->pwm, ddata->pwmstate);

. Would you consider ddata->pwmstate driver-specific for the consumer
driver? It doesn't contain any data that isn't contained in pwm->state
when using your approach. And both approaches break (or don't break) in
the same way when we decide to add a .color member to struct pwm_state.

IMHO the only difference is that for your approach you need one PWM call
more and there is more data movement involved in return to smaller
(consumer) driver data.

> > > So yes, if you want to toggle between 50% and 75% duty cycles,
> > > consumers should spell that out explicitly, perhaps by caching the PWM
> > > args and reusing period from that, for example.
> > 
> > You really confuse me. The obvious way to cache the PWM args is using a
> > pwm_state, isn't it? So you're saying exactly what I proposed?!
> 
> See the "So yes" confirmation at the beginning of that sentence? Looks
> like I did agree with what you were proposing, although, admittedly, I'm
> having trouble finding in the backlog what exactly that proposal was.

What you did agree with was that caching the pwm state in the consumer
driver is necessary/sensible. Together with the fact(?) that the most
effective way to store this is a struct pwm_state you agreed to my idea
that you just opposed to in the paragraphs before. That's what confuses
me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v5 2/7] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Implements .get_state to read-out the current hardware state.

The hardware readout may return slightly different values than those
that were set in apply due to the limited range of possible prescale and
counter register values.

Also note that although the datasheet mentions 200 Hz as default
frequency when using the internal 25 MHz oscillator, the calculated
period from the default prescaler register setting of 30 is 5079040ns.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 1b5b5fb93b43..b3398963c0ff 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -331,6 +331,46 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct pca9685 *pca = to_pca(chip);
+	unsigned long long duty;
+	unsigned int val;
+
+	/* Calculate (chip-wide) period from prescale value */
+	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
+			(val + 1);
+
+	/* The (per-channel) polarity is fixed */
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
+		/*
+		 * The "all LEDs" channel does not support HW readout
+		 * Return 0 and disabled for backwards compatibility
+		 */
+		state->duty_cycle = 0;
+		state->enabled = false;
+		return;
+	}
+
+	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
+
+	state->enabled = !!duty;
+	if (!state->enabled) {
+		state->duty_cycle = 0;
+		return;
+	} else if (duty == PCA9685_COUNTER_RANGE) {
+		state->duty_cycle = state->period;
+		return;
+	}
+
+	duty *= state->period;
+	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
+}
+
 static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
@@ -353,6 +393,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static const struct pwm_ops pca9685_pwm_ops = {
 	.apply = pca9685_pwm_apply,
+	.get_state = pca9685_pwm_get_state,
 	.request = pca9685_pwm_request,
 	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,
-- 
2.29.2