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

Message ID 20180806155129.cjcc7okmwtaujf43@pengutronix.de
State Rejected
Headers show
Series
  • RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)
Related show

Commit Message

Uwe Kleine-König Aug. 6, 2018, 3:51 p.m.
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?

 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.

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

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 grepped in the kernel for callers of pwm_disable and adapted them for
this. I didn't verify all changes are correct, so take with a grain of
salt. The only fishy one is drivers/gpu/drm/i915/intel_panel.c in my
eyes, I'm not sure about drivers/pwm/pwm-lpc18xx-sct.c.

See below for my wip changes.

Looking forward to your comments.

Best regards
Uwe

Comments

Thierry Reding Aug. 9, 2018, 11:30 a.m. | #1
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
Uwe Kleine-König Aug. 9, 2018, 3:23 p.m. | #2
Hello Thierry,

On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > (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.

I cannot follow you. You're saying that pwm_disable() is not the same as
duty-cycle := 0. That's different from what I said. I only talked about
pwm_disable() after config(pwm, duty=0, period).

Actually the current semantic of pwm_disable isn't clear to me. Is it:
"Freeze the pin in the current state"? If so, this isn't possible to
implement for some hardware I know. If it's like that, the pin state is
undefined after pwm_disable unless the duty-cycle was 0% or 100%
before, right?

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

Unfortunately there is no way (that I know of at least) to notice when a
period is over, so only an unconditional delay/sleep is the way out
here.
 
> >     - 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.

So the disable callback must look as follows:

	def imx_pwm_disable(pwm):
	    state = pwm_get_state(...)
	    if state.polarity == PWM_POLARITY_INVERSED:
	        return

	    hw_access_to_disable_pwm()

(Alternatively the pin could be configured to gpio, the right level set
and the hw disable unconditionally.)

And actually because for an inverted polarity after setting the
duty_cycle to 100% it is possible to already disable the hardware in the
config callback. And for the users that config for duty=0 but don't call
pwm_disable() the symmetric case could be handled, too, so the config
callback optimally looks like:

	def imx_pwm_config(chip, duty, period):
	    state = pwm_get_state(...)
	    if (duty == 0 and not state.polarity == PWM_POLARITY_INVERSED) ||
	       (duty == period and state.polarity == PWM_POLARITY_INVERSED):
	        hw_access_to_disable_pwm()
	    else:
	        hw_access_to_config(duty, period)

The obvious downsides are:

  a) if pwm_disable() was called because the user doesn't need the pwm to
     toggle any more, it stays enabled.
  b) pwm_disable() after pwm_config() that allows to disable the
     hardware, is a noop

So it would be much saner from the backend implementations POV if the
PWM-API user doesn't call pwm_disable() if he cares about the output
level of the pin. And I cannot imagine any pwm implementation where this
requirement would make the implementation worse.

Unless I miss something currently there is no user-visible effect of
calling pwm_disable after duty was set to 0. So as an API-user I cannot
differentiate between

 - set the output to low; and
 - don't care about the pwm pin state, turn off and save power

All downsides could be fixed if the only way to set the output to low
would be

	pwm_config(pwm, 0, period);

and pwm_disable() would imply to turn off with no guarantees about the pin
state.

This would simplify some drivers but not complicate any.

This would make the API more expressive because it allows to
differentiate between "output low" and "power off".

The only downside I see is that all users must be reviewed and
eventually fixed for the slightly different semantic. Parts of that was
already done in my patch in the previous mail.

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

Right, that's what I want. pwm_disable() should make no guarantees about
the pin level. So only call it if you don't care about the pin state. If
you care, only do

	pwm_config(pwm, duty=0, period);

and let the backend driver save power if it possible already in the
corresponding callback.

That is actually easier and more expressive! Currently the semantic of
pwm_disable() is ambiguous. The two possible meanings are:

   - save power and keep the pin state
   - save power

and the backend driver cannot know which one the caller actually wants.
So the only way is to assume the stronger one and keep the pin state
which results in more power usage for some hardware.

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

Can you show me a use case that isn't possible to express with the
suggested change in semantic?

Best regards
Uwe
Uwe Kleine-König Aug. 20, 2018, 9:52 a.m. | #3
Hello Thierry,

On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > 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.
> 
> Right, that's what I want. pwm_disable() should make no guarantees about
> the pin level. [...]
>
> > 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.
> 
> Can you show me a use case that isn't possible to express with the
> suggested change in semantic?

To get forward here the only missing points are:

 a) Your claim that this simplification looses expressive power.
 b) Actually convert the users (assuming a) can be resolved)

I cannot find a usecase that cannot be handled with the suggested change
in semantic. So can I lure you in explaining what setup you have in
mind?

Best regards
Uwe
Uwe Kleine-König Sept. 4, 2018, 8:37 p.m. | #4
Hello Thierry,

On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > 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.
> > 
> > Right, that's what I want. pwm_disable() should make no guarantees about
> > the pin level. [...]
> >
> > > 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.
> > 
> > Can you show me a use case that isn't possible to express with the
> > suggested change in semantic?
> 
> To get forward here the only missing points are:
> 
>  a) Your claim that this simplification looses expressive power.
>  b) Actually convert the users (assuming a) can be resolved)
> 
> I cannot find a usecase that cannot be handled with the suggested change
> in semantic. So can I lure you in explaining what setup you have in
> mind?

I'd really like to get forward here, so you feedback would be very
welcome. What is the use case you have in mind when writing "it also
significantly reduces the flexibility of the API, and I'm not sure that
it is enough"?

Best regards
Uwe
Uwe Kleine-König Sept. 21, 2018, 4:02 p.m. | #5
Hello Thierry,

On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > 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.
> > > 
> > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > the pin level. [...]
> > >
> > > > 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.
> > > 
> > > Can you show me a use case that isn't possible to express with the
> > > suggested change in semantic?
> > 
> > To get forward here the only missing points are:
> > 
> >  a) Your claim that this simplification looses expressive power.
> >  b) Actually convert the users (assuming a) can be resolved)
> > 
> > I cannot find a usecase that cannot be handled with the suggested change
> > in semantic. So can I lure you in explaining what setup you have in
> > mind?
> 
> I'd really like to get forward here, so you feedback would be very
> welcome. What is the use case you have in mind when writing "it also
> significantly reduces the flexibility of the API, and I'm not sure that
> it is enough"?

I'm a bit irritated that you don't continue to communicate here. On
August 9 I had the impression that we can discuss this topic to an end.
But as you stopped replying we unfortunately cannot :-|

Best regards
Uwe
Uwe Kleine-König Sept. 27, 2018, 6:26 p.m. | #6
Hello Thierry,

On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > > 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.
> > > > 
> > > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > > the pin level. [...]
> > > >
> > > > > 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.
> > > > 
> > > > Can you show me a use case that isn't possible to express with the
> > > > suggested change in semantic?
> > > 
> > > To get forward here the only missing points are:
> > > 
> > >  a) Your claim that this simplification looses expressive power.
> > >  b) Actually convert the users (assuming a) can be resolved)
> > > 
> > > I cannot find a usecase that cannot be handled with the suggested change
> > > in semantic. So can I lure you in explaining what setup you have in
> > > mind?
> > 
> > I'd really like to get forward here, so you feedback would be very
> > welcome. What is the use case you have in mind when writing "it also
> > significantly reduces the flexibility of the API, and I'm not sure that
> > it is enough"?
> 
> I'm a bit irritated that you don't continue to communicate here. On
> August 9 I had the impression that we can discuss this topic to an end.
> But as you stopped replying we unfortunately cannot :-|

Seeing you reply on other topics I wonder what is the problem here. Can
you at least quickly confirm that you received my mails?

Best regards
Uwe
Thierry Reding Sept. 27, 2018, 8:29 p.m. | #7
On Thu, Sep 27, 2018 at 08:26:07PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > > > 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.
> > > > > 
> > > > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > > > the pin level. [...]
> > > > >
> > > > > > 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.
> > > > > 
> > > > > Can you show me a use case that isn't possible to express with the
> > > > > suggested change in semantic?
> > > > 
> > > > To get forward here the only missing points are:
> > > > 
> > > >  a) Your claim that this simplification looses expressive power.
> > > >  b) Actually convert the users (assuming a) can be resolved)
> > > > 
> > > > I cannot find a usecase that cannot be handled with the suggested change
> > > > in semantic. So can I lure you in explaining what setup you have in
> > > > mind?
> > > 
> > > I'd really like to get forward here, so you feedback would be very
> > > welcome. What is the use case you have in mind when writing "it also
> > > significantly reduces the flexibility of the API, and I'm not sure that
> > > it is enough"?
> > 
> > I'm a bit irritated that you don't continue to communicate here. On
> > August 9 I had the impression that we can discuss this topic to an end.
> > But as you stopped replying we unfortunately cannot :-|
> 
> Seeing you reply on other topics I wonder what is the problem here. Can
> you at least quickly confirm that you received my mails?

Confirming that I'm receiving your emails. Sorry, but I haven't found
the necessary time to look at the details here any closer to continue
the discussion in a meaningful way.

I will hopefully get around to it within the next couple of days.

Thierry
Uwe Kleine-König Oct. 8, 2018, 8:02 p.m. | #8
Hello Thierry,

On Thu, Sep 27, 2018 at 10:29:55PM +0200, Thierry Reding wrote:
> On Thu, Sep 27, 2018 at 08:26:07PM +0200, Uwe Kleine-König wrote:
> > On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > > > > the pin level. [...]
> > > > > >
> > > > > > > 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.
> > > > > > 
> > > > > > Can you show me a use case that isn't possible to express with the
> > > > > > suggested change in semantic?
> > > > > 
> > > > > To get forward here the only missing points are:
> > > > > 
> > > > >  a) Your claim that this simplification looses expressive power.
> > > > >  b) Actually convert the users (assuming a) can be resolved)
> > > > > 
> > > > > I cannot find a usecase that cannot be handled with the suggested change
> > > > > in semantic. So can I lure you in explaining what setup you have in
> > > > > mind?
> > > > 
> > > > I'd really like to get forward here, so you feedback would be very
> > > > welcome. What is the use case you have in mind when writing "it also
> > > > significantly reduces the flexibility of the API, and I'm not sure that
> > > > it is enough"?
> > > 
> > > I'm a bit irritated that you don't continue to communicate here. On
> > > August 9 I had the impression that we can discuss this topic to an end.
> > > But as you stopped replying we unfortunately cannot :-|
> > 
> > Seeing you reply on other topics I wonder what is the problem here. Can
> > you at least quickly confirm that you received my mails?
> 
> Confirming that I'm receiving your emails. Sorry, but I haven't found
> the necessary time to look at the details here any closer to continue
> the discussion in a meaningful way.
> 
> I will hopefully get around to it within the next couple of days.

I would really like to continue this discussion before it is swapped out
of my mind again.

Did you find the time to consider my thoughts?

Best regards
Uwe
Thierry Reding Oct. 9, 2018, 7:53 a.m. | #9
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
Uwe Kleine-König Oct. 9, 2018, 9:35 a.m. | #10
Hello Thierry,

On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> 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.

Did you notice you didn't argue against me here? We seem to agree that
calling disabling a PWM after setting the duty cycle to zero is bad.

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

Full ack. So let's fix the users to not call pwm_disable after setting
the duty cycle to zero!

Really, only the driver knows when it's safe to disable the clock while
it's expected to keep the pin at a given state. So better don't expect a
certain state after pwm_disable().

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

I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can
(in a sane way) be mapped by the current PWM API. That's because there
are assumptions in the API that are wrong for i.MX. Still the hardware
allows to implement all use cases. That's a sign that the API is bad and
is in need for generalisation.

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

Right. I cannot fix all hardware problems in software. But with my
suggested change I can at least make it as good as possible in software
without having to resort to ugly hacks.
 
> 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?

Most of the time it's not a problem during boot because the pin is muxed
as input. But when the clock of the i.MX PWM is disabled it actively
drives a zero. And I already had cases where there really was a problem
during boot until the bootloader fixed the stuff in software. For these
cases I need patches ("hacks") that remove calls to pwm_disable in the
backlight driver to at least work around the issue during runtime, which
is the best that can be done in software.
 
> 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.

I don't agree here. The PWM API as it is now would make it necessary
that the imx driver becomes ugly. By changing the semantic of the API

 - the i.MX driver can be implemented nicely;
 - the other drivers for "saner" hardware can stay as they are;
 - the API users can still express all their needs;
 - the API stops having two ways to express the same needs;
 - most API users can be simplified because they can drop
     if (duty == 0) pwm_disable()
 - on i.MX the clock can be disabled when not needed saving some energy;

Regarding you claim that the API worked fine for a decade: I tried
already in 2013 to address this problem. And even independent of that,
that is not a good reason to not improve stuff.

Best regards
Uwe
Thierry Reding Oct. 10, 2018, 12:26 p.m. | #11
On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > 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.
> 
> Did you notice you didn't argue against me here? We seem to agree that
> calling disabling a PWM after setting the duty cycle to zero is bad.

I don't see why you think I agree with that. The only thing I'm saying
is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
same as the PWM being disabled.

> > 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.
> 
> Full ack. So let's fix the users to not call pwm_disable after setting
> the duty cycle to zero!

I don't understand why you come to that conclusion. It seems like we at
least agree on what the problem is and what potential issues there could
be with different setups.

The point that we don't agree with is how to fix this.

> Really, only the driver knows when it's safe to disable the clock while
> it's expected to keep the pin at a given state. So better don't expect a
> certain state after pwm_disable().

Yes, I agree that only the driver knows when it is safe to do so, but I
still fail to understand why we should change the API in order to
accomodate the specifics of a particular driver.

So we already know that currently all users expect that the pin state
after disable will be low (because they previously set duty cycle to 0,
which is the active equivalent of low). Given that stricter definition,
the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
state. I think the fix for that is for the i.MX PWM driver to make sure
the pin state doesn't actually change on ->disable(). If that means the
clock needs to remain on, then that's exactly what the driver should be
implementing.

> > >  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.
> 
> I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can
> (in a sane way) be mapped by the current PWM API. That's because there
> are assumptions in the API that are wrong for i.MX. Still the hardware
> allows to implement all use cases. That's a sign that the API is bad and
> is in need for generalisation.

It's perfectly fine for the driver to make any of the PWM operations a
no-op if that's what it takes to make it conform with the API.

> > 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?
> 
> Right. I cannot fix all hardware problems in software. But with my
> suggested change I can at least make it as good as possible in software
> without having to resort to ugly hacks.
> 
> > 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?
> 
> Most of the time it's not a problem during boot because the pin is muxed
> as input. But when the clock of the i.MX PWM is disabled it actively
> drives a zero. And I already had cases where there really was a problem
> during boot until the bootloader fixed the stuff in software. For these
> cases I need patches ("hacks") that remove calls to pwm_disable in the
> backlight driver to at least work around the issue during runtime, which
> is the best that can be done in software.

Are you aware of this:

	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987

This seems to address exactly the problem that you're describing. The
solution matches what I had expected a fix for this problem to look
like. Can you try it and see if that fixes the issue?

It looks as if that also fixes the shutdown problem, so it's better than
what you're proposing here yet it doesn't require any API change at all
to make it work.

Cc'ing Michal for visibility.

> > 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.
> 
> I don't agree here. The PWM API as it is now would make it necessary
> that the imx driver becomes ugly.

Beauty is very subjective. This doesn't count as an argument. Michal's
proposed solution looks perfectly fine.

> By changing the semantic of the API
> 
>  - the i.MX driver can be implemented nicely;
>  - the other drivers for "saner" hardware can stay as they are;
>  - the API users can still express all their needs;
>  - the API stops having two ways to express the same needs;

Again, duty-cycle = 0 is not the same as setting the state to disabled.
Practically they usually have the same effect, but that's just because
in the typical use-case we don't care about the subtle differences.

>  - most API users can be simplified because they can drop
>      if (duty == 0) pwm_disable()
>  - on i.MX the clock can be disabled when not needed saving some energy;

That's one reason why we have pwm_disable(). It's what you use to save
some energy if you don't need the PWM. Also, you claim that if the clock
is disabled the attached backlight will turn on at full brightness, so I
fail to see how this would work.

Also, you leave out the fact that even after all this work that touches
all users and all PWM drivers you still don't get fully correct
functionality on your devices because when the driver is removed or shut
down you run into the same problem.

> Regarding you claim that the API worked fine for a decade: I tried
> already in 2013 to address this problem. And even independent of that,
> that is not a good reason to not improve stuff.

My claim was that it has worked fine "for everyone else". I know that
this has been an issue on i.MX for a long time and I've done my best to
lay out why I think your proposal is not the right way to fix it. The
proposal from Michal shows that there is another way that properly fixes
the issue, so let's focus on getting that to work for your use-cases.

Thierry
Vokáč Michal Oct. 10, 2018, 1:50 p.m. | #12
Ahoj,
thank you for bringing me to the discussion Thierry.

