[PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Uwe Kleine-König]

Hello,

on arm/i.MX28 the leds connected to a pwm are still broken and it's more
than three years ago that I came up with these patches. I still consider
them to do the right thing and they fix a real bug.

Old threads to this topic include:

	http://thread.gmane.org/gmane.linux.kernel/1381289
	http://thread.gmane.org/gmane.linux.ports.arm.kernel/282593/focus=282596
	http://thread.gmane.org/gmane.linux.pwm/1749

Uwe Kleine-König (2):
  pwm/doc: Clearify that the pin state after pwm_disable is undefined
  leds/pwm: Don't disable pwm when setting brightness to 0

 Documentation/pwm.txt   |  5 +++++
 drivers/leds/leds-pwm.c | 10 +++++-----
 2 files changed, 10 insertions(+), 5 deletions(-)

-- 
2.1.4

[PATCH 1/2] pwm/doc: Clearify that the pin state after pwm_disable is undefined

[Uwe Kleine-König]

This makes assumptions on the behaviour of the pwm API explicit.

Note that there are drivers that assume that pwm_disable does result in
a stable output matching the last pwm_config; leds-pwm is an example
that is fixed in a followup patch.

On arm/i.MX28 it can really happen that after

	pwm_config(led_dat->pwm, 0, 100);
	pwm_disable(led_dat->pwm);

the output is stuck at 1. That's because the pwm only works with the
newly configured config when a period is over for the previously
configured setting to prevent spikes.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 Documentation/pwm.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index ca895fd211e4..abdd21d047ca 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -46,6 +46,11 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
 
 To start/stop toggling the PWM output use pwm_enable()/pwm_disable().
 
+Note that after calling pwm_disable() it's undefined if the output stops in a
+high or low state, even if the duty cycle is set to 0% or 100%. So don't call
+pwm_disable() if there is a need for a specific level. The same applies when
+pwm_enable() was called, but pwm_config() was not.
+
 Using PWMs with the sysfs interface
 -----------------------------------
 
-- 
2.1.4

[PATCH 2/2] leds/pwm: Don't disable pwm when setting brightness to 0

[Uwe Kleine-König]

This fixes disabling the LED on i.MX28. The PWM hardware delays using
the newly set pwm-config until the beginning of a new period. It's very
likely that pwm_disable is called before the current period ends. In
case the LED was on brightness=max before the LED stays on because in
the disabled PWM block the period never ends.

Also only call pwm_enable only once in the probe call back and the
matching pwm_disable in .remove(). Moreover the pwm is explicitly
initialized to off.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/leds-pwm.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index f668500a2157..5dae0d2dc3dc 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -44,11 +44,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 void led_pwm_work(struct work_struct *work)
@@ -93,6 +88,7 @@ static void led_pwm_cleanup(struct led_pwm_priv *priv)
 		led_classdev_unregister(&priv->leds[priv->num_leds].cdev);
 		if (priv->leds[priv->num_leds].can_sleep)
 			cancel_work_sync(&priv->leds[priv->num_leds].work);
+		pwm_disable(priv->leds[i].pwm);
 	}
 }
 
@@ -132,6 +128,10 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	if (!led_data->period && (led->pwm_period_ns > 0))
 		led_data->period = led->pwm_period_ns;
 
+	pwm_config(led_data->pwm, led_data->active_low ? led_data->period: 0,
+		   led_data->period);
+	pwm_enable(led_data->pwm);
+
 	ret = led_classdev_register(dev, &led_data->cdev);
 	if (ret == 0) {
 		priv->num_leds++;
-- 
2.1.4

Re: [PATCH 2/2] leds/pwm: Don't disable pwm when setting brightness to 0

[Uwe Kleine-König]

On Thu, Feb 12, 2015 at 10:44:50AM +0100, Uwe Kleine-König wrote:
> This fixes disabling the LED on i.MX28. The PWM hardware delays using
> the newly set pwm-config until the beginning of a new period. It's very
> likely that pwm_disable is called before the current period ends. In
> case the LED was on brightness=max before the LED stays on because in
> the disabled PWM block the period never ends.
> 
> Also only call pwm_enable only once in the probe call back and the
> matching pwm_disable in .remove(). Moreover the pwm is explicitly
> initialized to off.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
sigh, I just noticed I did my compile test with LEDS_PWM off. This
fails to build, but as this patch (soft) depends on patch 1 anyhow, I
will resend once patch 1 is ok to go in. (I hope to reach this state!)

Best regards
Uwe


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

Re: [PATCH 2/2] leds/pwm: Don't disable pwm when setting brightness to 0

[Stefan Wahren]

Hi Uwe,

> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> hat am 12. Februar 2015 um
> 10:44 geschrieben:
>
>
> This fixes disabling the LED on i.MX28. The PWM hardware delays using
> the newly set pwm-config until the beginning of a new period. It's very
> likely that pwm_disable is called before the current period ends. In
> case the LED was on brightness=max before the LED stays on because in
> the disabled PWM block the period never ends.
>
> Also only call pwm_enable only once in the probe call back and the
> matching pwm_disable in .remove(). Moreover the pwm is explicitly
> initialized to off.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/leds/leds-pwm.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index f668500a2157..5dae0d2dc3dc 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -44,11 +44,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 void led_pwm_work(struct work_struct *work)
> @@ -93,6 +88,7 @@ static void led_pwm_cleanup(struct led_pwm_priv *priv)
> led_classdev_unregister(&priv->leds[priv->num_leds].cdev);
> if (priv->leds[priv->num_leds].can_sleep)
> cancel_work_sync(&priv->leds[priv->num_leds].work);
> + pwm_disable(priv->leds[i].pwm);
> }
> }
>

