Messages in this thread | | | Date | Fri, 25 Jan 2013 14:04:09 +0100 | From | Florian Vaussard <> | Subject | Re: [PATCH 3/3] leds: leds-pwm: Defer led_pwm_set() if PWM can sleep |
| |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |