Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

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

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

On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> (If you have a déjà vu: Yes, I tried to argue this case already in the
> past, it's just that I'm just faced with a new use case that's broken
> with the current state.)
> 
> Currently the idiom
> 
> 	pwm_config(mypwm, value, period);
> 	if (!value)
> 		pwm_disable(mypwm);
> 	else
> 		pwm_enable(mypwm);
> 
> (and it's equivalent:
> 
> 	state.duty_cycle = ...
> 	state.enable = state.duty_cycle ? true : false;
> 	pwm_apply_state(...);
> 
> ) is quite present in the kernel.
> 
> In my eyes this is bad for two reasons:
> 
>  a) if the caller is supposed to call pwm_disable() after configuring the
>     duty cycle to zero, then why doesn't pwm_config() contains this to
>     unburden pwm-API users and so reduce open coding this assumption?

That's because it's not true in all cases. pwm_disable() is not
equivalent to setting the duty cycle to zero. Many users don't care
about the difference, but that doesn't mean it should be assumed to
be always the case.

>  b) it is actually wrong in at least two cases:
>     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
>       and only after the current period is elapsed this is clocked into
>       the hardware. So it is very likely that when pwm_disable() is
>       called the period is still not over, and then the clk is disabled
>       and the period never ends resulting in the pwm pin being frozen in
>       whatever state it happend to be when the clk was stopped.

So that's a driver bug then. By definition, after the call to
pwm_config() completes, the hardware should be in the configured state.
If a device doesn't allow the programming to happen immediately, then it
is up to the driver to ensure it waits long enough for the settings to
have been properly applied.

>     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
>       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
>       puts it to 0. In the current case this implies that the backlight
>       is fully enabled instead of off.

Again, that's a driver bug. pwm_disable() isn't supposed to change the
pin polarity. It's not supposed to change the pin level.

> Both issues are hard (or at least ugly) to fix. The IMHO most sane
> approach is to not let PWM-API-users care about power saving when they
> still care about the output level and update the samantic of
> pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
> toggling, I don't care about the level of the output." in contrast to
> the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
> called before, stop the pin at (maybe inverted) low level (for most pwms
> at least).".

I don't see how you could make that work. In practically all cases a
pwm_disable() would have to be assumed to cause the pin level to become
undefined. That makes it pretty much useless.

> This is in my eyes more clear, removes code duplication and power saving
> can (and should) be implemented in the pwm backend drivers that have the
> domain specific knowledge about the effects of pwm_disable and
> eventually unwanted side effects when duty cycle is 0 and just do the
> corresponding action if its safe. We could even put something like:
> 
> 	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
> 		pwm_disable(pwm);
> 
> in pwm_config to help backend driver authors of "sane" PWMs.

I agree that this is simpler and clearer. However it also significantly
reduces the flexibility of the API, and I'm not sure that it is enough.
Again, yes, for many cases this is sufficient, but it doesn't fully
expose the capabilities of hardware.

Thierry

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