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.vauss...@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;
> }
>
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