After replacing "i" with "priv->num_leds" in this patch we are able to use pwm
led with trigger heartbeat.

This is the functional part of this issue, since the warning has been fixed [1]

Thanks Stefan

[1] -
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-April/244925.html

Re: [PATCH 2/2] leds/pwm: Don't disable pwm when setting brightness to 0

[Uwe Kleine-König]

Hello Stefan,

On Tue, Feb 24, 2015 at 07:56:52PM +0100, Stefan Wahren wrote:
> After replacing "i" with "priv->num_leds" in this patch we are able to use pwm
> led with trigger heartbeat.
Right, that's what I did in my tree to to please the build robot :-)

> This is the functional part of this issue, since the warning has been fixed [1]
Can I interpret this as an Tested-by: for this patch and an Ack for
patch 1?

Best regards
Uwe

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

Re: [PATCH 2/2] leds/pwm: Don't disable pwm when setting brightness to 0

[Stefan Wahren]

Hi Uwe,

Am 24.02.2015 um 20:06 schrieb Uwe Kleine-König:
> Hello Stefan,
>
> On Tue, Feb 24, 2015 at 07:56:52PM +0100, Stefan Wahren wrote:
>> After replacing "i" with "priv->num_leds" in this patch we are able to use pwm
>> led with trigger heartbeat.
> Right, that's what I did in my tree to to please the build robot :-)

in that case, here is my:

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

>> This is the functional part of this issue, since the warning has been fixed [1]
> Can I interpret this as an Tested-by: for this patch and an Ack for
> patch 1?

Sorry, but i don't think i'm the right person for acking patch 1.

Stefan

> Best regards
> Uwe
>

Re: [PATCH 2/2] leds/pwm: Don't disable pwm when setting brightness to 0

[Uwe Kleine-König]

Hello Stefan,

On Wed, Feb 25, 2015 at 09:13:26AM +0100, Stefan Wahren wrote:
> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Thanks.

> >> This is the functional part of this issue, since the warning has been fixed [1]
> > Can I interpret this as an Tested-by: for this patch and an Ack for
> > patch 1?
> 
> Sorry, but i don't think i'm the right person for acking patch 1.
IMHO it doesn't matter who you are for an ack. You can consider it right
and tell it. How much this ack is useful to convince a maintainer to
take this patch is a different story.

But given that I try to fix the problem at hand for >2 years and I'm
stuck because Thierry isn't convinced that it's a correct way to go
forward but also doesn't come up with an alternative I'm happy about
everyone who gives his/her opinion. Still more if the opinion matches
mine.

Thanks
Uwe

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

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Uwe Kleine-König]

[Cc: += akpm]

On Thu, Feb 12, 2015 at 10:44:48AM +0100, Uwe Kleine-König wrote:
> on arm/i.MX28 the leds connected to a pwm are still broken and it's more
> than three years ago that I came up with these patches. I still consider
> them to do the right thing and they fix a real bug.
I'm really frustrated here. I want to fix a real bug, made several
suggestions (with patches) how to fix it and still have to include my
local patches in each project that uses leds on i.MX28's pwm output.

Thierry, how can we get this resolved?

Best regards
Uwe

> Old threads to this topic include:
> 
> 	http://thread.gmane.org/gmane.linux.kernel/1381289
> 	http://thread.gmane.org/gmane.linux.ports.arm.kernel/282593/focus=282596
> 	http://thread.gmane.org/gmane.linux.pwm/1749
> 
> Uwe Kleine-König (2):
>   pwm/doc: Clearify that the pin state after pwm_disable is undefined
>   leds/pwm: Don't disable pwm when setting brightness to 0
> 
>  Documentation/pwm.txt   |  5 +++++
>  drivers/leds/leds-pwm.c | 10 +++++-----
>  2 files changed, 10 insertions(+), 5 deletions(-)

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

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Thierry Reding]

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

