| Message ID | 1359381659-24454-4-git-send-email-florian.vaussard@epfl.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/28/2013 03:00 PM, Florian Vaussard wrote: > Call to led_pwm_set() can happen inside atomic context, like triggers. > If the PWM call can sleep, defer using a worker. > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> Reviewed-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > drivers/leds/leds-pwm.c | 50 +++++++++++++++++++++++++++++++++++++++------- > 1 files changed, 42 insertions(+), 8 deletions(-) > > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c > index a1ea5f6..faf52c0 100644 > --- a/drivers/leds/leds-pwm.c > +++ b/drivers/leds/leds-pwm.c > @@ -23,12 +23,16 @@ > #include <linux/pwm.h> > #include <linux/leds_pwm.h> > #include <linux/slab.h> > +#include <linux/workqueue.h> > > struct led_pwm_data { > struct led_classdev cdev; > struct pwm_device *pwm; > + struct work_struct work; > unsigned int active_low; > unsigned int period; > + int duty; > + bool can_sleep; > }; > > struct led_pwm_priv { > @@ -36,6 +40,26 @@ struct led_pwm_priv { > struct led_pwm_data leds[0]; > }; > > +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) > +{ > + struct led_pwm_data *led_dat = > + container_of(work, struct led_pwm_data, work); > + > + __led_pwm_set(led_dat); > +} > + > static void led_pwm_set(struct led_classdev *led_cdev, > enum led_brightness brightness) > { > @@ -44,13 +68,12 @@ 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); > - } > + led_dat->duty = brightness * period / max; > + > + if (led_dat->can_sleep) > + schedule_work(&led_dat->work); > + else > + __led_pwm_set(led_dat); > } > > static inline size_t sizeof_pwm_leds_priv(int num_leds) > @@ -100,6 +123,10 @@ static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev) > led_dat->cdev.brightness = LED_OFF; > led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME; > > + led_dat->can_sleep = pwm_can_sleep(led_dat->pwm); > + if (led_dat->can_sleep) > + INIT_WORK(&led_dat->work, led_pwm_work); > + > ret = led_classdev_register(&pdev->dev, &led_dat->cdev); > if (ret < 0) { > dev_err(&pdev->dev, "failed to register for %s\n", > @@ -153,6 +180,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->can_sleep = pwm_can_sleep(led_dat->pwm); > + if (led_dat->can_sleep) > + INIT_WORK(&led_dat->work, led_pwm_work); > + > ret = led_classdev_register(&pdev->dev, &led_dat->cdev); > if (ret < 0) > goto err; > @@ -180,8 +211,11 @@ static int led_pwm_remove(struct platform_device *pdev) > struct led_pwm_priv *priv = platform_get_drvdata(pdev); > int i; > > - for (i = 0; i < priv->num_leds; i++) > + for (i = 0; i < priv->num_leds; i++) { > led_classdev_unregister(&priv->leds[i].cdev); > + if (priv->leds[i].can_sleep) > + cancel_work_sync(&priv->leds[i].work); > + } > > return 0; > } >
On Mon, Jan 28, 2013 at 03:00:59PM +0100, Florian Vaussard wrote: > Call to led_pwm_set() can happen inside atomic context, like triggers. > If the PWM call can sleep, defer using a worker. > > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> > --- > drivers/leds/leds-pwm.c | 50 +++++++++++++++++++++++++++++++++++++++------- > 1 files changed, 42 insertions(+), 8 deletions(-) Bryan, I assume that you'll be taking this? It doesn't apply cleanly to my tree, probably because of Peter's recent changes that you took through your tree and Florian based his patches on top of that. The conflict resolution should be trivial, though. Thierry
On 01/30/2013 09:17 AM, Thierry Reding wrote: > On Mon, Jan 28, 2013 at 03:00:59PM +0100, Florian Vaussard wrote: >> Call to led_pwm_set() can happen inside atomic context, like triggers. >> If the PWM call can sleep, defer using a worker. >> >> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> >> --- >> drivers/leds/leds-pwm.c | 50 +++++++++++++++++++++++++++++++++++++++------- >> 1 files changed, 42 insertions(+), 8 deletions(-) > > Bryan, I assume that you'll be taking this? It doesn't apply cleanly to > my tree, probably because of Peter's recent changes that you took > through your tree and Florian based his patches on top of that. The > conflict resolution should be trivial, though. > Thank you Thierry. Indeed, this patchset was based on linux-leds due to the dependancy with Peter's patches. Bryan, can you take this patch? Regards, Florian
Hi Bryan, On 01/30/2013 09:17 AM, Thierry Reding wrote: > On Mon, Jan 28, 2013 at 03:00:59PM +0100, Florian Vaussard wrote: >> Call to led_pwm_set() can happen inside atomic context, like triggers. >> If the PWM call can sleep, defer using a worker. >> >> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> >> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> >> --- >> drivers/leds/leds-pwm.c | 50 +++++++++++++++++++++++++++++++++++++++------- >> 1 files changed, 42 insertions(+), 8 deletions(-) > > Bryan, I assume that you'll be taking this? It doesn't apply cleanly to > my tree, probably because of Peter's recent changes that you took > through your tree and Florian based his patches on top of that. The > conflict resolution should be trivial, though. > It seems that this patch was not taken. Could you pull it, as the other part is already in linux-pwm? Or is there any show stopper? Many thanks. Florian
Hi Bryan, Richard, > On 01/30/2013 09:17 AM, Thierry Reding wrote: >> On Mon, Jan 28, 2013 at 03:00:59PM +0100, Florian Vaussard wrote: >>> Call to led_pwm_set() can happen inside atomic context, like triggers. >>> If the PWM call can sleep, defer using a worker. >>> >>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> >>> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> >>> --- >>> drivers/leds/leds-pwm.c | 50 >>> +++++++++++++++++++++++++++++++++++++++------- >>> 1 files changed, 42 insertions(+), 8 deletions(-) >> >> Bryan, I assume that you'll be taking this? It doesn't apply cleanly to >> my tree, probably because of Peter's recent changes that you took >> through your tree and Florian based his patches on top of that. The >> conflict resolution should be trivial, though. >> > > It seems that this patch was not taken. Could you pull it, as the other > part is already in linux-pwm? Or is there any show stopper? > Ping? Who should merge this? Many thanks! Florian
On Mon, Mar 04, 2013 at 11:44:22AM +0100, Florian Vaussard wrote: > Hi Bryan, Richard, > > >On 01/30/2013 09:17 AM, Thierry Reding wrote: > >>On Mon, Jan 28, 2013 at 03:00:59PM +0100, Florian Vaussard wrote: > >>>Call to led_pwm_set() can happen inside atomic context, like triggers. > >>>If the PWM call can sleep, defer using a worker. > >>> > >>>Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > >>>Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> > >>>--- > >>> drivers/leds/leds-pwm.c | 50 > >>>+++++++++++++++++++++++++++++++++++++++------- > >>> 1 files changed, 42 insertions(+), 8 deletions(-) > >> > >>Bryan, I assume that you'll be taking this? It doesn't apply cleanly to > >>my tree, probably because of Peter's recent changes that you took > >>through your tree and Florian based his patches on top of that. The > >>conflict resolution should be trivial, though. > >> > > > >It seems that this patch was not taken. Could you pull it, as the other > >part is already in linux-pwm? Or is there any show stopper? > > > > Ping? Who should merge this? I assume Bryan will take it through the LED tree? Patches 1 & 2 went into v3.9-rc1 through the PWM tree. Thierry
On 03/04/2013 11:58 AM, Thierry Reding wrote: > On Mon, Mar 04, 2013 at 11:44:22AM +0100, Florian Vaussard wrote: >> Hi Bryan, Richard, >> >>> On 01/30/2013 09:17 AM, Thierry Reding wrote: >>>> On Mon, Jan 28, 2013 at 03:00:59PM +0100, Florian Vaussard wrote: >>>>> Call to led_pwm_set() can happen inside atomic context, like triggers. >>>>> If the PWM call can sleep, defer using a worker. >>>>> >>>>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> >>>>> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> >>>>> --- >>>>> drivers/leds/leds-pwm.c | 50 >>>>> +++++++++++++++++++++++++++++++++++++++------- >>>>> 1 files changed, 42 insertions(+), 8 deletions(-) >>>> >>>> Bryan, I assume that you'll be taking this? It doesn't apply cleanly to >>>> my tree, probably because of Peter's recent changes that you took >>>> through your tree and Florian based his patches on top of that. The >>>> conflict resolution should be trivial, though. >>>> >>> >>> It seems that this patch was not taken. Could you pull it, as the other >>> part is already in linux-pwm? Or is there any show stopper? >>> >> >> Ping? Who should merge this? > > I assume Bryan will take it through the LED tree? Patches 1 & 2 went > into v3.9-rc1 through the PWM tree. > This was also my assumption, but I never got the feedback from Brian on this patch. The crach is still there, and the patch missed the merge window :) Cheers, Florian
Hi Bryan, >>>> >>>> Bryan, I assume that you'll be taking this? It doesn't apply cleanly to >>>> my tree, probably because of Peter's recent changes that you took >>>> through your tree and Florian based his patches on top of that. The >>>> conflict resolution should be trivial, though. >>>> >>> >>> It seems that this patch was not taken. Could you pull it, as the other >>> part is already in linux-pwm? Or is there any show stopper? >>> >> >> Ping? Who should merge this? > > I assume Bryan will take it through the LED tree? Patches 1 & 2 went > into v3.9-rc1 through the PWM tree. > Ping ? Florian
On Tue, Mar 12, 2013 at 3:06 AM, Florian Vaussard <florian.vaussard@epfl.ch> wrote: > Hi Bryan, > > >>>>> >>>>> Bryan, I assume that you'll be taking this? It doesn't apply cleanly to >>>>> my tree, probably because of Peter's recent changes that you took >>>>> through your tree and Florian based his patches on top of that. The >>>>> conflict resolution should be trivial, though. >>>>> >>>> >>>> It seems that this patch was not taken. Could you pull it, as the other >>>> part is already in linux-pwm? Or is there any show stopper? >>>> >>> >>> Ping? Who should merge this? >> >> >> I assume Bryan will take it through the LED tree? Patches 1 & 2 went >> into v3.9-rc1 through the PWM tree. >> > > Ping ? > Hi Florian and Thierry, Sorry, guys. My bad! I'm using GMAIL filtering my emails. This email goes into my arm-linux mail list archive and I totally missed it. I can merge this patch into my for-next branch, but it will be in 3.10 not 3.9-rcX. Or Thierry, can you merge that to 3.9-rcX through your tree with my Ack? Thanks, -Bryan
On Fri, Mar 15, 2013 at 10:08:57AM -0700, Bryan Wu wrote: > On Tue, Mar 12, 2013 at 3:06 AM, Florian Vaussard > <florian.vaussard@epfl.ch> wrote: > > Hi Bryan, > > > > > >>>>> > >>>>> Bryan, I assume that you'll be taking this? It doesn't apply cleanly to > >>>>> my tree, probably because of Peter's recent changes that you took > >>>>> through your tree and Florian based his patches on top of that. The > >>>>> conflict resolution should be trivial, though. > >>>>> > >>>> > >>>> It seems that this patch was not taken. Could you pull it, as the other > >>>> part is already in linux-pwm? Or is there any show stopper? > >>>> > >>> > >>> Ping? Who should merge this? > >> > >> > >> I assume Bryan will take it through the LED tree? Patches 1 & 2 went > >> into v3.9-rc1 through the PWM tree. > >> > > > > Ping ? > > > > Hi Florian and Thierry, > > Sorry, guys. My bad! I'm using GMAIL filtering my emails. This email > goes into my arm-linux mail list archive and I totally missed it. > > I can merge this patch into my for-next branch, but it will be in 3.10 > not 3.9-rcX. > > Or Thierry, can you merge that to 3.9-rcX through your tree with my Ack? I think 3.10 would be fine. We missed the merge window on this one and it isn't really a fix so it's a bit late for 3.9 now. If Florian really absolutely needs this it can probably be done, but if he can wait another few weeks it's certainly worth taking it through linux-next for a while just to be safe. Thierry
Hi, On 03/15/2013 07:59 PM, Thierry Reding wrote: >> >> Hi Florian and Thierry, >> >> Sorry, guys. My bad! I'm using GMAIL filtering my emails. This email >> goes into my arm-linux mail list archive and I totally missed it. >> >> I can merge this patch into my for-next branch, but it will be in 3.10 >> not 3.9-rcX. >> >> Or Thierry, can you merge that to 3.9-rcX through your tree with my Ack? > > I think 3.10 would be fine. We missed the merge window on this one and > it isn't really a fix so it's a bit late for 3.9 now. > > If Florian really absolutely needs this it can probably be done, but if > he can wait another few weeks it's certainly worth taking it through > linux-next for a while just to be safe. > I agree, another few weeks is not a problem :) Bryan, I let you merge this, ok? Thank you, Florian
On Fri, Mar 15, 2013 at 1:09 PM, Florian Vaussard <florian.vaussard@epfl.ch> wrote: > Hi, > > > On 03/15/2013 07:59 PM, Thierry Reding wrote: >>> >>> >>> Hi Florian and Thierry, >>> >>> Sorry, guys. My bad! I'm using GMAIL filtering my emails. This email >>> goes into my arm-linux mail list archive and I totally missed it. >>> >>> I can merge this patch into my for-next branch, but it will be in 3.10 >>> not 3.9-rcX. >>> >>> Or Thierry, can you merge that to 3.9-rcX through your tree with my Ack? >> >> >> I think 3.10 would be fine. We missed the merge window on this one and >> it isn't really a fix so it's a bit late for 3.9 now. >> >> If Florian really absolutely needs this it can probably be done, but if >> he can wait another few weeks it's certainly worth taking it through >> linux-next for a while just to be safe. >> > > I agree, another few weeks is not a problem :) Bryan, I let you merge this, > ok? > Cool, I merged it into my for-next branch and it will be in linux-next soon. Thanks, -Bryan
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c index a1ea5f6..faf52c0 100644 --- a/drivers/leds/leds-pwm.c +++ b/drivers/leds/leds-pwm.c @@ -23,12 +23,16 @@ #include <linux/pwm.h> #include <linux/leds_pwm.h> #include <linux/slab.h> +#include <linux/workqueue.h> struct led_pwm_data { struct led_classdev cdev; struct pwm_device *pwm; + struct work_struct work; unsigned int active_low; unsigned int period; + int duty; + bool can_sleep; }; struct led_pwm_priv { @@ -36,6 +40,26 @@ struct led_pwm_priv { struct led_pwm_data leds[0]; }; +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) +{ + struct led_pwm_data *led_dat = + container_of(work, struct led_pwm_data, work); + + __led_pwm_set(led_dat); +} + static void led_pwm_set(struct led_classdev *led_cdev, enum led_brightness brightness) { @@ -44,13 +68,12 @@ 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); - } + led_dat->duty = brightness * period / max; + + if (led_dat->can_sleep) + schedule_work(&led_dat->work); + else + __led_pwm_set(led_dat); } static inline size_t sizeof_pwm_leds_priv(int num_leds) @@ -100,6 +123,10 @@ static struct led_pwm_priv *led_pwm_create_of(struct platform_device *pdev) led_dat->cdev.brightness = LED_OFF; led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME; + led_dat->can_sleep = pwm_can_sleep(led_dat->pwm); + if (led_dat->can_sleep) + INIT_WORK(&led_dat->work, led_pwm_work); + ret = led_classdev_register(&pdev->dev, &led_dat->cdev); if (ret < 0) { dev_err(&pdev->dev, "failed to register for %s\n", @@ -153,6 +180,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->can_sleep = pwm_can_sleep(led_dat->pwm); + if (led_dat->can_sleep) + INIT_WORK(&led_dat->work, led_pwm_work); + ret = led_classdev_register(&pdev->dev, &led_dat->cdev); if (ret < 0) goto err; @@ -180,8 +211,11 @@ static int led_pwm_remove(struct platform_device *pdev) struct led_pwm_priv *priv = platform_get_drvdata(pdev); int i; - for (i = 0; i < priv->num_leds; i++) + for (i = 0; i < priv->num_leds; i++) { led_classdev_unregister(&priv->leds[i].cdev); + if (priv->leds[i].can_sleep) + cancel_work_sync(&priv->leds[i].work); + } return 0; }
Call to led_pwm_set() can happen inside atomic context, like triggers. If the PWM call can sleep, defer using a worker. Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> --- drivers/leds/leds-pwm.c | 50 +++++++++++++++++++++++++++++++++++++++------- 1 files changed, 42 insertions(+), 8 deletions(-)