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

Just to reiterate my thoughts on this: while the config/enable/disable
split may seem arbitrary for some use-cases (you may argue most
use-cases, and you'd probably be right), I think there's value in having
the additional flexibility to explicitly turn off the PWM. Also, duty
cycle of zero doesn't always mean "off". If your PWM is inverted, then
the duty cycle of zero actually means "on". And that of course also
still depends on other components on your board. You may have an
inverter somewhere, or you may actually be driving a circuit that
expects an inverted PWM.

So saying that duty-cycle of 0 equals disable is simplifying things too
much, in my opinion.

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

I think I've explained this in the past: these are i.MX specific issues
that you need to work around in the driver. It's also mostly orthogonal
to the API issue because consider the case where you want to unload the
PWM driver on your board. Your remove function would need disable all
PWM channels in order to properly clean up the device, but if you do
that, then your backlight would turn fully on, right?

On that note, I'm not sure I understand how this would even work, since
you should have the very same state on boot. I'm assuming here that the
PWM will be off at boot time by default, in which case, given that it's
disabled, it should cause the backlight to turn on. How do you solve
that problem? Or if it's not a problem at boot, why does it become a
problem on PWM disable or driver removal?

What you're currently proposing is unjustified at this point: you're
trying to work around an issue specific to i.MX by changing an API that
has worked fine for everyone else for a decade.

Thierry

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