[PATCH 0/3] leds-pwm: Defer PWM calls if PWM can sleep

[PATCH 0/3] leds-pwm: Defer PWM calls if PWM can sleep

[Florian Vaussard]

Hello,

When using the leds-pwm module with external PWM chips connected through
I2C, the kernel will panic when settings a trigger. In this case, PWM
calls can sleep, and should be deferred.

Patch 1 and 2 add the necessary API to the PWM subsystem, and update
matching drivers.

Patch 3 updates leds-pwm to use a worker if the PWM chip can sleep, or performs
direct calls otherwise.

This patchset is based on the for-next branch of the led subsystem [1],
as patch 3 depends on the recent work of Peter for DT bindings in leds-pwm.
The pwm patches should probably follow the same path.

Tested on Overo (OMAP3 + TWL4030) with device tree.

Best regards,

Florian

[1] git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds.git


Florian Vaussard (3):
  pwm: Add pwm_cansleep() as exported API to users
  pwm: Add can_sleep property to drivers
  leds: leds-pwm: Defer led_pwm_set() if PWM can sleep

 drivers/leds/leds-pwm.c   |   45 +++++++++++++++++++++++++++++++++++++++------
 drivers/pwm/core.c        |   12 ++++++++++++
 drivers/pwm/pwm-twl-led.c |    1 +
 drivers/pwm/pwm-twl.c     |    1 +
 include/linux/pwm.h       |   10 ++++++++++
 5 files changed, 63 insertions(+), 6 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] pwm: Add pwm_cansleep() as exported API to users

[Florian Vaussard]

Calls to some external PWM chips can sleep. To help users,
add pwm_cansleep() API.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/pwm/core.c  |   12 ++++++++++++
 include/linux/pwm.h |   10 ++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4a13da4..f49bfa6 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 }
 EXPORT_SYMBOL_GPL(devm_pwm_put);
 
+/**
+  * pwm_can_sleep() - report whether pwm access will sleep
+  * @pwm: PWM device
+  *
+  * It returns nonzero if accessing the PWM can sleep.
+  */
+int pwm_can_sleep(struct pwm_device *pwm)
+{
+	return pwm->chip->can_sleep;
+}
+EXPORT_SYMBOL_GPL(pwm_can_sleep);
+
 #ifdef CONFIG_DEBUG_FS
 static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 {
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 70655a2..2aee75d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -146,6 +146,8 @@ struct pwm_ops {
  * @base: number of first PWM controlled by this chip
  * @npwm: number of PWMs controlled by this chip
  * @pwms: array of PWM devices allocated by the framework
+ * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
+ *      as they must while accessing PWM chips over I2C or SPI
  */
 struct pwm_chip {
 	struct device		*dev;
@@ -159,6 +161,7 @@ struct pwm_chip {
 	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
 					    const struct of_phandle_args *args);
 	unsigned int		of_pwm_n_cells;
+	unsigned int		can_sleep:1;
 };
 
 #if IS_ENABLED(CONFIG_PWM)
@@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
 				   const char *con_id);
 void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
+
+int pwm_can_sleep(struct pwm_device *pwm);
 #else
 static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
 {
@@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
 static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
 {
 }
+
+static inline int pwm_can_sleep(struct pwm_device *pwm)
+{
+	return -EINVAL;
+}
 #endif
 
 struct pwm_lookup {
-- 
1.7.5.4

[PATCH 2/3] pwm: Add can_sleep property to drivers

[Florian Vaussard]

Calls to PWM drivers connected through I2C can sleep.
Use the new can_sleep property.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/pwm/pwm-twl-led.c |    1 +
 drivers/pwm/pwm-twl.c     |    1 +
 2 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 9dfa0f3..6261193 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -300,6 +300,7 @@ static int twl_pwmled_probe(struct platform_device *pdev)
 
 	twl->chip.dev = &pdev->dev;
 	twl->chip.base = -1;
+	twl->chip.can_sleep = 1;
 
 	mutex_init(&twl->mutex);
 
diff --git a/drivers/pwm/pwm-twl.c b/drivers/pwm/pwm-twl.c
index e65db95..4e56cd8 100644
--- a/drivers/pwm/pwm-twl.c
+++ b/drivers/pwm/pwm-twl.c
@@ -315,6 +315,7 @@ static int twl_pwm_probe(struct platform_device *pdev)
 	twl->chip.dev = &pdev->dev;
 	twl->chip.base = -1;
 	twl->chip.npwm = 2;
+	twl->chip.can_sleep = 1;
 
 	mutex_init(&twl->mutex);
 
-- 
1.7.5.4

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

[Florian Vaussard]

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;
 };
 
 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;
 }
-- 
1.7.5.4

[PATCH 1/3] pwm: Add pwm_cansleep() as exported API to users

[Peter Ujfalusi]

On 01/25/2013 11:01 AM, Florian Vaussard wrote:
> Calls to some external PWM chips can sleep. To help users,
> add pwm_cansleep() API.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
> ---
>  drivers/pwm/core.c  |   12 ++++++++++++
>  include/linux/pwm.h |   10 ++++++++++
>  2 files changed, 22 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 4a13da4..f49bfa6 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>  }
>  EXPORT_SYMBOL_GPL(devm_pwm_put);
>  
> +/**
> +  * pwm_can_sleep() - report whether pwm access will sleep
> +  * @pwm: PWM device
> +  *
> +  * It returns nonzero if accessing the PWM can sleep.
> +  */
> +int pwm_can_sleep(struct pwm_device *pwm)
> +{
> +	return pwm->chip->can_sleep;
> +}
> +EXPORT_SYMBOL_GPL(pwm_can_sleep);

