pwm pin stays on 1 on mxs after pwm_config(pwm, 0, period); pwm_disable(pwm);

pwm pin stays on 1 on mxs after pwm_config(pwm, 0, period); pwm_disable(pwm);

[Uwe Kleine-König]

Hello,

the leds-pwm driver does the following in its led_pwm_set routine:

	if (brightness == 0) {
		pwm_config(led_dat->pwm, 0, period);
		pwm_disable(led_dat->pwm);
	} else {
		pwm_config(led_dat->pwm, brightness * period / max, period);
		pwm_enable(led_dat->pwm);
	}

The effect on setting brightness = 0 on an i.MX28 based machine after
having brightness = max = 255 before is:

pwm_config(..., 0, period) doesn't disable the output at once because of
some logic (in hardware) to prevent glitches. According to the Hardware
manual the new values take effect after the current period (which has the
output on 1). Now pwm_disable stopps the counter, the current period
doesn't run out and the output keeps the 1. So the LED stays on.

Where do I fix that? In the pwm driver (somehow block in pwm_config
until the current period is over, don't know how yet), remove the
pwm_disable in the brightness == 0 case (and then probably making the
whole if block a simple pwm_config(led_dat->pwm, brightness * period /
max, period)) or do we need a new API for that introducing a wait flag
or something different I didn't think of?

Best regards
Uwe

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

pwm pin stays on 1 on mxs after pwm_config(pwm, 0, period); pwm_disable(pwm);

[Uwe Kleine-König]

Hello,

the leds-pwm driver does the following in its led_pwm_set routine:

	if (brightness == 0) {
		pwm_config(led_dat->pwm, 0, period);
		pwm_disable(led_dat->pwm);
	} else {
		pwm_config(led_dat->pwm, brightness * period / max, period);
		pwm_enable(led_dat->pwm);
	}

The effect on setting brightness = 0 on an i.MX28 based machine after
having brightness = max = 255 before is:

pwm_config(..., 0, period) doesn't disable the output at once because of
some logic (in hardware) to prevent glitches. According to the Hardware
manual the new values take effect after the current period (which has the
output on 1). Now pwm_disable stopps the counter, the current period
doesn't run out and the output keeps the 1. So the LED stays on.

Where do I fix that? In the pwm driver (somehow block in pwm_config
until the current period is over, don't know how yet), remove the
pwm_disable in the brightness == 0 case (and then probably making the
whole if block a simple pwm_config(led_dat->pwm, brightness * period /
max, period)) or do we need a new API for that introducing a wait flag
or something different I didn't think of?

Best regards
Uwe

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

[PATCH] RFC: 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.

It's unclear if the mxs-pwm driver doesn't implement the API as expected
(i.e. it should block until the newly set config is effective) or if the
leds-pwm driver makes wrong assumptions. This patch assumes the latter.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

I'm not sure this is correct, but this is the workaround I'm using until
I get some feed back.

Best regards
Uwe

 drivers/leds/leds-pwm.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index f2e44c7..a909f4f 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -38,13 +38,8 @@ static void led_pwm_set(struct led_classdev *led_cdev,
 	unsigned int max = led_dat->cdev.max_brightness;
 	unsigned int period =  led_dat->period;
 
-	if (brightness == 0) {
-		pwm_config(led_dat->pwm, 0, period);
-		pwm_disable(led_dat->pwm);
-	} else {
-		pwm_config(led_dat->pwm, brightness * period / max, period);
-		pwm_enable(led_dat->pwm);
-	}
+	pwm_config(led_dat->pwm, brightness * period / max, period);
+	pwm_enable(led_dat->pwm);
 }
 
 static int led_pwm_probe(struct platform_device *pdev)
-- 
1.7.10.4

[PATCH] RFC: 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.

It's unclear if the mxs-pwm driver doesn't implement the API as expected
(i.e. it should block until the newly set config is effective) or if the
leds-pwm driver makes wrong assumptions. This patch assumes the latter.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

I'm not sure this is correct, but this is the workaround I'm using until
I get some feed back.

Best regards
Uwe

 drivers/leds/leds-pwm.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index f2e44c7..a909f4f 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -38,13 +38,8 @@ static void led_pwm_set(struct led_classdev *led_cdev,
 	unsigned int max = led_dat->cdev.max_brightness;
 	unsigned int period =  led_dat->period;
 
-	if (brightness == 0) {
-		pwm_config(led_dat->pwm, 0, period);
-		pwm_disable(led_dat->pwm);
-	} else {
-		pwm_config(led_dat->pwm, brightness * period / max, period);
-		pwm_enable(led_dat->pwm);
-	}
+	pwm_config(led_dat->pwm, brightness * period / max, period);
+	pwm_enable(led_dat->pwm);
 }
 
 static int led_pwm_probe(struct platform_device *pdev)
-- 
1.7.10.4

[PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Shawn Guo]

On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> 
> It's unclear if the mxs-pwm driver doesn't implement the API as expected
> (i.e. it should block until the newly set config is effective) or if the
> leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I'm not sure this is correct, but this is the workaround I'm using until
> I get some feed back.

I'm fine with it, since it fixes a real problem.  Let's see what
Thierry says.

Shawn

> 
> Best regards
> Uwe
> 
>  drivers/leds/leds-pwm.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index f2e44c7..a909f4f 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -38,13 +38,8 @@ static void led_pwm_set(struct led_classdev *led_cdev,
>  	unsigned int max = led_dat->cdev.max_brightness;
>  	unsigned int period =  led_dat->period;
>  
> -	if (brightness == 0) {
> -		pwm_config(led_dat->pwm, 0, period);
> -		pwm_disable(led_dat->pwm);
> -	} else {
> -		pwm_config(led_dat->pwm, brightness * period / max, period);
> -		pwm_enable(led_dat->pwm);
> -	}
> +	pwm_config(led_dat->pwm, brightness * period / max, period);
> +	pwm_enable(led_dat->pwm);
>  }
>  
>  static int led_pwm_probe(struct platform_device *pdev)
> -- 
> 1.7.10.4
> 

Re: [PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Shawn Guo]

On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> 
> It's unclear if the mxs-pwm driver doesn't implement the API as expected
> (i.e. it should block until the newly set config is effective) or if the
> leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I'm not sure this is correct, but this is the workaround I'm using until
> I get some feed back.

I'm fine with it, since it fixes a real problem.  Let's see what
Thierry says.

Shawn

> 
> Best regards
> Uwe
> 
>  drivers/leds/leds-pwm.c |    9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index f2e44c7..a909f4f 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -38,13 +38,8 @@ static void led_pwm_set(struct led_classdev *led_cdev,
>  	unsigned int max = led_dat->cdev.max_brightness;
>  	unsigned int period =  led_dat->period;
>  
> -	if (brightness == 0) {
> -		pwm_config(led_dat->pwm, 0, period);
> -		pwm_disable(led_dat->pwm);
> -	} else {
> -		pwm_config(led_dat->pwm, brightness * period / max, period);
> -		pwm_enable(led_dat->pwm);
> -	}
> +	pwm_config(led_dat->pwm, brightness * period / max, period);
> +	pwm_enable(led_dat->pwm);
>  }
>  
>  static int led_pwm_probe(struct platform_device *pdev)
> -- 
> 1.7.10.4
> 

[PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Thierry Reding]

On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > 
> > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > (i.e. it should block until the newly set config is effective) or if the
> > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > I'm not sure this is correct, but this is the workaround I'm using until
> > I get some feed back.
> 
> I'm fine with it, since it fixes a real problem.  Let's see what
> Thierry says.

I lost track of this thread somehow, so sorry for not getting back to
you earlier. The root cause of this problem seems to be that it isn't
very well defined (actually not at all) what is supposed to happen in
the case when a PWM is disabled.

There really are only two ways forward: a) we need to write down what
the PWM subsystem expects to happen when a PWM is disabled or b) keep
the currently undefined behaviour. With the latter I expect this kind
of issue to keep popping up every once in a while with all sorts of
ad-hoc solutions being implemented to solve the problem.

