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

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

[Uwe Kleine-König]

Hello,

some time ago I pointed out a problem with leds-pwm on i.MX28, because
on this SoC it can happen that after:

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

the connected LED stays on. The mail thread is available at

	http://thread.gmane.org/gmane.linux.kernel/1381289

This series implements the outcome that didn't get any opposition, i.e.
the pin state is explicitly undefined after pwm_disable. The first patch
adds this fact to the general pwm docs, the second patch fixes the
leds-pwm driver accordingly. The third patch is just a trivial fix that
I noticed while patching the leds-pwm driver and not really related to
any problem.

Best regards
Uwe

Uwe Kleine-König (3):
  pwm/doc: Clearify that the pin state after pwm_disable is undefined
  leds/pwm: Don't disable pwm when setting brightness to 0
  leds/pwm: fix driver description and make license match the header

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

-- 
1.8.4.2

[PATCH 1/3] 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 on i.MX28 the behaviour is very surprising at the first
glance. The pwm counter isn't reset (in hardware) when the registers are
reprogrammed with a new config to prevent unwanted spikes. So when doing:

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

the output might still stop at 1 when the period programmed before the
duty was set to 0 didn't elapse until pwm_disable is called which just
stops the counter and so the current period doesn't elapse ever. This
affects (at least) the leds-pwm driver which is fixed in a follow-up
patch.

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 93cb979..09f83c1 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -45,6 +45,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. So set the duty cycle to 0% or 100% and 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
 -----------------------------------
 
-- 
1.8.4.2

[PATCH 2/3] 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 2848171..cdc3e4d 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -45,11 +45,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)
@@ -180,6 +175,10 @@ static int led_pwm_probe(struct platform_device *pdev)
 			led_dat->cdev.max_brightness = cur_led->max_brightness;
 			led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 
+			led_dat->duty = 0;
+			__led_pwm_set(led_dat);
+			pwm_enable(led_dat->pwm);
+
 			led_dat->can_sleep = pwm_can_sleep(led_dat->pwm);
 			if (led_dat->can_sleep)
 				INIT_WORK(&led_dat->work, led_pwm_work);
@@ -215,6 +214,7 @@ static int led_pwm_remove(struct platform_device *pdev)
 		led_classdev_unregister(&priv->leds[i].cdev);
 		if (priv->leds[i].can_sleep)
 			cancel_work_sync(&priv->leds[i].work);
+		pwm_disable(priv->leds[i].pwm);
 	}
 
 	return 0;
-- 
1.8.4.2

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

[Thierry Reding]

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

On Mon, Nov 25, 2013 at 09:43:44PM +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.

While I do understand the reasoning behind this, if this is really the
behaviour that we need then there's no use in having pwm_enable() and
pwm_disable() at all. They can just be folded into pwm_get() and
pwm_put(), respectively.

Thierry

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

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

[Uwe Kleine-König]

On Mon, Dec 02, 2013 at 01:33:19PM +0100, Thierry Reding wrote:
> On Mon, Nov 25, 2013 at 09:43:44PM +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.
> 
> While I do understand the reasoning behind this, if this is really the
> behaviour that we need then there's no use in having pwm_enable() and
> pwm_disable() at all. They can just be folded into pwm_get() and
> pwm_put(), respectively.
So after the first pwm_get the pwm starts with an unspecified duty
cycle? That's not that nice, is it?

Best regards
Uwe

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

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

[Uwe Kleine-König]

Hello Thierry,

On Mon, Dec 02, 2013 at 02:18:07PM +0100, Uwe Kleine-König wrote:
> On Mon, Dec 02, 2013 at 01:33:19PM +0100, Thierry Reding wrote:
> > On Mon, Nov 25, 2013 at 09:43:44PM +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.
> > 
> > While I do understand the reasoning behind this, if this is really the
> > behaviour that we need then there's no use in having pwm_enable() and
> > pwm_disable() at all. They can just be folded into pwm_get() and
> > pwm_put(), respectively.
> So after the first pwm_get the pwm starts with an unspecified duty
> cycle? That's not that nice, is it?
How can we come forward here? After all it's a real bug being fixed.

Best regards
Uwe

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

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

[Thierry Reding]

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

On Thu, Dec 05, 2013 at 10:26:41PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Dec 02, 2013 at 02:18:07PM +0100, Uwe Kleine-König wrote:
> > On Mon, Dec 02, 2013 at 01:33:19PM +0100, Thierry Reding wrote:
> > > On Mon, Nov 25, 2013 at 09:43:44PM +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.
> > > 
> > > While I do understand the reasoning behind this, if this is really the
> > > behaviour that we need then there's no use in having pwm_enable() and
> > > pwm_disable() at all. They can just be folded into pwm_get() and
> > > pwm_put(), respectively.
> > So after the first pwm_get the pwm starts with an unspecified duty
> > cycle? That's not that nice, is it?
> How can we come forward here? After all it's a real bug being fixed.