On Wed, Mar 25, 2015 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> [Cc: += akpm]
> 
> On Thu, Feb 12, 2015 at 10:44:48AM +0100, Uwe Kleine-König wrote:
> > on arm/i.MX28 the leds connected to a pwm are still broken and it's more
> > than three years ago that I came up with these patches. I still consider
> > them to do the right thing and they fix a real bug.
> I'm really frustrated here. I want to fix a real bug, made several
> suggestions (with patches) how to fix it and still have to include my
> local patches in each project that uses leds on i.MX28's pwm output.
> 
> Thierry, how can we get this resolved?

We have a couple of other drivers already solving similar issues. Look
for example at the pwm-bcm-kona driver. It explicitly sets the duty
cycle to 0 on ->disable() and then waits for some time before actually
disabling the clock (specifically to avoid a similar than you seem to
have on i.MX). See also the notes near the top of the driver source
file.

Another example is pwm-samsung. It also has code specifically aimed at
making sure the signal doesn't unexpectedly stay high after calling
pwm_disable(). See also the commit message of:

	6da89312a39b pwm: samsung: Fix output race on disabling

Both of these examples sound very similar to what you're describing, so
I think it'd be best to follow these examples and fix the i.MX driver to
behave as expected.

Irrespective of the behaviour of current drivers, the assumptions are
already codified in the user drivers and they all assume that calling
pwm_disable() means the PWM is off.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Uwe Kleine-König]

Hello Thierry,

On Wed, Mar 25, 2015 at 01:00:40PM +0100, Thierry Reding wrote:
> On Wed, Mar 25, 2015 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> > On Thu, Feb 12, 2015 at 10:44:48AM +0100, Uwe Kleine-König wrote:
> > > on arm/i.MX28 the leds connected to a pwm are still broken and it's more
> > > than three years ago that I came up with these patches. I still consider
> > > them to do the right thing and they fix a real bug.
> > I'm really frustrated here. I want to fix a real bug, made several
> > suggestions (with patches) how to fix it and still have to include my
> > local patches in each project that uses leds on i.MX28's pwm output.
> > 
> > Thierry, how can we get this resolved?
> 
> We have a couple of other drivers already solving similar issues. Look
> for example at the pwm-bcm-kona driver. It explicitly sets the duty
> cycle to 0 on ->disable() and then waits for some time before actually
> disabling the clock (specifically to avoid a similar than you seem to
> have on i.MX). See also the notes near the top of the driver source
> file.
> 
> Another example is pwm-samsung. It also has code specifically aimed at
> making sure the signal doesn't unexpectedly stay high after calling
> pwm_disable(). See also the commit message of:
> 
> 	6da89312a39b pwm: samsung: Fix output race on disabling
> 
> Both of these examples sound very similar to what you're describing, so
> I think it'd be best to follow these examples and fix the i.MX driver to
> behave as expected.
Seeing that more hardware behaves like the mxs pwm makes me still more
convinced that the pwm core should be aware of things like double
buffering and different behaviour for disabling. Adding code to fulfil
the API/user expectation to all three (and maybe more?) drivers seems
wrong, don't you think?

There are IMHO two possibilities to define the outcome of
pwm_config(duty=0) + pwm_disable():

 a) the output must be 0
 b) the output is undefined

For a) there are two further cases:

 a1) each driver has to assert a) on its own
 a2) the pwm framework knows about a few common behaviours and can
     handle them.

Currently we are at a1) (which is undocumented in Documentation/pwm.txt
btw). IMHO a1) is the worst of the three alternatives!

BTW, what is the (expected but also undocumented) outcome of

	pwm_set_polarity(polarity=PWM_POLARITY_INVERSED);
	pwm_config(duty=0)
	pwm_disable()

?

> Irrespective of the behaviour of current drivers, the assumptions are
> already codified in the user drivers and they all assume that calling
> pwm_disable() means the PWM is off.
Well, if you call pwm_disable for the mxs pwm, it is disabled
instantly. Disabling just doesn't imply that the output goes to 0.

The problem at hand is updating the configuration doesn't end the
current period. And then stopping the clock lets the output keep the
last driven value.

Considering

	$ git grep \\\<pwm_disable | wc -l
	34

going through all callers and fixing them up for changed semantics
doesn't seem too hard. Still more as there are several false hits
(approx 50%).

Best regards
Uwe

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

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Thierry Reding]

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