Can we name this as pwm_cansleep() to be more alligned with the
gpio_cansleep() API?

> +
>  #ifdef CONFIG_DEBUG_FS
>  static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  {
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 70655a2..2aee75d 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -146,6 +146,8 @@ struct pwm_ops {
>   * @base: number of first PWM controlled by this chip
>   * @npwm: number of PWMs controlled by this chip
>   * @pwms: array of PWM devices allocated by the framework
> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
> + *      as they must while accessing PWM chips over I2C or SPI
>   */
>  struct pwm_chip {
>  	struct device		*dev;
> @@ -159,6 +161,7 @@ struct pwm_chip {
>  	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>  					    const struct of_phandle_args *args);
>  	unsigned int		of_pwm_n_cells;
> +	unsigned int		can_sleep:1;
>  };
>  
>  #if IS_ENABLED(CONFIG_PWM)
> @@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
>  struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
>  				   const char *con_id);
>  void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
> +
> +int pwm_can_sleep(struct pwm_device *pwm);
>  #else
>  static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
>  {
> @@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
>  static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>  {
>  }
> +
> +static inline int pwm_can_sleep(struct pwm_device *pwm)
> +{
> +	return -EINVAL;

I think we should return 0 here instead an error.

> +}
>  #endif
>  
>  struct pwm_lookup {
> 


-- 
P?ter

[PATCH 1/3] pwm: Add pwm_cansleep() as exported API to users

[Florian Vaussard]

Le 25/01/2013 13:32, Peter Ujfalusi a ?crit :
> On 01/25/2013 11:01 AM, Florian Vaussard wrote:
>> Calls to some external PWM chips can sleep. To help users,
>> add pwm_cansleep() API.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
>> ---
>>   drivers/pwm/core.c  |   12 ++++++++++++
>>   include/linux/pwm.h |   10 ++++++++++
>>   2 files changed, 22 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 4a13da4..f49bfa6 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -763,6 +763,18 @@ void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>>   }
>>   EXPORT_SYMBOL_GPL(devm_pwm_put);
>>
>> +/**
>> +  * pwm_can_sleep() - report whether pwm access will sleep
>> +  * @pwm: PWM device
>> +  *
>> +  * It returns nonzero if accessing the PWM can sleep.
>> +  */
>> +int pwm_can_sleep(struct pwm_device *pwm)
>> +{
>> +	return pwm->chip->can_sleep;
>> +}
>> +EXPORT_SYMBOL_GPL(pwm_can_sleep);
>
> Can we name this as pwm_cansleep() to be more alligned with the
> gpio_cansleep() API?
>

Sure

>> +
>>   #ifdef CONFIG_DEBUG_FS
>>   static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>>   {
>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>> index 70655a2..2aee75d 100644
>> --- a/include/linux/pwm.h
>> +++ b/include/linux/pwm.h
>> @@ -146,6 +146,8 @@ struct pwm_ops {
>>    * @base: number of first PWM controlled by this chip
>>    * @npwm: number of PWMs controlled by this chip
>>    * @pwms: array of PWM devices allocated by the framework
>> + * @can_sleep: flag must be set iff config()/enable()/disable() methods sleep,
>> + *      as they must while accessing PWM chips over I2C or SPI
>>    */
>>   struct pwm_chip {
>>   	struct device		*dev;
>> @@ -159,6 +161,7 @@ struct pwm_chip {
>>   	struct pwm_device *	(*of_xlate)(struct pwm_chip *pc,
>>   					    const struct of_phandle_args *args);
>>   	unsigned int		of_pwm_n_cells;
>> +	unsigned int		can_sleep:1;
>>   };
>>
>>   #if IS_ENABLED(CONFIG_PWM)
>> @@ -182,6 +185,8 @@ struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
>>   struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
>>   				   const char *con_id);
>>   void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
>> +
>> +int pwm_can_sleep(struct pwm_device *pwm);
>>   #else
>>   static inline int pwm_set_chip_data(struct pwm_device *pwm, void *data)
>>   {
>> @@ -242,6 +247,11 @@ static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
>>   static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
>>   {
>>   }
>> +
>> +static inline int pwm_can_sleep(struct pwm_device *pwm)
>> +{
>> +	return -EINVAL;
>
> I think we should return 0 here instead an error.
>

Ok, it makes sense.

>> +}
>>   #endif
>>
>>   struct pwm_lookup {
>>
>
>

Thank you,

Florian

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

[Peter Ujfalusi]

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;
>  }
> 


-- 
P?ter

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

[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

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

[Peter Ujfalusi]

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


-- 
P?ter