I think the best option would be to have some definition about what the
PWM signal should look like after a call to pwm_disable(). However this
doesn't turn out to be as trivial as it sounds. For instance, the most
straightforward definition in my opinion would be to specify that a PWM
signal should be constantly low after the call to pwm_disable(). It is
what I think most people would assume is the natural disable state of a
PWM.

However, one case where a similar problem was encountered involved a
hardware design that used an external inverter to change the polarity of
a PWM signal that was used to drive a backlight. In such a case, if the
controller were programmed to keep the output low when disabling, the
display would in fact be fully lit. This is further complicated by the
fact that the controller allows the output level of the disabled PWM
signal to be configured. This is nice because it means that pretty much
any scenario is covered, but it also doesn't make it any easier to put
this into a generic framework.

Having said that, I'm tempted to go with a simple definition like the
above anyway and handle obscure cases with board-specific quirks. I
don't see any other alternative that would allow the PWM framework to
stay relatively simple and clean.

Now I only have access to a limited amount of hardware and use-cases, so
any comments and suggestions as well as requirements on other platforms
are very welcome.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130103/7afc30a1/attachment-0001.sig>

Re: [PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Thierry Reding]

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

On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > 
> > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > (i.e. it should block until the newly set config is effective) or if the
> > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > I'm not sure this is correct, but this is the workaround I'm using until
> > I get some feed back.
> 
> I'm fine with it, since it fixes a real problem.  Let's see what
> Thierry says.

I lost track of this thread somehow, so sorry for not getting back to
you earlier. The root cause of this problem seems to be that it isn't
very well defined (actually not at all) what is supposed to happen in
the case when a PWM is disabled.

There really are only two ways forward: a) we need to write down what
the PWM subsystem expects to happen when a PWM is disabled or b) keep
the currently undefined behaviour. With the latter I expect this kind
of issue to keep popping up every once in a while with all sorts of
ad-hoc solutions being implemented to solve the problem.

I think the best option would be to have some definition about what the
PWM signal should look like after a call to pwm_disable(). However this
doesn't turn out to be as trivial as it sounds. For instance, the most
straightforward definition in my opinion would be to specify that a PWM
signal should be constantly low after the call to pwm_disable(). It is
what I think most people would assume is the natural disable state of a
PWM.

However, one case where a similar problem was encountered involved a
hardware design that used an external inverter to change the polarity of
a PWM signal that was used to drive a backlight. In such a case, if the
controller were programmed to keep the output low when disabling, the
display would in fact be fully lit. This is further complicated by the
fact that the controller allows the output level of the disabled PWM
signal to be configured. This is nice because it means that pretty much
any scenario is covered, but it also doesn't make it any easier to put
this into a generic framework.

Having said that, I'm tempted to go with a simple definition like the
above anyway and handle obscure cases with board-specific quirks. I
don't see any other alternative that would allow the PWM framework to
stay relatively simple and clean.

Now I only have access to a limited amount of hardware and use-cases, so
any comments and suggestions as well as requirements on other platforms
are very welcome.

Thierry

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

[PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Uwe Kleine-König]

Hi Thierry,

On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > 
> > > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > > (i.e. it should block until the newly set config is effective) or if the
> > > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > > 
> > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > I'm not sure this is correct, but this is the workaround I'm using until
> > > I get some feed back.
> > 
> > I'm fine with it, since it fixes a real problem.  Let's see what
> > Thierry says.
> 
> I lost track of this thread somehow, so sorry for not getting back to
> you earlier. The root cause of this problem seems to be that it isn't
> very well defined (actually not at all) what is supposed to happen in
> the case when a PWM is disabled.
> 
> There really are only two ways forward: a) we need to write down what
> the PWM subsystem expects to happen when a PWM is disabled or b) keep
> the currently undefined behaviour. With the latter I expect this kind
> of issue to keep popping up every once in a while with all sorts of
> ad-hoc solutions being implemented to solve the problem.
> 
> I think the best option would be to have some definition about what the
> PWM signal should look like after a call to pwm_disable(). However this
> doesn't turn out to be as trivial as it sounds. For instance, the most
> straightforward definition in my opinion would be to specify that a PWM
> signal should be constantly low after the call to pwm_disable(). It is
> what I think most people would assume is the natural disable state of a
> PWM.
> 
> However, one case where a similar problem was encountered involved a
> hardware design that used an external inverter to change the polarity of
> a PWM signal that was used to drive a backlight. In such a case, if the
> controller were programmed to keep the output low when disabling, the
> display would in fact be fully lit. This is further complicated by the
> fact that the controller allows the output level of the disabled PWM
> signal to be configured. This is nice because it means that pretty much
> any scenario is covered, but it also doesn't make it any easier to put
> this into a generic framework.
> 
> Having said that, I'm tempted to go with a simple definition like the
> above anyway and handle obscure cases with board-specific quirks. I
I don't understand what you mean with "the above" here. I guess it's
"PWM signal should be constantly low after the call to pwm_disable".

To cover this we could add a function pwm_disable_blurb() that accepts a
parameter specifying the desired signal state: "high", "low" or (maybe)
"don't care". pwm_disable would then (probably) mean
pwm_disable_blurb("don't care"). But maybe this already contradicts your
idea about being simple and clean?!

