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

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

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

On Sun, Jan 03, 2021 at 06:04:10PM +0100, Clemens Gruber wrote:
> Hi everyone,
> 
> happy new year, hope you are all well!
> 
> On Thu, Dec 17, 2020 at 12:52:42PM -0500, Sven Van Asbroeck wrote:
> > On Thu, Dec 17, 2020 at 12:43 PM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > > >
> > > > 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.
> > 
> > I'm not sure why we should even read out the period?
> > When a channel is disabled, the period is not externally visible,
> > therefore it's meaningless ?
> > 
> > As far as I can tell, we can use this for .get_state():
> > memset(&pwm->state, 0, sizeof(pwm_state));
> > 
> > >
> > > Thierry, Uwe, what's your take on this?
> 
> I will continue working on this series in the upcoming weeks.
> Feedback on the .get_state issue would be greatly appreciated.
> 
> To summarize:
> Is it OK for a driver to expect the chip->ops->get_state function to be
> only called from the place in pwm core it is currently called from?
> (Namely in pwm_device_request after chip->ops->request)
> 
> If yes, we could always return a 0 duty cycle and disabled state,
> because this is the state we left it in after .probe (and .free).
> 
> However, if in the future, the pwm core adds additional calls to
> chip->ops->get_state in other places, this could lead to problems.

It's not safe in general to assume that this function will be called
only at one specific point. If you implement the function, then it
should do what it says (i.e. read the current hardware state), and not
bother about when it might be called, or guess at the state that the PWM
might be in.

If you can't implement hardware readout, then that's fine (there are
some devices for which no physical way exists to read out the current
hardware state), but it doesn't sound like that's the problem here.

> 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?

No, if the PWM has separate bits for "enable" and "period", then they
should be read separately. The hardware state isn't about representing
what the currently configured output is, it's a representation of what
the current settings of the PWM channel are.

> And, if we never return anything but 0 in .get_state, should it be
> implemented at all?

Yes, not implementing .get_state() at all is better than lying. If you
always return an all-zeros state, you're inevitably going to return the
wrong result at some point in time.

Thierry

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