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

[Thierry Reding]

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

>  - doesn't accumulate rounding errors

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

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

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

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

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

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

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

Thierry

Attachment: signature.asc
Description: PGP signature