Also note that I had another/alternative issue with the API, i.e. when
the pwm routines should return.

> don't see any other alternative that would allow the PWM framework to
> stay relatively simple and clean.

Best regards
Uwe

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

Re: [PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Uwe Kleine-König]

Hi Thierry,

On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > 
> > > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > > (i.e. it should block until the newly set config is effective) or if the
> > > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > I'm not sure this is correct, but this is the workaround I'm using until
> > > I get some feed back.
> > 
> > I'm fine with it, since it fixes a real problem.  Let's see what
> > Thierry says.
> 
> I lost track of this thread somehow, so sorry for not getting back to
> you earlier. The root cause of this problem seems to be that it isn't
> very well defined (actually not at all) what is supposed to happen in
> the case when a PWM is disabled.
> 
> There really are only two ways forward: a) we need to write down what
> the PWM subsystem expects to happen when a PWM is disabled or b) keep
> the currently undefined behaviour. With the latter I expect this kind
> of issue to keep popping up every once in a while with all sorts of
> ad-hoc solutions being implemented to solve the problem.
> 
> I think the best option would be to have some definition about what the
> PWM signal should look like after a call to pwm_disable(). However this
> doesn't turn out to be as trivial as it sounds. For instance, the most
> straightforward definition in my opinion would be to specify that a PWM
> signal should be constantly low after the call to pwm_disable(). It is
> what I think most people would assume is the natural disable state of a
> PWM.
> 
> However, one case where a similar problem was encountered involved a
> hardware design that used an external inverter to change the polarity of
> a PWM signal that was used to drive a backlight. In such a case, if the
> controller were programmed to keep the output low when disabling, the
> display would in fact be fully lit. This is further complicated by the
> fact that the controller allows the output level of the disabled PWM
> signal to be configured. This is nice because it means that pretty much
> any scenario is covered, but it also doesn't make it any easier to put
> this into a generic framework.
> 
> Having said that, I'm tempted to go with a simple definition like the
> above anyway and handle obscure cases with board-specific quirks. I
I don't understand what you mean with "the above" here. I guess it's
"PWM signal should be constantly low after the call to pwm_disable".

To cover this we could add a function pwm_disable_blurb() that accepts a
parameter specifying the desired signal state: "high", "low" or (maybe)
"don't care". pwm_disable would then (probably) mean
pwm_disable_blurb("don't care"). But maybe this already contradicts your
idea about being simple and clean?!

Also note that I had another/alternative issue with the API, i.e. when
the pwm routines should return.

> don't see any other alternative that would allow the PWM framework to
> stay relatively simple and clean.

Best regards
Uwe

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

[PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Thierry Reding]

On Thu, Jan 03, 2013 at 09:55:14PM +0100, Uwe Kleine-K?nig wrote:
> Hi Thierry,
> 
> On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> > On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > > 
> > > > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > > > (i.e. it should block until the newly set config is effective) or if the
> > > > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > > > 
> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > > Hello,
> > > > 
> > > > I'm not sure this is correct, but this is the workaround I'm using until
> > > > I get some feed back.
> > > 
> > > I'm fine with it, since it fixes a real problem.  Let's see what
> > > Thierry says.
> > 
> > I lost track of this thread somehow, so sorry for not getting back to
> > you earlier. The root cause of this problem seems to be that it isn't
> > very well defined (actually not at all) what is supposed to happen in
> > the case when a PWM is disabled.
> > 
> > There really are only two ways forward: a) we need to write down what
> > the PWM subsystem expects to happen when a PWM is disabled or b) keep
> > the currently undefined behaviour. With the latter I expect this kind
> > of issue to keep popping up every once in a while with all sorts of
> > ad-hoc solutions being implemented to solve the problem.
> > 
> > I think the best option would be to have some definition about what the
> > PWM signal should look like after a call to pwm_disable(). However this
> > doesn't turn out to be as trivial as it sounds. For instance, the most
> > straightforward definition in my opinion would be to specify that a PWM
> > signal should be constantly low after the call to pwm_disable(). It is
> > what I think most people would assume is the natural disable state of a
> > PWM.
> > 
> > However, one case where a similar problem was encountered involved a
> > hardware design that used an external inverter to change the polarity of
> > a PWM signal that was used to drive a backlight. In such a case, if the
> > controller were programmed to keep the output low when disabling, the
> > display would in fact be fully lit. This is further complicated by the
> > fact that the controller allows the output level of the disabled PWM
> > signal to be configured. This is nice because it means that pretty much
> > any scenario is covered, but it also doesn't make it any easier to put
> > this into a generic framework.
> > 
> > Having said that, I'm tempted to go with a simple definition like the
> > above anyway and handle obscure cases with board-specific quirks. I
> I don't understand what you mean with "the above" here. I guess it's
> "PWM signal should be constantly low after the call to pwm_disable".

