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

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

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

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
> > > <clemens.gruber@pqgruber.com> 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

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