On Fri, Mar 27, 2015 at 09:59:43AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Mar 25, 2015 at 01:00:40PM +0100, Thierry Reding wrote:
> > On Wed, Mar 25, 2015 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> > > On Thu, Feb 12, 2015 at 10:44:48AM +0100, Uwe Kleine-König wrote:
> > > > on arm/i.MX28 the leds connected to a pwm are still broken and it's more
> > > > than three years ago that I came up with these patches. I still consider
> > > > them to do the right thing and they fix a real bug.
> > > I'm really frustrated here. I want to fix a real bug, made several
> > > suggestions (with patches) how to fix it and still have to include my
> > > local patches in each project that uses leds on i.MX28's pwm output.
> > > 
> > > Thierry, how can we get this resolved?
> > 
> > We have a couple of other drivers already solving similar issues. Look
> > for example at the pwm-bcm-kona driver. It explicitly sets the duty
> > cycle to 0 on ->disable() and then waits for some time before actually
> > disabling the clock (specifically to avoid a similar than you seem to
> > have on i.MX). See also the notes near the top of the driver source
> > file.
> > 
> > Another example is pwm-samsung. It also has code specifically aimed at
> > making sure the signal doesn't unexpectedly stay high after calling
> > pwm_disable(). See also the commit message of:
> > 
> > 	6da89312a39b pwm: samsung: Fix output race on disabling
> > 
> > Both of these examples sound very similar to what you're describing, so
> > I think it'd be best to follow these examples and fix the i.MX driver to
> > behave as expected.
> Seeing that more hardware behaves like the mxs pwm makes me still more
> convinced that the pwm core should be aware of things like double
> buffering and different behaviour for disabling. Adding code to fulfil
> the API/user expectation to all three (and maybe more?) drivers seems
> wrong, don't you think?

Erm... no. The whole point of a generic API is to abstract these kinds
of differences between various drivers. The goal is that users of the
API don't have to worry about the differences anymore and can expect a
certain behaviour.

> There are IMHO two possibilities to define the outcome of
> pwm_config(duty=0) + pwm_disable():
> 
>  a) the output must be 0
>  b) the output is undefined
> 
> For a) there are two further cases:
> 
>  a1) each driver has to assert a) on its own
>  a2) the pwm framework knows about a few common behaviours and can
>      handle them.
> 
> Currently we are at a1) (which is undocumented in Documentation/pwm.txt
> btw). IMHO a1) is the worst of the three alternatives!

Why do you think it is the worst? If we define the behaviour as b) what
does that gain us? Why would users want to call these functions if their
behaviour is undefined? What that will result in is that code happens to
work with some PWM drivers but not with others. Then drivers will go and
implement workarounds that work only for specific drivers and so on.

a2) isn't much better. It hides the specifics from the users, but at the
same time it complicates the core because you're trying to generically
describe something that's inherently not generic. Device-specific code
belongs in drivers, not in the framework.

> BTW, what is the (expected but also undocumented) outcome of
> 
> 	pwm_set_polarity(polarity=PWM_POLARITY_INVERSED);
> 	pwm_config(duty=0)
> 	pwm_disable()

I would expect the PWM signal to be constant high after that. With
normal polarity duty-cycle of 0 and disable means constant low, so
inversing that gives constant 1, doesn't it?

> > Irrespective of the behaviour of current drivers, the assumptions are
> > already codified in the user drivers and they all assume that calling
> > pwm_disable() means the PWM is off.
> Well, if you call pwm_disable for the mxs pwm, it is disabled
> instantly. Disabling just doesn't imply that the output goes to 0.

Right, and that's precisely what other drivers have extra code for to
handle. If you look at the pwm-bcm-kona driver it has an extra delay of
400 ns to make sure that the waveform is really updated.

> The problem at hand is updating the configuration doesn't end the
> current period. And then stopping the clock lets the output keep the
> last driven value.

So why can't you just block in ->disable() until you know the output is
actually what's been requested? If you can't read that out from the
hardware, then simply wait for a whole period before stopping the clock.

> Considering
> 
> 	$ git grep \\\<pwm_disable | wc -l
> 	34
> 
> going through all callers and fixing them up for changed semantics
> doesn't seem too hard. Still more as there are several false hits
> (approx 50%).

Keep in mind that you'd need to retest all combinations of these users
with all PWM drivers to make sure you're not inadvertently breaking
existing setups.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Uwe Kleine-König]

[added the people being involved with the other two drivers in question,
maybe you want to comment?]

Hello Thierry,