Yes, exactly.

> To cover this we could add a function pwm_disable_blurb() that accepts a
> parameter specifying the desired signal state: "high", "low" or (maybe)
> "don't care". pwm_disable would then (probably) mean
> pwm_disable_blurb("don't care"). But maybe this already contradicts your
> idea about being simple and clean?!

I'm wondering if that's really necessary. This really seems more of a
board-specific question. If you run pwm_disable() on a PWM device, it
should be turned "off" (whatever that means in the context of a board
design) after the call terminates.

Part of the problem is that we want to keep the board-specific
complexities out of client drivers. For instance in the case you
encountered, the leds-pwm driver shouldn't have to know any of the
details pertaining to the i.MX28. That is, leds-pwm should be able to
call pwm_disable() if it wants to turn off the PWM signal.

If we add pwm_disable_blurb() as you suggest, what is leds-pwm supposed
to pass in? Usually this would be "low", but on other hardware (with
additional inverter circuitry) it would be "high". We certainly don't
want to have leds-pwm handling that kind of logic. The PWM signal
polarity is entirely defined at the board-level and therefore should be
handled by board setup code (or encoded in DT).

> Also note that I had another/alternative issue with the API, i.e. when
> the pwm routines should return.

Right. All of the above would entail that pwm_config() should either
block until the configuration is active, or alternatively that when
pwm_disable() is called without the new configuration being active yet,
it is pwm_disable() that needs to wait until the configuration becomes
active.

Another alternative would be that leds-pwm wouldn't have to call
pwm_config() with a 0% duty cycle in the first place if pwm_disable() is
guaranteed to force the PWM signal to constant low.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130104/69110543/attachment-0001.sig>

Re: [PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Thierry Reding]

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

On Thu, Jan 03, 2013 at 09:55:14PM +0100, Uwe Kleine-König wrote:
> Hi Thierry,
> 
> On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> > On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > > 
> > > > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > > > (i.e. it should block until the newly set config is effective) or if the
> > > > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > > Hello,
> > > > 
> > > > I'm not sure this is correct, but this is the workaround I'm using until
> > > > I get some feed back.
> > > 
> > > I'm fine with it, since it fixes a real problem.  Let's see what
> > > Thierry says.
> > 
> > I lost track of this thread somehow, so sorry for not getting back to
> > you earlier. The root cause of this problem seems to be that it isn't
> > very well defined (actually not at all) what is supposed to happen in
> > the case when a PWM is disabled.
> > 
> > There really are only two ways forward: a) we need to write down what
> > the PWM subsystem expects to happen when a PWM is disabled or b) keep
> > the currently undefined behaviour. With the latter I expect this kind
> > of issue to keep popping up every once in a while with all sorts of
> > ad-hoc solutions being implemented to solve the problem.
> > 
> > I think the best option would be to have some definition about what the
> > PWM signal should look like after a call to pwm_disable(). However this
> > doesn't turn out to be as trivial as it sounds. For instance, the most
> > straightforward definition in my opinion would be to specify that a PWM
> > signal should be constantly low after the call to pwm_disable(). It is
> > what I think most people would assume is the natural disable state of a
> > PWM.
> > 
> > However, one case where a similar problem was encountered involved a
> > hardware design that used an external inverter to change the polarity of
> > a PWM signal that was used to drive a backlight. In such a case, if the
> > controller were programmed to keep the output low when disabling, the
> > display would in fact be fully lit. This is further complicated by the
> > fact that the controller allows the output level of the disabled PWM
> > signal to be configured. This is nice because it means that pretty much
> > any scenario is covered, but it also doesn't make it any easier to put
> > this into a generic framework.
> > 
> > Having said that, I'm tempted to go with a simple definition like the
> > above anyway and handle obscure cases with board-specific quirks. I
> I don't understand what you mean with "the above" here. I guess it's
> "PWM signal should be constantly low after the call to pwm_disable".