On 10.10.2018 14:26, Thierry Reding wrote:
> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
>> Hello Thierry,
>>
>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
>>> 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.
>>
>> Did you notice you didn't argue against me here? We seem to agree that
>> calling disabling a PWM after setting the duty cycle to zero is bad.
> 
> I don't see why you think I agree with that. The only thing I'm saying
> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> same as the PWM being disabled.
> 
>>> 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.
>>
>> Full ack. So let's fix the users to not call pwm_disable after setting
>> the duty cycle to zero!
> 
> I don't understand why you come to that conclusion. It seems like we at
> least agree on what the problem is and what potential issues there could
> be with different setups.
> 
> The point that we don't agree with is how to fix this.
> 
>> Really, only the driver knows when it's safe to disable the clock while
>> it's expected to keep the pin at a given state. So better don't expect a
>> certain state after pwm_disable().
> 
> Yes, I agree that only the driver knows when it is safe to do so, but I
> still fail to understand why we should change the API in order to
> accomodate the specifics of a particular driver.
> 
> So we already know that currently all users expect that the pin state
> after disable will be low (because they previously set duty cycle to 0,
> which is the active equivalent of low). Given that stricter definition,
> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> state. I think the fix for that is for the i.MX PWM driver to make sure
> the pin state doesn't actually change on ->disable(). If that means the
> clock needs to remain on, then that's exactly what the driver should be
> implementing.
> 
>>>>   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.
>>
>> I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can
>> (in a sane way) be mapped by the current PWM API. That's because there
>> are assumptions in the API that are wrong for i.MX. Still the hardware
>> allows to implement all use cases. That's a sign that the API is bad and
>> is in need for generalisation.
> 
> It's perfectly fine for the driver to make any of the PWM operations a
> no-op if that's what it takes to make it conform with the API.
> 
>>> 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?
>>
>> Right. I cannot fix all hardware problems in software. But with my
>> suggested change I can at least make it as good as possible in software
>> without having to resort to ugly hacks.
>>
>>> 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?
>>
>> Most of the time it's not a problem during boot because the pin is muxed
>> as input. But when the clock of the i.MX PWM is disabled it actively
>> drives a zero. And I already had cases where there really was a problem
>> during boot until the bootloader fixed the stuff in software. For these
>> cases I need patches ("hacks") that remove calls to pwm_disable in the
>> backlight driver to at least work around the issue during runtime, which
>> is the best that can be done in software.
> 
> Are you aware of this:
> 
> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> 
> This seems to address exactly the problem that you're describing. The
> solution matches what I had expected a fix for this problem to look
> like. Can you try it and see if that fixes the issue?
>
> It looks as if that also fixes the shutdown problem, so it's better than
> what you're proposing here yet it doesn't require any API change at all
> to make it work.
> 
> Cc'ing Michal for visibility
 From my very brief skim of your previous discussion it looks like the
exact same problem. So I think it should hopefully solve Uwe's issue.

I would also like to bring attention to an other series I submitted
recently:

	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=68445

It implements the get_state() function that is call from PWM core.
It allows to read the actual state of the PWM HW at probe time.

So if you enable PWM in bootloader (I mean you configure it to produce
a PWM signal) and you also use it in Linux (just enabling the pwm chip
in DT is enough) you will end up with running PWM in userspace. And you
can read the actual period/duty cycle form sysfs.

And vice versa. If the PWM chip is disabled when Linux is booting,
you will end up with disabled PWM in userspace.

In combination with the RFC series this should produce undistorted
PWM signal from power-up to userspace for normal and inverted PWMs
for both cases: if you want it enabled from bootloader to userspace
or if you want it disabled from bootloader to userspace.

>>> 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.
>>
>> I don't agree here. The PWM API as it is now would make it necessary
>> that the imx driver becomes ugly.
> 
> Beauty is very subjective. This doesn't count as an argument. Michal's
> proposed solution looks perfectly fine.

Good to hear that, thanks ;)

>> By changing the semantic of the API
>>
>>   - the i.MX driver can be implemented nicely;
>>   - the other drivers for "saner" hardware can stay as they are;
>>   - the API users can still express all their needs;
>>   - the API stops having two ways to express the same needs;
> 
> Again, duty-cycle = 0 is not the same as setting the state to disabled.
> Practically they usually have the same effect, but that's just because
> in the typical use-case we don't care about the subtle differences.
> 
>>   - most API users can be simplified because they can drop
>>       if (duty == 0) pwm_disable()
>>   - on i.MX the clock can be disabled when not needed saving some energy;
> 
> That's one reason why we have pwm_disable(). It's what you use to save
> some energy if you don't need the PWM. Also, you claim that if the clock
> is disabled the attached backlight will turn on at full brightness, so I
> fail to see how this would work.
> 
> Also, you leave out the fact that even after all this work that touches
> all users and all PWM drivers you still don't get fully correct
> functionality on your devices because when the driver is removed or shut
> down you run into the same problem.
> 
>> Regarding you claim that the API worked fine for a decade: I tried
>> already in 2013 to address this problem. And even independent of that,
>> that is not a good reason to not improve stuff.
> 
> My claim was that it has worked fine "for everyone else". I know that
> this has been an issue on i.MX for a long time and I've done my best to
> lay out why I think your proposal is not the right way to fix it. The
> proposal from Michal shows that there is another way that properly fixes
> the issue, so let's focus on getting that to work for your use-cases.
> 
> Thierry
>
Uwe Kleine-König Oct. 11, 2018, 10:19 a.m. | #13
Hello Thierry,

On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > > 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.
> > 
> > Did you notice you didn't argue against me here? We seem to agree that
> > calling disabling a PWM after setting the duty cycle to zero is bad.

You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
also don't seem to refuse to change pwm users that do:

	pwm_config(pwm, 0, 100);
	pwm_disable(pwm);

I interpreted your sentence as confirmation that this sequence isn't
correct in general.

Where is the difference you care about between duty=0 and disabled?

> I don't see why you think I agree with that. The only thing I'm saying
> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> same as the PWM being disabled.

What is in your view the current difference for the hardware behind the
PWM?

> > > 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.
> > 
> > Full ack. So let's fix the users to not call pwm_disable after setting
> > the duty cycle to zero!
> 
> I don't understand why you come to that conclusion. It seems like we at
> least agree on what the problem is and what potential issues there could
> be with different setups.
> 
> The point that we don't agree with is how to fix this.

ack.

> > Really, only the driver knows when it's safe to disable the clock while
> > it's expected to keep the pin at a given state. So better don't expect a
> > certain state after pwm_disable().
> 
> Yes, I agree that only the driver knows when it is safe to do so, but I
> still fail to understand why we should change the API in order to
> accomodate the specifics of a particular driver.

The incentive to change the API is a combination of:

 - The restriction to keep the pin driving at a certain level after
   disable isn't a natural and obvious property of the hardware.
   The designers of the imx-pwm for example did it differently.

 - The change removes ambiguity from from the demands on the hardware
   drivers.

 - The change doesn't make the API less expressive.

 - The change doesn't doesn't complicate anmy pwm hardware driver.

 - The change allows to simplify most pwm users.

 - The change simplifies the imx-pwm driver.

In my experience this is enough to justify such a change.

> So we already know that currently all users expect that the pin state
> after disable will be low (because they previously set duty cycle to 0,
> which is the active equivalent of low).

My theory is that most users don't expect this and/or don't make use of
it. As of 9dcd936c5312 we have 50 pwm drivers
($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
and I think there are a few false positive matches, pwm-sun4i.c for
example). I'd be willing to bet that as of now imx isn't the only driver
with this problem.

> Given that stricter definition,
> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> state. I think the fix for that is for the i.MX PWM driver to make sure
> the pin state doesn't actually change on ->disable(). If that means the
> clock needs to remain on, then that's exactly what the driver should be
> implementing.

Yes, if we keep the API "stricter" this is what should be done. This
doesn't justify to keep the API strict though.

> Are you aware of this:
> 
> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> 
> This seems to address exactly the problem that you're describing. The
> solution matches what I had expected a fix for this problem to look
> like. Can you try it and see if that fixes the issue?

Right. Apart from bugs in it (for example
http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
as output) this would solve the issue for sure. This doesn't void my
arguments to relax the PWM API though. Also look at the costs: The patch
adds 86 lines to the imx driver complicating it considerably. Each
machine that wants to profit of this change has to adapt it's device
tree to make the additional pinmuxing and gpio known.
To be fair, my change doesn't fix the gap between pinctrl being active
and the actual enablement of the pwm. But this could be better solved in
the pwm framework without touching any hardware driver with pinctrl
mechanism that are already in place. (Make use of the "init" pinctrl
state and only switch to "default" when the pwm is configured.)
There is nothing imx-pwm specific in hooking a gpio and adapting the
pinmuxing in it. So if you want to go this path, please implement it in
a generic way such that all pwm devices could benefit.

> It looks as if that also fixes the shutdown problem, so it's better than
> what you're proposing here yet it doesn't require any API change at all
> to make it work.

I don't care much about the shutdown problem. (Also, if the imx-pwm
driver is unbound, the gpio is freed, too, so it's only by chance if the
level is kept.)

> > > 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.
> > 
> > I don't agree here. The PWM API as it is now would make it necessary
> > that the imx driver becomes ugly.
> 
> Beauty is very subjective. This doesn't count as an argument. Michal's
> proposed solution looks perfectly fine.

In this case the opposite of ugly is "simple". This is a real argument.
And this is a reason to apply my idea. It simplifies stuff and solves
some problems.

A framework is good if it is general enough to cover all use cases for
its users and demands very little from it's implementors. Each callback
to implement by a hardware driver should be as simple as possible.
Implying "keep the pin level constant" in .disable makes it more
complicated for some drivers. So this is a bad restriction that should
better be dropped if there are no relvant downsides.

Sidenote: With the current capabilities of the pwm framework there is no
technical reason to expose to the hardware drivers that the pwm user uses
inverted logic. The framework could just apply

	duty = period - duty;

before calling the hardware specific .config callback and so allow to
simplify some drivers, reducing complexity to a single place.

> > By changing the semantic of the API
> > 
> >  - the i.MX driver can be implemented nicely;
> >  - the other drivers for "saner" hardware can stay as they are;
> >  - the API users can still express all their needs;
> >  - the API stops having two ways to express the same needs;
> 
> Again, duty-cycle = 0 is not the same as setting the state to disabled.
> 
> Practically they usually have the same effect, but that's just because
> in the typical use-case we don't care about the subtle differences.

I fail to see the subtle difference.

> >  - most API users can be simplified because they can drop
> >      if (duty == 0) pwm_disable()
> >  - on i.MX the clock can be disabled when not needed saving some energy;
> 
> That's one reason why we have pwm_disable(). It's what you use to save
> some energy if you don't need the PWM. Also, you claim that if the clock
> is disabled the attached backlight will turn on at full brightness, so I
> fail to see how this would work.

As long as you care about the backlight to be off, don't disable the
pwm. And if in this state the clk can be disabled, then this should be
done without an additional poke by the hardware driver which is the only
place that can know for sure that it is safe to do so. There is no good
reason to let the pwm user have to know that the currently configured
state might make some energy saving possible and impose the burden on
him to then call pwm_disable().

> Also, you leave out the fact that even after all this work that touches
> all users and all PWM drivers you still don't get fully correct
> functionality on your devices because when the driver is removed or shut
> down you run into the same problem.

It's ok for me that all bets are off when the driver is removed. I'm
happy when I fix the problem during active operation because on the
machine I currently care about the shutdown problem isn't relevant.
(Also as noted above, even Michal's patch doesn't solve the problem
formally.)

> > Regarding you claim that the API worked fine for a decade: I tried
> > already in 2013 to address this problem. And even independent of that,
> > that is not a good reason to not improve stuff.
> 
> My claim was that it has worked fine "for everyone else". I know that
> this has been an issue on i.MX for a long time and I've done my best to
> lay out why I think your proposal is not the right way to fix it.

I still fail to see a single technical reason to not evolve the API. I
read from your mails:

 - it has been like that for ages
 - it works for everyone but i.MX
 - it could be made working for i.MX by involving pinctrl and gpio in
   the hardware specific pwm driver.

Did I miss anything? None of these justifies to keep the status quo in
my eyes.

Best regards
Uwe
Thierry Reding Oct. 11, 2018, noon | #14
On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > > > 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.
> > > 
> > > Did you notice you didn't argue against me here? We seem to agree that
> > > calling disabling a PWM after setting the duty cycle to zero is bad.
> 
> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
> also don't seem to refuse to change pwm users that do:
> 
> 	pwm_config(pwm, 0, 100);
> 	pwm_disable(pwm);
> 
> I interpreted your sentence as confirmation that this sequence isn't
> correct in general.

Yes, it is not correct in general, though it is correct in many cases.

> Where is the difference you care about between duty=0 and disabled?

This is described in the kerneldoc of the corresponding functions. In the
case of pwm_enable() it starts the output toggling, whereas pwm_enable()
disables the output toggling. So strictly by going according to the PWM
API a driver should only need to call pwm_config() if the duty cycle
actually changes. If all you do is disable and enable, then pwm_enable()
and pwm_disable() should be good enough. The implicit assumption is that
the configuration will persist across pwm_disable()/pwm_enable(). In
practice this doesn't matter that much because most users will be
setting the duty cycle and enable/disable at the same time. I can easily
imagine other use-cases where you would want to make the different. One
other difference could be that pwm_enable() and pwm_disable() require a
more involved sequence of operations than pwm_config() or vice versa, in
which cases you may want to avoid one or the other if possible.

In addition, we do expose these details to userspace via sysfs, so not
allowing an explicit disable after config(duty=0) would break that ABI.

> > I don't see why you think I agree with that. The only thing I'm saying
> > is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> > same as the PWM being disabled.
> 
> What is in your view the current difference for the hardware behind the
> PWM?

It depends on the driver implementation, but the kerneldoc is pretty
specific. I many cases I would expect pwm_disable() to allow a bit of
extra power to be saved because clocks could be turned off.

> > > > 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.
> > > 
> > > Full ack. So let's fix the users to not call pwm_disable after setting
> > > the duty cycle to zero!
> > 
> > I don't understand why you come to that conclusion. It seems like we at
> > least agree on what the problem is and what potential issues there could
> > be with different setups.
> > 
> > The point that we don't agree with is how to fix this.
> 
> ack.
> 
> > > Really, only the driver knows when it's safe to disable the clock while
> > > it's expected to keep the pin at a given state. So better don't expect a
> > > certain state after pwm_disable().
> > 
> > Yes, I agree that only the driver knows when it is safe to do so, but I
> > still fail to understand why we should change the API in order to
> > accomodate the specifics of a particular driver.
> 
> The incentive to change the API is a combination of:
> 
>  - The restriction to keep the pin driving at a certain level after
>    disable isn't a natural and obvious property of the hardware.
>    The designers of the imx-pwm for example did it differently.

I would disagree. A pin that you can't keep at a defined level if it
isn't actively driven is a fundamentally useless concept. You said
yourself that the hardware design is problematic because it causes the
pin to change unexpectedly when you disable the PWM. One could argue
that that's a bug in the hardware. However, given that there is a way to
avoid the unexpected change, there is a mechanism outside of PWM to deal
with this use-case. And that's also perfectly fine. Dealing with pins
that aren't actively driven is part of the job of a pinmux controller.

>  - The change removes ambiguity from from the demands on the hardware
>    drivers.

I don't think there's currently any ambiguity. We may not be stating
explicitly that the assumption is for a PWM to be "off" after
pwm_disable(), but it's certainly how everyone has interpreted it. Also
if there's an ambiguity, then that's what should be addressed.

>  - The change doesn't make the API less expressive.

Yes it does. You remove the distinction between duty cycle == 0 and
the PWM being disabled.

>  - The change doesn't doesn't complicate anmy pwm hardware driver.

True. It also doesn't simplify any PWM hardware driver.

>  - The change allows to simplify most pwm users.

Yes, the subset that doesn't care about the difference is marginally
simplified. Although if the API was more rigorous about the definition
of pwm_disable(), then PWM users would be even more simplified.

>  - The change simplifies the imx-pwm driver.

True. But you leave out that the change doesn't actually fix your
problem. If your PWM user goes away, it will still want to disable the
PWM because it is no longer in use. However, you don't want that to
mean that the backlight goes to full brightness again. If you unbind
the backlight driver, the backlight should remain off (or turn off if
it isn't already).

> In my experience this is enough to justify such a change.

I would consider the change if it was actually going to fix the issue
that you're seeing. As it is, all your proposal does is paper over one
specific symptom, and I'm sorry, but that's just not good enough.

> > So we already know that currently all users expect that the pin state
> > after disable will be low (because they previously set duty cycle to 0,
> > which is the active equivalent of low).
> 
> My theory is that most users don't expect this and/or don't make use of
> it. As of 9dcd936c5312 we have 50 pwm drivers
> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
> and I think there are a few false positive matches, pwm-sun4i.c for
> example). I'd be willing to bet that as of now imx isn't the only driver
> with this problem.

