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

[Thierry Reding]

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

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

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

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

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

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

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

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

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

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

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

Note that there's technically also:

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

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

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

Yes.

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

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

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

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

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

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

Thierry

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

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

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

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

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

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

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

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

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

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

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

Thierry

Attachment: signature.asc
Description: PGP signature