Re: [PATCH v3 4/5] pwm: omap-dmtimer: Do not disable pwm before changing period/duty_cycle

[Thierry Reding]

On Mon, Mar 30, 2020 at 09:16:54PM +0200, Uwe Kleine-KÃnig wrote:
> Hello Thierry,
> 
> On Mon, Mar 30, 2020 at 04:14:36PM +0200, Thierry Reding wrote:
> > On Thu, Mar 12, 2020 at 07:40:42AM +0100, Uwe Kleine-KÃnig wrote:
> > > On Thu, Mar 12, 2020 at 09:52:09AM +0530, Lokesh Vutla wrote:
> > > > Only the Timer control register(TCLR) cannot be updated when the timer
> > > > is running. Registers like Counter register(TCRR), loader register(TLDR),
> > > > match register(TMAR) can be updated when the counter is running. Since
> > > > TCLR is not updated in pwm_omap_dmtimer_config(), do not stop the
> > > > timer for period/duty_cycle update.
> > > 
> > > I'm not sure what is sensible here. Stopping the PWM for a short period
> > > is bad, but maybe emitting a wrong period isn't better. You can however
> > > optimise it if only one of period or duty_cycle changes.
> > > 
> > > @Thierry, what is your position here? I tend to say a short stop is
> > > preferable.
> > 
> > It's not clear to me from the above description how exactly the device
> > behaves, but I suspect that it may latch the values in those registers
> > and only update the actual signal output once a period has finished. I
> > know of a couple of other devices that do that, so it wouldn't be
> > surprising.
> > 
> > Even if that was not the case, I think this is just the kind of thing
> > that we have to live with. Sometimes it just isn't possible to have all
> > supported devices adhere strictly to an API. So I think the best we can
> > do is have an API that loosely defines what's supposed to happen and
> > make a best effort to implement those semantics. If a device deviates
> > slightly from those expectations, we can always cross fingers and hope
> > that things still work. And it looks like they are.
> > 
> > So I think if Lokesh and Tony agree that this is the right thing to do
> > and have verified that things still work after this, that's about as
> > good as it's going to get.
> 
> I'd say this isn't for the platform people to decide. My position here
> is that the PWM drivers should behave as uniform as possible to minimize
> surprises for consumers. And so it's a "PWM decision" that is to be made
> here, not an "omap decision".

I think there's a fine line to be walked here. I agree that we should
aim to have as much consistency between drivers as possible. At the same
time I think we need to be pragmatic. As Lokesh said, the particular use
case here requires this type of on-the-fly adjustment of the PWM period
without stopping and restarting the PWM. It doesn't work otherwise. So
th alternative that you're proposing is to say that we don't support
that use-case, even though it works just fine given this particular
hardware. That's not really an option.

> > I know this is perhaps cheating a little, or turning a blind eye, but I
> > don't know what the alternative would be. Do we want to tell people that
> > a given PWM controller can't be used if it doesn't work according to our
> > expectations? That's hard to argue if that controller works just fine
> > for all known use-cases.
> 
> I'd like have some official policy here which of the alternatives is the
> preferred cheat.
> 
> The situation here is that period and duty_cycle cannot be updated
> atomically. So the two options are:
> 
>  - stop shortly
>  - update with hardware running and maybe emit a broken period

I think we can already support both of those with the existing API. If
a consumer wants to stop the PWM while reconfiguring, they should be
able to do pwm_enable(), pwm_config(), pwm_enable() (or the atomic
equivalent) and for the second case they can just do pwm_config() (or
the atomic equivalent).

Some hardware may actually require the PWM to be disabled before
reconfiguring, so they won't be able to strictly adhere to the second
use-case.

But as discussed above, I don't want to strive for a lowest common
denominator that would preclude some more specific use-cases from
working if the hardware supports it.

So I think we should aim for drivers to implement the semantics as
closely as possible. If the hardware doesn't support some of these
requirements strictly while a particular use-case depends on that, then
that just means that the hardware isn't compatible with that use-case.
Chances are that the system just isn't going to be designed to support
that use-case in the first place if the hardware can't do it.

The sysfs interface is a bit of a special case here because it isn't
possible to know what use-cases people are going to come up with. It's
most likely that they'll try something and if it doesn't work they can
see if a driver patch can improve things. If not, perhaps the hardware
just isn't up to the task and that'll be the end of it.

I haven't yet come across a case where things actually fail because we
are too flexible in what the API permits, so I don't see a need to add
arbitrary restrictions.

> I tend to say "stop shortly" is the better alternative.

That's clearly subjective. In this particular case it's certainly not
the case. If the API had that assumption baked in there'd be no way to
support this use-case, even though hardware evidently supports it.

So I certainly think that there are areas where we need to find common
ground for abstraction, but I think being overly restrictive can make an
API completely useless.

One possible extension that I can imagine would be to introduce some
sort of capability structure that drivers can fill in to describe the
behaviour of the hardware. Drivers like pwm-omap-dmtimer, for example,
could describe that they are able to change the period and/or duty cycle
while the PWM is on. There could be another capability bit that says
that the current period will finish before new settings are applied. Yet
another capability could describe that duty-cycle and period can be
applied atomically. Consumers could then check those capabilities to see
if they match their requirements.

But then again, I think that would just make things overly complicated.
None of the existing consumers need that, so it doesn't seem like there
is much demand for that feature. In practice I suspect most consumers
work fine despite potentially small deviations in how the PWM behaves.

Thierry

Attachment: signature.asc
Description: PGP signature