On Fri, Mar 27, 2015 at 12:26:08PM +0100, Thierry Reding wrote:
> On Fri, Mar 27, 2015 at 09:59:43AM +0100, Uwe Kleine-König wrote:
> > On Wed, Mar 25, 2015 at 01:00:40PM +0100, Thierry Reding wrote:
> > > On Wed, Mar 25, 2015 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> > > > On Thu, Feb 12, 2015 at 10:44:48AM +0100, Uwe Kleine-König wrote:
> > > > > on arm/i.MX28 the leds connected to a pwm are still broken and it's more
> > > > > than three years ago that I came up with these patches. I still consider
> > > > > them to do the right thing and they fix a real bug.
> > > > I'm really frustrated here. I want to fix a real bug, made several
> > > > suggestions (with patches) how to fix it and still have to include my
> > > > local patches in each project that uses leds on i.MX28's pwm output.
> > > > 
> > > > Thierry, how can we get this resolved?
> > > 
> > > We have a couple of other drivers already solving similar issues. Look
> > > for example at the pwm-bcm-kona driver. It explicitly sets the duty
> > > cycle to 0 on ->disable() and then waits for some time before actually
> > > disabling the clock (specifically to avoid a similar than you seem to
> > > have on i.MX). See also the notes near the top of the driver source
> > > file.
> > > 
> > > Another example is pwm-samsung. It also has code specifically aimed at
> > > making sure the signal doesn't unexpectedly stay high after calling
> > > pwm_disable(). See also the commit message of:
> > > 
> > > 	6da89312a39b pwm: samsung: Fix output race on disabling
> > > 
> > > Both of these examples sound very similar to what you're describing, so
> > > I think it'd be best to follow these examples and fix the i.MX driver to
> > > behave as expected.
> > Seeing that more hardware behaves like the mxs pwm makes me still more
> > convinced that the pwm core should be aware of things like double
> > buffering and different behaviour for disabling. Adding code to fulfil
> > the API/user expectation to all three (and maybe more?) drivers seems
> > wrong, don't you think?
> 
> Erm... no. The whole point of a generic API is to abstract these kinds
> of differences between various drivers. The goal is that users of the
> API don't have to worry about the differences anymore and can expect a
> certain behaviour.
I agree to a certain degree here. Yes, an API is there to abstract
between different drivers. But if the content of an API that was once
been considered "generic" turns out to be not so generic after all, the
right thing to do is to change it. If it's possible to keep the
behaviour for users that's great, if not this shouldn't stop you.

> > There are IMHO two possibilities to define the outcome of
> > pwm_config(duty=0) + pwm_disable():
> > 
> >  a) the output must be 0
> >  b) the output is undefined
> > 
> > For a) there are two further cases:
> > 
> >  a1) each driver has to assert a) on its own
> >  a2) the pwm framework knows about a few common behaviours and can
> >      handle them.
> > 
> > Currently we are at a1) (which is undocumented in Documentation/pwm.txt
> > btw). IMHO a1) is the worst of the three alternatives!
> 
> Why do you think it is the worst? If we define the behaviour as b) what
> does that gain us? Why would users want to call these functions if their
The gain is that the three (or more?) pwm drivers don't need to special
case an artificial requirement of the framework. OK, users have to
adapt, but it's as simple as substituting all questionable calls to
pwm_disable by a call to pwm_config(duty=0).

> behaviour is undefined? What that will result in is that code happens to
> work with some PWM drivers but not with others. Then drivers will go and
> implement workarounds that work only for specific drivers and so on.
In this case the only "workaround" is to drop the pwm_disable. This is a
correct fix however with my suggested change.

> a2) isn't much better. It hides the specifics from the users, but at the
a1) hides the specifics, don't it? a2) explicitly states that there are
specifics and users should be aware. a1) *ignores* specifics at the cost
of additional effort for drivers where a1) is wrong in hardware!

> same time it complicates the core because you're trying to generically
> describe something that's inherently not generic. Device-specific code
You claim that "pwms yield a constant 0 on disable". I say: "It depends
on the pwm what happens if you disable it". Your claim is wrong for at
least three pwms. Mine holds for all. So which is more generic?

Also note that with a) the API has two properties that are bad for an
API:

 - there are two ways to request the output to be constant low:
    1) pwm_config(duty=0)
    2) pwm_config(duty=0) + pwm_disable()

 - pwm_disable fills two different purposes:
    1) The user has to (or can) use it as part of requesting a 0
    2) The user tells to not need the pwm any more

Both are fixed by my suggestion.

> belongs in drivers, not in the framework.
Device-specific code belongs into the framework if it is common enough
to allow simplification of several drivers. IMHO an API has two sides,
in the case of the pwm framework it's:

 - the drivers that use a pwm (e.g. drivers/leds/leds-pwm.c)
 - the drivers that know how to operate a pwm (e.g.
   drivers/pwm/pwm-mxs.c)

If the API pleases both sides that's better than only being nice to one
side.

> > > Irrespective of the behaviour of current drivers, the assumptions are
> > > already codified in the user drivers and they all assume that calling
> > > pwm_disable() means the PWM is off.
> > Well, if you call pwm_disable for the mxs pwm, it is disabled
> > instantly. Disabling just doesn't imply that the output goes to 0.
> 
> Right, and that's precisely what other drivers have extra code for to
> handle. If you look at the pwm-bcm-kona driver it has an extra delay of
> 400 ns to make sure that the waveform is really updated.
I claim it was already wrong to add this extra code to pwm-bcm-kona.
Also note that in some usecases this delay is not needed (i.e. depending
on where the delay is added, the user might not care if pwm_config is in
effect when pwm_config returns or pwm_disable doesn't need to result in
a 0 because it was just called in the error path of another driver).

