[PATCH 3/3] leds: leds-pwm: Defer led_pwm_set() if PWM can sleep

[florian.vaussard@epfl.ch (Florian Vaussard)]

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