We can speculate about that as much as we want. i.MX is the only one
that this issue has been reported against, so I'm going to have to
assume that it's working fine for all of the others. If it isn't then
people should come forward and report it.

> > Given that stricter definition,
> > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> > state. I think the fix for that is for the i.MX PWM driver to make sure
> > the pin state doesn't actually change on ->disable(). If that means the
> > clock needs to remain on, then that's exactly what the driver should be
> > implementing.
> 
> Yes, if we keep the API "stricter" this is what should be done. This
> doesn't justify to keep the API strict though.

But an API that isn't strict is useless. If the behaviour of a PWM isn't
predictable, how can you use it to drive a backlight. If at any point it
could just go 100% and turn on the backlight to full brightness, how can
anyone use it for anything sensible?

> > Are you aware of this:
> > 
> > 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> > 
> > This seems to address exactly the problem that you're describing. The
> > solution matches what I had expected a fix for this problem to look
> > like. Can you try it and see if that fixes the issue?
> 
> Right. Apart from bugs in it (for example
> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> as output) this would solve the issue for sure. This doesn't void my
> arguments to relax the PWM API though. Also look at the costs: The patch
> adds 86 lines to the imx driver complicating it considerably.

Now you're just being dramatic. 86 lines of code is not a lot. And I
don't think it's complicated. I'm willing to carry the burden of those
86 lines of code if it fixes the issue for good.

>                                                               Each
> machine that wants to profit of this change has to adapt it's device
> tree to make the additional pinmuxing and gpio known.
> To be fair, my change doesn't fix the gap between pinctrl being active
> and the actual enablement of the pwm. But this could be better solved in
> the pwm framework without touching any hardware driver with pinctrl
> mechanism that are already in place. (Make use of the "init" pinctrl
> state and only switch to "default" when the pwm is configured.)
> There is nothing imx-pwm specific in hooking a gpio and adapting the
> pinmuxing in it. So if you want to go this path, please implement it in
> a generic way such that all pwm devices could benefit.

You're wrong. Judging by all the evidence that we have at this point,
this is exactly an imx-pwm specific issue, therefore the burden is on
the imx-pwm driver to implement a solution for it. If there ever is a
second driver that has this issue we can revisit moving this into the
core. I don't see any reason to do so at this point in time with the
information that we have.

> > It looks as if that also fixes the shutdown problem, so it's better than
> > what you're proposing here yet it doesn't require any API change at all
> > to make it work.
> 
> I don't care much about the shutdown problem. (Also, if the imx-pwm
> driver is unbound, the gpio is freed, too, so it's only by chance if the
> level is kept.)

Who else do you think would use the GPIO and change the level? If you've
got multiple concurrent users of the pin that's a whole other problem. I
also don't see why you wouldn't want to care about the shutdown problem.
It's annoying if you shut down the device and all of a sudden your
backlight comes back up again, perhaps in the middle of the display
controller shutting down and producing garbage on the screen.

> 
> > > > 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.
> > > 
> > > I don't agree here. The PWM API as it is now would make it necessary
> > > that the imx driver becomes ugly.
> > 
> > Beauty is very subjective. This doesn't count as an argument. Michal's
> > proposed solution looks perfectly fine.
> 
> In this case the opposite of ugly is "simple". This is a real argument.
> And this is a reason to apply my idea. It simplifies stuff and solves
> some problems.
> 
> A framework is good if it is general enough to cover all use cases for
> its users and demands very little from it's implementors. Each callback
> to implement by a hardware driver should be as simple as possible.
> Implying "keep the pin level constant" in .disable makes it more
> complicated for some drivers. So this is a bad restriction that should
> better be dropped if there are no relvant downsides.

Okay, so I'm not suggesting that pwm_disable() keep the pin level
constant. What I was proposing is that it be required to set the pin
level to "off" (whatever that means). And the reason for doing that is
because there are relevant downsides if we leave this undefined, as I
described at length above.

> Sidenote: With the current capabilities of the pwm framework there is no
> technical reason to expose to the hardware drivers that the pwm user uses
> inverted logic. The framework could just apply
> 
> 	duty = period - duty;
> 
> before calling the hardware specific .config callback and so allow to
> simplify some drivers, reducing complexity to a single place.

No, you're wrong. If you only consider the case of a backlight or an
LED, then yes, you could simplify it that way. However there are other
use-cases that require more fine-grained control and in those cases the
actual edges of the PWM are important.

Also, this has been discussed before and I have said elsewhere that I
have no objection to adding a helper that would simplify this for
consumers that don't care about the difference.

> > > By changing the semantic of the API
> > > 
> > >  - the i.MX driver can be implemented nicely;
> > >  - the other drivers for "saner" hardware can stay as they are;
> > >  - the API users can still express all their needs;
> > >  - the API stops having two ways to express the same needs;
> > 
> > Again, duty-cycle = 0 is not the same as setting the state to disabled.
> > 
> > Practically they usually have the same effect, but that's just because
> > in the typical use-case we don't care about the subtle differences.
> 
> I fail to see the subtle difference.

That's okay. I've tried my best to describe it to you in different ways.
If I still haven't been able to convey this, I don't know what else to
say.

> > >  - most API users can be simplified because they can drop
> > >      if (duty == 0) pwm_disable()
> > >  - on i.MX the clock can be disabled when not needed saving some energy;
> > 
> > That's one reason why we have pwm_disable(). It's what you use to save
> > some energy if you don't need the PWM. Also, you claim that if the clock
> > is disabled the attached backlight will turn on at full brightness, so I
> > fail to see how this would work.
> 
> As long as you care about the backlight to be off, don't disable the
> pwm. And if in this state the clk can be disabled, then this should be
> done without an additional poke by the hardware driver which is the only
> place that can know for sure that it is safe to do so. There is no good
> reason to let the pwm user have to know that the currently configured
> state might make some energy saving possible and impose the burden on
> him to then call pwm_disable().

This doesn't have anything to do with consumers knowing about potential
energy savings. All we do is give the consumers an opportunity to say: I
need the PWM to be enabled because I want to use the PWM signal now or I
don't really need the PWM signal any more, just stop sending a signal.
This is merely a hint from the consumer. What the driver does with it is
ultimately up to the driver. If the driver wants to disable the clock
when the duty cycle goes to zero because it knows that it is safe to do
so, then by all means, feel free to do that.

> > Also, you leave out the fact that even after all this work that touches
> > all users and all PWM drivers you still don't get fully correct
> > functionality on your devices because when the driver is removed or shut
> > down you run into the same problem.
> 
> It's ok for me that all bets are off when the driver is removed.

Well, it's not okay for me. Especially if we can do better.

>                                                                  I'm
> happy when I fix the problem during active operation because on the
> machine I currently care about the shutdown problem isn't relevant.

Why is it not relevant? Surely at some point you will want to shut down
that machine. And even if not, imx-pwm is used by others and they do
seem to care about shutdown, so they should have a say in the matter as
well.

> (Also as noted above, even Michal's patch doesn't solve the problem
> formally.)

Now I'm confused, you said above that "Apart from bugs in it ... this
would solve the issue for sure". So which one is it?

> > > Regarding you claim that the API worked fine for a decade: I tried
> > > already in 2013 to address this problem. And even independent of that,
> > > that is not a good reason to not improve stuff.
> > 
> > My claim was that it has worked fine "for everyone else". I know that
> > this has been an issue on i.MX for a long time and I've done my best to
> > lay out why I think your proposal is not the right way to fix it.
> 
> I still fail to see a single technical reason to not evolve the API. I
> read from your mails:
> 
>  - it has been like that for ages
>  - it works for everyone but i.MX
>  - it could be made working for i.MX by involving pinctrl and gpio in
>    the hardware specific pwm driver.
> 
> Did I miss anything? None of these justifies to keep the status quo in
> my eyes.

I don't understand your logic here. You want to change the API, so the
burden is on you to convince me *why* it needs to be changed. So far you
have not been able to do that, so don't try to turn this around and make
me come up with reasons why it shouldn't change.

Thierry
Vokáč Michal Oct. 11, 2018, 1:15 p.m. | #15
Uwe, Thierry,
just a few comments as I am not able to dive deeper into your discussion.

On 11.10.2018 14:00, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
>> Hello Thierry,
>>
>> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
>>> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
>>>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
>>>>> 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.
>>>>
>>>> Did you notice you didn't argue against me here? We seem to agree that
>>>> calling disabling a PWM after setting the duty cycle to zero is bad.
>>
>> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
>> also don't seem to refuse to change pwm users that do:
>>
>> 	pwm_config(pwm, 0, 100);
>> 	pwm_disable(pwm);
>>
>> I interpreted your sentence as confirmation that this sequence isn't
>> correct in general.
> 
> Yes, it is not correct in general, though it is correct in many cases.
> 
>> Where is the difference you care about between duty=0 and disabled?
> 
> This is described in the kerneldoc of the corresponding functions. In the
> case of pwm_enable() it starts the output toggling, whereas pwm_enable()
> disables the output toggling. So strictly by going according to the PWM
> API a driver should only need to call pwm_config() if the duty cycle
> actually changes. If all you do is disable and enable, then pwm_enable()
> and pwm_disable() should be good enough. The implicit assumption is that
> the configuration will persist across pwm_disable()/pwm_enable(). In
> practice this doesn't matter that much because most users will be
> setting the duty cycle and enable/disable at the same time. I can easily
> imagine other use-cases where you would want to make the different. One
> other difference could be that pwm_enable() and pwm_disable() require a
> more involved sequence of operations than pwm_config() or vice versa, in
> which cases you may want to avoid one or the other if possible.
> 
> In addition, we do expose these details to userspace via sysfs, so not
> allowing an explicit disable after config(duty=0) would break that ABI.
> 
>>> I don't see why you think I agree with that. The only thing I'm saying
>>> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
>>> same as the PWM being disabled.
>>
>> What is in your view the current difference for the hardware behind the
>> PWM?
> 
> It depends on the driver implementation, but the kerneldoc is pretty
> specific. I many cases I would expect pwm_disable() to allow a bit of
> extra power to be saved because clocks could be turned off.
> 
>>>>> 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.
>>>>
>>>> Full ack. So let's fix the users to not call pwm_disable after setting
>>>> the duty cycle to zero!
>>>
>>> I don't understand why you come to that conclusion. It seems like we at
>>> least agree on what the problem is and what potential issues there could
>>> be with different setups.
>>>
>>> The point that we don't agree with is how to fix this.
>>
>> ack.
>>
>>>> Really, only the driver knows when it's safe to disable the clock while
>>>> it's expected to keep the pin at a given state. So better don't expect a
>>>> certain state after pwm_disable().
>>>
>>> Yes, I agree that only the driver knows when it is safe to do so, but I
>>> still fail to understand why we should change the API in order to
>>> accomodate the specifics of a particular driver.
>>
>> The incentive to change the API is a combination of:
>>
>>   - The restriction to keep the pin driving at a certain level after
>>     disable isn't a natural and obvious property of the hardware.
>>     The designers of the imx-pwm for example did it differently.
> 
> I would disagree. A pin that you can't keep at a defined level if it
> isn't actively driven is a fundamentally useless concept. You said
> yourself that the hardware design is problematic because it causes the
> pin to change unexpectedly when you disable the PWM. One could argue
> that that's a bug in the hardware. However, given that there is a way to
> avoid the unexpected change, there is a mechanism outside of PWM to deal
> with this use-case. And that's also perfectly fine. Dealing with pins
> that aren't actively driven is part of the job of a pinmux controller.
> 
>>   - The change removes ambiguity from from the demands on the hardware
>>     drivers.
> 
> I don't think there's currently any ambiguity. We may not be stating
> explicitly that the assumption is for a PWM to be "off" after
> pwm_disable(), but it's certainly how everyone has interpreted it. Also
> if there's an ambiguity, then that's what should be addressed.
> 
>>   - The change doesn't make the API less expressive.
> 
> Yes it does. You remove the distinction between duty cycle == 0 and
> the PWM being disabled.
> 
>>   - The change doesn't doesn't complicate anmy pwm hardware driver.
> 
> True. It also doesn't simplify any PWM hardware driver.
> 
>>   - The change allows to simplify most pwm users.
> 
> Yes, the subset that doesn't care about the difference is marginally
> simplified. Although if the API was more rigorous about the definition
> of pwm_disable(), then PWM users would be even more simplified.
> 
>>   - The change simplifies the imx-pwm driver.
> 
> True. But you leave out that the change doesn't actually fix your
> problem. If your PWM user goes away, it will still want to disable the
> PWM because it is no longer in use. However, you don't want that to
> mean that the backlight goes to full brightness again. If you unbind
> the backlight driver, the backlight should remain off (or turn off if
> it isn't already).
> 
>> In my experience this is enough to justify such a change.
> 
> I would consider the change if it was actually going to fix the issue
> that you're seeing. As it is, all your proposal does is paper over one
> specific symptom, and I'm sorry, but that's just not good enough.
> 
>>> So we already know that currently all users expect that the pin state
>>> after disable will be low (because they previously set duty cycle to 0,
>>> which is the active equivalent of low).
>>
>> My theory is that most users don't expect this and/or don't make use of
>> it. As of 9dcd936c5312 we have 50 pwm drivers
>> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
>> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
>> and I think there are a few false positive matches, pwm-sun4i.c for
>> example). I'd be willing to bet that as of now imx isn't the only driver
>> with this problem.
> 
> We can speculate about that as much as we want. i.MX is the only one
> that this issue has been reported against, so I'm going to have to
> assume that it's working fine for all of the others. If it isn't then
> people should come forward and report it.
> 
>>> Given that stricter definition,
>>> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
>>> state. I think the fix for that is for the i.MX PWM driver to make sure
>>> the pin state doesn't actually change on ->disable(). If that means the
>>> clock needs to remain on, then that's exactly what the driver should be
>>> implementing.
>>
>> Yes, if we keep the API "stricter" this is what should be done. This
>> doesn't justify to keep the API strict though.
> 
> But an API that isn't strict is useless. If the behaviour of a PWM isn't
> predictable, how can you use it to drive a backlight. If at any point it
> could just go 100% and turn on the backlight to full brightness, how can
> anyone use it for anything sensible?
> 
>>> Are you aware of this:
>>>
>>> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
>>>
>>> This seems to address exactly the problem that you're describing. The
>>> solution matches what I had expected a fix for this problem to look
>>> like. Can you try it and see if that fixes the issue?
>>
>> Right. Apart from bugs in it (for example
>> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
>> as output)

It is a RFC and the past discussion about it was mostly theoretical than
about technical details. So I thought that nobody would really expect
excellent technical solution at this point. I just tried to do my best
and tested dozens of variations of [enabled/disabled/inverted/non-
inverted/ bootloader/device-tree/sysfs] configurations to be sure how it
affects current users and how it improves the situation.

Regarding the GPIO output configuration. Yes, the pin is configured as
input in the imx_pwm_init_pinctrl_info() function. At this stage you
do not really know how to configure the pin. Configuring it as input
is the safest option I think.

I thought that the pin is configured as output whenever gpiod_set_value()
is called. None of the other examples/usages in kernel I looked at used
the gpiod_direction_output(). So thanks for pointing that out, I can fix
that.

>> this would solve the issue for sure. This doesn't void my
>> arguments to relax the PWM API though. Also look at the costs: The patch
>> adds 86 lines to the imx driver complicating it considerably.
> 
> Now you're just being dramatic. 86 lines of code is not a lot. And I
> don't think it's complicated. I'm willing to carry the burden of those
> 86 lines of code if it fixes the issue for good.
> 
>>                                                                Each
>> machine that wants to profit of this change has to adapt it's device
>> tree to make the additional pinmuxing and gpio known.
>> To be fair, my change doesn't fix the gap between pinctrl being active
>> and the actual enablement of the pwm. But this could be better solved in
>> the pwm framework without touching any hardware driver with pinctrl
>> mechanism that are already in place. (Make use of the "init" pinctrl
>> state and only switch to "default" when the pwm is configured.)
>> There is nothing imx-pwm specific in hooking a gpio and adapting the
>> pinmuxing in it. So if you want to go this path, please implement it in
>> a generic way such that all pwm devices could benefit.
> 
> You're wrong. Judging by all the evidence that we have at this point,
> this is exactly an imx-pwm specific issue, therefore the burden is on
> the imx-pwm driver to implement a solution for it. If there ever is a
> second driver that has this issue we can revisit moving this into the
> core. I don't see any reason to do so at this point in time with the
> information that we have.
> 
>>> It looks as if that also fixes the shutdown problem, so it's better than
>>> what you're proposing here yet it doesn't require any API change at all
>>> to make it work.
>>
>> I don't care much about the shutdown problem. (Also, if the imx-pwm
>> driver is unbound, the gpio is freed, too, so it's only by chance if the
>> level is kept.)
> 
> Who else do you think would use the GPIO and change the level? If you've
> got multiple concurrent users of the pin that's a whole other problem. I
> also don't see why you wouldn't want to care about the shutdown problem.
> It's annoying if you shut down the device and all of a sudden your
> backlight comes back up again, perhaps in the middle of the display
> controller shutting down and producing garbage on the screen.
> 
>>
>>>>> 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.
>>>>
>>>> I don't agree here. The PWM API as it is now would make it necessary
>>>> that the imx driver becomes ugly.
>>>
>>> Beauty is very subjective. This doesn't count as an argument. Michal's
>>> proposed solution looks perfectly fine.
>>
>> In this case the opposite of ugly is "simple". This is a real argument.
>> And this is a reason to apply my idea. It simplifies stuff and solves
>> some problems.
>>
>> A framework is good if it is general enough to cover all use cases for
>> its users and demands very little from it's implementors. Each callback
>> to implement by a hardware driver should be as simple as possible.
>> Implying "keep the pin level constant" in .disable makes it more
>> complicated for some drivers. So this is a bad restriction that should
>> better be dropped if there are no relvant downsides.
> 
> Okay, so I'm not suggesting that pwm_disable() keep the pin level
> constant. What I was proposing is that it be required to set the pin
> level to "off" (whatever that means). And the reason for doing that is
> because there are relevant downsides if we leave this undefined, as I
> described at length above.
> 
>> Sidenote: With the current capabilities of the pwm framework there is no
>> technical reason to expose to the hardware drivers that the pwm user uses
>> inverted logic. The framework could just apply
>>
>> 	duty = period - duty;
>>