> > The problem at hand is updating the configuration doesn't end the
> > current period. And then stopping the clock lets the output keep the
> > last driven value.
> 
> So why can't you just block in ->disable() until you know the output is
> actually what's been requested? If you can't read that out from the
> hardware, then simply wait for a whole period before stopping the clock.
I can, but that's not the point. It complicates the driver while the
availability of a few helper constructs in the framework could do this
for three drivers.

> > Considering
> > 
> > 	$ git grep \\\<pwm_disable | wc -l
> > 	34
> > 
> > going through all callers and fixing them up for changed semantics
> > doesn't seem too hard. Still more as there are several false hits
> > (approx 50%).
> 
> Keep in mind that you'd need to retest all combinations of these users
> with all PWM drivers to make sure you're not inadvertently breaking
> existing setups.
That's what we have release candidates for. If you really only want to
take changes that with a probability of 0 break any existing setups, we
can substitute you by a procmail rule that naks all patches.
This is about best effort. Yes, API changes are annoying, but in the
long run they are good.
Having said that, I don't consider my suggested change as "dangerous".
Changing the documented behaviour doesn't affect any setup. Then fixing
each bug that pops up is good enough.

Best regards
Uwe

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

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Thierry Reding]

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

On Fri, Mar 27, 2015 at 03:35:59PM +0100, Uwe Kleine-König wrote:
> [added the people being involved with the other two drivers in question,
> maybe you want to comment?]
> 
> Hello Thierry,
> 
> On Fri, Mar 27, 2015 at 12:26:08PM +0100, Thierry Reding wrote:
> > On Fri, Mar 27, 2015 at 09:59:43AM +0100, Uwe Kleine-König wrote:
> > > On Wed, Mar 25, 2015 at 01:00:40PM +0100, Thierry Reding wrote:
> > > > On Wed, Mar 25, 2015 at 11:14:28AM +0100, Uwe Kleine-König wrote:
> > > > > On Thu, Feb 12, 2015 at 10:44:48AM +0100, Uwe Kleine-König wrote:
> > > > > > on arm/i.MX28 the leds connected to a pwm are still broken and it's more
> > > > > > than three years ago that I came up with these patches. I still consider
> > > > > > them to do the right thing and they fix a real bug.
> > > > > I'm really frustrated here. I want to fix a real bug, made several
> > > > > suggestions (with patches) how to fix it and still have to include my
> > > > > local patches in each project that uses leds on i.MX28's pwm output.
> > > > > 
> > > > > Thierry, how can we get this resolved?
> > > > 
> > > > We have a couple of other drivers already solving similar issues. Look
> > > > for example at the pwm-bcm-kona driver. It explicitly sets the duty
> > > > cycle to 0 on ->disable() and then waits for some time before actually
> > > > disabling the clock (specifically to avoid a similar than you seem to
> > > > have on i.MX). See also the notes near the top of the driver source
> > > > file.
> > > > 
> > > > Another example is pwm-samsung. It also has code specifically aimed at
> > > > making sure the signal doesn't unexpectedly stay high after calling
> > > > pwm_disable(). See also the commit message of:
> > > > 
> > > > 	6da89312a39b pwm: samsung: Fix output race on disabling
> > > > 
> > > > Both of these examples sound very similar to what you're describing, so
> > > > I think it'd be best to follow these examples and fix the i.MX driver to
> > > > behave as expected.
> > > Seeing that more hardware behaves like the mxs pwm makes me still more
> > > convinced that the pwm core should be aware of things like double
> > > buffering and different behaviour for disabling. Adding code to fulfil
> > > the API/user expectation to all three (and maybe more?) drivers seems
> > > wrong, don't you think?
> > 
> > Erm... no. The whole point of a generic API is to abstract these kinds
> > of differences between various drivers. The goal is that users of the
> > API don't have to worry about the differences anymore and can expect a
> > certain behaviour.
> I agree to a certain degree here. Yes, an API is there to abstract
> between different drivers. But if the content of an API that was once
> been considered "generic" turns out to be not so generic after all, the
> right thing to do is to change it. If it's possible to keep the
> behaviour for users that's great, if not this shouldn't stop you.

What can possibly be more generic than:

	pwm_get()
	pwm_config()
	pwm_enable()
	pwm_disable()
	pwm_put()

? And you're not even arguing that the API isn't generic. What you're
complaining about is that the assumptions that it makes aren't what your
hardware does. Those are two orthogonal things.