Yes, exactly.

> To cover this we could add a function pwm_disable_blurb() that accepts a
> parameter specifying the desired signal state: "high", "low" or (maybe)
> "don't care". pwm_disable would then (probably) mean
> pwm_disable_blurb("don't care"). But maybe this already contradicts your
> idea about being simple and clean?!

I'm wondering if that's really necessary. This really seems more of a
board-specific question. If you run pwm_disable() on a PWM device, it
should be turned "off" (whatever that means in the context of a board
design) after the call terminates.

Part of the problem is that we want to keep the board-specific
complexities out of client drivers. For instance in the case you
encountered, the leds-pwm driver shouldn't have to know any of the
details pertaining to the i.MX28. That is, leds-pwm should be able to
call pwm_disable() if it wants to turn off the PWM signal.

If we add pwm_disable_blurb() as you suggest, what is leds-pwm supposed
to pass in? Usually this would be "low", but on other hardware (with
additional inverter circuitry) it would be "high". We certainly don't
want to have leds-pwm handling that kind of logic. The PWM signal
polarity is entirely defined at the board-level and therefore should be
handled by board setup code (or encoded in DT).

> Also note that I had another/alternative issue with the API, i.e. when
> the pwm routines should return.

Right. All of the above would entail that pwm_config() should either
block until the configuration is active, or alternatively that when
pwm_disable() is called without the new configuration being active yet,
it is pwm_disable() that needs to wait until the configuration becomes
active.

Another alternative would be that leds-pwm wouldn't have to call
pwm_config() with a 0% duty cycle in the first place if pwm_disable() is
guaranteed to force the PWM signal to constant low.

Thierry

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

[PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Sascha Hauer]

On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > 
> > > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > > (i.e. it should block until the newly set config is effective) or if the
> > > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > > 
> > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > I'm not sure this is correct, but this is the workaround I'm using until
> > > I get some feed back.
> > 
> > I'm fine with it, since it fixes a real problem.  Let's see what
> > Thierry says.
> 
> I lost track of this thread somehow, so sorry for not getting back to
> you earlier. The root cause of this problem seems to be that it isn't
> very well defined (actually not at all) what is supposed to happen in
> the case when a PWM is disabled.
> 
> There really are only two ways forward: a) we need to write down what
> the PWM subsystem expects to happen when a PWM is disabled or b) keep
> the currently undefined behaviour. With the latter I expect this kind
> of issue to keep popping up every once in a while with all sorts of
> ad-hoc solutions being implemented to solve the problem.
> 
> I think the best option would be to have some definition about what the
> PWM signal should look like after a call to pwm_disable(). However this
> doesn't turn out to be as trivial as it sounds. For instance, the most
> straightforward definition in my opinion would be to specify that a PWM
> signal should be constantly low after the call to pwm_disable(). It is
> what I think most people would assume is the natural disable state of a
> PWM.
> 
> However, one case where a similar problem was encountered involved a
> hardware design that used an external inverter to change the polarity of
> a PWM signal that was used to drive a backlight. In such a case, if the
> controller were programmed to keep the output low when disabling, the
> display would in fact be fully lit. This is further complicated by the
> fact that the controller allows the output level of the disabled PWM
> signal to be configured. This is nice because it means that pretty much
> any scenario is covered, but it also doesn't make it any easier to put
> this into a generic framework.
> 
> Having said that, I'm tempted to go with a simple definition like the
> above anyway and handle obscure cases with board-specific quirks. I
> don't see any other alternative that would allow the PWM framework to
> stay relatively simple and clean.
> 
> Now I only have access to a limited amount of hardware and use-cases, so
> any comments and suggestions as well as requirements on other platforms
> are very welcome.

How about keeping the current behaviour, so:

duty cycle 0    -> output constant low
duty cycle 100% -> output constant high
pwm disabled    -> output unspecified

This would allow the pwm drivers to implement whatever powersaving is
possible for 0/100%, or, if that's not possible, use pwm_disable for
powersaving. A pwm client driver wouldn't have to call pwm_en|disable at
all anymore during runtime (only pwm_enable during probe)

Background is that some board here needs 100% duty cycle to set the
backlight dark. This inversion can be easily archieved in software, but
on this board we would expect the pwm_disable output state to be high
whereas on most other boards we would expect the pwm_disable output
state to be low. The inversion could also be made in the pwm hardware
(possible on i.MX for example), I currently don't know what this means
for the pwm_disable output state.

