| Message ID | 1359108114-16998-4-git-send-email-florian.vaussard@epfl.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/25/2013 11:01 AM, 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. > > Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> > --- > drivers/leds/leds-pwm.c | 45 +++++++++++++++++++++++++++++++++++++++------ > 1 files changed, 39 insertions(+), 6 deletions(-) > > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c > index a1ea5f6..1ffb6ba 100644 > --- a/drivers/leds/leds-pwm.c > +++ b/drivers/leds/leds-pwm.c > @@ -23,12 +23,15 @@ > #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; Would it be better if we also store the can_sleep in led_pwm_data struct and initialize it when we probe the LEDs? It is not going to change runtime and we will save one API call every time the LED has been changed. > }; > > struct led_pwm_priv { > @@ -36,6 +39,20 @@ struct led_pwm_priv { > struct led_pwm_data leds[0]; > }; > > +static void led_pwm_work(struct work_struct *work) > +{ > + struct led_pwm_data *led_dat = > + container_of(work, struct led_pwm_data, work); > + 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); > +} I think we can remove the duplicated code by doing something like this: 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) { struct led_pwm_data *led_dat = container_of(led_cdev, struct led_pwm_data, cdev); unsigned int max = led_dat->cdev.max_brightness; led_dat->duty = brightness * led_dat->period / max; if (led_dat->can_sleep) schedule_work(&led_dat->work); else __led_pwm_set(led_dat); } What do you think? > + > static void led_pwm_set(struct led_classdev *led_cdev, > enum led_brightness brightness) > { > @@ -43,13 +60,23 @@ static void led_pwm_set(struct led_classdev *led_cdev, > container_of(led_cdev, struct led_pwm_data, cdev); > unsigned int max = led_dat->cdev.max_brightness; > unsigned int period = led_dat->period; > + int duty; > > - if (brightness == 0) { > - pwm_config(led_dat->pwm, 0, period); > - pwm_disable(led_dat->pwm); > + if (brightness == 0) > + duty = 0; > + else > + duty = brightness * period / max; > + > + if (pwm_can_sleep(led_dat->pwm)) { > + led_dat->duty = duty; > + schedule_work(&led_dat->work); > } else { > - pwm_config(led_dat->pwm, brightness * period / max, period); > - pwm_enable(led_dat->pwm); > + pwm_config(led_dat->pwm, duty, period); > + > + if (duty == 0) > + pwm_disable(led_dat->pwm); > + else > + pwm_enable(led_dat->pwm); > } > } > > @@ -100,6 +127,8 @@ 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; > > + 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 +182,8 @@ 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; > > + INIT_WORK(&led_dat->work, led_pwm_work); If we query the can_sleep before we can skip the INIT_WORK() when it is not set. > + > ret = led_classdev_register(&pdev->dev, &led_dat->cdev); > if (ret < 0) > goto err; > @@ -180,8 +211,10 @@ 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); > + cancel_work_sync(&priv->leds[i].work); > + } > > return 0; > } >
Le 25/01/2013 13:53, Peter Ujfalusi a écrit : > On 01/25/2013 11:01 AM, 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. >> >> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> >> --- >> drivers/leds/leds-pwm.c | 45 +++++++++++++++++++++++++++++++++++++++------ >> 1 files changed, 39 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c >> index a1ea5f6..1ffb6ba 100644 >> --- a/drivers/leds/leds-pwm.c >> +++ b/drivers/leds/leds-pwm.c >> @@ -23,12 +23,15 @@ >> #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; > > Would it be better if we also store the can_sleep in led_pwm_data struct and > initialize it when we probe the LEDs? It is not going to change runtime and we > will save one API call every time the LED has been changed. > Indeed, we will save a call each time. I will do it. >> }; >> >> struct led_pwm_priv { >> @@ -36,6 +39,20 @@ struct led_pwm_priv { >> struct led_pwm_data leds[0]; >> }; >> >> +static void led_pwm_work(struct work_struct *work) >> +{ >> + struct led_pwm_data *led_dat = >> + container_of(work, struct led_pwm_data, work); >> + 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); >> +} > > I think we can remove the duplicated code by doing something like this: > > 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) > { > struct led_pwm_data *led_dat = > container_of(led_cdev, struct led_pwm_data, cdev); > unsigned int max = led_dat->cdev.max_brightness; > > led_dat->duty = brightness * led_dat->period / max; > > if (led_dat->can_sleep) > schedule_work(&led_dat->work); > else > __led_pwm_set(led_dat); > } > > What do you think? > It is much better :) >> + >> static void led_pwm_set(struct led_classdev *led_cdev, >> enum led_brightness brightness) >> { >> @@ -43,13 +60,23 @@ static void led_pwm_set(struct led_classdev *led_cdev, >> container_of(led_cdev, struct led_pwm_data, cdev); >> unsigned int max = led_dat->cdev.max_brightness; >> unsigned int period = led_dat->period; >> + int duty; >> >> - if (brightness == 0) { >> - pwm_config(led_dat->pwm, 0, period); >> - pwm_disable(led_dat->pwm); >> + if (brightness == 0) >> + duty = 0; >> + else >> + duty = brightness * period / max; >> + >> + if (pwm_can_sleep(led_dat->pwm)) { >> + led_dat->duty = duty; >> + schedule_work(&led_dat->work); >> } else { >> - pwm_config(led_dat->pwm, brightness * period / max, period); >> - pwm_enable(led_dat->pwm); >> + pwm_config(led_dat->pwm, duty, period); >> + >> + if (duty == 0) >> + pwm_disable(led_dat->pwm); >> + else >> + pwm_enable(led_dat->pwm); >> } >> } > > > >> >> @@ -100,6 +127,8 @@ 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; >> >> + 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 +182,8 @@ 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; >> >> + INIT_WORK(&led_dat->work, led_pwm_work); > > If we query the can_sleep before we can skip the INIT_WORK() when it is not set. > I will do like this. >> + >> ret = led_classdev_register(&pdev->dev, &led_dat->cdev); >> if (ret < 0) >> goto err; >> @@ -180,8 +211,10 @@ 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); >> + cancel_work_sync(&priv->leds[i].work); >> + } >> >> return 0; >> } >> > > Thank you, Florian
On 01/25/2013 02:04 PM, Florian Vaussard wrote: >>> @@ -153,6 +182,8 @@ 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; >>> >>> + INIT_WORK(&led_dat->work, led_pwm_work); >> >> If we query the can_sleep before we can skip the INIT_WORK() when it is not >> set. >> > > I will do like this. On the other hand you should also check at remove time that you are not going to cancel a work on a non initialized one. > >>> + >>> ret = led_classdev_register(&pdev->dev, &led_dat->cdev); >>> if (ret < 0) >>> goto err; >>> @@ -180,8 +211,10 @@ 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); >>> + cancel_work_sync(&priv->leds[i].work); >>> + } >>> >>> return 0; >>> } >>> >> >> > > Thank you, > > Florian
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c index a1ea5f6..1ffb6ba 100644 --- a/drivers/leds/leds-pwm.c +++ b/drivers/leds/leds-pwm.c @@ -23,12 +23,15 @@ #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; }; struct led_pwm_priv { @@ -36,6 +39,20 @@ struct led_pwm_priv { struct led_pwm_data leds[0]; }; +static void led_pwm_work(struct work_struct *work) +{ + struct led_pwm_data *led_dat = + container_of(work, struct led_pwm_data, work); + 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_set(struct led_classdev *led_cdev, enum led_brightness brightness) { @@ -43,13 +60,23 @@ static void led_pwm_set(struct led_classdev *led_cdev, container_of(led_cdev, struct led_pwm_data, cdev); unsigned int max = led_dat->cdev.max_brightness; unsigned int period = led_dat->period; + int duty; - if (brightness == 0) { - pwm_config(led_dat->pwm, 0, period); - pwm_disable(led_dat->pwm); + if (brightness == 0) + duty = 0; + else + duty = brightness * period / max; + + if (pwm_can_sleep(led_dat->pwm)) { + led_dat->duty = duty; + schedule_work(&led_dat->work); } else { - pwm_config(led_dat->pwm, brightness * period / max, period); - pwm_enable(led_dat->pwm); + pwm_config(led_dat->pwm, duty, period); + + if (duty == 0) + pwm_disable(led_dat->pwm); + else + pwm_enable(led_dat->pwm); } } @@ -100,6 +127,8 @@ 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; + 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 +182,8 @@ 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; + 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,10 @@ 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); + 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. Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch> --- drivers/leds/leds-pwm.c | 45 +++++++++++++++++++++++++++++++++++++++------ 1 files changed, 39 insertions(+), 6 deletions(-)