I've thought about this some more, and this isn't actually fixing a bug.
Rather it's working around some quirk of the hardware in your setup. So
in the long run I think the best option would have to be to define the
behaviour of the PWM subsystem, and then make sure in drivers that they
behave accordingly. So if we define, or rather keep with the existing
implied behaviour, that the configuration is active when pwm_config()
returns, we can easily write generic client drivers because it means
they can rely on said behaviour. If a driver doesn't behave accordingly
it needs to be fixed.

If that means that a particular PWM controller applies the configuration
only after the current period has expired, then that's something only
the driver can now and therefore the driver should handle this by only
returning from pwm_config() when that's actually happened.

That seems to me the only reasonable approach. Otherwise we'll have to
keep changing API whenever some new controller comes around that behaves
slightly differently.

Thierry

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

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

[Uwe Kleine-König]

Hello Thierry,

first of all thanks for your reply.

On Wed, Dec 11, 2013 at 04:30:51PM +0100, Thierry Reding wrote:
> On Thu, Dec 05, 2013 at 10:26:41PM +0100, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Mon, Dec 02, 2013 at 02:18:07PM +0100, Uwe Kleine-König wrote:
> > > On Mon, Dec 02, 2013 at 01:33:19PM +0100, Thierry Reding wrote:
> > > > On Mon, Nov 25, 2013 at 09:43:44PM +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.
> > > > 
> > > > While I do understand the reasoning behind this, if this is really the
> > > > behaviour that we need then there's no use in having pwm_enable() and
> > > > pwm_disable() at all. They can just be folded into pwm_get() and
> > > > pwm_put(), respectively.
> > > So after the first pwm_get the pwm starts with an unspecified duty
> > > cycle? That's not that nice, is it?
> > How can we come forward here? After all it's a real bug being fixed.
> 
> I've thought about this some more, and this isn't actually fixing a bug.
> Rather it's working around some quirk of the hardware in your setup. So
<pedantic>
Yes, it does fix a bug. Without my patch setting the brightness of the
LED in question to zero doesn't work most of the time, with my patch it
works. Also I wouldn't call it quirk as that has a negative connotation
(at least for me). I'd call it a nice feature, that is missing from the
other pwms I know, but I don't care much how you call it.
</pedantic>

> in the long run I think the best option would have to be to define the
> behaviour of the PWM subsystem, and then make sure in drivers that they
> behave accordingly. So if we define, or rather keep with the existing
> implied behaviour, that the configuration is active when pwm_config()
> returns, we can easily write generic client drivers because it means
> they can rely on said behaviour. If a driver doesn't behave accordingly
> it needs to be fixed.
This definition doesn't fix the problem at hand. (It would fix it for
i.MX28, but not in general.) leds-pwm is doing:

	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);

. The problem is not that i.MX28 takes some time until it works with the
new config, but that it does something on pwm_disable that the author of
leds-pwm didn't take into account.
The only thing special on i.MX28 is that currently pwm_config might
return before the new config is active and that is the reason why the
state after pwm_disable is undefined for a different reason than on
other platforms (probably).

There are several things I can imagine that happen with the pin on
pwm_disable:

 - constant 0
 - constant 1
 - constant what it currently is
 - high-z

leds-pwm seems to assume the first option, the i.MX28 hardware behaves
according to the third option. I don't think either of them is an
assumption you should put in the API. I still think my suggestion is
sane.

> If that means that a particular PWM controller applies the configuration
> only after the current period has expired, then that's something only
> the driver can now and therefore the driver should handle this by only
> returning from pwm_config() when that's actually happened.
> 
> That seems to me the only reasonable approach. Otherwise we'll have to
> keep changing API whenever some new controller comes around that behaves
> slightly differently.
Currently even in the presence of the mentioned problems I'd say the
i.MX28 pwm driver "conforms" to the current pwm API/documentation. The
problem is that there are some things not formalized yet. And if there
are different implementations behaving differently obviously you must
specify more details if the users of the API need more
control/information (and then fix the implementations that became wrong
by the new details and fix the users that assumed wrong).

The two changes in question here are:

 - After pwm_config returns the new setting must already be active.
   This would result in the need to add a delay to the mxs-pwm driver
   (and maybe others). There is probably no need to fix users.
   I'm not yet convinced that this specification is necessary, at least
   it didn't bite yet and I cannot imagine a use case where it would
   matter.
 - Explicitly make the state of the pwm pin after pwm_disable
   implementation defined.
   There is probably no need to fix implementers, but at least leds-pwm
   as suggested in my patches.