I prefer to utilize as much HW capabilities as possible. And it seems
like most PWM chips support HW output inversion (to some extent) so to
me it seems perfectly OK that that information can be propagated from
the upper SW levels to the lowest one.

>> before calling the hardware specific .config callback and so allow to
>> simplify some drivers, reducing complexity to a single place.
> 
> No, you're wrong. If you only consider the case of a backlight or an
> LED, then yes, you could simplify it that way. However there are other
> use-cases that require more fine-grained control and in those cases the
> actual edges of the PWM are important.
> 
> Also, this has been discussed before and I have said elsewhere that I
> have no objection to adding a helper that would simplify this for
> consumers that don't care about the difference.
> 
>>>> By changing the semantic of the API
>>>>
>>>>   - the i.MX driver can be implemented nicely;
>>>>   - the other drivers for "saner" hardware can stay as they are;
>>>>   - the API users can still express all their needs;
>>>>   - the API stops having two ways to express the same needs;
>>>
>>> Again, duty-cycle = 0 is not the same as setting the state to disabled.
>>>
>>> Practically they usually have the same effect, but that's just because
>>> in the typical use-case we don't care about the subtle differences.
>>
>> I fail to see the subtle difference.
> > That's okay. I've tried my best to describe it to you in different ways.
> If I still haven't been able to convey this, I don't know what else to
> say.
> 
>>>>   - most API users can be simplified because they can drop
>>>>       if (duty == 0) pwm_disable()
>>>>   - on i.MX the clock can be disabled when not needed saving some energy;
>>>
>>> That's one reason why we have pwm_disable(). It's what you use to save
>>> some energy if you don't need the PWM. Also, you claim that if the clock
>>> is disabled the attached backlight will turn on at full brightness, so I
>>> fail to see how this would work.
>>
>> As long as you care about the backlight to be off, don't disable the
>> pwm. And if in this state the clk can be disabled, then this should be
>> done without an additional poke by the hardware driver which is the only
>> place that can know for sure that it is safe to do so. There is no good
>> reason to let the pwm user have to know that the currently configured
>> state might make some energy saving possible and impose the burden on
>> him to then call pwm_disable().
> 
> This doesn't have anything to do with consumers knowing about potential
> energy savings. All we do is give the consumers an opportunity to say: I
> need the PWM to be enabled because I want to use the PWM signal now or I
> don't really need the PWM signal any more, just stop sending a signal.
> This is merely a hint from the consumer. What the driver does with it is
> ultimately up to the driver. If the driver wants to disable the clock
> when the duty cycle goes to zero because it knows that it is safe to do
> so, then by all means, feel free to do that.
> 
>>> Also, you leave out the fact that even after all this work that touches
>>> all users and all PWM drivers you still don't get fully correct
>>> functionality on your devices because when the driver is removed or shut
>>> down you run into the same problem.
>>
>> It's ok for me that all bets are off when the driver is removed.
> 
> Well, it's not okay for me. Especially if we can do better.
> 
>>                                                                   I'm
>> happy when I fix the problem during active operation because on the
>> machine I currently care about the shutdown problem isn't relevant.
> 
> Why is it not relevant? Surely at some point you will want to shut down
> that machine. And even if not, imx-pwm is used by others and they do
> seem to care about shutdown, so they should have a say in the matter as
> well.
> 
>> (Also as noted above, even Michal's patch doesn't solve the problem
>> formally.)
> 
> Now I'm confused, you said above that "Apart from bugs in it ... this
> would solve the issue for sure". So which one is it?
> 
>>>> Regarding you claim that the API worked fine for a decade: I tried
>>>> already in 2013 to address this problem. And even independent of that,
>>>> that is not a good reason to not improve stuff.
>>>
>>> My claim was that it has worked fine "for everyone else". I know that
>>> this has been an issue on i.MX for a long time and I've done my best to
>>> lay out why I think your proposal is not the right way to fix it.
>>
>> I still fail to see a single technical reason to not evolve the API. I
>> read from your mails:
>>
>>   - it has been like that for ages
>>   - it works for everyone but i.MX
>>   - it could be made working for i.MX by involving pinctrl and gpio in
>>     the hardware specific pwm driver.
>>
>> Did I miss anything? None of these justifies to keep the status quo in
>> my eyes.
> 
> I don't understand your logic here. You want to change the API, so the
> burden is on you to convince me *why* it needs to be changed. So far you
> have not been able to do that, so don't try to turn this around and make
> me come up with reasons why it shouldn't change.
> 
> Thierry
>
Uwe Kleine-König Oct. 11, 2018, 8:34 p.m. | #16
Hello Thierry,

On Thu, Oct 11, 2018 at 02:00:07PM +0200, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
> > On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> > > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > > > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
> > also don't seem to refuse to change pwm users that do:

Just noticed the above has a "n't" to much. I don't think you
misunderstood my point because of that though.
 
> > 	pwm_config(pwm, 0, 100);
> > 	pwm_disable(pwm);
> > 
> > I interpreted your sentence as confirmation that this sequence isn't
> > correct in general.
> 
> Yes, it is not correct in general, though it is correct in many cases.

Can you point out where it would not be correct?

> > Where is the difference you care about between duty=0 and disabled?
> 
> This is described in the kerneldoc of the corresponding functions. In the
> case of pwm_enable() it starts the output toggling, whereas pwm_enable()

s/pwm_enable()$/pwm_disable/ I assume.

> disables the output toggling. So strictly by going according to the PWM
> API a driver should only need to call pwm_config() if the duty cycle
> actually changes. If all you do is disable and enable, then pwm_enable()
> and pwm_disable() should be good enough.

Not sure we're looking on the same kerneldoc. pwm_disable only
specifies:

	pwm_disable() - stop a PWM output toggling

. If you call pwm_disable() for an i.MX device it doesn't toggle any
more. OK, after configuring the pwm as inverted and 0% duty (or not
inverted and 100% duty) you get an additional edge, but I do think it's
a bit bold to deduce that the pin should keep it's state from the above
specification.

> The implicit assumption is that the configuration will persist across
> pwm_disable()/pwm_enable(). In practice this doesn't matter that much
> because most users will be setting the duty cycle and enable/disable
> at the same time. I can easily imagine other use-cases where you would
> want to make the different. One other difference could be that
> pwm_enable() and pwm_disable() require a more involved sequence of
> operations than pwm_config() or vice versa, in which cases you may
> want to avoid one or the other if possible.

OK, A PWM user that has to switch between say 50% duty cycle and a
constant value can use pwm_enable() and pwm_disable() to change between
these states (after calling pwm_config(pwm, period / 2, period) once).

Similarly it can use

	pwm_config(pwm, 0, period);

and

	pwm_config(pwm, period / 2, period);

to toggle.

I confirm, with my suggestion the first alternative would go away. This
isn't a big loss though because the second stays around.

Now as you refuse to remove this first alternative there should be a
relevant semantic difference between the two. There are only a single
difference I see: With the first way (using pwm_enable() and
pwm_disable()) the PWM user doesn't control if the PWM stops at 0 or 1,
using the 2nd approach the PWM has to stop at 0.

Currently power consumption might be another difference, but you stated
below this wasn't relevant.

> In addition, we do expose these details to userspace via sysfs, so not
> allowing an explicit disable after config(duty=0) would break that ABI.

I wouldn't have a big problem with that even if the "strict" expectation
would be documented explicitly. The documentation for
/sys/class/pwm/pwmchipN/pwmX/enable only states:

	Enable/disable the PWM signal.

which I wouldn't consider to break with my suggestion.

> > What is in your view the current difference for the hardware behind the
> > PWM?
> 
> It depends on the driver implementation, but the kerneldoc is pretty
> specific. I many cases I would expect pwm_disable() to allow a bit of
> extra power to be saved because clocks could be turned off.

With my approach that expectation is given. With your's it is not in
every case because of the ambiguity of pwm_disable. On i.MX the clock
can only be disabled if the caller doesn't care about the resulting pin
state. The hardware driver however cannot know if this is the case and
so must keep the clock on even in the cases where it would be ok to
disable it.

> > > > Really, only the driver knows when it's safe to disable the clock while
> > > > it's expected to keep the pin at a given state. So better don't expect a
> > > > certain state after pwm_disable().
> > > 
> > > Yes, I agree that only the driver knows when it is safe to do so, but I
> > > still fail to understand why we should change the API in order to
> > > accomodate the specifics of a particular driver.
> > 
> > The incentive to change the API is a combination of:
> > 
> >  - The restriction to keep the pin driving at a certain level after
> >    disable isn't a natural and obvious property of the hardware.
> >    The designers of the imx-pwm for example did it differently.
> 
> I would disagree. A pin that you can't keep at a defined level if it
> isn't actively driven is a fundamentally useless concept. You said
> yourself that the hardware design is problematic because it causes the
> pin to change unexpectedly when you disable the PWM.

It's only unexpected if you are used to the strict API design.

And if you don't agree to this point, I'm missing ambition on your side.
If the strict API is only suitable for "sane" pwm hardware instead of
being general enough to also cover some of the "stranger" designs,
that's what I call useless.

> One could argue that that's a bug in the hardware. 

And one could argue further that it's a shortcoming of the PWM framework
that it is unable to handle it better, though it easily could.

> However, given that there is a way to avoid the unexpected change,
> there is a mechanism outside of PWM to deal with this use-case. And
> that's also perfectly fine. Dealing with pins that aren't actively
> driven is part of the job of a pinmux controller.
> 
> >  - The change removes ambiguity from from the demands on the hardware
> >    drivers.
> 
> I don't think there's currently any ambiguity. We may not be stating
> explicitly that the assumption is for a PWM to be "off" after
> pwm_disable(), but it's certainly how everyone has interpreted it. Also
> if there's an ambiguity, then that's what should be addressed.

"off" isn't a clear definition. For you it is (assuming I understood you
right) "stop immediately and keep driving the current level". For the
imx hardware designer it is, "stop toggling, output a 0". For others it
might be: "Stop driving the pin".

The ambiguity is that sometimes the PWM is disabled with the need to
keep the output driving at a certain level. And sometimes the PWM is
disabled and the output doesn't matter. In both cases pwm_disable() is
used and the driver cannot know in which of the two cases we are and if
it's ok to release the pin or not.

> >  - The change doesn't make the API less expressive.
> 
> Yes it does. You remove the distinction between duty cycle == 0 and
> the PWM being disabled.

Currently there is no distinction that is relevant for PWM users. (If
you don't agree, please provide an example.) In both cases the output is
constant 0 (assuming a non-inverted PWM).

The PWM user can still request that constant 0 with the laxer API. The
only difference in the strict API is that

	pwm_config(pwm, 0, 100)

doesn't disable the clock and

	pwm_disable(pwm)

does. There is no good reason however for a driver of a "sane" PWM to
not disable the clock in reply to pwm_config(pwm, 0, 100) though.

If you don't agree, can you show me a single PWM user that cares about
the distinction?

> >  - The change doesn't doesn't complicate any pwm hardware driver.
> 
> True. It also doesn't simplify any PWM hardware driver.

see "The change simplifies the imx-pwm driver." below that you
confirmed.

> >  - The change allows to simplify most pwm users.
> 
> Yes, the subset that doesn't care about the difference is marginally
> simplified. Although if the API was more rigorous about the definition
> of pwm_disable(), then PWM users would be even more simplified.

I cannot follow. How should/could the API be more rigorous to simplify
PWM users? Is this off-topic for the current discussion?

> >  - The change simplifies the imx-pwm driver.
> 
> True. But you leave out that the change doesn't actually fix your
> problem. If your PWM user goes away, it will still want to disable the
> PWM because it is no longer in use. However, you don't want that to
> mean that the backlight goes to full brightness again. If you unbind
> the backlight driver, the backlight should remain off (or turn off if
> it isn't already).

This advantage isn't about "solving my problems". It is about
simplification (as I wrote).

> > In my experience this is enough to justify such a change.
> 
> I would consider the change if it was actually going to fix the issue
> that you're seeing. As it is, all your proposal does is paper over one
> specific symptom, and I'm sorry, but that's just not good enough.

Correct, it doesn't completely fix the issue. It improves the situation
considerably though and has the advantages that you confirmed above.

> > > So we already know that currently all users expect that the pin state
> > > after disable will be low (because they previously set duty cycle to 0,
> > > which is the active equivalent of low).
> > 
> > My theory is that most users don't expect this and/or don't make use of
> > it. As of 9dcd936c5312 we have 50 pwm drivers
> > ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
> > at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
> > and I think there are a few false positive matches, pwm-sun4i.c for
> > example). I'd be willing to bet that as of now imx isn't the only driver
> > with this problem.
> 
> We can speculate about that as much as we want. i.MX is the only one
> that this issue has been reported against, so I'm going to have to
> assume that it's working fine for all of the others. If it isn't then
> people should come forward and report it.

You really assume that all the drivers that don't make use of
PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as
you expect when the PWM user does:

	pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; })

? I'd say your assumption that all are working fine is treading on thin
ice. The effect maybe is only that the i.MX users use more corners of
the PWM API and/or are more open to report actual problems.

Also if there are two ways to implement something, one works for 49
different hardware implementations and one for 50 this is already a good
reason to stick to the latter.

> > > Given that stricter definition,
> > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> > > state. I think the fix for that is for the i.MX PWM driver to make sure
> > > the pin state doesn't actually change on ->disable(). If that means the
> > > clock needs to remain on, then that's exactly what the driver should be
> > > implementing.
> > 
> > Yes, if we keep the API "stricter" this is what should be done. This
> > doesn't justify to keep the API strict though.
> 
> But an API that isn't strict is useless. If the behaviour of a PWM isn't
> predictable, how can you use it to drive a backlight. If at any point it
> could just go 100% and turn on the backlight to full brightness, how can
> anyone use it for anything sensible?

As long as I don't call pwm_disable() (or pwm_config(pwm, period,
period)) it must not go to 100%.

> > > Are you aware of this:
> > > 
> > > 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> > > 
> > > This seems to address exactly the problem that you're describing. The
> > > solution matches what I had expected a fix for this problem to look
> > > like. Can you try it and see if that fixes the issue?
> > 
> > Right. Apart from bugs in it (for example
> > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> > as output) this would solve the issue for sure. This doesn't void my
> > arguments to relax the PWM API though. Also look at the costs: The patch
> > adds 86 lines to the imx driver complicating it considerably.
> 
> Now you're just being dramatic. 86 lines of code is not a lot. And I
> don't think it's complicated. I'm willing to carry the burden of those
> 86 lines of code if it fixes the issue for good.

Right, the complexity doesn't come with the 86 lines of code. With my
approach the pwm-imx driver only has to care about its 6 registers. With
Michal's patch it suddenly has to care about gpio and pinmuxing.

> >                                                               Each
> > machine that wants to profit of this change has to adapt it's device
> > tree to make the additional pinmuxing and gpio known.
> > To be fair, my change doesn't fix the gap between pinctrl being active
> > and the actual enablement of the pwm. But this could be better solved in
> > the pwm framework without touching any hardware driver with pinctrl
> > mechanism that are already in place. (Make use of the "init" pinctrl
> > state and only switch to "default" when the pwm is configured.)
> > There is nothing imx-pwm specific in hooking a gpio and adapting the
> > pinmuxing in it. So if you want to go this path, please implement it in
> > a generic way such that all pwm devices could benefit.
> 
> You're wrong. Judging by all the evidence that we have at this point,
> this is exactly an imx-pwm specific issue, therefore the burden is on
> the imx-pwm driver to implement a solution for it.

You're the first maintainer I met with this attitude. Whenever I
implemented something in a driver specific way fixing an issue that
other drivers might have to, I was requested to find a generic solution
without the need to analyse if other drivers are affected or not.