> > > There are IMHO two possibilities to define the outcome of
> > > pwm_config(duty=0) + pwm_disable():
> > > 
> > >  a) the output must be 0
> > >  b) the output is undefined
> > > 
> > > For a) there are two further cases:
> > > 
> > >  a1) each driver has to assert a) on its own
> > >  a2) the pwm framework knows about a few common behaviours and can
> > >      handle them.
> > > 
> > > Currently we are at a1) (which is undocumented in Documentation/pwm.txt
> > > btw). IMHO a1) is the worst of the three alternatives!
> > 
> > Why do you think it is the worst? If we define the behaviour as b) what
> > does that gain us? Why would users want to call these functions if their
> The gain is that the three (or more?) pwm drivers don't need to special
> case an artificial requirement of the framework. OK, users have to
> adapt, but it's as simple as substituting all questionable calls to
> pwm_disable by a call to pwm_config(duty=0).

There is no gain. If users suddenly have to care about hardware
specifics I call that a loss. Equivalently if the PWM core framework
needs to know about these specifics that's also a loss because it puts
complexity in the core where device-specific properties clearly
shouldn't be handled.

> > behaviour is undefined? What that will result in is that code happens to
> > work with some PWM drivers but not with others. Then drivers will go and
> > implement workarounds that work only for specific drivers and so on.
> In this case the only "workaround" is to drop the pwm_disable. This is a
> correct fix however with my suggested change.

pwm_config(0) and pwm_disable() aren't the same. If you simply drop
these function calls the PWM framework and drivers loose potentially
useful information.

> > a2) isn't much better. It hides the specifics from the users, but at the
> a1) hides the specifics, don't it?

Yes it does. And your point is? I'm saying a2) hides the specifics from
users and that's a good thing. But the disadvantage is that you put the
burden on the core to deal with something that you could deal with much
better in the driver. There's no reason to do that.

> a2) explicitly states that there are specifics and users should be
> aware.

That's not how I read a2). I read it as the PWM core knowing the details
of drivers and internally handling what's necessary to hide them from
the users. There is absolutely no reason why users should have to care
about what happens if you turn off a clock immediately after applying a
new configuration. That's *exactly* what the API is supposed to hide.

> a1) *ignores* specifics at the cost of additional effort for drivers
> where a1) is wrong in hardware!

Oh hey, there's a good definition of what a generic API is. It is doing
exactly what it should do, why are you complaining?

> > same time it complicates the core because you're trying to generically
> > describe something that's inherently not generic. Device-specific code
> You claim that "pwms yield a constant 0 on disable".

No, that's not what I'm claiming. I'm saying that "a PWM outputs a
constant 0" is what the PWM framework defines to happen on disable.

> I say: "It depends on the pwm what happens if you disable it".

Of course it depends on the hardware. But that's exactly what we have
the abstraction for: so that we can treat all PWM devices in the same
way.

> Your claim is wrong for at least three pwms. Mine holds for all. So
> which is more generic?

Of course your claim is generic because it doesn't specify anything. You
want pwm_disable() to have an undefined result and call that generic.
Here's an idea: why don't we define the result of all functions to be
undefined? That way we get a really generic API.

We also get an API that's completely useless.

> Also note that with a) the API has two properties that are bad for an
> API:
> 
>  - there are two ways to request the output to be constant low:
>     1) pwm_config(duty=0)
>     2) pwm_config(duty=0) + pwm_disable()

No, they are not the same. pwm_config() sets the duty-cycle but keeps
the PWM on. pwm_disable() disables the PWM. The waveform might be the
same, but the semantics are different. Subtly, but different.

>  - pwm_disable fills two different purposes:
>     1) The user has to (or can) use it as part of requesting a 0

It *can* use it if there's no need to keep the PWM running. For most
purposes there is no difference because the resulting signal is the
same. Think of pwm_disable() as suspending the PWM in addition to
keeping the signal at a constant low.

I'll grant you that for many use-cases the difference is insignificant
and most drivers will probably want to just always call:

	pwm_config(0)
	pwm_disable()

>     2) The user tells to not need the pwm any more

If you don't need the PWM any more you can just pwm_put() it. That's
different.

> Both are fixed by my suggestion.

Your suggestion doesn't fix anything. You keep saying that the API is
broken. But it isn't. By removing the pwm_disable() altogether you also
remove some amount of information that drivers may find useful.

> > belongs in drivers, not in the framework.
> Device-specific code belongs into the framework if it is common enough
> to allow simplification of several drivers. IMHO an API has two sides,
> in the case of the pwm framework it's:
> 
>  - the drivers that use a pwm (e.g. drivers/leds/leds-pwm.c)
>  - the drivers that know how to operate a pwm (e.g.
>    drivers/pwm/pwm-mxs.c)
> 
> If the API pleases both sides that's better than only being nice to one
> side.

I completely agree. But I don't see anything common here. There seem to
be two drivers now (pwm-bcm-kona and pwm-mxs) that require a delay after
applying a new configuration. There's over 30 PWM drivers, so 2 hardly
qualifies as "common".

