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

[Uwe Kleine-König]

On Thu, Mar 12, 2020 at 01:35:32PM +0530, Lokesh Vutla wrote:
> On 12/03/20 12:10 PM, 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.
> 
> Short stop has side effects especially in the case where 1PPS is generated using
> this PWM. In this case where PWM period is continuously synced with PTP clock,
> cannot expect any breaks in PWM. This doesn't fall in the above limitations as
> well. as duty_cycle is not a worry and only the rising edge is all that matters.
> 
> Also any specific reason why you wanted to stop rather than having the mentioned
> limitation? it is just a corner anyway and doesn't happen all the time.

I'm a bit torn here. Which of the two steps out of line is worse depends
on what is driven by the PWM in question. And also I think ignoring
"just corner cases" is a reliable way into trouble.

The usual PWM contributer (understandably) cares mostly about their own
problem they have to solve. If however you take a step back and care
about the PWM framework as a whole to be capable to solve problems in
general, such that any consumer just has to know that there is a PWM and
start requesting specific settings for their work to get done, it gets
obvious that you want some kind of uniform behaviour of each hardware
driver. And then a short inactive break between two periods is more
common and better understandable than a mixed period.

Also being a corner case that only happens (say) once in 100000 cases
isn't a clear upside. This just results in a machine leaving the development
department, passing the production test and then behave unexpected once
per year in the field.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |