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

[Thierry Reding]

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

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

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

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

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

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

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

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

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

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

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

Thierry

Attachment: signature.asc
Description: PGP signature