On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
<[email protected]> 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 <[email protected]>
> ---
> 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
>
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
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >
On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber
<[email protected]> 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.
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
> <[email protected]> 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
On Sun, Jan 3, 2021 at 12:04 PM Clemens Gruber
<[email protected]> 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.
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
> > <[email protected]> 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/ |
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/ |
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
> > > <[email protected]> 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
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/ |
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
> > <[email protected]> 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
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
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
> > > <[email protected]> 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
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
> > > > <[email protected]> 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
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/ |
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
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/ |
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
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/ |