* [RFC] [PATCH 0/2] leds: use workqueue in led_set_brightness() API internally @ 2012-09-14 7:53 Bryan Wu 2012-09-14 7:53 ` [PATCH 1/2] " Bryan Wu 2012-09-14 7:53 ` [PATCH 2/2] leds-gpio: remove workqueue in .brightness_set() Bryan Wu 0 siblings, 2 replies; 5+ messages in thread From: Bryan Wu @ 2012-09-14 7:53 UTC (permalink / raw) To: rpurdie, linux-kernel, akpm, linux-leds, broonie, fabio.baltieri, shuahkhan, raph, tpiepho LED class drivers use duplicated workqueue method to implement .brightness_set() and some of them forget to use workqueue, since workqueue is required by .brightness_set() in atomic context. This patchset add workqueue into API function led_set_brightness(). So class drivers don't need to worry about it. With the first patch, we can remove all the workqueue handling code in class drivers. For example, workqueue was removed from leds-gpio.c Bryan Wu (2): leds: use workqueue in led_set_brightness() API internally leds-gpio: remove workqueue in .brightness_set() drivers/leds/led-class.c | 23 +++++++++++---------- drivers/leds/led-core.c | 15 +++++++------- drivers/leds/leds-gpio.c | 43 ++++++--------------------------------- drivers/leds/leds.h | 11 ++-------- drivers/leds/ledtrig-backlight.c | 8 ++++---- drivers/leds/ledtrig-default-on.c | 2 +- drivers/leds/ledtrig-gpio.c | 6 +++--- drivers/leds/ledtrig-heartbeat.c | 2 +- drivers/leds/ledtrig-oneshot.c | 4 ++-- drivers/leds/ledtrig-transient.c | 8 ++++---- include/linux/leds.h | 12 ++++++----- 11 files changed, 49 insertions(+), 85 deletions(-) -- 1.7.11.4 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] leds: use workqueue in led_set_brightness() API internally 2012-09-14 7:53 [RFC] [PATCH 0/2] leds: use workqueue in led_set_brightness() API internally Bryan Wu @ 2012-09-14 7:53 ` Bryan Wu 2012-09-14 22:25 ` Shuah Khan 2012-09-15 15:23 ` Fabio Baltieri 2012-09-14 7:53 ` [PATCH 2/2] leds-gpio: remove workqueue in .brightness_set() Bryan Wu 1 sibling, 2 replies; 5+ messages in thread From: Bryan Wu @ 2012-09-14 7:53 UTC (permalink / raw) To: rpurdie, linux-kernel, akpm, linux-leds, broonie, fabio.baltieri, shuahkhan, raph, tpiepho The API function led_set_brightness() and __led_set_brightness will call .brightness_set() function provided by led class drivers. So .brightness_set() function will run in atomic context, which requires led class drivers use workqueue in .brightness_set(). Finally, all the led class driver implemented their own workqueue in .brightness_set(). For those missing this, we will face runtime warning or error when running it in atomic context. This patch adds a workqueue in led_set_brightness() internally. LED class driver doesn't need to care about duplicated workqueue stuff in their own driver. Also this patch unified the led_set_brightness() and __led_set_brightness() Signed-off-by: Bryan Wu <bryan.wu@canonical.com> --- drivers/leds/led-class.c | 23 ++++++++++++----------- drivers/leds/led-core.c | 15 +++++++-------- drivers/leds/leds.h | 11 ++--------- drivers/leds/ledtrig-backlight.c | 8 ++++---- drivers/leds/ledtrig-default-on.c | 2 +- drivers/leds/ledtrig-gpio.c | 6 +++--- drivers/leds/ledtrig-heartbeat.c | 2 +- drivers/leds/ledtrig-oneshot.c | 4 ++-- drivers/leds/ledtrig-transient.c | 8 ++++---- include/linux/leds.h | 12 +++++++----- 10 files changed, 43 insertions(+), 48 deletions(-) diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 48cce18..7a3e886 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -53,7 +53,7 @@ static ssize_t led_brightness_store(struct device *dev, if (state == LED_OFF) led_trigger_remove(led_cdev); - __led_set_brightness(led_cdev, state); + led_set_brightness(led_cdev, state); return size; } @@ -82,7 +82,7 @@ static void led_timer_function(unsigned long data) unsigned long delay; if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { - __led_set_brightness(led_cdev, LED_OFF); + led_set_brightness(led_cdev, LED_OFF); return; } @@ -105,7 +105,7 @@ static void led_timer_function(unsigned long data) delay = led_cdev->blink_delay_off; } - __led_set_brightness(led_cdev, brightness); + led_set_brightness(led_cdev, brightness); /* Return in next iteration if led is in one-shot mode and we are in * the final blink state so that the led is toggled each delay_on + @@ -124,14 +124,15 @@ static void led_timer_function(unsigned long data) mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay)); } -static void set_brightness_delayed(struct work_struct *ws) +static void led_set_brightness_work(struct work_struct *ws) { struct led_classdev *led_cdev = container_of(ws, struct led_classdev, set_brightness_work); - led_stop_software_blink(led_cdev); + if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) + led_stop_software_blink(led_cdev); - __led_set_brightness(led_cdev, led_cdev->delayed_set_value); + led_cdev->brightness_set(led_cdev, led_cdev->brightness); } /** @@ -141,7 +142,7 @@ static void set_brightness_delayed(struct work_struct *ws) void led_classdev_suspend(struct led_classdev *led_cdev) { led_cdev->flags |= LED_SUSPENDED; - led_cdev->brightness_set(led_cdev, 0); + led_set_brightness(led_cdev, 0); } EXPORT_SYMBOL_GPL(led_classdev_suspend); @@ -151,7 +152,7 @@ EXPORT_SYMBOL_GPL(led_classdev_suspend); */ void led_classdev_resume(struct led_classdev *led_cdev) { - led_cdev->brightness_set(led_cdev, led_cdev->brightness); + led_set_brightness(led_cdev, led_cdev->brightness); led_cdev->flags &= ~LED_SUSPENDED; } EXPORT_SYMBOL_GPL(led_classdev_resume); @@ -201,7 +202,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) led_update_brightness(led_cdev); - INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed); + INIT_WORK(&led_cdev->set_brightness_work, led_set_brightness_work); init_timer(&led_cdev->blink_timer); led_cdev->blink_timer.function = led_timer_function; @@ -233,12 +234,12 @@ void led_classdev_unregister(struct led_classdev *led_cdev) up_write(&led_cdev->trigger_lock); #endif - cancel_work_sync(&led_cdev->set_brightness_work); - /* Stop blinking */ led_stop_software_blink(led_cdev); led_set_brightness(led_cdev, LED_OFF); + flush_work(&led_cdev->set_brightness_work); + device_unregister(led_cdev->dev); down_write(&leds_list_lock); diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index ce8921a..c212262 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -45,7 +45,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, /* never off - just set to brightness */ if (!delay_off) { - __led_set_brightness(led_cdev, led_cdev->blink_brightness); + led_set_brightness(led_cdev, led_cdev->blink_brightness); return; } @@ -114,13 +114,12 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink); void led_set_brightness(struct led_classdev *led_cdev, enum led_brightness brightness) { - /* delay brightness setting if need to stop soft-blink timer */ - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { - led_cdev->delayed_set_value = brightness; - schedule_work(&led_cdev->set_brightness_work); - return; - } + if (brightness > led_cdev->max_brightness) + brightness = led_cdev->max_brightness; - __led_set_brightness(led_cdev, brightness); + led_cdev->brightness = brightness; + + if (!(led_cdev->flags & LED_SUSPENDED)) + schedule_work(&led_cdev->set_brightness_work); } EXPORT_SYMBOL(led_set_brightness); diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h index 4c50365..745bf06 100644 --- a/drivers/leds/leds.h +++ b/drivers/leds/leds.h @@ -17,15 +17,8 @@ #include <linux/rwsem.h> #include <linux/leds.h> -static inline void __led_set_brightness(struct led_classdev *led_cdev, - enum led_brightness value) -{ - if (value > led_cdev->max_brightness) - value = led_cdev->max_brightness; - led_cdev->brightness = value; - if (!(led_cdev->flags & LED_SUSPENDED)) - led_cdev->brightness_set(led_cdev, value); -} +void led_set_brightness(struct led_classdev *led_cdev, + enum led_brightness value); static inline int led_get_brightness(struct led_classdev *led_cdev) { diff --git a/drivers/leds/ledtrig-backlight.c b/drivers/leds/ledtrig-backlight.c index b941685..e272686 100644 --- a/drivers/leds/ledtrig-backlight.c +++ b/drivers/leds/ledtrig-backlight.c @@ -46,9 +46,9 @@ static int fb_notifier_callback(struct notifier_block *p, if ((n->old_status == UNBLANK) ^ n->invert) { n->brightness = led->brightness; - __led_set_brightness(led, LED_OFF); + led_set_brightness(led, LED_OFF); } else { - __led_set_brightness(led, n->brightness); + led_set_brightness(led, n->brightness); } n->old_status = new_status; @@ -87,9 +87,9 @@ static ssize_t bl_trig_invert_store(struct device *dev, /* After inverting, we need to update the LED. */ if ((n->old_status == BLANK) ^ n->invert) - __led_set_brightness(led, LED_OFF); + led_set_brightness(led, LED_OFF); else - __led_set_brightness(led, n->brightness); + led_set_brightness(led, n->brightness); return num; } diff --git a/drivers/leds/ledtrig-default-on.c b/drivers/leds/ledtrig-default-on.c index eac1f1b..a4ef54b 100644 --- a/drivers/leds/ledtrig-default-on.c +++ b/drivers/leds/ledtrig-default-on.c @@ -19,7 +19,7 @@ static void defon_trig_activate(struct led_classdev *led_cdev) { - __led_set_brightness(led_cdev, led_cdev->max_brightness); + led_set_brightness(led_cdev, led_cdev->max_brightness); } static struct led_trigger defon_led_trigger = { diff --git a/drivers/leds/ledtrig-gpio.c b/drivers/leds/ledtrig-gpio.c index ba215dc..f057c10 100644 --- a/drivers/leds/ledtrig-gpio.c +++ b/drivers/leds/ledtrig-gpio.c @@ -54,12 +54,12 @@ static void gpio_trig_work(struct work_struct *work) if (tmp) { if (gpio_data->desired_brightness) - __led_set_brightness(gpio_data->led, + led_set_brightness(gpio_data->led, gpio_data->desired_brightness); else - __led_set_brightness(gpio_data->led, LED_FULL); + led_set_brightness(gpio_data->led, LED_FULL); } else { - __led_set_brightness(gpio_data->led, LED_OFF); + led_set_brightness(gpio_data->led, LED_OFF); } } diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c index 1edc746..a019fbb 100644 --- a/drivers/leds/ledtrig-heartbeat.c +++ b/drivers/leds/ledtrig-heartbeat.c @@ -74,7 +74,7 @@ static void led_heartbeat_function(unsigned long data) break; } - __led_set_brightness(led_cdev, brightness); + led_set_brightness(led_cdev, brightness); mod_timer(&heartbeat_data->timer, jiffies + delay); } diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c index 2c029aa..2b10b28 100644 --- a/drivers/leds/ledtrig-oneshot.c +++ b/drivers/leds/ledtrig-oneshot.c @@ -63,9 +63,9 @@ static ssize_t led_invert_store(struct device *dev, oneshot_data->invert = !!state; if (oneshot_data->invert) - __led_set_brightness(led_cdev, LED_FULL); + led_set_brightness(led_cdev, LED_FULL); else - __led_set_brightness(led_cdev, LED_OFF); + led_set_brightness(led_cdev, LED_OFF); return size; } diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c index 398f104..83179f4 100644 --- a/drivers/leds/ledtrig-transient.c +++ b/drivers/leds/ledtrig-transient.c @@ -41,7 +41,7 @@ static void transient_timer_function(unsigned long data) struct transient_trig_data *transient_data = led_cdev->trigger_data; transient_data->activate = 0; - __led_set_brightness(led_cdev, transient_data->restore_state); + led_set_brightness(led_cdev, transient_data->restore_state); } static ssize_t transient_activate_show(struct device *dev, @@ -72,7 +72,7 @@ static ssize_t transient_activate_store(struct device *dev, if (state == 0 && transient_data->activate == 1) { del_timer(&transient_data->timer); transient_data->activate = state; - __led_set_brightness(led_cdev, transient_data->restore_state); + led_set_brightness(led_cdev, transient_data->restore_state); return size; } @@ -80,7 +80,7 @@ static ssize_t transient_activate_store(struct device *dev, if (state == 1 && transient_data->activate == 0 && transient_data->duration != 0) { transient_data->activate = state; - __led_set_brightness(led_cdev, transient_data->state); + led_set_brightness(led_cdev, transient_data->state); transient_data->restore_state = (transient_data->state == LED_FULL) ? LED_OFF : LED_FULL; mod_timer(&transient_data->timer, @@ -203,7 +203,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev) if (led_cdev->activated) { del_timer_sync(&transient_data->timer); - __led_set_brightness(led_cdev, transient_data->restore_state); + led_set_brightness(led_cdev, transient_data->restore_state); device_remove_file(led_cdev->dev, &dev_attr_activate); device_remove_file(led_cdev->dev, &dev_attr_duration); device_remove_file(led_cdev->dev, &dev_attr_state); diff --git a/include/linux/leds.h b/include/linux/leds.h index 5676197..d7f5697 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -43,10 +43,15 @@ struct led_classdev { #define LED_BLINK_ONESHOT_STOP (1 << 18) #define LED_BLINK_INVERT (1 << 19) - /* Set LED brightness level */ - /* Must not sleep, use a workqueue if needed */ + /* + * Set LED brightness level + * use a workqueue internally, so driver writer doesn't need to + * care about using duplicated workqueue in brightness_set() + */ void (*brightness_set)(struct led_classdev *led_cdev, enum led_brightness brightness); + struct work_struct set_brightness_work; + /* Get LED brightness level */ enum led_brightness (*brightness_get)(struct led_classdev *led_cdev); @@ -70,9 +75,6 @@ struct led_classdev { struct timer_list blink_timer; int blink_brightness; - struct work_struct set_brightness_work; - int delayed_set_value; - #ifdef CONFIG_LEDS_TRIGGERS /* Protects the trigger data below */ struct rw_semaphore trigger_lock; -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] leds: use workqueue in led_set_brightness() API internally 2012-09-14 7:53 ` [PATCH 1/2] " Bryan Wu @ 2012-09-14 22:25 ` Shuah Khan 2012-09-15 15:23 ` Fabio Baltieri 1 sibling, 0 replies; 5+ messages in thread From: Shuah Khan @ 2012-09-14 22:25 UTC (permalink / raw) To: Bryan Wu Cc: rpurdie, linux-kernel, akpm, linux-leds, broonie, fabio.baltieri, raph, tpiepho On Fri, Sep 14, 2012 at 1:53 AM, Bryan Wu <bryan.wu@canonical.com> wrote: > The API function led_set_brightness() and __led_set_brightness will > call .brightness_set() function provided by led class drivers. So > .brightness_set() function will run in atomic context, which requires > led class drivers use workqueue in .brightness_set(). > > Finally, all the led class driver implemented their own workqueue in > .brightness_set(). For those missing this, we will face runtime warning > or error when running it in atomic context. > > This patch adds a workqueue in led_set_brightness() internally. LED > class driver doesn't need to care about duplicated workqueue stuff in > their own driver. > > Also this patch unified the led_set_brightness() and __led_set_brightness() > > Signed-off-by: Bryan Wu <bryan.wu@canonical.com> > --- > drivers/leds/led-class.c | 23 ++++++++++++----------- > drivers/leds/led-core.c | 15 +++++++-------- > drivers/leds/leds.h | 11 ++--------- > drivers/leds/ledtrig-backlight.c | 8 ++++---- > drivers/leds/ledtrig-default-on.c | 2 +- > drivers/leds/ledtrig-gpio.c | 6 +++--- > drivers/leds/ledtrig-heartbeat.c | 2 +- > drivers/leds/ledtrig-oneshot.c | 4 ++-- > drivers/leds/ledtrig-transient.c | 8 ++++---- Bryan, ledtrig-transient.c change looks good. Thanks for updating it. -- Shuah ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] leds: use workqueue in led_set_brightness() API internally 2012-09-14 7:53 ` [PATCH 1/2] " Bryan Wu 2012-09-14 22:25 ` Shuah Khan @ 2012-09-15 15:23 ` Fabio Baltieri 1 sibling, 0 replies; 5+ messages in thread From: Fabio Baltieri @ 2012-09-15 15:23 UTC (permalink / raw) To: Bryan Wu Cc: rpurdie, linux-kernel, akpm, linux-leds, broonie, shuahkhan, raph, tpiepho Hello Bryan, On Fri, Sep 14, 2012 at 03:53:02PM +0800, Bryan Wu wrote: > The API function led_set_brightness() and __led_set_brightness will > call .brightness_set() function provided by led class drivers. So > .brightness_set() function will run in atomic context, which requires > led class drivers use workqueue in .brightness_set(). > > Finally, all the led class driver implemented their own workqueue in > .brightness_set(). For those missing this, we will face runtime warning > or error when running it in atomic context. > > This patch adds a workqueue in led_set_brightness() internally. LED > class driver doesn't need to care about duplicated workqueue stuff in > their own driver. > > Also this patch unified the led_set_brightness() and __led_set_brightness() I like the idea of a semplification of led driver's code and the removal of the confusion between the two variants of led_set_brightness. Despite that, due to the lockless nature of the soft-blink code in the led subsystem I expect it to break quite easility with this kind of modification. > Signed-off-by: Bryan Wu <bryan.wu@canonical.com> > --- > drivers/leds/led-class.c | 23 ++++++++++++----------- > drivers/leds/led-core.c | 15 +++++++-------- > drivers/leds/leds.h | 11 ++--------- > drivers/leds/ledtrig-backlight.c | 8 ++++---- > drivers/leds/ledtrig-default-on.c | 2 +- > drivers/leds/ledtrig-gpio.c | 6 +++--- > drivers/leds/ledtrig-heartbeat.c | 2 +- > drivers/leds/ledtrig-oneshot.c | 4 ++-- > drivers/leds/ledtrig-transient.c | 8 ++++---- > include/linux/leds.h | 12 +++++++----- > 10 files changed, 43 insertions(+), 48 deletions(-) > > diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c > index 48cce18..7a3e886 100644 > --- a/drivers/leds/led-class.c > +++ b/drivers/leds/led-class.c > @@ -53,7 +53,7 @@ static ssize_t led_brightness_store(struct device *dev, > > if (state == LED_OFF) > led_trigger_remove(led_cdev); > - __led_set_brightness(led_cdev, state); > + led_set_brightness(led_cdev, state); > > return size; > } > @@ -82,7 +82,7 @@ static void led_timer_function(unsigned long data) > unsigned long delay; > > if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { > - __led_set_brightness(led_cdev, LED_OFF); > + led_set_brightness(led_cdev, LED_OFF); > return; > } > > @@ -105,7 +105,7 @@ static void led_timer_function(unsigned long data) > delay = led_cdev->blink_delay_off; > } > > - __led_set_brightness(led_cdev, brightness); > + led_set_brightness(led_cdev, brightness); > > /* Return in next iteration if led is in one-shot mode and we are in > * the final blink state so that the led is toggled each delay_on + > @@ -124,14 +124,15 @@ static void led_timer_function(unsigned long data) > mod_timer(&led_cdev->blink_timer, jiffies + msecs_to_jiffies(delay)); > } > > -static void set_brightness_delayed(struct work_struct *ws) > +static void led_set_brightness_work(struct work_struct *ws) > { > struct led_classdev *led_cdev = > container_of(ws, struct led_classdev, set_brightness_work); > > - led_stop_software_blink(led_cdev); > + if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) > + led_stop_software_blink(led_cdev); This actually breaks soft-blink, as led_set_brightness_work is called by the soft-blink code itself when soft blink is active. You may probabily want replicate the actual set code in led_timer_function() or to keep it in an internal function. One of the effects is that blink_delay_{on,off} are erroneously set to 0 when setting 'timer' or 'oneshot' as current trigger. > - __led_set_brightness(led_cdev, led_cdev->delayed_set_value); > + led_cdev->brightness_set(led_cdev, led_cdev->brightness); Is it ok to access led_cdev->brightness without locks? I'm afraid that this may race in SMP if another CPU is running led_set_brightness. The lockless solution would be to make sure the work is canceled before setting the new value, through that may not be kind to hardirq context. > } > > /** > @@ -141,7 +142,7 @@ static void set_brightness_delayed(struct work_struct *ws) > void led_classdev_suspend(struct led_classdev *led_cdev) > { > led_cdev->flags |= LED_SUSPENDED; > - led_cdev->brightness_set(led_cdev, 0); > + led_set_brightness(led_cdev, 0); > } > EXPORT_SYMBOL_GPL(led_classdev_suspend); > > @@ -151,7 +152,7 @@ EXPORT_SYMBOL_GPL(led_classdev_suspend); > */ > void led_classdev_resume(struct led_classdev *led_cdev) > { > - led_cdev->brightness_set(led_cdev, led_cdev->brightness); > + led_set_brightness(led_cdev, led_cdev->brightness); > led_cdev->flags &= ~LED_SUSPENDED; > } > EXPORT_SYMBOL_GPL(led_classdev_resume); > @@ -201,7 +202,7 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev) > > led_update_brightness(led_cdev); > > - INIT_WORK(&led_cdev->set_brightness_work, set_brightness_delayed); > + INIT_WORK(&led_cdev->set_brightness_work, led_set_brightness_work); > > init_timer(&led_cdev->blink_timer); > led_cdev->blink_timer.function = led_timer_function; > @@ -233,12 +234,12 @@ void led_classdev_unregister(struct led_classdev *led_cdev) > up_write(&led_cdev->trigger_lock); > #endif > > - cancel_work_sync(&led_cdev->set_brightness_work); > - > /* Stop blinking */ > led_stop_software_blink(led_cdev); > led_set_brightness(led_cdev, LED_OFF); > > + flush_work(&led_cdev->set_brightness_work); > + Is it possible that this code runs with led_set_brightness_work() already scheduled on another CPU? In that case flush_work would not wait for it. Fabio > device_unregister(led_cdev->dev); > > down_write(&leds_list_lock); > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index ce8921a..c212262 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -45,7 +45,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, > > /* never off - just set to brightness */ > if (!delay_off) { > - __led_set_brightness(led_cdev, led_cdev->blink_brightness); > + led_set_brightness(led_cdev, led_cdev->blink_brightness); > return; > } > > @@ -114,13 +114,12 @@ EXPORT_SYMBOL_GPL(led_stop_software_blink); > void led_set_brightness(struct led_classdev *led_cdev, > enum led_brightness brightness) > { > - /* delay brightness setting if need to stop soft-blink timer */ > - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { > - led_cdev->delayed_set_value = brightness; > - schedule_work(&led_cdev->set_brightness_work); > - return; > - } > + if (brightness > led_cdev->max_brightness) > + brightness = led_cdev->max_brightness; > > - __led_set_brightness(led_cdev, brightness); > + led_cdev->brightness = brightness; > + > + if (!(led_cdev->flags & LED_SUSPENDED)) > + schedule_work(&led_cdev->set_brightness_work); > } > EXPORT_SYMBOL(led_set_brightness); > diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h > index 4c50365..745bf06 100644 > --- a/drivers/leds/leds.h > +++ b/drivers/leds/leds.h > @@ -17,15 +17,8 @@ > #include <linux/rwsem.h> > #include <linux/leds.h> > > -static inline void __led_set_brightness(struct led_classdev *led_cdev, > - enum led_brightness value) > -{ > - if (value > led_cdev->max_brightness) > - value = led_cdev->max_brightness; > - led_cdev->brightness = value; > - if (!(led_cdev->flags & LED_SUSPENDED)) > - led_cdev->brightness_set(led_cdev, value); > -} > +void led_set_brightness(struct led_classdev *led_cdev, > + enum led_brightness value); > > static inline int led_get_brightness(struct led_classdev *led_cdev) > { > diff --git a/drivers/leds/ledtrig-backlight.c b/drivers/leds/ledtrig-backlight.c > index b941685..e272686 100644 > --- a/drivers/leds/ledtrig-backlight.c > +++ b/drivers/leds/ledtrig-backlight.c > @@ -46,9 +46,9 @@ static int fb_notifier_callback(struct notifier_block *p, > > if ((n->old_status == UNBLANK) ^ n->invert) { > n->brightness = led->brightness; > - __led_set_brightness(led, LED_OFF); > + led_set_brightness(led, LED_OFF); > } else { > - __led_set_brightness(led, n->brightness); > + led_set_brightness(led, n->brightness); > } > > n->old_status = new_status; > @@ -87,9 +87,9 @@ static ssize_t bl_trig_invert_store(struct device *dev, > > /* After inverting, we need to update the LED. */ > if ((n->old_status == BLANK) ^ n->invert) > - __led_set_brightness(led, LED_OFF); > + led_set_brightness(led, LED_OFF); > else > - __led_set_brightness(led, n->brightness); > + led_set_brightness(led, n->brightness); > > return num; > } > diff --git a/drivers/leds/ledtrig-default-on.c b/drivers/leds/ledtrig-default-on.c > index eac1f1b..a4ef54b 100644 > --- a/drivers/leds/ledtrig-default-on.c > +++ b/drivers/leds/ledtrig-default-on.c > @@ -19,7 +19,7 @@ > > static void defon_trig_activate(struct led_classdev *led_cdev) > { > - __led_set_brightness(led_cdev, led_cdev->max_brightness); > + led_set_brightness(led_cdev, led_cdev->max_brightness); > } > > static struct led_trigger defon_led_trigger = { > diff --git a/drivers/leds/ledtrig-gpio.c b/drivers/leds/ledtrig-gpio.c > index ba215dc..f057c10 100644 > --- a/drivers/leds/ledtrig-gpio.c > +++ b/drivers/leds/ledtrig-gpio.c > @@ -54,12 +54,12 @@ static void gpio_trig_work(struct work_struct *work) > > if (tmp) { > if (gpio_data->desired_brightness) > - __led_set_brightness(gpio_data->led, > + led_set_brightness(gpio_data->led, > gpio_data->desired_brightness); > else > - __led_set_brightness(gpio_data->led, LED_FULL); > + led_set_brightness(gpio_data->led, LED_FULL); > } else { > - __led_set_brightness(gpio_data->led, LED_OFF); > + led_set_brightness(gpio_data->led, LED_OFF); > } > } > > diff --git a/drivers/leds/ledtrig-heartbeat.c b/drivers/leds/ledtrig-heartbeat.c > index 1edc746..a019fbb 100644 > --- a/drivers/leds/ledtrig-heartbeat.c > +++ b/drivers/leds/ledtrig-heartbeat.c > @@ -74,7 +74,7 @@ static void led_heartbeat_function(unsigned long data) > break; > } > > - __led_set_brightness(led_cdev, brightness); > + led_set_brightness(led_cdev, brightness); > mod_timer(&heartbeat_data->timer, jiffies + delay); > } > > diff --git a/drivers/leds/ledtrig-oneshot.c b/drivers/leds/ledtrig-oneshot.c > index 2c029aa..2b10b28 100644 > --- a/drivers/leds/ledtrig-oneshot.c > +++ b/drivers/leds/ledtrig-oneshot.c > @@ -63,9 +63,9 @@ static ssize_t led_invert_store(struct device *dev, > oneshot_data->invert = !!state; > > if (oneshot_data->invert) > - __led_set_brightness(led_cdev, LED_FULL); > + led_set_brightness(led_cdev, LED_FULL); > else > - __led_set_brightness(led_cdev, LED_OFF); > + led_set_brightness(led_cdev, LED_OFF); > > return size; > } > diff --git a/drivers/leds/ledtrig-transient.c b/drivers/leds/ledtrig-transient.c > index 398f104..83179f4 100644 > --- a/drivers/leds/ledtrig-transient.c > +++ b/drivers/leds/ledtrig-transient.c > @@ -41,7 +41,7 @@ static void transient_timer_function(unsigned long data) > struct transient_trig_data *transient_data = led_cdev->trigger_data; > > transient_data->activate = 0; > - __led_set_brightness(led_cdev, transient_data->restore_state); > + led_set_brightness(led_cdev, transient_data->restore_state); > } > > static ssize_t transient_activate_show(struct device *dev, > @@ -72,7 +72,7 @@ static ssize_t transient_activate_store(struct device *dev, > if (state == 0 && transient_data->activate == 1) { > del_timer(&transient_data->timer); > transient_data->activate = state; > - __led_set_brightness(led_cdev, transient_data->restore_state); > + led_set_brightness(led_cdev, transient_data->restore_state); > return size; > } > > @@ -80,7 +80,7 @@ static ssize_t transient_activate_store(struct device *dev, > if (state == 1 && transient_data->activate == 0 && > transient_data->duration != 0) { > transient_data->activate = state; > - __led_set_brightness(led_cdev, transient_data->state); > + led_set_brightness(led_cdev, transient_data->state); > transient_data->restore_state = > (transient_data->state == LED_FULL) ? LED_OFF : LED_FULL; > mod_timer(&transient_data->timer, > @@ -203,7 +203,7 @@ static void transient_trig_deactivate(struct led_classdev *led_cdev) > > if (led_cdev->activated) { > del_timer_sync(&transient_data->timer); > - __led_set_brightness(led_cdev, transient_data->restore_state); > + led_set_brightness(led_cdev, transient_data->restore_state); > device_remove_file(led_cdev->dev, &dev_attr_activate); > device_remove_file(led_cdev->dev, &dev_attr_duration); > device_remove_file(led_cdev->dev, &dev_attr_state); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 5676197..d7f5697 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -43,10 +43,15 @@ struct led_classdev { > #define LED_BLINK_ONESHOT_STOP (1 << 18) > #define LED_BLINK_INVERT (1 << 19) > > - /* Set LED brightness level */ > - /* Must not sleep, use a workqueue if needed */ > + /* > + * Set LED brightness level > + * use a workqueue internally, so driver writer doesn't need to > + * care about using duplicated workqueue in brightness_set() > + */ > void (*brightness_set)(struct led_classdev *led_cdev, > enum led_brightness brightness); > + struct work_struct set_brightness_work; > + > /* Get LED brightness level */ > enum led_brightness (*brightness_get)(struct led_classdev *led_cdev); > > @@ -70,9 +75,6 @@ struct led_classdev { > struct timer_list blink_timer; > int blink_brightness; > > - struct work_struct set_brightness_work; > - int delayed_set_value; > - > #ifdef CONFIG_LEDS_TRIGGERS > /* Protects the trigger data below */ > struct rw_semaphore trigger_lock; > -- > 1.7.11.4 > -- Fabio Baltieri ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] leds-gpio: remove workqueue in .brightness_set() 2012-09-14 7:53 [RFC] [PATCH 0/2] leds: use workqueue in led_set_brightness() API internally Bryan Wu 2012-09-14 7:53 ` [PATCH 1/2] " Bryan Wu @ 2012-09-14 7:53 ` Bryan Wu 1 sibling, 0 replies; 5+ messages in thread From: Bryan Wu @ 2012-09-14 7:53 UTC (permalink / raw) To: rpurdie, linux-kernel, akpm, linux-leds, broonie, fabio.baltieri, shuahkhan, raph, tpiepho LED core use workqueue internally now, need to use it in the class driver. Signed-off-by: Bryan Wu <bryan.wu@canonical.com> --- drivers/leds/leds-gpio.c | 43 ++++++------------------------------------- 1 file changed, 6 insertions(+), 37 deletions(-) diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index 087d1e6..c89dd15 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -18,36 +18,18 @@ #include <linux/of_platform.h> #include <linux/of_gpio.h> #include <linux/slab.h> -#include <linux/workqueue.h> #include <linux/module.h> #include <linux/pinctrl/consumer.h> struct gpio_led_data { struct led_classdev cdev; unsigned gpio; - struct work_struct work; - u8 new_level; - u8 can_sleep; u8 active_low; u8 blinking; int (*platform_gpio_blink_set)(unsigned gpio, int state, unsigned long *delay_on, unsigned long *delay_off); }; -static void gpio_led_work(struct work_struct *work) -{ - struct gpio_led_data *led_dat = - container_of(work, struct gpio_led_data, work); - - if (led_dat->blinking) { - led_dat->platform_gpio_blink_set(led_dat->gpio, - led_dat->new_level, - NULL, NULL); - led_dat->blinking = 0; - } else - gpio_set_value_cansleep(led_dat->gpio, led_dat->new_level); -} - static void gpio_led_set(struct led_classdev *led_cdev, enum led_brightness value) { @@ -63,21 +45,12 @@ static void gpio_led_set(struct led_classdev *led_cdev, if (led_dat->active_low) level = !level; - /* Setting GPIOs with I2C/etc requires a task context, and we don't - * seem to have a reliable way to know if we're already in one; so - * let's just assume the worst. - */ - if (led_dat->can_sleep) { - led_dat->new_level = level; - schedule_work(&led_dat->work); - } else { - if (led_dat->blinking) { - led_dat->platform_gpio_blink_set(led_dat->gpio, level, - NULL, NULL); - led_dat->blinking = 0; - } else - gpio_set_value(led_dat->gpio, level); - } + if (led_dat->blinking) { + led_dat->platform_gpio_blink_set(led_dat->gpio, level, + NULL, NULL); + led_dat->blinking = 0; + } else + gpio_set_value(led_dat->gpio, level); } static int gpio_blink_set(struct led_classdev *led_cdev, @@ -113,7 +86,6 @@ static int __devinit create_gpio_led(const struct gpio_led *template, led_dat->cdev.name = template->name; led_dat->cdev.default_trigger = template->default_trigger; led_dat->gpio = template->gpio; - led_dat->can_sleep = gpio_cansleep(template->gpio); led_dat->active_low = template->active_low; led_dat->blinking = 0; if (blink_set) { @@ -133,8 +105,6 @@ static int __devinit create_gpio_led(const struct gpio_led *template, if (ret < 0) goto err; - INIT_WORK(&led_dat->work, gpio_led_work); - ret = led_classdev_register(parent, &led_dat->cdev); if (ret < 0) goto err; @@ -150,7 +120,6 @@ static void delete_gpio_led(struct gpio_led_data *led) if (!gpio_is_valid(led->gpio)) return; led_classdev_unregister(&led->cdev); - cancel_work_sync(&led->work); gpio_free(led->gpio); } -- 1.7.11.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-15 15:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-14 7:53 [RFC] [PATCH 0/2] leds: use workqueue in led_set_brightness() API internally Bryan Wu 2012-09-14 7:53 ` [PATCH 1/2] " Bryan Wu 2012-09-14 22:25 ` Shuah Khan 2012-09-15 15:23 ` Fabio Baltieri 2012-09-14 7:53 ` [PATCH 2/2] leds-gpio: remove workqueue in .brightness_set() Bryan Wu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).