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

[Thierry Reding <thierry.reding@gmail.com>]

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

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

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

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

    - duty: 0
      period: 100

    - duty: 0
      period: 200

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

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

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

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

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

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

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

Thierry

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