Overall I think with this a client driver would have full control over
the output and we would avoid confusion with the pwm_en|disable calls.

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 |

Re: [PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Sascha Hauer]

On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > 
> > > It's unclear if the mxs-pwm driver doesn't implement the API as expected
> > > (i.e. it should block until the newly set config is effective) or if the
> > > leds-pwm driver makes wrong assumptions. This patch assumes the latter.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > I'm not sure this is correct, but this is the workaround I'm using until
> > > I get some feed back.
> > 
> > I'm fine with it, since it fixes a real problem.  Let's see what
> > Thierry says.
> 
> I lost track of this thread somehow, so sorry for not getting back to
> you earlier. The root cause of this problem seems to be that it isn't
> very well defined (actually not at all) what is supposed to happen in
> the case when a PWM is disabled.
> 
> There really are only two ways forward: a) we need to write down what
> the PWM subsystem expects to happen when a PWM is disabled or b) keep
> the currently undefined behaviour. With the latter I expect this kind
> of issue to keep popping up every once in a while with all sorts of
> ad-hoc solutions being implemented to solve the problem.
> 
> I think the best option would be to have some definition about what the
> PWM signal should look like after a call to pwm_disable(). However this
> doesn't turn out to be as trivial as it sounds. For instance, the most
> straightforward definition in my opinion would be to specify that a PWM
> signal should be constantly low after the call to pwm_disable(). It is
> what I think most people would assume is the natural disable state of a
> PWM.
> 
> However, one case where a similar problem was encountered involved a
> hardware design that used an external inverter to change the polarity of
> a PWM signal that was used to drive a backlight. In such a case, if the
> controller were programmed to keep the output low when disabling, the
> display would in fact be fully lit. This is further complicated by the
> fact that the controller allows the output level of the disabled PWM
> signal to be configured. This is nice because it means that pretty much
> any scenario is covered, but it also doesn't make it any easier to put
> this into a generic framework.
> 
> Having said that, I'm tempted to go with a simple definition like the
> above anyway and handle obscure cases with board-specific quirks. I
> don't see any other alternative that would allow the PWM framework to
> stay relatively simple and clean.
> 
> Now I only have access to a limited amount of hardware and use-cases, so
> any comments and suggestions as well as requirements on other platforms
> are very welcome.

How about keeping the current behaviour, so:

duty cycle 0    -> output constant low
duty cycle 100% -> output constant high
pwm disabled    -> output unspecified

This would allow the pwm drivers to implement whatever powersaving is
possible for 0/100%, or, if that's not possible, use pwm_disable for
powersaving. A pwm client driver wouldn't have to call pwm_en|disable at
all anymore during runtime (only pwm_enable during probe)

Background is that some board here needs 100% duty cycle to set the
backlight dark. This inversion can be easily archieved in software, but
on this board we would expect the pwm_disable output state to be high
whereas on most other boards we would expect the pwm_disable output
state to be low. The inversion could also be made in the pwm hardware
(possible on i.MX for example), I currently don't know what this means
for the pwm_disable output state.

Overall I think with this a client driver would have full control over
the output and we would avoid confusion with the pwm_en|disable calls.

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 |

[PATCH] RFC: leds-pwm: don't disable pwm when setting brightness to 0

[Matt Sealey]

On Fri, Jan 4, 2013 at 5:55 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Thu, Jan 03, 2013 at 10:01:18AM +0100, Thierry Reding wrote:
> > On Thu, Oct 25, 2012 at 04:03:49PM +0800, Shawn Guo wrote:
> > > On Wed, Oct 24, 2012 at 03:52:46PM +0200, 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.
> > > >
> > > > It's unclear if the mxs-pwm driver doesn't implement the API as
> expected
> > > > (i.e. it should block until the newly set config is effective) or if
> the
> > > > leds-pwm driver makes wrong assumptions. This patch assumes the
> latter.
> > > >
> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > > Hello,
> > > >
> > > > I'm not sure this is correct, but this is the workaround I'm using
> until
> > > > I get some feed back.
> > >
> > > I'm fine with it, since it fixes a real problem.  Let's see what
> > > Thierry says.
> >
> > I lost track of this thread somehow, so sorry for not getting back to
> > you earlier. The root cause of this problem seems to be that it isn't
> > very well defined (actually not at all) what is supposed to happen in
> > the case when a PWM is disabled.
> >
> > There really are only two ways forward: a) we need to write down what
> > the PWM subsystem expects to happen when a PWM is disabled or b) keep
> > the currently undefined behaviour. With the latter I expect this kind
> > of issue to keep popping up every once in a while with all sorts of
> > ad-hoc solutions being implemented to solve the problem.
> >
> > I think the best option would be to have some definition about what the
> > PWM signal should look like after a call to pwm_disable(). However this
> > doesn't turn out to be as trivial as it sounds. For instance, the most
> > straightforward definition in my opinion would be to specify that a PWM
> > signal should be constantly low after the call to pwm_disable(). It is
> > what I think most people would assume is the natural disable state of a
> > PWM.
> >
> > However, one case where a similar problem was encountered involved a
> > hardware design that used an external inverter to change the polarity of
> > a PWM signal that was used to drive a backlight. In such a case, if the
> > controller were programmed to keep the output low when disabling, the
> > display would in fact be fully lit. This is further complicated by the
> > fact that the controller allows the output level of the disabled PWM
> > signal to be configured. This is nice because it means that pretty much
> > any scenario is covered, but it also doesn't make it any easier to put
> > this into a generic framework.
> >
> > Having said that, I'm tempted to go with a simple definition like the
> > above anyway and handle obscure cases with board-specific quirks. I
> > don't see any other alternative that would allow the PWM framework to
> > stay relatively simple and clean.
> >
> > Now I only have access to a limited amount of hardware and use-cases, so
> > any comments and suggestions as well as requirements on other platforms
> > are very welcome.
>
> How about keeping the current behaviour, so:
>
> duty cycle 0    -> output constant low
> duty cycle 100% -> output constant high
> pwm disabled    -> output unspecified
>
> This would allow the pwm drivers to implement whatever powersaving is
> possible for 0/100%, or, if that's not possible, use pwm_disable for
> powersaving. A pwm client driver wouldn't have to call pwm_en|disable at
> all anymore during runtime (only pwm_enable during probe)
>
> Background is that some board here needs 100% duty cycle to set the
> backlight dark. This inversion can be easily archieved in software, but
> on this board we would expect the pwm_disable output state to be high
> whereas on most other boards we would expect the pwm_disable output
> state to be low. The inversion could also be made in the pwm hardware
> (possible on i.MX for example), I currently don't know what this means
> for the pwm_disable output state.
>
> Overall I think with this a client driver would have full control over
> the output and we would avoid confusion with the pwm_en|disable calls.
>
> Sascha
>


Ack, I prefer the old method too, but the other way around for reasoning;

It could be that pwm_disable actually turns off LCDs as well, but that "0"
brightness is still usable.

pwm_config with whatever brightness is "0" the driver (pwm_bl for example)
should be just configuring the pwm to the lowest permissible duty cycle
(since some panels require the pwm never runs below a certain duty cycle).

That doesn't mean you want to turn the backlight off, and in fact having
the pwm turned off but other logic still turned on is against some panel
datasheets (for instance there may be a backlight enable gpio which also
needs to be turned off if the PWM duty cycle is below the abovementioned
lowest permissible value, and in this case this would be handled by the
abovementioned power management and explicitly pwm_disable somehow).

In this case, pwm_disable should still disable the unit (i.e. toggle some
PWM controller enable bit inside the SoC or peripheral chip), but this is
handled by power management - most likely framebuffer or DRM DPMS activity.

We see however that leds-pwm is an island unto itself and has no clue about
other subsystems, so it has no way of knowing that power management is
required by some other subsystem like display or so.

~

What's really needed for a real fix here is a framebuffer-or-other aware
backlight driver which is a subclass of leds-pwm that actually receives fb
notifier events and can be informed of display power management (dpms or
other) such that it can do the right thing. I think using leds-pwm.c
directly - unless you can guarantee it is under some other control - is
probably a terrible idea for most real-life panel backlights even if it is
a fantastic solution for the big glowing Apple/Sony/Dell logo on the back
of the lid the panel lives on - or the systems where it was intended to run
where display is either off or the backlight is set to a lovely constant
value.

I have about 4 panel reference docs to hand, and I mean real laptop panels
not just developer board RGB ones with low resolutions or 1990's PDA
panels, with some pretty strict requirements and leds-pwm does not cut it.
drivers/video/backlight/pwm_bl.c is DT capable and does this properly as
described above.

So the real fix for i.MX28 is use the correct driver, isn't it?

-- 
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130104/ae15273c/attachment-0001.html>