Best regards
Uwe

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

[PATCH 3/3] leds/pwm: fix driver description and make license match the header

[Uwe Kleine-König]

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

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index cdc3e4d..0796ca3 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -239,6 +239,6 @@ static struct platform_driver led_pwm_driver = {
 module_platform_driver(led_pwm_driver);
 
 MODULE_AUTHOR("Luotao Fu <l.fu@pengutronix.de>");
-MODULE_DESCRIPTION("PWM LED driver for PXA");
-MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("generic PWM LED driver");
+MODULE_LICENSE("GPL v2");
 MODULE_ALIAS("platform:leds-pwm");
-- 
1.8.4.2

Re: [PATCH 3/3] leds/pwm: fix driver description and make license match the header

[Uwe Kleine-König]

Hello,

while the other two patches in this thread are controversal, this one
shouldn't. It still sits in my branch with unapplied patches since
nearly two years, which is enormous considering it's such an easy patch.

Best regards
Uwe

On Mon, Nov 25, 2013 at 09:43:45PM +0100, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/leds/leds-pwm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index cdc3e4d..0796ca3 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -239,6 +239,6 @@ static struct platform_driver led_pwm_driver = {
>  module_platform_driver(led_pwm_driver);
>  
>  MODULE_AUTHOR("Luotao Fu <l.fu@pengutronix.de>");
> -MODULE_DESCRIPTION("PWM LED driver for PXA");
> -MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("generic PWM LED driver");
> +MODULE_LICENSE("GPL v2");
>  MODULE_ALIAS("platform:leds-pwm");
> -- 
> 1.8.4.2
> 
> 

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

Re: [PATCH 3/3] leds/pwm: fix driver description and make license match the header

[Jacek Anaszewski]

Hi Uwe,

Applied, thanks.

Best Regards,
Jacek Anaszewski

On 11/19/2015 08:17 PM, Uwe Kleine-König wrote:
> Hello,
>
> while the other two patches in this thread are controversal, this one
> shouldn't. It still sits in my branch with unapplied patches since
> nearly two years, which is enormous considering it's such an easy patch.
>
> Best regards
> Uwe
>
> On Mon, Nov 25, 2013 at 09:43:45PM +0100, Uwe Kleine-König wrote:
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>   drivers/leds/leds-pwm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
>> index cdc3e4d..0796ca3 100644
>> --- a/drivers/leds/leds-pwm.c
>> +++ b/drivers/leds/leds-pwm.c
>> @@ -239,6 +239,6 @@ static struct platform_driver led_pwm_driver = {
>>   module_platform_driver(led_pwm_driver);
>>
>>   MODULE_AUTHOR("Luotao Fu <l.fu@pengutronix.de>");
>> -MODULE_DESCRIPTION("PWM LED driver for PXA");
>> -MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("generic PWM LED driver");
>> +MODULE_LICENSE("GPL v2");
>>   MODULE_ALIAS("platform:leds-pwm");
>> --
>> 1.8.4.2
>>
>>
>

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

[Uwe Kleine-König]

On Mon, Nov 25, 2013 at 09:43:42PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> some time ago I pointed out a problem with leds-pwm on i.MX28, because
> on this SoC it can happen that after:
> 
> 	pwm_config(pwm, 0, period);
> 	pwm_disable(pwm);
> 
> the connected LED stays on. The mail thread is available at
> 
> 	http://thread.gmane.org/gmane.linux.kernel/1381289
> 
> This series implements the outcome that didn't get any opposition, i.e.
> the pin state is explicitly undefined after pwm_disable. The first patch
> adds this fact to the general pwm docs, the second patch fixes the
> leds-pwm driver accordingly. The third patch is just a trivial fix that
> I noticed while patching the leds-pwm driver and not really related to
> any problem.
> 
> Best regards
> Uwe
Ping. Even if patches 1 and 2 are maybe controversal, patch 3 should be
ok.

Do you still have the problem addressed by the first two patches on your
radar?

Best regards
Uwe
 
> Uwe Kleine-König (3):
>   pwm/doc: Clearify that the pin state after pwm_disable is undefined
>   leds/pwm: Don't disable pwm when setting brightness to 0
>   leds/pwm: fix driver description and make license match the header
> 
>  Documentation/pwm.txt   |  5 +++++
>  drivers/leds/leds-pwm.c | 14 +++++++-------
>  2 files changed, 12 insertions(+), 7 deletions(-)

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