Besides, the only commonality is that they require a certain time
between applying a configuration and disabling the PWM. The time isn't
constant, and while you say there's a correlation between the period and
the time it takes the configuration to apply on pwm-mxs the delay is
actually fixed for pwm-bcm-kona. Really, I don't see a way to add
support for this in the core in any beneficial way.

I don't get it. From what you're saying all that is required to fix your
driver is to add a single call to usleep_range() or msleep() with a
value that depends on the currently configured period. Why do you want
to make changes to the core for that?

> > > > Irrespective of the behaviour of current drivers, the assumptions are
> > > > already codified in the user drivers and they all assume that calling
> > > > pwm_disable() means the PWM is off.
> > > Well, if you call pwm_disable for the mxs pwm, it is disabled
> > > instantly. Disabling just doesn't imply that the output goes to 0.
> > 
> > Right, and that's precisely what other drivers have extra code for to
> > handle. If you look at the pwm-bcm-kona driver it has an extra delay of
> > 400 ns to make sure that the waveform is really updated.
> I claim it was already wrong to add this extra code to pwm-bcm-kona.
> Also note that in some usecases this delay is not needed (i.e. depending
> on where the delay is added, the user might not care if pwm_config is in
> effect when pwm_config returns or pwm_disable doesn't need to result in
> a 0 because it was just called in the error path of another driver).

Oh hey, we could add an asynchronous notification mechanism so that
users get notified when the configuration was actually applied...

> > > The problem at hand is updating the configuration doesn't end the
> > > current period. And then stopping the clock lets the output keep the
> > > last driven value.
> > 
> > So why can't you just block in ->disable() until you know the output is
> > actually what's been requested? If you can't read that out from the
> > hardware, then simply wait for a whole period before stopping the clock.
> I can, but that's not the point. It complicates the driver while the
> availability of a few helper constructs in the framework could do this
> for three drivers.

Feel free to give this a shot if you feel the need. I'm unlikely to
merge anything that complicates the core to save a couple of lines in
the driver code.

The way to create these helper constructs is by looking for common
patterns and then providing common code to do it. As of now I don't see
any such common patterns, so there's no need to provide helpers that may
end up unused or used by only a single driver.

> > > Considering
> > > 
> > > 	$ git grep \\\<pwm_disable | wc -l
> > > 	34
> > > 
> > > going through all callers and fixing them up for changed semantics
> > > doesn't seem too hard. Still more as there are several false hits
> > > (approx 50%).
> > 
> > Keep in mind that you'd need to retest all combinations of these users
> > with all PWM drivers to make sure you're not inadvertently breaking
> > existing setups.
> That's what we have release candidates for. If you really only want to
> take changes that with a probability of 0 break any existing setups, we
> can substitute you by a procmail rule that naks all patches.
> This is about best effort. Yes, API changes are annoying, but in the
> long run they are good.
> Having said that, I don't consider my suggested change as "dangerous".
> Changing the documented behaviour doesn't affect any setup. Then fixing
> each bug that pops up is good enough.

Since you seem to think that I oppose API changes on principle I should
clarify that I don't. I merely think that in this case it's completely
unnecessary and you've already admitted yourself that it's easy to make
your driver comply with the established assumptions.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/2] leds/pwm: don't call pwm_disable when setting brightness

[Sascha Hauer]

On Fri, Mar 27, 2015 at 04:43:03PM +0100, Thierry Reding wrote:
> On Fri, Mar 27, 2015 at 03:35:59PM +0100, Uwe Kleine-König wrote:
> > > Why do you think it is the worst? If we define the behaviour as b) what
> > > does that gain us? Why would users want to call these functions if their
> > The gain is that the three (or more?) pwm drivers don't need to special
> > case an artificial requirement of the framework. OK, users have to
> > adapt, but it's as simple as substituting all questionable calls to
> > pwm_disable by a call to pwm_config(duty=0).
> 
> There is no gain. If users suddenly have to care about hardware
> specifics I call that a loss. Equivalently if the PWM core framework
> needs to know about these specifics that's also a loss because it puts
> complexity in the core where device-specific properties clearly
> shouldn't be handled.

The easy way out for PWM users, the core and the driver is:

Declare the output status of a disabled PWM as undefined.

The core is not affected by this change. The users are even simplified
by this change, because at the moment they all have to special case in
some way if (duty == 0) pwm_disable(pwm);, this code could be removed.
The PWM drivers could be simplified aswell since they no longer have to
care about getting the PWM in a well defined state and don't have to
take care about the correct disabled state of inverted PWMs. If a PWM
driver wishes it can do some special casing for 0 and 100% duty cycle in
order to save some power, but that would only be a bonus. As an
additional bonus inverted PWMs could be completely software emulated in
the core, then this code could also be removed from the drivers.
PWM Users generally don't want to disable or enable the PWMs, instead
they want a duty cycle of 0 or 100%. If they really don't care about the
output status then they can call pwm_disable().

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |