| Message ID | 1423734290-19750-3-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Rejected |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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++;
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(-)