[PATCHv6 0/3] pwm: imx: support output polarity inversion

[stefan@agner.ch (Stefan Agner)]

Hi,

Thanks for that insight Uwe!

On 2016-09-12 07:04, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Mon, Sep 12, 2016 at 02:45:53PM +0200, Alexandre Belloni wrote:
>> Isn't a properly designed PWM putting a high level on its pin when
>> disabled and configured with inversed polarity ?
> 
> it's not well defined. When trying several times over the years to
> properly define and document it, I didn't manage to agree with Thierry
> what is the right thing to define.
> 
> IMHO it would be sensible to make it explicitly undefined what happens
> when a PMW is disabled. This would simplify drivers from
> 
> 	pwm_config(mypwm, value, period);
> 	if (!value)
> 		pwm_disable(mypwm)
> 	else
> 		pwm_enable(led_dat->pwm);
> 
> to
> 
> 	pwm_config(mypwm, value, period);
> 
> and let the pwm driver disable it's clock (or whatever) when value is 0
> and there are energy saving benefits that don't hurt the expected
> behaviour of the pin. So the hardware specific stuff is handled in the
> hardware specific driver and usage in pwm-consumers is simplified.
> Moreover this also simplifies some pwm drivers because they don't have
> to catch in software the cases where the hardware differs from the
> expectation[1].

That sounds like a sane definition to me and what I would have expected
from the PWM framework. That the pin is not defined after pwm_disable is
totally understandable. It is usually a case which the board designer
anyway needs to take care of (e.g. what is the state right after power
on? If the designer cares about, he will put a pull-up/down in place).

And it seems also Sascha suggested that:
https://lkml.org/lkml/2013/1/4/139

I did not found where Thierry disagreed to that...?

> Looking at drivers/leds/leds-pwm.c it doesn't ensure that each
> pwm_enable is paired by an pwm_disable (e.g. on .remove). Is this a bug?
> With my purposed semantics of .config and .disable this would be much
> easier to fix.

That looks like a bug to me.

> 
> Regarding your question: Yeah, maybe all properly designed PWMs behave
> like you expect. But reality isn't only about properly designed
> hardware, so I wouldn't expect all hardware to behave. The inverse
> property might be software emulated and so on pwm_disable the pin might
> become 0.
> 
> The obvious downside of my suggestion is that this is a change in what
> most people expect (because it was "safe" to call pwm_enable before),
> but the resulting code is simpler and cleaner.
> 
> Today it's a (maybe small) bug, when a pwm consumer calls pwm_config with
> value=0 and doesn't disable it afterwards. IMHO that's a bug in the pwm
> API that pwm_config with value=0 doesn't imply (the wanted effects of)
> pwm_disable.

I don't quite get what you are saying here. What wanted effects of
pwm_disable would you like to move into pwm_config with value=0?

--
Stefan

> 
> Best regards
> Uwe
> 
> [1] This might even be impossible: Consider a PWM that gets 0 (or
> high-z) on hw-disable independent of configured duty or inversion. The
> driver now sees for an inverted pwm: pwm_config(this, 0, 100);
> pwm_disable(this); The driver cannot know if it should continue to drive
> the pin at 1, or if the pwm consumer stopped caring about the pwm and
> disabling the hardware is OK.