There are some subsystems (e.g. tty/serial) where there is much stuff
implemented in each driver even though some things could well be
implemented in a generic way (e.g. rs485 handling). This is really a big
mess.

> > > It looks as if that also fixes the shutdown problem, so it's better than
> > > what you're proposing here yet it doesn't require any API change at all
> > > to make it work.
> > 
> > I don't care much about the shutdown problem. (Also, if the imx-pwm
> > driver is unbound, the gpio is freed, too, so it's only by chance if the
> > level is kept.)
> 
> Who else do you think would use the GPIO and change the level? If you've
> got multiple concurrent users of the pin that's a whole other problem. I
> also don't see why you wouldn't want to care about the shutdown problem.
> It's annoying if you shut down the device and all of a sudden your
> backlight comes back up again, perhaps in the middle of the display
> controller shutting down and producing garbage on the screen.

If someone on my machine is able to unbind the backlight driver making
the display flicker I have much more and graver problems than the
flickering display. And I didn't test (as I currently don't have an
affected device on my desk) but I think machine shutdown is no problem.

> > Sidenote: With the current capabilities of the pwm framework there is no
> > technical reason to expose to the hardware drivers that the pwm user uses
> > inverted logic. The framework could just apply
> > 
> > 	duty = period - duty;
> > 
> > before calling the hardware specific .config callback and so allow to
> > simplify some drivers, reducing complexity to a single place.
> 
> No, you're wrong. If you only consider the case of a backlight or an
> LED, then yes, you could simplify it that way. However there are other
> use-cases that require more fine-grained control and in those cases the
> actual edges of the PWM are important.

Can you please point out such a use case that is also possible to
implement with the current API?
 
> Also, this has been discussed before and I have said elsewhere that I
> have no objection to adding a helper that would simplify this for
> consumers that don't care about the difference.

I suggest a compromise below, maybe we can agree on that one.

> > > > By changing the semantic of the API
> > > > 
> > > >  - the i.MX driver can be implemented nicely;
> > > >  - the other drivers for "saner" hardware can stay as they are;
> > > >  - the API users can still express all their needs;
> > > >  - the API stops having two ways to express the same needs;
> > > 
> > > Again, duty-cycle = 0 is not the same as setting the state to disabled.
> > > 
> > > Practically they usually have the same effect, but that's just because
> > > in the typical use-case we don't care about the subtle differences.
> > 
> > I fail to see the subtle difference.
> 
> That's okay. I've tried my best to describe it to you in different ways.
> If I still haven't been able to convey this, I don't know what else to
> say.

I reread your explanation several times and fail to understand it. It
would be very helpful if you could come up with a scenario where it's
obvious that the distinction is needed. You're writing about "typical
use-cases" and "value in having the additional flexibility" without
providing a use-case that is atypical or actually need the flexibility.

Maybe I don't understand the semantic you're giving to the calls or
we're using the same words to describe different things. I try
to describe the semantic of pwm_disable of the strict API in my words,
can you please confirm or correct that?:

	pwm_disable stops toggling the PWM pin at whatever state the pin
	currently is. Given a duty cycle between 0% and 100% (exclusive)
	that means the pin stops at an undefined level. If the duty
	cycle is 0% or 100% it stops (for an uninverted PWM) at 0 or 1
	respectively (and vice versa for an inverted one).

The semantic of pwm_enable in my understanding is:

	pwm_enable for an uninverted PWM starts with $duty_cycle
	nanoseconds at 0 and then is 1 for $period - $duty_cycle
	nanoseconds. Then repeat.
	For an inverted it is first 1 for $duty_cycle nanoseconds and
	then 0 for the rest of the period.

I think pwm_config is to be defined like this:

	pwm_config changes period and duty cycle without any
	specification about how the transition should be done.

All calls should be synchronous, that is should only return when the
change is actually "on the pin".

> > > >  - most API users can be simplified because they can drop
> > > >      if (duty == 0) pwm_disable()
> > > >  - on i.MX the clock can be disabled when not needed saving some energy;
> > > 
> > > That's one reason why we have pwm_disable(). It's what you use to save
> > > some energy if you don't need the PWM. Also, you claim that if the clock
> > > is disabled the attached backlight will turn on at full brightness, so I
> > > fail to see how this would work.
> > 
> > As long as you care about the backlight to be off, don't disable the
> > pwm. And if in this state the clk can be disabled, then this should be
> > done without an additional poke by the hardware driver which is the only
> > place that can know for sure that it is safe to do so. There is no good
> > reason to let the pwm user have to know that the currently configured
> > state might make some energy saving possible and impose the burden on
> > him to then call pwm_disable().
> 
> This doesn't have anything to do with consumers knowing about potential
> energy savings. All we do is give the consumers an opportunity to say: I
> need the PWM to be enabled because I want to use the PWM signal now or I
> don't really need the PWM signal any more, just stop sending a signal.

There are two different degrees of "I don't need the PWM signal any
more". In the first case you still care about the output level and in
the other you don't. Currently the API doesn't give the PWM user the
possibility to specify which of the two are intended.

> This is merely a hint from the consumer. What the driver does with it is
> ultimately up to the driver. If the driver wants to disable the clock
> when the duty cycle goes to zero because it knows that it is safe to do
> so, then by all means, feel free to do that.

How should the driver know it is safe if the PWM user doesn't have a way
to tell if he cares about the output level?

> > > Also, you leave out the fact that even after all this work that touches
> > > all users and all PWM drivers you still don't get fully correct
> > > functionality on your devices because when the driver is removed or shut
> > > down you run into the same problem.
> > 
> > It's ok for me that all bets are off when the driver is removed.
> 
> Well, it's not okay for me. Especially if we can do better.

Fine, then lets give a way to PWM users to specify if we have to care
about the pin state after disable or not.

> >                                                                  I'm
> > happy when I fix the problem during active operation because on the
> > machine I currently care about the shutdown problem isn't relevant.
> 
> Why is it not relevant? Surely at some point you will want to shut down
> that machine.

Does a shutdown do anything with a running pwm software wise? The
leds-pwm driver doesn't have a shutdown hook. The pwm_bl driver calls
pwm_free in some legacy configuration but otherwise does nothing that
would undo it's pwm_backlight_power_off() in the shutdown hook. So I'm
convinced shutdown is fine here.

> And even if not, imx-pwm is used by others and they do
> seem to care about shutdown, so they should have a say in the matter as
> well.

Note that my suggestion doesn't make things worse for those users who
care. I'm used to that it's good enough if a patch improves the
situation for some users and doesn't make it worse for the others.

> > (Also as noted above, even Michal's patch doesn't solve the problem
> > formally.)
> 
> Now I'm confused, you said above that "Apart from bugs in it ... this
> would solve the issue for sure". So which one is it?

I'm sure that you must not assume that after

	gpio_direction_output(gpio, 0);
	gpio_free(gpio);

the line still drives the 0. In most cases this will be the case, but if
this makes the GPIO switch to being an input that should be correct
behaviour, too. That's why I wrote "formally" above.

Michal's patch solves the gap at startup and when the PWM user calls
disable() formally. At unbind when the gpio is freed it's "only" in
practise.

> > > > Regarding you claim that the API worked fine for a decade: I tried
> > > > already in 2013 to address this problem. And even independent of that,
> > > > that is not a good reason to not improve stuff.
> > > 
> > > My claim was that it has worked fine "for everyone else". I know that
> > > this has been an issue on i.MX for a long time and I've done my best to
> > > lay out why I think your proposal is not the right way to fix it.
> > 
> > I still fail to see a single technical reason to not evolve the API. I
> > read from your mails:
> > 
> >  - it has been like that for ages
> >  - it works for everyone but i.MX
> >  - it could be made working for i.MX by involving pinctrl and gpio in
> >    the hardware specific pwm driver.
> > 
> > Did I miss anything? None of these justifies to keep the status quo in
> > my eyes.
> 
> I don't understand your logic here. You want to change the API, so the
> burden is on you to convince me *why* it needs to be changed.

Changing the API isn't *needed*. But it's the simplest way to fix some
(not all) issues I see. Additionally it simplifies some things.

> So far you have not been able to do that, so don't try to turn this
> around and make me come up with reasons why it shouldn't change.

I listed several reasons to change the API, for some you confirmed them
to be valid. You listed some for keeping the API as is. As I'm
convinced my suggestion is good and your reasons are smaller (or even
bad), I try to argue against them.

The above question was my try to summarize your reasons and to make sure
the list is complete. I didn't try to turn anything around and if that
was your impression that's a misunderstanding.

With my current understanding the list of reasons to stick to the status
quo is:

 - it has been like that for ages
   (which is a poor reason to oppose to change);
 - it works for everyone but i.MX
   (which I doubt and with my replacement it works for all drivers and
   users as now including i.MX and maybe others that have the same issue
   without us knowing it currently);
 - it could be made working for i.MX by involving pinctrl and gpio in
   the hardware specific pwm driver (which is ridiculous because my
   solution is simpler); and
 - we're changing API
   (which I'd say is ok given that the specification isn't very specific
   and breaking the API for good reasons is not nice but acceptable IMHO)

In my eyes in the above list there is one valid reason to not change
(the last one). If you don't agree to change the userspace API we could
keep that constant and only change the kernel-internal API (which is a
bit ugly though).

The list of reasons in favour of a change are:

 - the pwm-imx driver can stay/become easier as with the stricter API;
 - ambiguity removed
   (hw drivers know in .disable if they can put the pin at a previously
   "forbidden" value)
 - users can be simplified by dropping some pwm_disable()

So now how can we come to a conclusion? Here are my suggestions for a
compromise:

What about adding a new callback that we could for example call
"disable_lax" that implements disabling the PWM without the guarantee to
put the pin in the "off" state?

We could also introduce a callback "freeze" (or disable_strict) that is
supposed to implement the "strict" disable. Then there is an objective
way to determine when all drivers are adapted to the updated model.

Do you agree to my suggestion that pwm_config(pwm, 0, period) or similar
should already imply disabling clocks if possible without the need to
explicitly call pwm_disable() afterwards?

Best regards
Uwe
Uwe Kleine-König Oct. 12, 2018, 9:45 a.m. | #17
On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> Uwe, Thierry,
> just a few comments as I am not able to dive deeper into your discussion.
> 
> On 11.10.2018 14:00, Thierry Reding wrote:
> > On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
> >> Hello Thierry,
> >>
> >> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> >>> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> >>>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> >>>>> 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.
> >>>>
> >>>> Did you notice you didn't argue against me here? We seem to agree that
> >>>> calling disabling a PWM after setting the duty cycle to zero is bad.
> >>
> >> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
> >> also don't seem to refuse to change pwm users that do:
> >>
> >> 	pwm_config(pwm, 0, 100);
> >> 	pwm_disable(pwm);
> >>
> >> I interpreted your sentence as confirmation that this sequence isn't
> >> correct in general.
> > 
> > Yes, it is not correct in general, though it is correct in many cases.
> > 
> >> Where is the difference you care about between duty=0 and disabled?
> > 
> > This is described in the kerneldoc of the corresponding functions. In the
> > case of pwm_enable() it starts the output toggling, whereas pwm_enable()
> > disables the output toggling. So strictly by going according to the PWM
> > API a driver should only need to call pwm_config() if the duty cycle
> > actually changes. If all you do is disable and enable, then pwm_enable()
> > and pwm_disable() should be good enough. The implicit assumption is that
> > the configuration will persist across pwm_disable()/pwm_enable(). In
> > practice this doesn't matter that much because most users will be
> > setting the duty cycle and enable/disable at the same time. I can easily
> > imagine other use-cases where you would want to make the different. One
> > other difference could be that pwm_enable() and pwm_disable() require a
> > more involved sequence of operations than pwm_config() or vice versa, in
> > which cases you may want to avoid one or the other if possible.
> > 
> > In addition, we do expose these details to userspace via sysfs, so not
> > allowing an explicit disable after config(duty=0) would break that ABI.
> > 
> >>> I don't see why you think I agree with that. The only thing I'm saying
> >>> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> >>> same as the PWM being disabled.
> >>
> >> What is in your view the current difference for the hardware behind the
> >> PWM?
> > 
> > It depends on the driver implementation, but the kerneldoc is pretty
> > specific. I many cases I would expect pwm_disable() to allow a bit of
> > extra power to be saved because clocks could be turned off.
> > 
> >>>>> 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.
> >>>>
> >>>> Full ack. So let's fix the users to not call pwm_disable after setting
> >>>> the duty cycle to zero!
> >>>
> >>> I don't understand why you come to that conclusion. It seems like we at
> >>> least agree on what the problem is and what potential issues there could
> >>> be with different setups.
> >>>
> >>> The point that we don't agree with is how to fix this.
> >>
> >> ack.
> >>
> >>>> Really, only the driver knows when it's safe to disable the clock while
> >>>> it's expected to keep the pin at a given state. So better don't expect a
> >>>> certain state after pwm_disable().
> >>>
> >>> Yes, I agree that only the driver knows when it is safe to do so, but I
> >>> still fail to understand why we should change the API in order to
> >>> accomodate the specifics of a particular driver.
> >>
> >> The incentive to change the API is a combination of:
> >>
> >>   - The restriction to keep the pin driving at a certain level after
> >>     disable isn't a natural and obvious property of the hardware.
> >>     The designers of the imx-pwm for example did it differently.
> > 
> > I would disagree. A pin that you can't keep at a defined level if it
> > isn't actively driven is a fundamentally useless concept. You said
> > yourself that the hardware design is problematic because it causes the
> > pin to change unexpectedly when you disable the PWM. One could argue
> > that that's a bug in the hardware. However, given that there is a way to
> > avoid the unexpected change, there is a mechanism outside of PWM to deal
> > with this use-case. And that's also perfectly fine. Dealing with pins
> > that aren't actively driven is part of the job of a pinmux controller.
> > 
> >>   - The change removes ambiguity from from the demands on the hardware
> >>     drivers.
> > 
> > I don't think there's currently any ambiguity. We may not be stating
> > explicitly that the assumption is for a PWM to be "off" after
> > pwm_disable(), but it's certainly how everyone has interpreted it. Also
> > if there's an ambiguity, then that's what should be addressed.
> > 
> >>   - The change doesn't make the API less expressive.
> > 
> > Yes it does. You remove the distinction between duty cycle == 0 and
> > the PWM being disabled.
> > 
> >>   - The change doesn't doesn't complicate anmy pwm hardware driver.
> > 
> > True. It also doesn't simplify any PWM hardware driver.
> > 
> >>   - The change allows to simplify most pwm users.
> > 
> > Yes, the subset that doesn't care about the difference is marginally
> > simplified. Although if the API was more rigorous about the definition
> > of pwm_disable(), then PWM users would be even more simplified.
> > 
> >>   - The change simplifies the imx-pwm driver.
> > 
> > True. But you leave out that the change doesn't actually fix your
> > problem. If your PWM user goes away, it will still want to disable the
> > PWM because it is no longer in use. However, you don't want that to
> > mean that the backlight goes to full brightness again. If you unbind
> > the backlight driver, the backlight should remain off (or turn off if
> > it isn't already).
> > 
> >> In my experience this is enough to justify such a change.
> > 
> > I would consider the change if it was actually going to fix the issue
> > that you're seeing. As it is, all your proposal does is paper over one
> > specific symptom, and I'm sorry, but that's just not good enough.
> > 
> >>> So we already know that currently all users expect that the pin state
> >>> after disable will be low (because they previously set duty cycle to 0,
> >>> which is the active equivalent of low).
> >>
> >> My theory is that most users don't expect this and/or don't make use of
> >> it. As of 9dcd936c5312 we have 50 pwm drivers
> >> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
> >> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
> >> and I think there are a few false positive matches, pwm-sun4i.c for
> >> example). I'd be willing to bet that as of now imx isn't the only driver
> >> with this problem.
> > 
> > We can speculate about that as much as we want. i.MX is the only one
> > that this issue has been reported against, so I'm going to have to
> > assume that it's working fine for all of the others. If it isn't then
> > people should come forward and report it.
> > 
> >>> Given that stricter definition,
> >>> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> >>> state. I think the fix for that is for the i.MX PWM driver to make sure
> >>> the pin state doesn't actually change on ->disable(). If that means the
> >>> clock needs to remain on, then that's exactly what the driver should be
> >>> implementing.
> >>
> >> Yes, if we keep the API "stricter" this is what should be done. This
> >> doesn't justify to keep the API strict though.
> > 
> > But an API that isn't strict is useless. If the behaviour of a PWM isn't
> > predictable, how can you use it to drive a backlight. If at any point it
> > could just go 100% and turn on the backlight to full brightness, how can
> > anyone use it for anything sensible?
> > 
> >>> Are you aware of this:
> >>>
> >>> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> >>>
> >>> This seems to address exactly the problem that you're describing. The
> >>> solution matches what I had expected a fix for this problem to look
> >>> like. Can you try it and see if that fixes the issue?
> >>
> >> Right. Apart from bugs in it (for example
> >> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> >> as output)
> 
> It is a RFC and the past discussion about it was mostly theoretical than
> about technical details. So I thought that nobody would really expect
> excellent technical solution at this point. I just tried to do my best
> and tested dozens of variations of [enabled/disabled/inverted/non-
> inverted/ bootloader/device-tree/sysfs] configurations to be sure how it
> affects current users and how it improves the situation.

I replied to the mail and pointed out a few other things now. This is
easier than to talk about your patch in this discussion.
 
> Regarding the GPIO output configuration. Yes, the pin is configured as
> input in the imx_pwm_init_pinctrl_info() function. At this stage you
> do not really know how to configure the pin. Configuring it as input
> is the safest option I think.

Did it work without setting the gpio as output in your tests?

> I thought that the pin is configured as output whenever gpiod_set_value()
> is called. None of the other examples/usages in kernel I looked at used
> the gpiod_direction_output()

Maybe they use devm_gpio_request_one or similar that return your gpio's
direction already configured?

> >> Sidenote: With the current capabilities of the pwm framework there is no
> >> technical reason to expose to the hardware drivers that the pwm user uses
> >> inverted logic. The framework could just apply
> >>
> >> 	duty = period - duty;
> >>
> 
> I prefer to utilize as much HW capabilities as possible. And it seems
> like most PWM chips support HW output inversion (to some extent) so to
> me it seems perfectly OK that that information can be propagated from
> the upper SW levels to the lowest one.

If the hardware capability is useful I fully agree. But if inversion can
be accomplished by a chip that doesn't support inversion in hardware by
just using duty = period - duty (and so can be accomplished by a chip
that has inversion support without making use of it) then the drivers
shouldn't be confronted with it.

(I'm not sure if inversion is really relevant with the current status
quo, as the expectations are not documented in a place I'm aware of.)

Best regards
Uwe
Thierry Reding Oct. 12, 2018, 9:53 a.m. | #18
On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote:
[...]
> > We can speculate about that as much as we want. i.MX is the only one
> > that this issue has been reported against, so I'm going to have to
> > assume that it's working fine for all of the others. If it isn't then
> > people should come forward and report it.
> 
> You really assume that all the drivers that don't make use of
> PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as
> you expect when the PWM user does:
> 
> 	pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; })
> 
> ? I'd say your assumption that all are working fine is treading on thin
> ice. The effect maybe is only that the i.MX users use more corners of
> the PWM API and/or are more open to report actual problems.

Look, it's unrealistic to expect me to actually test or verify all of
the different PWM controllers out there. The only thing I can go by is
by what the maintainers of the individual drivers tell me, or what has
been reported by users. So I'm going to assume that a driver works until
somebody reports that it is broken.

So again, if somebody notices that any of the other drivers are broken,
please do report it so that it can be fixed. Otherwise I don't have a
choice but assume that they still work.

> Also if there are two ways to implement something, one works for 49
> different hardware implementations and one for 50 this is already a good
> reason to stick to the latter.

This discussion isn't going anywhere, we're going in circles. I was not
suggesting that we don't find a solution for i.MX, I'm just saying that
changing the API is papering over some more fundamental issue on i.MX.
You can't magically fix it by changing the API, you have to get pinctrl
involved to properly solve this problem.

> > > > Given that stricter definition,
> > > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> > > > state. I think the fix for that is for the i.MX PWM driver to make sure
> > > > the pin state doesn't actually change on ->disable(). If that means the
> > > > clock needs to remain on, then that's exactly what the driver should be
> > > > implementing.
> > > 
> > > Yes, if we keep the API "stricter" this is what should be done. This
> > > doesn't justify to keep the API strict though.
> > 
> > But an API that isn't strict is useless. If the behaviour of a PWM isn't
> > predictable, how can you use it to drive a backlight. If at any point it
> > could just go 100% and turn on the backlight to full brightness, how can
> > anyone use it for anything sensible?
> 
> As long as I don't call pwm_disable() (or pwm_config(pwm, period,
> period)) it must not go to 100%.

I've been arguing that even if you call pwm_disable() it shouldn't go to
100%. So to summarize the problem for i.MX here: at reset time, the pin
driving the PWM is configured as GPIO and is configured as input with a
high impedance, so that it will be high on boot, prior to any software
running. This is reasonable in order to make sure that the backlight
stays off until it is properly initialized. What I've been trying to
convey is that when you do pwm_disable() the pin should go into exactly
that initial configuration to make sure the backlight doesn't behave in
an undefined way.

> > > > Are you aware of this:
> > > > 
> > > > 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> > > > 
> > > > This seems to address exactly the problem that you're describing. The
> > > > solution matches what I had expected a fix for this problem to look
> > > > like. Can you try it and see if that fixes the issue?
> > > 
> > > Right. Apart from bugs in it (for example
> > > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> > > as output) this would solve the issue for sure. This doesn't void my
> > > arguments to relax the PWM API though. Also look at the costs: The patch
> > > adds 86 lines to the imx driver complicating it considerably.
> > 
> > Now you're just being dramatic. 86 lines of code is not a lot. And I
> > don't think it's complicated. I'm willing to carry the burden of those
> > 86 lines of code if it fixes the issue for good.
> 
> Right, the complexity doesn't come with the 86 lines of code. With my
> approach the pwm-imx driver only has to care about its 6 registers. With
> Michal's patch it suddenly has to care about gpio and pinmuxing.

Why is that a bad thing? They are fundamentally entangled here, so why
do you think that's inappropriate?

> > >                                                               Each
> > > machine that wants to profit of this change has to adapt it's device
> > > tree to make the additional pinmuxing and gpio known.
> > > To be fair, my change doesn't fix the gap between pinctrl being active
> > > and the actual enablement of the pwm. But this could be better solved in
> > > the pwm framework without touching any hardware driver with pinctrl
> > > mechanism that are already in place. (Make use of the "init" pinctrl
> > > state and only switch to "default" when the pwm is configured.)
> > > There is nothing imx-pwm specific in hooking a gpio and adapting the
> > > pinmuxing in it. So if you want to go this path, please implement it in
> > > a generic way such that all pwm devices could benefit.
> > 
> > You're wrong. Judging by all the evidence that we have at this point,
> > this is exactly an imx-pwm specific issue, therefore the burden is on
> > the imx-pwm driver to implement a solution for it.
> 
> You're the first maintainer I met with this attitude. Whenever I
> implemented something in a driver specific way fixing an issue that
> other drivers might have to, I was requested to find a generic solution
> without the need to analyse if other drivers are affected or not.

Well, you should be happy. Dealing with this at a driver level is really
much easier than trying to come up with a generic solution. And maybe
you have met maintainers with a different attitude, so what? People have
different opinions and experiences. This shouldn't be a surprise.

In my opinion it's better to not try and generalize upfront. If you try
to create a generic solution if all you have is a single use-case, you
are bound to get it wrong. You're "generic" solution is likely to tailor
to the particular use-case and make it unusable for any others. So I try
not to attempt genericity until there are at least two or more use-cases
that can be the basis for a generic solution.

> There are some subsystems (e.g. tty/serial) where there is much stuff
> implemented in each driver even though some things could well be
> implemented in a generic way (e.g. rs485 handling). This is really a big
> mess.

One person's mess is merely another person's filing system.

> I reread your explanation several times and fail to understand it. It
> would be very helpful if you could come up with a scenario where it's
> obvious that the distinction is needed. You're writing about "typical
> use-cases" and "value in having the additional flexibility" without
> providing a use-case that is atypical or actually need the flexibility.
> 
> Maybe I don't understand the semantic you're giving to the calls or
> we're using the same words to describe different things. I try
> to describe the semantic of pwm_disable of the strict API in my words,
> can you please confirm or correct that?:
> 
> 	pwm_disable stops toggling the PWM pin at whatever state the pin
> 	currently is. Given a duty cycle between 0% and 100% (exclusive)
> 	that means the pin stops at an undefined level. If the duty
> 	cycle is 0% or 100% it stops (for an uninverted PWM) at 0 or 1
> 	respectively (and vice versa for an inverted one).

No, I don't think that's entirely correct. The current assumption by all
users is more that the pin would stop at the no power state, which would
be 0 for normal PWM and 1 for inverted.

This is what's reasonable for most or all cases. It's pretty much the
same as for a clock, or a regulator or any other number of devices.

> The semantic of pwm_enable in my understanding is:
> 
> 	pwm_enable for an uninverted PWM starts with $duty_cycle
> 	nanoseconds at 0 and then is 1 for $period - $duty_cycle
> 	nanoseconds. Then repeat.
> 	For an inverted it is first 1 for $duty_cycle nanoseconds and
> 	then 0 for the rest of the period.
> 
> I think pwm_config is to be defined like this:
> 
> 	pwm_config changes period and duty cycle without any
> 	specification about how the transition should be done.
> 
> All calls should be synchronous, that is should only return when the
> change is actually "on the pin".

Correct.

> > > > >  - most API users can be simplified because they can drop
> > > > >      if (duty == 0) pwm_disable()
> > > > >  - on i.MX the clock can be disabled when not needed saving some energy;
> > > > 
> > > > That's one reason why we have pwm_disable(). It's what you use to save
> > > > some energy if you don't need the PWM. Also, you claim that if the clock
> > > > is disabled the attached backlight will turn on at full brightness, so I
> > > > fail to see how this would work.
> > > 
> > > As long as you care about the backlight to be off, don't disable the
> > > pwm. And if in this state the clk can be disabled, then this should be
> > > done without an additional poke by the hardware driver which is the only
> > > place that can know for sure that it is safe to do so. There is no good
> > > reason to let the pwm user have to know that the currently configured
> > > state might make some energy saving possible and impose the burden on
> > > him to then call pwm_disable().
> > 
> > This doesn't have anything to do with consumers knowing about potential
> > energy savings. All we do is give the consumers an opportunity to say: I
> > need the PWM to be enabled because I want to use the PWM signal now or I
> > don't really need the PWM signal any more, just stop sending a signal.
> 
> There are two different degrees of "I don't need the PWM signal any
> more". In the first case you still care about the output level and in
> the other you don't. Currently the API doesn't give the PWM user the
> possibility to specify which of the two are intended.

I've said before that "don't care about the output level" is completely
useless. In most cases what you really want is the "no power" state and
that's what pwm_disable() should ensure. You're PWM could be controlling
a fan, and when you disable the PWM you want the fan to stop spinning,
right? You wouldn't want the fan to keep spinning after you've disabled
it.

This is something that most hardware engineers will also understand.
They tend to design their systems in such a way that the reset defaults
will have sensible effects. I think it's good in general to attempt, if
possible, to restore those reset defaults when you disable the device
that you're driving.

> > > happy when I fix the problem during active operation because on the
> > > machine I currently care about the shutdown problem isn't relevant.
> > 
> > Why is it not relevant? Surely at some point you will want to shut down
> > that machine.
> 
> Does a shutdown do anything with a running pwm software wise? The
> leds-pwm driver doesn't have a shutdown hook. The pwm_bl driver calls
> pwm_free in some legacy configuration but otherwise does nothing that
> would undo it's pwm_backlight_power_off() in the shutdown hook. So I'm
> convinced shutdown is fine here.

But pwm_backlight_power_off() will also end up calling pwm_disable(),
which by your description of the problem would fully light up the
backlight. That is, unless your driver implement ->apply(), but in that
case you wouldn't have a problem anyway.

> > And even if not, imx-pwm is used by others and they do
> > seem to care about shutdown, so they should have a say in the matter as
> > well.
> 
> Note that my suggestion doesn't make things worse for those users who
> care. I'm used to that it's good enough if a patch improves the
> situation for some users and doesn't make it worse for the others.
> 
> > > (Also as noted above, even Michal's patch doesn't solve the problem
> > > formally.)
> > 
> > Now I'm confused, you said above that "Apart from bugs in it ... this
> > would solve the issue for sure". So which one is it?
> 
> I'm sure that you must not assume that after
> 
> 	gpio_direction_output(gpio, 0);
> 	gpio_free(gpio);
> 
> the line still drives the 0. In most cases this will be the case, but if
> this makes the GPIO switch to being an input that should be correct
> behaviour, too. That's why I wrote "formally" above.
> 
> Michal's patch solves the gap at startup and when the PWM user calls
> disable() formally. At unbind when the gpio is freed it's "only" in
> practise.

That's why pinctrl is also involved. That should ensure that the correct
level is driven to the pin even if it isn't actively driven by either
GPIO or PWM.

> I listed several reasons to change the API, for some you confirmed them
> to be valid. You listed some for keeping the API as is. As I'm
> convinced my suggestion is good and your reasons are smaller (or even
> bad), I try to argue against them.
> 
> The above question was my try to summarize your reasons and to make sure
> the list is complete. I didn't try to turn anything around and if that
> was your impression that's a misunderstanding.
> 
> With my current understanding the list of reasons to stick to the status
> quo is:
> 
>  - it has been like that for ages
>    (which is a poor reason to oppose to change);
>  - it works for everyone but i.MX
>    (which I doubt and with my replacement it works for all drivers and
>    users as now including i.MX and maybe others that have the same issue
>    without us knowing it currently);
>  - it could be made working for i.MX by involving pinctrl and gpio in
>    the hardware specific pwm driver (which is ridiculous because my
>    solution is simpler); and
>  - we're changing API
>    (which I'd say is ok given that the specification isn't very specific
>    and breaking the API for good reasons is not nice but acceptable IMHO)
> 
> In my eyes in the above list there is one valid reason to not change
> (the last one). If you don't agree to change the userspace API we could
> keep that constant and only change the kernel-internal API (which is a
> bit ugly though).
> 
> The list of reasons in favour of a change are:
> 
>  - the pwm-imx driver can stay/become easier as with the stricter API;
>  - ambiguity removed
>    (hw drivers know in .disable if they can put the pin at a previously
>    "forbidden" value)
>  - users can be simplified by dropping some pwm_disable()
> 
> So now how can we come to a conclusion? Here are my suggestions for a
> compromise:
> 
> What about adding a new callback that we could for example call
> "disable_lax" that implements disabling the PWM without the guarantee to
> put the pin in the "off" state?
> 
> We could also introduce a callback "freeze" (or disable_strict) that is
> supposed to implement the "strict" disable. Then there is an objective
> way to determine when all drivers are adapted to the updated model.
> 
> Do you agree to my suggestion that pwm_config(pwm, 0, period) or similar
> should already imply disabling clocks if possible without the need to
> explicitly call pwm_disable() afterwards?

I don't think it has to be that explicit. Drivers should certainly feel
free to do so if it works and is reasonable.

To be honest, the whole discussion here is somewhat unproductive. You're
arguing about changing an API that's deprecated anyway, even if not
officially. We already introduced the atomic API a few years ago that is
meant to address most of these issues.

So instead of trying to "fix" a legacy API, have you considered moving
the i.MX driver to the atomic API? That should give you much better
control over when and how to program the registers.

Thierry
Thierry Reding Oct. 12, 2018, 10 a.m. | #19
On Fri, Oct 12, 2018 at 11:53:49AM +0200, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote:
[...]
> > The semantic of pwm_enable in my understanding is:
> > 
> > 	pwm_enable for an uninverted PWM starts with $duty_cycle
> > 	nanoseconds at 0 and then is 1 for $period - $duty_cycle
> > 	nanoseconds. Then repeat.
> > 	For an inverted it is first 1 for $duty_cycle nanoseconds and
> > 	then 0 for the rest of the period.

Slight correction here: these are actually reversed. If you look at the
kerneldoc for enum pwm_polarity it explains what the expectations are.

Thierry
Thierry Reding Oct. 12, 2018, 10:11 a.m. | #20
On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
[...]
> > >> Sidenote: With the current capabilities of the pwm framework there is no
> > >> technical reason to expose to the hardware drivers that the pwm user uses
> > >> inverted logic. The framework could just apply
> > >>
> > >> 	duty = period - duty;
> > >>
> > 
> > I prefer to utilize as much HW capabilities as possible. And it seems
> > like most PWM chips support HW output inversion (to some extent) so to
> > me it seems perfectly OK that that information can be propagated from
> > the upper SW levels to the lowest one.
> 
> If the hardware capability is useful I fully agree. But if inversion can
> be accomplished by a chip that doesn't support inversion in hardware by
> just using duty = period - duty (and so can be accomplished by a chip
> that has inversion support without making use of it) then the drivers
> shouldn't be confronted with it.

These two are orthogonal problems. If all you care about is the power
output of the PWM (as is the case for backlights, LEDs or fans, for
example), then inverted polarity has the same effect as duty = period -
duty.

However, the two don't result in identical waveforms. Again, a backlight
or LED doesn't care about the exact waveform, but there are other uses
for PWMs where these actually are important. I have admittedly never
used any of them myself, but this was brought to my attention a long
time ago when polarity support was introduced. I'm sure you can find the
discussions in some archives if you are inclined to do so.

> (I'm not sure if inversion is really relevant with the current status
> quo, as the expectations are not documented in a place I'm aware of.)

See the kerneldoc for enum pwm_polarity which does document this.

Thierry
Vokáč Michal Oct. 12, 2018, 12:25 p.m. | #21
On 12.10.2018 11:45, Uwe Kleine-König wrote:
> On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
>> Uwe, Thierry,

[...]

>> It is a RFC and the past discussion about it was mostly theoretical than
>> about technical details. So I thought that nobody would really expect
>> excellent technical solution at this point. I just tried to do my best
>> and tested dozens of variations of [enabled/disabled/inverted/non-
>> inverted/ bootloader/device-tree/sysfs] configurations to be sure how it
>> affects current users and how it improves the situation.
> 
> I replied to the mail and pointed out a few other things now. This is
> easier than to talk about your patch in this discussion.

Ack.

>> Regarding the GPIO output configuration. Yes, the pin is configured as
>> input in the imx_pwm_init_pinctrl_info() function. At this stage you
>> do not really know how to configure the pin. Configuring it as input
>> is the safest option I think.
> 
> Did it work without setting the gpio as output in your tests?

Yes, I am using it like that for aprox. 2 months without issues.

>> I thought that the pin is configured as output whenever gpiod_set_value()
>> is called. None of the other examples/usages in kernel I looked at used
>> the gpiod_direction_output()
> 
> Maybe they use devm_gpio_request_one or similar that return your gpio's
> direction already configured?

Nope. Examples I looked at (mainly I2C recovery) use only the gpiolib
functions.

Michal
Uwe Kleine-König Oct. 12, 2018, 3:47 p.m. | #22
Hello Michal,

On Fri, Oct 12, 2018 at 12:25:01PM +0000, Vokáč Michal wrote:
> >> Regarding the GPIO output configuration. Yes, the pin is configured as
> >> input in the imx_pwm_init_pinctrl_info() function. At this stage you
> >> do not really know how to configure the pin. Configuring it as input
> >> is the safest option I think.
> > 
> > Did it work without setting the gpio as output in your tests?
> 
> Yes, I am using it like that for aprox. 2 months without issues.

and you're sure the gpio is configured as output? Strange.
 
> >> I thought that the pin is configured as output whenever gpiod_set_value()
> >> is called. None of the other examples/usages in kernel I looked at used
> >> the gpiod_direction_output()
> > 
> > Maybe they use devm_gpio_request_one or similar that return your gpio's
> > direction already configured?
> 
> Nope. Examples I looked at (mainly I2C recovery) use only the gpiolib
> functions.

For example drivers/i2c/busses/i2c-imx.c uses devm_gpiod_get(&pdev->dev,
"scl", GPIOD_OUT_HIGH_OPEN_DRAIN); which configures as output (somewhat).

Best regards
Uwe
Uwe Kleine-König Oct. 12, 2018, 5:14 p.m. | #23
Hello Thierry,

On Fri, Oct 12, 2018 at 11:53:49AM +0200, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote:
> [...]
> > > We can speculate about that as much as we want. i.MX is the only one
> > > that this issue has been reported against, so I'm going to have to
> > > assume that it's working fine for all of the others. If it isn't then
> > > people should come forward and report it.
> > 
> > You really assume that all the drivers that don't make use of
> > PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as
> > you expect when the PWM user does:
> > 
> > 	pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; })
> > 
> > ? I'd say your assumption that all are working fine is treading on thin
> > ice. The effect maybe is only that the i.MX users use more corners of
> > the PWM API and/or are more open to report actual problems.
> 
> Look, it's unrealistic to expect me to actually test or verify all of
> the different PWM controllers out there. The only thing I can go by is
> by what the maintainers of the individual drivers tell me, or what has
> been reported by users. So I'm going to assume that a driver works until
> somebody reports that it is broken.

I didn't expect from you to test everything. I just pointed out that
your argument "I assume all drivers to behave correctly because there is
no report that any of them doesn't work" doesn't build on solid facts.

But lets make this explicit and official: I looked into all 48 pwm
drivers (at commit 6b3944e42e2e) and sorted them into categories:

a) not ported to pwm_state and no support .set_polarity:
   - pwm-ab8500.c
   - pwm-brcmstb.c
   - pwm-clps711x.c
   - pwm-crc.c
   - pwm-img.c
   - pwm-imx.c (1 of 2 pwm_ops in this driver)
   - pwm-lp3943.c
   - pwm-lpc32xx.c
   - pwm-mediatek.c
   - pwm-mtk-disp.c
   - pwm-mxs.c
   - pwm-pca9685.c
   - pwm-puv3.c
   - pwm-pxa.c
   - pwm-rcar.c
   - pwm-spear.c
   - pwm-sti.c
   - pwm-stmpe.c
   - pwm-twl-led.c
   - pwm-twl.c
   - pwm-tegra.c
  
b) supports inversion in hardware and pwm_state, polarity handling looks fine:
   - pwm-bcm-iproc.c
   - pwm-hibvt.c
   - pwm-rockchip.c
   - pwm-sun4i.c
   - pwm-zx.c

c) not ported to pwm_state, supports .set_polarity in hardware:
   - pwm-atmel-tcb.c
   - pwm-bcm2835.c
   - pwm-bcm-kona.c
   - pwm-berlin.c
   - pwm-ep93xx.c
   - pwm-fsl-ftm.c
   - pwm-lpc18xx-sct.c
   - pwm-jz4740.c
   - pwm-omap-dmtimer.c (not sure here, didn't find the actual implementation)
   - pwm-renesas-tpu.c
   - pwm-samsung.c
   - pwm-vt8500.c
   - pwm-tiehrpwm.c
   - pwm-tiecap.c

d) implements pwm_state, but ignores polarity:
   - pwm-cros-ec.c
   - pwm-lpss.c (with pwm-lpss-pci.c, pwm-lpss-platform.c)

e) supports state, but doesn't check .polarity on disable
   - pwm-atmel.c
   - pwm-atmel-hlcdc.c
   - pwm-imx.c (1 of 2 pwm_ops in this driver)
   - pwm-meson.c
   - pwm-stm32.c
   - pwm-stm32-lp.c

f) not actually a pwm driver (huh?):
   - pwm-tipwmss.c

So it's not as worse as I expected, mainly because some drivers don't
mention PWM_POLARITY_INVERSED but test for PWM_POLARITY_NORMAL.

The drivers in categories d) and e) are definitively broken and need
fixing. Some in the categories b) and c) might have the same problem as
imx (i.e. the output doesn't behave in pwm_disable(pwm) as in
pwm_config(pwm, 0, period)), but I'm unable to research that without the
respective hardware on my desk. (Reading the HW manual probably isn't
enough because most of them won't be explicit what happens in "off"
state. At least that's the case for the imx manual.)

> So again, if somebody notices that any of the other drivers are broken,
> please do report it so that it can be fixed. Otherwise I don't have a
> choice but assume that they still work.

done. :-)

> [...]

I deleted a big part of your mail and the repeating arguments I already
wrote in reply to it. I hope to work towards a compromise this way and
taking some heat out of the discussion.

> To be honest, the whole discussion here is somewhat unproductive.

I think the problem in this discussion is that there is a
misunderstanding between us. In my eyes the current state is:

I want to implement a change to the PWM API because I see a benefit in
it. I listed several advantages, some of them you accepted. You claimed
there was a disadvantage that makes the API less expressive. I tried to
understand that, asked several times for an example where the difference
you see actually matters but I don't get a reply to this question.

In this state I cannot invalidate your claim and I stay dissatisfied
because my incentive for a change is blocked by an argument I think is
void.

So I'll try to only discuss in the remaining part of this mail the
single issue that I think is key to our discussion:

You state there was a difference between

	pwm_disable(pwm);

and

	pwm_config(pwm, 0, period);

In my eyes they are equivalent. Both are expected to set the output to
inactive (that is 0 for an uninverted PWM, 1 for an inverted one).

The only differences I see are:

 - hardware driver details
   Some driver might not disable some resources after pwm_config and so
   failing to save some power. You stated however this is not the
   relevant issue.
 - after pwm_config(pwm, 0, period) pwm_enable(pwm) doesn't restore the old
   duty cycle.
   So with my suggested change the PWM framework stops caching the duty
   cycle. This appears too tiny for a reason to me to actually matter.

I assume I failed to list the detail you consider relevant. Can you
please make an effort to describe a use-case where the difference you
care about actually matters?

Best regards
Uwe
Uwe Kleine-König Oct. 14, 2018, 11:34 a.m. | #24
Hello Thierry,

On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> [...]
> > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > >> inverted logic. The framework could just apply
> > > >>
> > > >> 	duty = period - duty;
> > > >>
> > > 
> > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > like most PWM chips support HW output inversion (to some extent) so to
> > > me it seems perfectly OK that that information can be propagated from
> > > the upper SW levels to the lowest one.
> > 
> > If the hardware capability is useful I fully agree. But if inversion can
> > be accomplished by a chip that doesn't support inversion in hardware by
> > just using duty = period - duty (and so can be accomplished by a chip
> > that has inversion support without making use of it) then the drivers
> > shouldn't be confronted with it.
> 
> These two are orthogonal problems. If all you care about is the power
> output of the PWM (as is the case for backlights, LEDs or fans, for
> example), then inverted polarity has the same effect as duty = period -
> duty.
> 
> However, the two don't result in identical waveforms.

OK, let's find the difference then. Consider a pwm that is off at start,
then it's configured to run at 66% duty cycle at time A and then it's
disabled again at time B.

The resulting wave form is:

______________       __       __       __       _____________________
              \_____/  \_____/  \_____/  \_____/
            A                                B
              ^        ^        ^        ^

With the ^ markers pointing out where the periods started.

Correct?

Now with the duty = period - duty trick the wave would look as follows:


_________________       __       __       __       __________________
                 \_____/  \_____/  \_____/  \_____/
            A                                B
              ^        ^        ^        ^

Apart from a shift I cannot see any difference. I confirm that this
might be bad if there are two (or more) PWMs that are expected to run in
sync, but given that the current API doesn't provide the possibility to
map such a use case this is irrelevant.

Another difference I confirm there might be is that this might result in
one more (or less) periods depending on timing and the capabilities of
the hardware. Is this bad? I'd say it isn't.

What am I missing?

Best regards
Uwe
Uwe Kleine-König Oct. 15, 2018, 8:14 a.m. | #25
Hello,

adding the author of the commit that introduced the .set_polarity
callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add
some value to this discussion.

On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > [...]
> > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > >> inverted logic. The framework could just apply
> > > > >>
> > > > >> 	duty = period - duty;
> > > > >>
> > > > 
> > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > me it seems perfectly OK that that information can be propagated from
> > > > the upper SW levels to the lowest one.
> > > 
> > > If the hardware capability is useful I fully agree. But if inversion can
> > > be accomplished by a chip that doesn't support inversion in hardware by
> > > just using duty = period - duty (and so can be accomplished by a chip
> > > that has inversion support without making use of it) then the drivers
> > > shouldn't be confronted with it.
> > 
> > These two are orthogonal problems. If all you care about is the power
> > output of the PWM (as is the case for backlights, LEDs or fans, for
> > example), then inverted polarity has the same effect as duty = period -
> > duty.
> > 
> > However, the two don't result in identical waveforms.
> 
> OK, let's find the difference then. Consider a pwm that is off at start,
> then it's configured to run at 66% duty cycle at time A and then it's
> disabled again at time B.
> 
> The resulting wave form is:
> 
> ______________       __       __       __       _____________________
>               \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> With the ^ markers pointing out where the periods started.
> 
> Correct?
> 
> Now with the duty = period - duty trick the wave would look as follows:
> 
> 
> _________________       __       __       __       __________________
>                  \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> Apart from a shift I cannot see any difference. I confirm that this
> might be bad if there are two (or more) PWMs that are expected to run in
> sync, but given that the current API doesn't provide the possibility to
> map such a use case this is irrelevant.
> 
> Another difference I confirm there might be is that this might result in
> one more (or less) periods depending on timing and the capabilities of
> the hardware. Is this bad? I'd say it isn't.
> 
> What am I missing?

The commit log of 0aa0869c3c9b10338dd92a20fa4a9b6959f177b5 mentions:

    A practical example where [changing polarity] can prove useful is to
    simulate inversion of the duty cycle. While inversion of polarity
    and duty cycle are not exactly the same, the differences for most
    use-cases are negligible.

This doesn't help me to understand why the polarity stuff is useful
though. Does this log say that the motivation to support changing
polarity was to simulate "inversion of the duty cycle"? What exactly is
"inversion of the duty cycle"? Just

	duty_cycle = period - duty_cycle;

? Why does this have to be simulated, the API was able to do this
without any simulation before?

Is there a single use-case that can be done with polarity support in the
API that isn't possible without it? I cannot imagine there is any but
would like to learn and understand why this is valuable.

Currently there is no user of pwm_set_polarity(), I suggest to remove it
to not give users an incentive to still rely on the non-atomic API.

Also among the users of pwm_apply_state there is none which makes use of
the polarity setting.

Best regards
Uwe
Uwe Kleine-König Oct. 15, 2018, 8:16 a.m. | #26
Hello,

On Mon, Oct 15, 2018 at 10:14:21AM +0200, Uwe Kleine-König wrote:
> adding the author of the commit that introduced the .set_polarity
> callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add
> some value to this discussion.

there is no insight to be expected from that, the email address doesn't
exist any more.

Best regards
Uwe
Thierry Reding Oct. 15, 2018, 9:22 a.m. | #27
On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > [...]
> > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > >> inverted logic. The framework could just apply
> > > > >>
> > > > >> 	duty = period - duty;
> > > > >>
> > > > 
> > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > me it seems perfectly OK that that information can be propagated from
> > > > the upper SW levels to the lowest one.
> > > 
> > > If the hardware capability is useful I fully agree. But if inversion can
> > > be accomplished by a chip that doesn't support inversion in hardware by
> > > just using duty = period - duty (and so can be accomplished by a chip
> > > that has inversion support without making use of it) then the drivers
> > > shouldn't be confronted with it.
> > 
> > These two are orthogonal problems. If all you care about is the power
> > output of the PWM (as is the case for backlights, LEDs or fans, for
> > example), then inverted polarity has the same effect as duty = period -
> > duty.
> > 
> > However, the two don't result in identical waveforms.
> 
> OK, let's find the difference then. Consider a pwm that is off at start,
> then it's configured to run at 66% duty cycle at time A and then it's
> disabled again at time B.
> 
> The resulting wave form is:
> 
> ______________       __       __       __       _____________________
>               \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> With the ^ markers pointing out where the periods started.
> 
> Correct?
> 
> Now with the duty = period - duty trick the wave would look as follows:
> 
> 
> _________________       __       __       __       __________________
>                  \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> Apart from a shift I cannot see any difference. I confirm that this
> might be bad if there are two (or more) PWMs that are expected to run in
> sync, but given that the current API doesn't provide the possibility to
> map such a use case this is irrelevant.
> 
> Another difference I confirm there might be is that this might result in
> one more (or less) periods depending on timing and the capabilities of
> the hardware. Is this bad? I'd say it isn't.
> 
> What am I missing?

I don't think that's entirely correct. For normal PWM with 66% duty
cycle you'd get something like this:

     ____    ____    ____
    /    \__/    \__/    \__   (1)
    ^       ^       ^

For "emulated" inversion (duty cycle = period - duty cycle, i.e. 33%
duty cycle) you'd get this:

     __      __      __
    /  \____/  \____/  \____   (2)
    ^       ^       ^

For a truly inversed PWM (duty cycle = 33%), it would look like this:

          __      __      __
    \____/  \____/  \____/     (3)
    ^       ^       ^

If you emulate inversion of the truly inversed PWM, you'd get this:

        ____    ____    ____
    \__/    \__/    \__/       (4)
    ^       ^       ^

As you can see, all of the above are slightly different. As you can see,
and you already noted that, the emulated inversion (2) and the true
inversion (3) are almost identical, except that they have a phase shift
that is equivalent to the duty cycle. The same is true of (1) and (4).
The phase shift doesn't affect the power output of the signal, so (1)
and (4) as well as (2) and (3) have the same power output. This is the
reason why the difference doesn't matter for backlight or LEDs, because
brightness is proportional to the power output.

However, you can also see that (2) and (3) are different in how the
period starts. (2) starts with a rising edge while (3) starts with a
falling edge.

Now, for any use-cases where the only thing we care about is the power
output, my opinion is that consumers should implement the inversion. If
the hardware is capable of inversion, then by all means they should use
that in the implementation. But, a lot of hardware is not capable of
doing the inversion, so they can still fall back to emulating the
inversion. Either way, the important point here is that the consumer
driver (pwm-backlight, leds-pwm, ...) knows that only power output is
relevant, so it is the consumer drivers that should make the decision.

On the other hand, other use-cases may rely on the exact shape of the
PWM signal, so if the edges are important, then they need to rely on the
hardware inversion for proper functionality. My understanding is that we
don't have any of those users in the kernel, but my recollection is that
people use the sysfs interfaces to make use of this. Like I said
elsewhere, if I remember correctly, people use this as a way of
synchronizing motors using PWM pulses. My knowledge of those use-cases
stops there because I've never used PWMs like that.

Thierry
Thierry Reding Oct. 15, 2018, 9:28 a.m. | #28
On Mon, Oct 15, 2018 at 10:14:21AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> adding the author of the commit that introduced the .set_polarity
> callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add
> some value to this discussion.
> 
> On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > > [...]
> > > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > > >> inverted logic. The framework could just apply
> > > > > >>
> > > > > >> 	duty = period - duty;
> > > > > >>
> > > > > 
> > > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > > me it seems perfectly OK that that information can be propagated from
> > > > > the upper SW levels to the lowest one.
> > > > 
> > > > If the hardware capability is useful I fully agree. But if inversion can
> > > > be accomplished by a chip that doesn't support inversion in hardware by
> > > > just using duty = period - duty (and so can be accomplished by a chip
> > > > that has inversion support without making use of it) then the drivers
> > > > shouldn't be confronted with it.
> > > 
> > > These two are orthogonal problems. If all you care about is the power
> > > output of the PWM (as is the case for backlights, LEDs or fans, for
> > > example), then inverted polarity has the same effect as duty = period -
> > > duty.
> > > 
> > > However, the two don't result in identical waveforms.
> > 
> > OK, let's find the difference then. Consider a pwm that is off at start,
> > then it's configured to run at 66% duty cycle at time A and then it's
> > disabled again at time B.
> > 
> > The resulting wave form is:
> > 
> > ______________       __       __       __       _____________________
> >               \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > With the ^ markers pointing out where the periods started.
> > 
> > Correct?
> > 
> > Now with the duty = period - duty trick the wave would look as follows:
> > 
> > 
> > _________________       __       __       __       __________________
> >                  \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > Apart from a shift I cannot see any difference. I confirm that this
> > might be bad if there are two (or more) PWMs that are expected to run in
> > sync, but given that the current API doesn't provide the possibility to
> > map such a use case this is irrelevant.
> > 
> > Another difference I confirm there might be is that this might result in
> > one more (or less) periods depending on timing and the capabilities of
> > the hardware. Is this bad? I'd say it isn't.
> > 
> > What am I missing?
> 
> The commit log of 0aa0869c3c9b10338dd92a20fa4a9b6959f177b5 mentions:
> 
>     A practical example where [changing polarity] can prove useful is to
>     simulate inversion of the duty cycle. While inversion of polarity
>     and duty cycle are not exactly the same, the differences for most
>     use-cases are negligible.
> 
> This doesn't help me to understand why the polarity stuff is useful
> though. Does this log say that the motivation to support changing
> polarity was to simulate "inversion of the duty cycle"? What exactly is
> "inversion of the duty cycle"? Just
> 
> 	duty_cycle = period - duty_cycle;
> 
> ? Why does this have to be simulated, the API was able to do this
> without any simulation before?
> 
> Is there a single use-case that can be done with polarity support in the
> API that isn't possible without it? I cannot imagine there is any but
> would like to learn and understand why this is valuable.

See my earlier mail which will hopefully answer these questions.

> Currently there is no user of pwm_set_polarity(), I suggest to remove it
> to not give users an incentive to still rely on the non-atomic API.

Yeah, that's probably a good idea. The long-term plan was always to get
rid of the legacy API, but I haven't gotten around to that yet.

> Also among the users of pwm_apply_state there is none which makes use of
> the polarity setting.

Well, this is primarily because our current users don't care much about
the polarity setting, so they just go with whatever initial setting they
get (usually via pwm_get_args()). These correspond to whatever was
specified in the flags of the PWM specifier in DT or in the lookup table
for non-DT devices (see for example of_pwm_xlate_with_flags()).

There is one user that allows changing the polarity, and that's sysfs.

Thierry
Uwe Kleine-König Oct. 15, 2018, 12:33 p.m. | #29
On Mon, Oct 15, 2018 at 11:22:13AM +0200, Thierry Reding wrote:
> On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > > [...]
> > > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > > >> inverted logic. The framework could just apply
> > > > > >>
> > > > > >> 	duty = period - duty;
> > > > > >>
> > > > > 
> > > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > > me it seems perfectly OK that that information can be propagated from
> > > > > the upper SW levels to the lowest one.
> > > > 
> > > > If the hardware capability is useful I fully agree. But if inversion can
> > > > be accomplished by a chip that doesn't support inversion in hardware by
> > > > just using duty = period - duty (and so can be accomplished by a chip
> > > > that has inversion support without making use of it) then the drivers
> > > > shouldn't be confronted with it.
> > > 
> > > These two are orthogonal problems. If all you care about is the power
> > > output of the PWM (as is the case for backlights, LEDs or fans, for
> > > example), then inverted polarity has the same effect as duty = period -
> > > duty.
> > > 
> > > However, the two don't result in identical waveforms.
> > 
> > OK, let's find the difference then. Consider a pwm that is off at start,
> > then it's configured to run at 66% duty cycle at time A and then it's
> > disabled again at time B.
> > 
> > The resulting wave form is:
> > 
> > ______________       __       __       __       _____________________
> >               \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > With the ^ markers pointing out where the periods started.
> > 
> > Correct?
> > 
> > Now with the duty = period - duty trick the wave would look as follows:
> > 
> > 
> > _________________       __       __       __       __________________
> >                  \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > Apart from a shift I cannot see any difference. I confirm that this
> > might be bad if there are two (or more) PWMs that are expected to run in
> > sync, but given that the current API doesn't provide the possibility to
> > map such a use case this is irrelevant.
> > 
> > Another difference I confirm there might be is that this might result in
> > one more (or less) periods depending on timing and the capabilities of
> > the hardware. Is this bad? I'd say it isn't.
> > 
> > What am I missing?
> 
> I don't think that's entirely correct. For normal PWM with 66% duty
> cycle you'd get something like this:
> 
>      ____    ____    ____
>     /    \__/    \__/    \__   (1)
>     ^       ^       ^
> 
> For "emulated" inversion (duty cycle = period - duty cycle, i.e. 33%
> duty cycle) you'd get this:
> 
>      __      __      __
>     /  \____/  \____/  \____   (2)
>     ^       ^       ^
> 
> For a truly inversed PWM (duty cycle = 33%), it would look like this:
> 
>           __      __      __
>     \____/  \____/  \____/     (3)
>     ^       ^       ^
> 
> If you emulate inversion of the truly inversed PWM, you'd get this:
> 
>         ____    ____    ____
>     \__/    \__/    \__/       (4)
>     ^       ^       ^
> 
> As you can see, all of the above are slightly different. As you can see,
> and you already noted that, the emulated inversion (2) and the true
> inversion (3) are almost identical, except that they have a phase shift
> that is equivalent to the duty cycle. The same is true of (1) and (4).
> The phase shift doesn't affect the power output of the signal, so (1)
> and (4) as well as (2) and (3) have the same power output. This is the
> reason why the difference doesn't matter for backlight or LEDs, because
> brightness is proportional to the power output.
> 
> However, you can also see that (2) and (3) are different in how the
> period starts. (2) starts with a rising edge while (3) starts with a
> falling edge.
> 
> Now, for any use-cases where the only thing we care about is the power
> output, my opinion is that consumers should implement the inversion. If
> the hardware is capable of inversion, then by all means they should use
> that in the implementation. But, a lot of hardware is not capable of
> doing the inversion, so they can still fall back to emulating the
> inversion. Either way, the important point here is that the consumer
> driver (pwm-backlight, leds-pwm, ...) knows that only power output is
> relevant, so it is the consumer drivers that should make the decision.
> 
> On the other hand, other use-cases may rely on the exact shape of the
> PWM signal, so if the edges are important, then they need to rely on the
> hardware inversion for proper functionality. My understanding is that we
> don't have any of those users in the kernel, but my recollection is that
> people use the sysfs interfaces to make use of this.

Your figures suggest that the first edge is different, too, but that's
wrong because you didn't take the idle state into account that was
active before the first period. If the output was already high before
there cannot be a raising edge. When this is corrected in your figures
they look as follows:

1) uninverted 
       ____    ____    ____
   ___/    \__/    \__/    \__
      ^       ^       ^

2) emulated inversion

   ______      __      __
         \____/  \____/  \____
      ^       ^       ^

3) hw inversion:
    __      __      __
      \____/  \____/  \____/
      ^       ^       ^

4) hw + emulated inversion:

          ____    ____    ____
   ______/    \__/    \__/       (4)
      ^       ^       ^

So the order of the edges is identical for hardware inversion and
software inversion.

The only difference is that with hardware inversion the first edge might
come earlier. But with the current PWM API you cannot make use of this
"advantage" because there is no way for the user to sync the period of
the PWM to anything else (e.g. another PWM). Also typically for a motor
control you want two PWMs running with a phase shift like:

         ___     ___     ___
    ____/   \___/   \___/
        ^  ___  ^  ___  ^
    ______/   \___/   \___/
          ^       ^       ^

So independant of inversion or not the periods of the two signals must
be shifted by (say) $period/2. How do you achive that? I'd say the best
you can do is:

	local_irq_disable();
	pwm_enable(pwm1);
	delay(period / 2);
	pwm_enable(pwm2);
	local_irq_enable();

Now given that it can take some time until pwm_enable() returns, this is
poor timing and you'd have to reduce the delay (by experimenting). If
you want to do that from userspace via sysfs you cannot even disable
irqs and the timing depends on how many ethernet packages are processed
between your two syscalls.

Trying this without additional hardware capabilities (e.g. a counter
shared by two PWMs) is broken. As the PWM API doesn't expose such
capabilities changing how inversion is implemented won't break any user
that isn't already broken before.

Best regards
Uwe

Patch

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 7f5a18fa305b..55db3b63add8 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -429,7 +429,6 @@  static void rx1950_lcd_power(int enable)
 		/* GPB1->OUTPUT, GPB1->0 */
 		gpio_direction_output(S3C2410_GPB(1), 0);
 		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
-		pwm_disable(lcd_pwm);
 
 		/* GPC0->0, GPC10->0 */
 		gpio_direction_output(S3C2410_GPC(0), 0);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index b443278e569c..a2b0e5f74ac8 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -784,8 +784,6 @@  static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta
 
 	/* Disable the backlight */
 	pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
-	usleep_range(2000, 3000);
-	pwm_disable(panel->backlight.pwm);
 }
 
 void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7838af58f92d..0f1b597bb252 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -51,7 +51,7 @@  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	pwm_init_state(ctx->pwm, &state);
 	period = ctx->pwm->args.period;
 	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
-	state.enabled = pwm ? true : false;
+	state.enabled = true;
 
 	ret = pwm_apply_state(ctx->pwm, &state);
 	if (!ret)
@@ -244,8 +244,7 @@  static int pwm_fan_probe(struct platform_device *pdev)
 						       ctx, pwm_fan_groups);
 	if (IS_ERR(hwmon)) {
 		dev_err(&pdev->dev, "Failed to register hwmon device\n");
-		ret = PTR_ERR(hwmon);
-		goto err_pwm_disable;
+		return PTR_ERR(hwmon);
 	}
 
 	ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx);
@@ -260,20 +259,13 @@  static int pwm_fan_probe(struct platform_device *pdev)
 		if (IS_ERR(cdev)) {
 			dev_err(&pdev->dev,
 				"Failed to register pwm-fan as cooling device");
-			ret = PTR_ERR(cdev);
-			goto err_pwm_disable;
+			return PTR_ERR(cdev);
 		}
 		ctx->cdev = cdev;
 		thermal_cdev_update(cdev);
 	}
 
 	return 0;
-
-err_pwm_disable:
-	state.enabled = false;
-	pwm_apply_state(ctx->pwm, &state);
-
-	return ret;
 }
 
 static int pwm_fan_remove(struct platform_device *pdev)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index df80c89ebe7f..755e13721a16 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -40,11 +40,6 @@  static void __led_pwm_set(struct led_pwm_data *led_dat)
 	int new_duty = led_dat->duty;
 
 	pwm_config(led_dat->pwm, new_duty, led_dat->period);
-
-	if (new_duty == 0)
-		pwm_disable(led_dat->pwm);
-	else
-		pwm_enable(led_dat->pwm);
 }
 
 static int led_pwm_set(struct led_classdev *led_cdev,
@@ -105,6 +100,8 @@  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		return ret;
 	}
 
+	pwm_enable(led_data->pwm);
+
 	led_data->cdev.brightness_set_blocking = led_pwm_set;
 
 	/*

(This should better make use of pwm_state instead.)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..38f17c8b6364 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -864,6 +864,8 @@  void pwm_put(struct pwm_device *pwm)
 	if (!pwm)
 		return;
 
+	pwm_disable(pwm);
+
 	mutex_lock(&pwm_lock);
 
 	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index d7f5f7de030d..61125052ac3d 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -306,8 +306,6 @@  static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
 	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
 
-	pwm_disable(pwm);
-	pwm_set_duty_cycle(pwm, 0);
 	clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
 }
 
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 2030a6b77a09..293abb823fde 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -165,12 +165,15 @@  static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 {
 	unsigned int period = pchip->pdata->pwm_period;
 	unsigned int duty = br * period / br_max;
+	struct pwm_state state;
 
-	pwm_config(pchip->pwmd, duty, period);
-	if (duty)
-		pwm_enable(pchip->pwmd);
-	else
-		pwm_disable(pchip->pwmd);
+	pwm_get_state(pchip->pwmd);
+
+	state.duty_cycle = duty;
+	state.period = period;
+	state.enabled = true;
+
+	pwm_apply_state(pchip->pwmd, &state);
 }
 
 /* update and get brightness */
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 73612485ed07..ba4e71b99c45 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -240,6 +240,7 @@  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 	unsigned int period = lp->pdata->period_ns;
 	unsigned int duty = br * period / max_br;
 	struct pwm_device *pwm;
+	struct pwm_state state;
 
 	/* request pwm device with the consumer name */
 	if (!lp->pwm) {
@@ -248,19 +249,15 @@  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 			return;
 
 		lp->pwm = pwm;
-
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pwm);
 	}
 
-	pwm_config(lp->pwm, duty, period);
-	if (duty)
-		pwm_enable(lp->pwm);
-	else
-		pwm_disable(lp->pwm);
+	pwm_get_state(lp->pwm, &state);
+
+	state.duty = duty;
+	state.period = period;
+	state.enabled = true;
+
+	pwm_apply_state(lp->pwm, &state);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 44ac5bde4e9d..e60831f3e29a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -46,18 +46,42 @@  struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+{
+	unsigned int lth = pb->lth_brightness;
+	u64 duty_cycle;
+
+	if (pb->levels)
+		duty_cycle = pb->levels[brightness];
+	else
+		duty_cycle = brightness;
+
+	duty_cycle *= pb->period - lth;
+	do_div(duty_cycle, pb->scale);
+
+	return duty_cycle + lth;
+}
+
 static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 {
 	int err;
+	int duty_cycle;
+	struct pwm_state state;
 
 	if (pb->enabled)
 		return;
 
+	duty_cycle = compute_duty_cycle(pb, brightness);
+
 	err = regulator_enable(pb->power_supply);
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
-	pwm_enable(pb->pwm);
+	pwm_get_state(pb->pwm, &state);
+	state.duty_cycle = duty_cycle;
+	state.period = pb->period;
+	state.enabled = true;
+	pwm_apply_state(pb->pwm, &state);
 
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
@@ -80,33 +104,15 @@  static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 		msleep(pb->pwm_off_delay);
 
 	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
 
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
-{
-	unsigned int lth = pb->lth_brightness;
-	u64 duty_cycle;
-
-	if (pb->levels)
-		duty_cycle = pb->levels[brightness];
-	else
-		duty_cycle = brightness;
-
-	duty_cycle *= pb->period - lth;
-	do_div(duty_cycle, pb->scale);
-
-	return duty_cycle + lth;
-}
-
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = bl_get_data(bl);
 	int brightness = bl->props.brightness;
-	int duty_cycle;
 
 	if (bl->props.power != FB_BLANK_UNBLANK ||
 	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
@@ -116,11 +122,9 @@  static int pwm_backlight_update_status(struct backlight_device *bl)
 	if (pb->notify)
 		brightness = pb->notify(pb->dev, brightness);
 
-	if (brightness > 0) {
-		duty_cycle = compute_duty_cycle(pb, brightness);
-		pwm_config(pb->pwm, duty_cycle, pb->period);
+	if (brightness > 0)
 		pwm_backlight_power_on(pb, brightness);
-	} else
+	else
 		pwm_backlight_power_off(pb);
 
 	if (pb->notify_after)
@@ -353,12 +357,6 @@  static int pwm_backlight_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "got pwm for backlight\n");
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(pb->pwm);
-
 	/*
 	 * The DT case will set the pwm_period_ns field to 0 and store the
 	 * period, parsed from the DT, in the PWM device. For the non-DT case,