* [PATCH v3 0/3] Extend the LED panic trigger @ 2016-04-28 22:03 Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic Ezequiel Garcia ` (4 more replies) 0 siblings, 5 replies; 18+ messages in thread From: Ezequiel Garcia @ 2016-04-28 22:03 UTC (permalink / raw) To: linux-leds, linux-kernel, linux-arm-kernel, devicetree Cc: Richard Purdie, Jacek Anaszewski, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek, Ezequiel Garcia As per commit 916fe619951f ("leds: trigger: Introduce a kernel panic LED trigger"), the kernel now supports a new LED trigger to hook on the panic blink. However, the only way of using this is to dedicate a LED device, making it rather useless. To overcome this limitation, the present series introduces the capability to switch the LED trigger of certain LED devices upon a kernel panic (using the panic notifier). The decision of which LEDs should be switched to the panic trigger is left to each LED device driver. As an example, a devicetree boolean property is introduced and used in the leds-gpio driver. The big change in this v3 is that I've moved the panic trigger switching away from the core code and it's now part of ledtrig-panic.c. Pavel, Jacek: How does it look? Changes from v2: * Added Rob's "panic-indicator" devicetree property Acked-by. * Fix typo, as pointed out by Robin Murphy. * Documented "panic-indicator" in bindings/leds/leds-gpio.txt. * Moved the panic trigger switching from the trigger core code, to ledtrig-panic.c. Changes from v1: * Dropped the led_trigger_event_nosleep API, and instead just clear the blink_delay_{on, off} when the panic is notified. This results in less changes. * Changed the flag to LED_PANIC_INDICATOR, as requested by Jacek. * Changed the firmware property name to "panic-indicator", as requested by Jacek. Ezequiel Garcia (3): leds: triggers: Allow to switch the trigger to "panic" on a kernel panic devicetree: leds: Introduce "panic-indicator" optional property leds: gpio: Support the "panic-indicator" firmware property Documentation/devicetree/bindings/leds/common.txt | 3 ++ .../devicetree/bindings/leds/leds-gpio.txt | 2 + drivers/leds/led-triggers.c | 2 +- drivers/leds/leds-gpio.c | 4 ++ drivers/leds/leds.h | 1 + drivers/leds/trigger/Kconfig | 3 ++ drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++ include/linux/leds.h | 2 + 8 files changed, 63 insertions(+), 1 deletion(-) -- 2.7.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic 2016-04-28 22:03 [PATCH v3 0/3] Extend the LED panic trigger Ezequiel Garcia @ 2016-04-28 22:03 ` Ezequiel Garcia 2016-04-29 7:20 ` Jacek Anaszewski 2016-04-28 22:03 ` [PATCH v3 2/3] devicetree: leds: Introduce "panic-indicator" optional property Ezequiel Garcia ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2016-04-28 22:03 UTC (permalink / raw) To: linux-leds, linux-kernel, linux-arm-kernel, devicetree Cc: Richard Purdie, Jacek Anaszewski, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek, Ezequiel Garcia This commit adds a new led_cdev flag LED_PANIC_INDICATOR, which allows to mark a specific LED to be switched to the "panic" trigger, on a kernel panic. This is useful to allow the user to assign a regular trigger to a given LED, and still blink that LED on a kernel panic. Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> --- drivers/leds/led-triggers.c | 2 +- drivers/leds/leds.h | 1 + drivers/leds/trigger/Kconfig | 3 +++ drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++++++++++++++++ include/linux/leds.h | 1 + 5 files changed, 53 insertions(+), 1 deletion(-) diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index 2181581795d3..55fa65e1ae03 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -26,7 +26,7 @@ * Nests outside led_cdev->trigger_lock */ static DECLARE_RWSEM(triggers_list_lock); -static LIST_HEAD(trigger_list); +LIST_HEAD(trigger_list); /* Used by LED Class */ diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h index db3f20da7221..7d38e6b9a740 100644 --- a/drivers/leds/leds.h +++ b/drivers/leds/leds.h @@ -30,5 +30,6 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev, extern struct rw_semaphore leds_list_lock; extern struct list_head leds_list; +extern struct list_head trigger_list; #endif /* __LEDS_H_INCLUDED */ diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig index beac8c31c51b..4e4521c9072a 100644 --- a/drivers/leds/trigger/Kconfig +++ b/drivers/leds/trigger/Kconfig @@ -121,6 +121,9 @@ config LEDS_TRIGGER_PANIC depends on LEDS_TRIGGERS help This allows LEDs to be configured to blink on a kernel panic. + Enabling this option will allow to mark certain LEDs as 'panic-indicators', + allowing to blink them on a kernel panic, even if they are set to + a different trigger. If unsure, say Y. endif # LEDS_TRIGGERS diff --git a/drivers/leds/trigger/ledtrig-panic.c b/drivers/leds/trigger/ledtrig-panic.c index 627b350c5ec3..3e447bd2064a 100644 --- a/drivers/leds/trigger/ledtrig-panic.c +++ b/drivers/leds/trigger/ledtrig-panic.c @@ -11,10 +11,54 @@ #include <linux/kernel.h> #include <linux/init.h> +#include <linux/notifier.h> #include <linux/leds.h> +#include "../leds.h" static struct led_trigger *trigger; +/* + * This is a called in a special context by the atomic panic + * notifier. This means the trigger can be changed without + * worrying about locking. + */ +static void led_trigger_set_panic(struct led_classdev *led_cdev) +{ + struct led_trigger *trig; + + list_for_each_entry(trig, &trigger_list, next_trig) { + if (strcmp("panic", trig->name)) + continue; + if (led_cdev->trigger) + list_del(&led_cdev->trig_list); + list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); + + /* Avoid the delayed blink path */ + led_cdev->blink_delay_on = 0; + led_cdev->blink_delay_off = 0; + + led_cdev->trigger = trig; + if (trig->activate) + trig->activate(led_cdev); + break; + } +} + +static int led_trigger_panic_notifier(struct notifier_block *nb, + unsigned long code, void *unused) +{ + struct led_classdev *led_cdev; + + list_for_each_entry(led_cdev, &leds_list, node) + if (led_cdev->flags & LED_PANIC_INDICATOR) + led_trigger_set_panic(led_cdev); + return NOTIFY_DONE; +} + +static struct notifier_block led_trigger_panic_nb = { + .notifier_call = led_trigger_panic_notifier, +}; + static long led_panic_blink(int state) { led_trigger_event(trigger, state ? LED_FULL : LED_OFF); @@ -23,6 +67,9 @@ static long led_panic_blink(int state) static int __init ledtrig_panic_init(void) { + atomic_notifier_chain_register(&panic_notifier_list, + &led_trigger_panic_nb); + led_trigger_register_simple("panic", &trigger); panic_blink = led_panic_blink; return 0; diff --git a/include/linux/leds.h b/include/linux/leds.h index 19eb10278bea..7e9fb00e15e8 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -50,6 +50,7 @@ struct led_classdev { #define LED_SYSFS_DISABLE (1 << 22) #define LED_DEV_CAP_FLASH (1 << 23) #define LED_HW_PLUGGABLE (1 << 24) +#define LED_PANIC_INDICATOR (1 << 25) /* Set LED brightness level * Must not sleep. Use brightness_set_blocking for drivers -- 2.7.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic 2016-04-28 22:03 ` [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic Ezequiel Garcia @ 2016-04-29 7:20 ` Jacek Anaszewski 2016-05-06 9:03 ` Jacek Anaszewski 0 siblings, 1 reply; 18+ messages in thread From: Jacek Anaszewski @ 2016-04-29 7:20 UTC (permalink / raw) To: Ezequiel Garcia Cc: linux-leds, linux-kernel, linux-arm-kernel, devicetree, Richard Purdie, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek Hi Ezequiel, Thanks for the update. It's indeed reasonable to have all the switching infrastructure in ledtrig-panic.c. I've noticed two minor issues below. On 04/29/2016 12:03 AM, Ezequiel Garcia wrote: > This commit adds a new led_cdev flag LED_PANIC_INDICATOR, which > allows to mark a specific LED to be switched to the "panic" > trigger, on a kernel panic. > > This is useful to allow the user to assign a regular trigger > to a given LED, and still blink that LED on a kernel panic. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > drivers/leds/led-triggers.c | 2 +- > drivers/leds/leds.h | 1 + > drivers/leds/trigger/Kconfig | 3 +++ > drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++++++++++++++++ > include/linux/leds.h | 1 + > 5 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c > index 2181581795d3..55fa65e1ae03 100644 > --- a/drivers/leds/led-triggers.c > +++ b/drivers/leds/led-triggers.c > @@ -26,7 +26,7 @@ > * Nests outside led_cdev->trigger_lock > */ > static DECLARE_RWSEM(triggers_list_lock); > -static LIST_HEAD(trigger_list); > +LIST_HEAD(trigger_list); > > /* Used by LED Class */ > > diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h > index db3f20da7221..7d38e6b9a740 100644 > --- a/drivers/leds/leds.h > +++ b/drivers/leds/leds.h > @@ -30,5 +30,6 @@ void led_set_brightness_nosleep(struct led_classdev *led_cdev, > > extern struct rw_semaphore leds_list_lock; > extern struct list_head leds_list; > +extern struct list_head trigger_list; > > #endif /* __LEDS_H_INCLUDED */ > diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig > index beac8c31c51b..4e4521c9072a 100644 > --- a/drivers/leds/trigger/Kconfig > +++ b/drivers/leds/trigger/Kconfig > @@ -121,6 +121,9 @@ config LEDS_TRIGGER_PANIC > depends on LEDS_TRIGGERS > help > This allows LEDs to be configured to blink on a kernel panic. > + Enabling this option will allow to mark certain LEDs as 'panic-indicators', s/"panic-indicators"/panic indicators/ I understand that you referred here to the DT property name, but this is not obvious at first glance, and it is an implementation detail. > + allowing to blink them on a kernel panic, even if they are set to > + a different trigger. > If unsure, say Y. > > endif # LEDS_TRIGGERS > diff --git a/drivers/leds/trigger/ b/drivers/leds/trigger/ledtrig-panic.c > index 627b350c5ec3..3e447bd2064a 100644 > --- a/drivers/leds/trigger/ledtrig-panic.c > +++ b/drivers/leds/trigger/ledtrig-panic.c > @@ -11,10 +11,54 @@ > > #include <linux/kernel.h> > #include <linux/init.h> > +#include <linux/notifier.h> > #include <linux/leds.h> > +#include "../leds.h" > > static struct led_trigger *trigger; > > +/* > + * This is a called in a special context by the atomic panic s/is a/is/ > + * notifier. This means the trigger can be changed without > + * worrying about locking. > + */ > +static void led_trigger_set_panic(struct led_classdev *led_cdev) > +{ > + struct led_trigger *trig; > + > + list_for_each_entry(trig, &trigger_list, next_trig) { > + if (strcmp("panic", trig->name)) > + continue; > + if (led_cdev->trigger) > + list_del(&led_cdev->trig_list); > + list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); > + > + /* Avoid the delayed blink path */ > + led_cdev->blink_delay_on = 0; > + led_cdev->blink_delay_off = 0; > + > + led_cdev->trigger = trig; > + if (trig->activate) > + trig->activate(led_cdev); > + break; > + } > +} > + > +static int led_trigger_panic_notifier(struct notifier_block *nb, > + unsigned long code, void *unused) > +{ > + struct led_classdev *led_cdev; > + > + list_for_each_entry(led_cdev, &leds_list, node) > + if (led_cdev->flags & LED_PANIC_INDICATOR) > + led_trigger_set_panic(led_cdev); > + return NOTIFY_DONE; > +} > + > +static struct notifier_block led_trigger_panic_nb = { > + .notifier_call = led_trigger_panic_notifier, > +}; > + > static long led_panic_blink(int state) > { > led_trigger_event(trigger, state ? LED_FULL : LED_OFF); > @@ -23,6 +67,9 @@ static long led_panic_blink(int state) > > static int __init ledtrig_panic_init(void) > { > + atomic_notifier_chain_register(&panic_notifier_list, > + &led_trigger_panic_nb); > + > led_trigger_register_simple("panic", &trigger); > panic_blink = led_panic_blink; > return 0; > diff --git a/include/linux/leds.h b/include/linux/leds.h > index 19eb10278bea..7e9fb00e15e8 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -50,6 +50,7 @@ struct led_classdev { > #define LED_SYSFS_DISABLE (1 << 22) > #define LED_DEV_CAP_FLASH (1 << 23) > #define LED_HW_PLUGGABLE (1 << 24) > +#define LED_PANIC_INDICATOR (1 << 25) > > /* Set LED brightness level > * Must not sleep. Use brightness_set_blocking for drivers > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic 2016-04-29 7:20 ` Jacek Anaszewski @ 2016-05-06 9:03 ` Jacek Anaszewski 2016-05-06 13:05 ` Ezequiel Garcia 0 siblings, 1 reply; 18+ messages in thread From: Jacek Anaszewski @ 2016-05-06 9:03 UTC (permalink / raw) To: Ezequiel Garcia Cc: linux-leds, linux-kernel, linux-arm-kernel, devicetree, Richard Purdie, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek On 04/29/2016 09:20 AM, Jacek Anaszewski wrote: > Hi Ezequiel, > > Thanks for the update. It's indeed reasonable to have all the > switching infrastructure in ledtrig-panic.c. > > I've noticed two minor issues below. Since the merge window is imminent, I've addressed those issues by myself and applied the patch set to the for-next branch of linux-leds.git. Thanks, Jacek Anaszewski > On 04/29/2016 12:03 AM, Ezequiel Garcia wrote: >> This commit adds a new led_cdev flag LED_PANIC_INDICATOR, which >> allows to mark a specific LED to be switched to the "panic" >> trigger, on a kernel panic. >> >> This is useful to allow the user to assign a regular trigger >> to a given LED, and still blink that LED on a kernel panic. >> >> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> >> --- >> drivers/leds/led-triggers.c | 2 +- >> drivers/leds/leds.h | 1 + >> drivers/leds/trigger/Kconfig | 3 +++ >> drivers/leds/trigger/ledtrig-panic.c | 47 >> ++++++++++++++++++++++++++++++++++++ >> include/linux/leds.h | 1 + >> 5 files changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c >> index 2181581795d3..55fa65e1ae03 100644 >> --- a/drivers/leds/led-triggers.c >> +++ b/drivers/leds/led-triggers.c >> @@ -26,7 +26,7 @@ >> * Nests outside led_cdev->trigger_lock >> */ >> static DECLARE_RWSEM(triggers_list_lock); >> -static LIST_HEAD(trigger_list); >> +LIST_HEAD(trigger_list); >> >> /* Used by LED Class */ >> >> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h >> index db3f20da7221..7d38e6b9a740 100644 >> --- a/drivers/leds/leds.h >> +++ b/drivers/leds/leds.h >> @@ -30,5 +30,6 @@ void led_set_brightness_nosleep(struct led_classdev >> *led_cdev, >> >> extern struct rw_semaphore leds_list_lock; >> extern struct list_head leds_list; >> +extern struct list_head trigger_list; >> >> #endif /* __LEDS_H_INCLUDED */ >> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig >> index beac8c31c51b..4e4521c9072a 100644 >> --- a/drivers/leds/trigger/Kconfig >> +++ b/drivers/leds/trigger/Kconfig >> @@ -121,6 +121,9 @@ config LEDS_TRIGGER_PANIC >> depends on LEDS_TRIGGERS >> help >> This allows LEDs to be configured to blink on a kernel panic. >> + Enabling this option will allow to mark certain LEDs as >> 'panic-indicators', > > s/"panic-indicators"/panic indicators/ > > I understand that you referred here to the DT property name, but this > is not obvious at first glance, and it is an implementation detail. > >> + allowing to blink them on a kernel panic, even if they are set to >> + a different trigger. >> If unsure, say Y. >> >> endif # LEDS_TRIGGERS >> diff --git a/drivers/leds/trigger/ b/drivers/leds/trigger/ledtrig-panic.c >> index 627b350c5ec3..3e447bd2064a 100644 >> --- a/drivers/leds/trigger/ledtrig-panic.c >> +++ b/drivers/leds/trigger/ledtrig-panic.c >> @@ -11,10 +11,54 @@ >> >> #include <linux/kernel.h> >> #include <linux/init.h> >> +#include <linux/notifier.h> >> #include <linux/leds.h> >> +#include "../leds.h" >> >> static struct led_trigger *trigger; >> >> +/* >> + * This is a called in a special context by the atomic panic > > s/is a/is/ > >> + * notifier. This means the trigger can be changed without >> + * worrying about locking. >> + */ >> +static void led_trigger_set_panic(struct led_classdev *led_cdev) >> +{ >> + struct led_trigger *trig; >> + >> + list_for_each_entry(trig, &trigger_list, next_trig) { >> + if (strcmp("panic", trig->name)) >> + continue; >> + if (led_cdev->trigger) >> + list_del(&led_cdev->trig_list); >> + list_add_tail(&led_cdev->trig_list, &trig->led_cdevs); >> + >> + /* Avoid the delayed blink path */ >> + led_cdev->blink_delay_on = 0; >> + led_cdev->blink_delay_off = 0; >> + >> + led_cdev->trigger = trig; >> + if (trig->activate) >> + trig->activate(led_cdev); >> + break; >> + } >> +} >> + >> +static int led_trigger_panic_notifier(struct notifier_block *nb, >> + unsigned long code, void *unused) >> +{ >> + struct led_classdev *led_cdev; >> + >> + list_for_each_entry(led_cdev, &leds_list, node) >> + if (led_cdev->flags & LED_PANIC_INDICATOR) >> + led_trigger_set_panic(led_cdev); >> + return NOTIFY_DONE; >> +} >> + >> +static struct notifier_block led_trigger_panic_nb = { >> + .notifier_call = led_trigger_panic_notifier, >> +}; >> + >> static long led_panic_blink(int state) >> { >> led_trigger_event(trigger, state ? LED_FULL : LED_OFF); >> @@ -23,6 +67,9 @@ static long led_panic_blink(int state) >> >> static int __init ledtrig_panic_init(void) >> { >> + atomic_notifier_chain_register(&panic_notifier_list, >> + &led_trigger_panic_nb); >> + >> led_trigger_register_simple("panic", &trigger); >> panic_blink = led_panic_blink; >> return 0; >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index 19eb10278bea..7e9fb00e15e8 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -50,6 +50,7 @@ struct led_classdev { >> #define LED_SYSFS_DISABLE (1 << 22) >> #define LED_DEV_CAP_FLASH (1 << 23) >> #define LED_HW_PLUGGABLE (1 << 24) >> +#define LED_PANIC_INDICATOR (1 << 25) >> >> /* Set LED brightness level >> * Must not sleep. Use brightness_set_blocking for drivers >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic 2016-05-06 9:03 ` Jacek Anaszewski @ 2016-05-06 13:05 ` Ezequiel Garcia [not found] ` <572CE1B0.8040001@daqri.com> 0 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2016-05-06 13:05 UTC (permalink / raw) To: Jacek Anaszewski Cc: Linux LED Subsystem, linux-kernel, linux-arm-kernel, devicetree, Richard Purdie, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek On 6 May 2016 at 06:03, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 04/29/2016 09:20 AM, Jacek Anaszewski wrote: >> >> Hi Ezequiel, >> >> Thanks for the update. It's indeed reasonable to have all the >> switching infrastructure in ledtrig-panic.c. >> >> I've noticed two minor issues below. > > > Since the merge window is imminent, I've addressed those issues by > myself and applied the patch set to the for-next branch > of linux-leds.git. > Thanks a lot for taking care of this, Jacek! -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <572CE1B0.8040001@daqri.com>]
[parent not found: <572CE715.6060504@gmail.com>]
[parent not found: <57309039.3060305@daqri.com>]
[parent not found: <5730A293.9050209@samsung.com>]
[parent not found: <5731ABB6.10607@daqri.com>]
[parent not found: <5731E194.1010004@samsung.com>]
[parent not found: <57321299.8090603@daqri.com>]
* Re: Brightness control irrespective of blink state. [not found] ` <57321299.8090603@daqri.com> @ 2016-05-11 9:41 ` Jacek Anaszewski 2016-05-11 13:42 ` Tony Makkiel 0 siblings, 1 reply; 18+ messages in thread From: Jacek Anaszewski @ 2016-05-11 9:41 UTC (permalink / raw) To: Tony Makkiel; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 05/10/2016 06:55 PM, Tony Makkiel wrote: > > > On 10/05/16 14:26, Jacek Anaszewski wrote: >> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>> >>> >>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>> Hi Tony, >>>> >>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>> Hi Jacek, >>>>> Thank you for getting back. I updated my kernel to 4.5 and have >>>>> the >>>>> updated "led_set_brightness" now. >>>>> >>>>> It sets >>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>> led_cdev->blink_brightness = brightness; >>>>> >>>>> The new implementation requires hardware specific drivers to poll >>>>> for flag change. Shouldn't the led-core driver be calling the hardware >>>>> specific brightness_set (led_set_brightness_nosleep) irrespective of >>>>> the >>>>> blink settings? >>>>> >>>>> Unfortunately, it place additional requirement on drivers, to >>>>> implement >>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>> brightness calls dependent on blink settings? >>>> >>>> If your question is still: >>>> >>>> "Is there a reason for rejecting brightness change requests when >>>> either of the blink_delays are set?" >>>> >>>> then the answer is: yes, brightness setting is deferred until >>>> the next timer tick to avoid avoid problems in case we are called >>>> from hard irq context. It should work fine for software blinking. >>> >>> >>> Sorry, was focused debugging 'hardware accelerated blink' on the driver >>> I am working on, I missed the software blinking implementation. >>> >>>> >>>> Nonetheless, your question, made it obvious that we have a problem >>>> here in case of hardware accelerated blinking, i.e. drivers that >>>> implement blink_set op. Is this your use case? >>>> >>> >>> Yes, the brightness requests from user space >>> (/sys/class/leds/*/brightness) does not get passed to hardware specific >>> driver via the blink_set implemented, unless led_cdev->flags is polled. >>> >>>> Anyway, I've noticed a discrepancy between the LED core code and >>>> both Documentation/leds/leds-class.txt and comment over blink_set() op >>>> in include/linux/leds.h, which say that blinking is deactivated >>>> upon setting the brightness again. Many drivers apply this rule. >>>> >>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>> and your question will be groundless, as changing the blink >>>> brightness should be impossible by design. >>>> >>> In my opinion, disabling blink, when brightness change requested doesn't >>> sound like the right thing to do. There could be cases in which the >>> brightness of the blinking LED needs to be changed. >> >> It could be accomplished with following sequence: >> >> $ echo LED_FULL > brightness //set brightness >> $ echo "timer" > trigger //enable blinking with brightness=LED_FULL >> $ echo 1 > brightness //stop blinking and set brightness to 1 >> $ echo "timer" > trigger //enable blinking with brightness=1 >> >> The only drawback here would be interfered blinking rhythm while >> resetting blink brightness. Most drivers that implement blink_set >> op observe what documentation says and disable blinking when >> new brightness is set. Unfortunately, led_set_brightness() after >> modifications doesn't take into account drivers that implement >> blink_set op. It needs to be fixed, i.e. made compatible with >> the docs. >> >>> Maybe we can let the >>> hardware driver deal with the blink request if it has implemented >>> brightness_set? The change below seem to work. >>> >>> >>> Subject: [PATCH] led-core: Use hardware blink when available >>> >>> At present hardware implemented brightness is not called >>> if blink delays are set. >>> >>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>> --- >>> drivers/leds/led-core.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>> index 19e1e60d..02dd0f6 100644 >>> --- a/drivers/leds/led-core.c >>> +++ b/drivers/leds/led-core.c >>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>> *led_cdev, >>> /* >>> * In case blinking is on delay brightness setting >>> * until the next timer tick. >>> + * Or if brightness_set is defined, use the associated >>> implementation. >>> */ >>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>> + if ((!led_cdev->brightness_set) && >> >> s/brightness_set/blink_set/ AFAICT >> >> It wouldn't cover all cases as the fact that a driver implements >> blink_set doesn't necessarily mean that hardware blinking is used >> for current blinking parameters. There are drivers that resort to >> using software fallback in case the LED controller device doesn't >> support requested blink intervals. >> >> I'm planning on addition of a LED_BLINKING_HW flag, that would >> be set after successful execution of blink_set op. It would allow to >> distinguish between hardware and software blinking modes reliably. >> > > Did you mean something like > > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 19e1e60d..4a8b46d 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev *led_cdev, > * until the next timer tick. > */ > if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { > + if (led_cdev->brightness_set) > + led_set_brightness_nosleep(led_cdev, brightness); brightness_set is always initialized, probably you meant blink_set. > + > + if (led_cdev->flags & LED_BLINKING_HW) { > + led_cdev->flags &= ~LED_BLINKING_HW; > + return; > + } The dependencies are quite versatile if we wanted to properly implement what documentation claims. Setting brightness to any value while blinking is on should stop blinking. It was so before the commit: 76931edd5 ('leds: fix brightness changing when software blinking is active') The problem was that timer trigger remained active after stopping blinking, which led us to changing the semantics on new brightness set, rather than solving the problem with unregistered trigger. This was also against documentation claims, which was overlooked. I tried yesterday to deactivate trigger on brightness set executed during blinking, but there are circular dependencies, since led_set_brightness(led_cdev, LED_OFF) is called on deactivation. It is also called from led_trigger_set in the trigger removal path. Generally it seems non-trivial to enforce current documentation claims in case of software blinking. The easiest thing to do now would be changing the semantics, so that only setting brightness to LED_OFF would disable the trigger, which in fact is true since few releases. The problem is that many drivers that implement hardware blinking resets it on any brightness change. We would have to left them intact, but apply a new semantics in the LED core, that would allow for new drivers to just update hardware blinking brightness upon updating the brightness. If we followed this path then the LED_BLINKING_HW flag would have to be set in led_blink_setup() after blink_set op returns 0. Thanks to that, we could distinguish in led_set_brightness whether hardware or software blinking is enabled. For !LED_BLINKING_HW case we would proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag and defer brightness setting until next timer tick. For the opposite case we wouldn't do anything and let the led_set_brightness_nosleep() to call the appropriate brightness_set/brightness_set_blocking op. Old drivers would proceed as currently, by disabling blinking on brightness change, and new ones could apply new semantics by changing brightness but leaving hardware blinking active. I wonder if it would be safe to rely on timer_pending() instead of introducing LED_BLINKING_HW flag... > /* > * If we need to disable soft blinking delegate this to > the > * work queue task to avoid problems in case we are called > diff --git a/include/linux/leds.h b/include/linux/leds.h > index bc1476f..f5fa566 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -48,6 +48,7 @@ struct led_classdev { > #define LED_BLINK_DISABLE (1 << 21) > #define LED_SYSFS_DISABLE (1 << 22) > #define LED_DEV_CAP_FLASH (1 << 23) > +#define LED_BLINKING_HW (1 << 24) > > /* Set LED brightness level */ > /* Must not sleep, use a workqueue if needed */ > >> >>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>> /* >>> * If we need to disable soft blinking delegate this to the >>> * work queue task to avoid problems in case we are called >> >> > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-11 9:41 ` Brightness control irrespective of blink state Jacek Anaszewski @ 2016-05-11 13:42 ` Tony Makkiel 2016-05-12 10:26 ` Jacek Anaszewski 0 siblings, 1 reply; 18+ messages in thread From: Tony Makkiel @ 2016-05-11 13:42 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 11/05/16 10:41, Jacek Anaszewski wrote: > On 05/10/2016 06:55 PM, Tony Makkiel wrote: >> >> >> On 10/05/16 14:26, Jacek Anaszewski wrote: >>> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>>> >>>> >>>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>>> Hi Tony, >>>>> >>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>>> Hi Jacek, >>>>>> Thank you for getting back. I updated my kernel to 4.5 and have >>>>>> the >>>>>> updated "led_set_brightness" now. >>>>>> >>>>>> It sets >>>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>>> led_cdev->blink_brightness = brightness; >>>>>> >>>>>> The new implementation requires hardware specific drivers to >>>>>> poll >>>>>> for flag change. Shouldn't the led-core driver be calling the >>>>>> hardware >>>>>> specific brightness_set (led_set_brightness_nosleep) irrespective of >>>>>> the >>>>>> blink settings? >>>>>> >>>>>> Unfortunately, it place additional requirement on drivers, to >>>>>> implement >>>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>>> brightness calls dependent on blink settings? >>>>> >>>>> If your question is still: >>>>> >>>>> "Is there a reason for rejecting brightness change requests when >>>>> either of the blink_delays are set?" >>>>> >>>>> then the answer is: yes, brightness setting is deferred until >>>>> the next timer tick to avoid avoid problems in case we are called >>>>> from hard irq context. It should work fine for software blinking. >>>> >>>> >>>> Sorry, was focused debugging 'hardware accelerated blink' on the driver >>>> I am working on, I missed the software blinking implementation. >>>> >>>>> >>>>> Nonetheless, your question, made it obvious that we have a problem >>>>> here in case of hardware accelerated blinking, i.e. drivers that >>>>> implement blink_set op. Is this your use case? >>>>> >>>> >>>> Yes, the brightness requests from user space >>>> (/sys/class/leds/*/brightness) does not get passed to hardware specific >>>> driver via the blink_set implemented, unless led_cdev->flags is polled. >>>> >>>>> Anyway, I've noticed a discrepancy between the LED core code and >>>>> both Documentation/leds/leds-class.txt and comment over blink_set() op >>>>> in include/linux/leds.h, which say that blinking is deactivated >>>>> upon setting the brightness again. Many drivers apply this rule. >>>>> >>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>>> and your question will be groundless, as changing the blink >>>>> brightness should be impossible by design. >>>>> >>>> In my opinion, disabling blink, when brightness change requested >>>> doesn't >>>> sound like the right thing to do. There could be cases in which the >>>> brightness of the blinking LED needs to be changed. >>> >>> It could be accomplished with following sequence: >>> >>> $ echo LED_FULL > brightness //set brightness >>> $ echo "timer" > trigger //enable blinking with brightness=LED_FULL >>> $ echo 1 > brightness //stop blinking and set brightness to 1 >>> $ echo "timer" > trigger //enable blinking with brightness=1 >>> >>> The only drawback here would be interfered blinking rhythm while >>> resetting blink brightness. Most drivers that implement blink_set >>> op observe what documentation says and disable blinking when >>> new brightness is set. Unfortunately, led_set_brightness() after >>> modifications doesn't take into account drivers that implement >>> blink_set op. It needs to be fixed, i.e. made compatible with >>> the docs. >>> >>>> Maybe we can let the >>>> hardware driver deal with the blink request if it has implemented >>>> brightness_set? The change below seem to work. >>>> >>>> >>>> Subject: [PATCH] led-core: Use hardware blink when available >>>> >>>> At present hardware implemented brightness is not called >>>> if blink delays are set. >>>> >>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>>> --- >>>> drivers/leds/led-core.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>> index 19e1e60d..02dd0f6 100644 >>>> --- a/drivers/leds/led-core.c >>>> +++ b/drivers/leds/led-core.c >>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>>> *led_cdev, >>>> /* >>>> * In case blinking is on delay brightness setting >>>> * until the next timer tick. >>>> + * Or if brightness_set is defined, use the associated >>>> implementation. >>>> */ >>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>> + if ((!led_cdev->brightness_set) && >>> >>> s/brightness_set/blink_set/ AFAICT >>> >>> It wouldn't cover all cases as the fact that a driver implements >>> blink_set doesn't necessarily mean that hardware blinking is used >>> for current blinking parameters. There are drivers that resort to >>> using software fallback in case the LED controller device doesn't >>> support requested blink intervals. >>> >>> I'm planning on addition of a LED_BLINKING_HW flag, that would >>> be set after successful execution of blink_set op. It would allow to >>> distinguish between hardware and software blinking modes reliably. >>> >> >> Did you mean something like >> >> >> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >> index 19e1e60d..4a8b46d 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev >> *led_cdev, >> * until the next timer tick. >> */ >> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >> + if (led_cdev->brightness_set) >> + led_set_brightness_nosleep(led_cdev, brightness); > > brightness_set is always initialized, probably you meant blink_set. Your solution from your last comment below sounds better (so probably can skip reading this section). But no, It was not a mistake. Actually, copied from 'led_set_brightness_nopm' in case to protect from any drivers which doesn't define it. The change should follow existing architecture, and was hoping to work for both existing and new drivers. Existing Chip drivers: Note, the added brightness_set is called only when the blink is active. The flag LED_BLINKING_HW won't be set, either because driver is not updated to include the flag, or driver wants software fallback to deal with it. The problems I can think of is - the software flow will also call brightness_set later when the task is serviced. But that is with the same values. So shouldn't make difference to the user. New Drivers with brightness support while blinking:- They set brightness and the flag. They wont need the software fallback. If for any reason brightness couldn't be set, flag is not set and normal procedure applies. Again problem I see here is - Additional responsibility on chip drivers to set the flag, if successfully managed to set brightness while blink is active. This shouldn't be a problem on new drivers, as they might just set the flag every time brightness change is requested, irrespective of blink settings. If they don't set the flag, it falls back to the software flow as in existing architecture. > >> + >> + if (led_cdev->flags & LED_BLINKING_HW) { >> + led_cdev->flags &= ~LED_BLINKING_HW; >> + return; >> + } > > The dependencies are quite versatile if we wanted to properly implement > what documentation claims. Setting brightness to any value while > blinking is on should stop blinking. It was so before the commit: > > 76931edd5 ('leds: fix brightness changing when software blinking is > active') > > The problem was that timer trigger remained active after stopping > blinking, which led us to changing the semantics on new brightness > set, rather than solving the problem with unregistered trigger. > This was also against documentation claims, which was overlooked. > > I tried yesterday to deactivate trigger on brightness set > executed during blinking, but there are circular dependencies, > since led_set_brightness(led_cdev, LED_OFF) is called on deactivation. > It is also called from led_trigger_set in the trigger removal path. > Generally it seems non-trivial to enforce current documentation claims > in case of software blinking. > > The easiest thing to do now would be changing the semantics, so that > only setting brightness to LED_OFF would disable the trigger, which > in fact is true since few releases. The problem is that many drivers > that implement hardware blinking resets it on any brightness change. > We would have to left them intact, but apply a new semantics in the > LED core, that would allow for new drivers to just update hardware > blinking brightness upon updating the brightness. > > If we followed this path then the LED_BLINKING_HW flag would have to > be set in led_blink_setup() after blink_set op returns 0. Thanks to > that, we could distinguish in led_set_brightness whether hardware > or software blinking is enabled. For !LED_BLINKING_HW case we would > proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag > and defer brightness setting until next timer tick. For the opposite > case we wouldn't do anything and let the led_set_brightness_nosleep() > to call the appropriate brightness_set/brightness_set_blocking op. > Old drivers would proceed as currently, by disabling blinking > on brightness change, and new ones could apply new semantics by > changing brightness but leaving hardware blinking active. > This sounds better as we do not have to rely on drivers to set the flag and does not have the problems mentioned above. I tried the following and works for my set up :) . diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 19e1e60d..3698b67 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev *led_cdev, { if (!(led_cdev->flags & LED_BLINK_ONESHOT) && led_cdev->blink_set && - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ + led_cdev->flags |= LED_BLINKING_HW; return; + } /* blink with 1 Hz as default if nothing specified */ if (!*delay_on && !*delay_off) @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev *led_cdev, * In case blinking is on delay brightness setting * until the next timer tick. */ - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { + if (!(led_cdev->flags & LED_BLINKING_HW) && + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { /* * If we need to disable soft blinking delegate this to the * work queue task to avoid problems in case we are called diff --git a/include/linux/leds.h b/include/linux/leds.h index bc1476f..f5fa566 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -48,6 +48,7 @@ struct led_classdev { #define LED_BLINK_DISABLE (1 << 21) #define LED_SYSFS_DISABLE (1 << 22) #define LED_DEV_CAP_FLASH (1 << 23) +#define LED_BLINKING_HW (1 << 24) /* Set LED brightness level */ /* Must not sleep, use a workqueue if needed */ > I wonder if it would be safe to rely on timer_pending() instead of > introducing LED_BLINKING_HW flag... > How would that work? I am assuming this has something to do with software blink? Does it take hardware blink to account? >> /* >> * If we need to disable soft blinking delegate this to >> the >> * work queue task to avoid problems in case we are >> called >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index bc1476f..f5fa566 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -48,6 +48,7 @@ struct led_classdev { >> #define LED_BLINK_DISABLE (1 << 21) >> #define LED_SYSFS_DISABLE (1 << 22) >> #define LED_DEV_CAP_FLASH (1 << 23) >> +#define LED_BLINKING_HW (1 << 24) >> >> /* Set LED brightness level */ >> /* Must not sleep, use a workqueue if needed */ >> >>> >>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>> /* >>>> * If we need to disable soft blinking delegate this to the >>>> * work queue task to avoid problems in case we are called >>> >>> >> >> > > ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-11 13:42 ` Tony Makkiel @ 2016-05-12 10:26 ` Jacek Anaszewski 2016-05-13 14:20 ` Tony Makkiel 0 siblings, 1 reply; 18+ messages in thread From: Jacek Anaszewski @ 2016-05-12 10:26 UTC (permalink / raw) To: Tony Makkiel; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 05/11/2016 03:42 PM, Tony Makkiel wrote: > > > On 11/05/16 10:41, Jacek Anaszewski wrote: >> On 05/10/2016 06:55 PM, Tony Makkiel wrote: >>> >>> >>> On 10/05/16 14:26, Jacek Anaszewski wrote: >>>> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>>>> >>>>> >>>>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>>>> Hi Tony, >>>>>> >>>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>>>> Hi Jacek, >>>>>>> Thank you for getting back. I updated my kernel to 4.5 and have >>>>>>> the >>>>>>> updated "led_set_brightness" now. >>>>>>> >>>>>>> It sets >>>>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>>>> led_cdev->blink_brightness = brightness; >>>>>>> >>>>>>> The new implementation requires hardware specific drivers to >>>>>>> poll >>>>>>> for flag change. Shouldn't the led-core driver be calling the >>>>>>> hardware >>>>>>> specific brightness_set (led_set_brightness_nosleep) irrespective of >>>>>>> the >>>>>>> blink settings? >>>>>>> >>>>>>> Unfortunately, it place additional requirement on drivers, to >>>>>>> implement >>>>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>>>> brightness calls dependent on blink settings? >>>>>> >>>>>> If your question is still: >>>>>> >>>>>> "Is there a reason for rejecting brightness change requests when >>>>>> either of the blink_delays are set?" >>>>>> >>>>>> then the answer is: yes, brightness setting is deferred until >>>>>> the next timer tick to avoid avoid problems in case we are called >>>>>> from hard irq context. It should work fine for software blinking. >>>>> >>>>> >>>>> Sorry, was focused debugging 'hardware accelerated blink' on the >>>>> driver >>>>> I am working on, I missed the software blinking implementation. >>>>> >>>>>> >>>>>> Nonetheless, your question, made it obvious that we have a problem >>>>>> here in case of hardware accelerated blinking, i.e. drivers that >>>>>> implement blink_set op. Is this your use case? >>>>>> >>>>> >>>>> Yes, the brightness requests from user space >>>>> (/sys/class/leds/*/brightness) does not get passed to hardware >>>>> specific >>>>> driver via the blink_set implemented, unless led_cdev->flags is >>>>> polled. >>>>> >>>>>> Anyway, I've noticed a discrepancy between the LED core code and >>>>>> both Documentation/leds/leds-class.txt and comment over >>>>>> blink_set() op >>>>>> in include/linux/leds.h, which say that blinking is deactivated >>>>>> upon setting the brightness again. Many drivers apply this rule. >>>>>> >>>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>>>> and your question will be groundless, as changing the blink >>>>>> brightness should be impossible by design. >>>>>> >>>>> In my opinion, disabling blink, when brightness change requested >>>>> doesn't >>>>> sound like the right thing to do. There could be cases in which the >>>>> brightness of the blinking LED needs to be changed. >>>> >>>> It could be accomplished with following sequence: >>>> >>>> $ echo LED_FULL > brightness //set brightness >>>> $ echo "timer" > trigger //enable blinking with brightness=LED_FULL >>>> $ echo 1 > brightness //stop blinking and set brightness to 1 >>>> $ echo "timer" > trigger //enable blinking with brightness=1 >>>> >>>> The only drawback here would be interfered blinking rhythm while >>>> resetting blink brightness. Most drivers that implement blink_set >>>> op observe what documentation says and disable blinking when >>>> new brightness is set. Unfortunately, led_set_brightness() after >>>> modifications doesn't take into account drivers that implement >>>> blink_set op. It needs to be fixed, i.e. made compatible with >>>> the docs. >>>> >>>>> Maybe we can let the >>>>> hardware driver deal with the blink request if it has implemented >>>>> brightness_set? The change below seem to work. >>>>> >>>>> >>>>> Subject: [PATCH] led-core: Use hardware blink when available >>>>> >>>>> At present hardware implemented brightness is not called >>>>> if blink delays are set. >>>>> >>>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>>>> --- >>>>> drivers/leds/led-core.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>> index 19e1e60d..02dd0f6 100644 >>>>> --- a/drivers/leds/led-core.c >>>>> +++ b/drivers/leds/led-core.c >>>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>>>> *led_cdev, >>>>> /* >>>>> * In case blinking is on delay brightness setting >>>>> * until the next timer tick. >>>>> + * Or if brightness_set is defined, use the associated >>>>> implementation. >>>>> */ >>>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>> + if ((!led_cdev->brightness_set) && >>>> >>>> s/brightness_set/blink_set/ AFAICT >>>> >>>> It wouldn't cover all cases as the fact that a driver implements >>>> blink_set doesn't necessarily mean that hardware blinking is used >>>> for current blinking parameters. There are drivers that resort to >>>> using software fallback in case the LED controller device doesn't >>>> support requested blink intervals. >>>> >>>> I'm planning on addition of a LED_BLINKING_HW flag, that would >>>> be set after successful execution of blink_set op. It would allow to >>>> distinguish between hardware and software blinking modes reliably. >>>> >>> >>> Did you mean something like >>> >>> >>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>> index 19e1e60d..4a8b46d 100644 >>> --- a/drivers/leds/led-core.c >>> +++ b/drivers/leds/led-core.c >>> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev >>> *led_cdev, >>> * until the next timer tick. >>> */ >>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>> + if (led_cdev->brightness_set) >>> + led_set_brightness_nosleep(led_cdev, >>> brightness); >> >> brightness_set is always initialized, probably you meant blink_set. > > Your solution from your last comment below sounds better (so probably > can skip reading this section). > > But no, It was not a mistake. Actually, copied from > 'led_set_brightness_nopm' in case to protect from any drivers which > doesn't define it. The change should follow existing architecture, and > was hoping to work for both existing and new drivers. Right, brightness_set can remain uninitialized while brightness_set_blocking is provided. > Existing Chip drivers: Note, the added brightness_set is called only > when the blink is active. The flag LED_BLINKING_HW won't be set, either > because driver is not updated to include the flag, or driver wants > software fallback to deal with it. The problems I can think of is > > - the software flow will also call brightness_set later when the task > is serviced. But that is with the same values. So shouldn't make > difference to the user. > > New Drivers with brightness support while blinking:- They set brightness > and the flag. They wont need the software fallback. If for any reason > brightness couldn't be set, flag is not set and normal procedure > applies. Again problem I see here is > > - Additional responsibility on chip drivers to set the flag, if Ah, now I understand your approach. > successfully managed to set brightness while blink is active. This > shouldn't be a problem on new drivers, as they might just set the flag > every time brightness change is requested, irrespective of blink > settings. If they don't set the flag, it falls back to the software flow > as in existing architecture. > >> >>> + >>> + if (led_cdev->flags & LED_BLINKING_HW) { >>> + led_cdev->flags &= ~LED_BLINKING_HW; >>> + return; >>> + } >> >> The dependencies are quite versatile if we wanted to properly implement >> what documentation claims. Setting brightness to any value while >> blinking is on should stop blinking. It was so before the commit: >> >> 76931edd5 ('leds: fix brightness changing when software blinking is >> active') >> >> The problem was that timer trigger remained active after stopping >> blinking, which led us to changing the semantics on new brightness >> set, rather than solving the problem with unregistered trigger. >> This was also against documentation claims, which was overlooked. >> >> I tried yesterday to deactivate trigger on brightness set >> executed during blinking, but there are circular dependencies, >> since led_set_brightness(led_cdev, LED_OFF) is called on deactivation. >> It is also called from led_trigger_set in the trigger removal path. >> Generally it seems non-trivial to enforce current documentation claims >> in case of software blinking. >> >> The easiest thing to do now would be changing the semantics, so that >> only setting brightness to LED_OFF would disable the trigger, which >> in fact is true since few releases. The problem is that many drivers >> that implement hardware blinking resets it on any brightness change. >> We would have to left them intact, but apply a new semantics in the >> LED core, that would allow for new drivers to just update hardware >> blinking brightness upon updating the brightness. >> >> If we followed this path then the LED_BLINKING_HW flag would have to >> be set in led_blink_setup() after blink_set op returns 0. Thanks to >> that, we could distinguish in led_set_brightness whether hardware >> or software blinking is enabled. For !LED_BLINKING_HW case we would >> proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag >> and defer brightness setting until next timer tick. For the opposite >> case we wouldn't do anything and let the led_set_brightness_nosleep() >> to call the appropriate brightness_set/brightness_set_blocking op. >> Old drivers would proceed as currently, by disabling blinking >> on brightness change, and new ones could apply new semantics by >> changing brightness but leaving hardware blinking active. >> > > This sounds better as we do not have to rely on drivers to set the flag > and does not have the problems mentioned above. I tried the following > and works for my set up :) . > > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 19e1e60d..3698b67 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev > *led_cdev, > { > if (!(led_cdev->flags & LED_BLINK_ONESHOT) && > led_cdev->blink_set && > - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) > + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ > + led_cdev->flags |= LED_BLINKING_HW; > return; > + } > > /* blink with 1 Hz as default if nothing specified */ > if (!*delay_on && !*delay_off) > @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev *led_cdev, > * In case blinking is on delay brightness setting > * until the next timer tick. > */ > - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { > + if (!(led_cdev->flags & LED_BLINKING_HW) && > + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { > /* > * If we need to disable soft blinking delegate this to > the > * work queue task to avoid problems in case we are called We would have to also clear the flag upon blink deactivation, i.e. when brightness to be set equals LED_OFF. Existing drivers that implement blink_set op and deactivate blinking on any brightness set would have to be modified to clear the LED_BLINKING_HW flag in their brightness_{set|set_blocking} ops. > diff --git a/include/linux/leds.h b/include/linux/leds.h > index bc1476f..f5fa566 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -48,6 +48,7 @@ struct led_classdev { > #define LED_BLINK_DISABLE (1 << 21) > #define LED_SYSFS_DISABLE (1 << 22) > #define LED_DEV_CAP_FLASH (1 << 23) > +#define LED_BLINKING_HW (1 << 24) > > /* Set LED brightness level */ > /* Must not sleep, use a workqueue if needed */ > >> I wonder if it would be safe to rely on timer_pending() instead of >> introducing LED_BLINKING_HW flag... >> > How would that work? I am assuming this has something to do with > software blink? Does it take hardware blink to account? This way we could distinguish between software and hardware blinking. It wouldn't require modifications in drivers: - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { + if (timer_pending(&led_cdev->blink_timer) && + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { /* * If we need to disable soft blinking delegate this It'd must be verified however if it isn't possible that led_set_brightness is called when timer is already expired, between two ticks. >>> /* >>> * If we need to disable soft blinking delegate this to >>> the >>> * work queue task to avoid problems in case we are >>> called >>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>> index bc1476f..f5fa566 100644 >>> --- a/include/linux/leds.h >>> +++ b/include/linux/leds.h >>> @@ -48,6 +48,7 @@ struct led_classdev { >>> #define LED_BLINK_DISABLE (1 << 21) >>> #define LED_SYSFS_DISABLE (1 << 22) >>> #define LED_DEV_CAP_FLASH (1 << 23) >>> +#define LED_BLINKING_HW (1 << 24) >>> >>> /* Set LED brightness level */ >>> /* Must not sleep, use a workqueue if needed */ >>> >>>> >>>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>>> /* >>>>> * If we need to disable soft blinking delegate this to the >>>>> * work queue task to avoid problems in case we are called >>>> >>>> >>> >>> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-leds" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-12 10:26 ` Jacek Anaszewski @ 2016-05-13 14:20 ` Tony Makkiel 2016-05-16 9:21 ` Jacek Anaszewski 0 siblings, 1 reply; 18+ messages in thread From: Tony Makkiel @ 2016-05-13 14:20 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 12/05/16 11:26, Jacek Anaszewski wrote: > On 05/11/2016 03:42 PM, Tony Makkiel wrote: >> >> >> On 11/05/16 10:41, Jacek Anaszewski wrote: >>> On 05/10/2016 06:55 PM, Tony Makkiel wrote: >>>> >>>> >>>> On 10/05/16 14:26, Jacek Anaszewski wrote: >>>>> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>>>>> >>>>>> >>>>>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>>>>> Hi Tony, >>>>>>> >>>>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>>>>> Hi Jacek, >>>>>>>> Thank you for getting back. I updated my kernel to 4.5 and >>>>>>>> have >>>>>>>> the >>>>>>>> updated "led_set_brightness" now. >>>>>>>> >>>>>>>> It sets >>>>>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>>>>> led_cdev->blink_brightness = brightness; >>>>>>>> >>>>>>>> The new implementation requires hardware specific drivers to >>>>>>>> poll >>>>>>>> for flag change. Shouldn't the led-core driver be calling the >>>>>>>> hardware >>>>>>>> specific brightness_set (led_set_brightness_nosleep) >>>>>>>> irrespective of >>>>>>>> the >>>>>>>> blink settings? >>>>>>>> >>>>>>>> Unfortunately, it place additional requirement on drivers, to >>>>>>>> implement >>>>>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>>>>> brightness calls dependent on blink settings? >>>>>>> >>>>>>> If your question is still: >>>>>>> >>>>>>> "Is there a reason for rejecting brightness change requests when >>>>>>> either of the blink_delays are set?" >>>>>>> >>>>>>> then the answer is: yes, brightness setting is deferred until >>>>>>> the next timer tick to avoid avoid problems in case we are called >>>>>>> from hard irq context. It should work fine for software blinking. >>>>>> >>>>>> >>>>>> Sorry, was focused debugging 'hardware accelerated blink' on the >>>>>> driver >>>>>> I am working on, I missed the software blinking implementation. >>>>>> >>>>>>> >>>>>>> Nonetheless, your question, made it obvious that we have a problem >>>>>>> here in case of hardware accelerated blinking, i.e. drivers that >>>>>>> implement blink_set op. Is this your use case? >>>>>>> >>>>>> >>>>>> Yes, the brightness requests from user space >>>>>> (/sys/class/leds/*/brightness) does not get passed to hardware >>>>>> specific >>>>>> driver via the blink_set implemented, unless led_cdev->flags is >>>>>> polled. >>>>>> >>>>>>> Anyway, I've noticed a discrepancy between the LED core code and >>>>>>> both Documentation/leds/leds-class.txt and comment over >>>>>>> blink_set() op >>>>>>> in include/linux/leds.h, which say that blinking is deactivated >>>>>>> upon setting the brightness again. Many drivers apply this rule. >>>>>>> >>>>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>>>>> and your question will be groundless, as changing the blink >>>>>>> brightness should be impossible by design. >>>>>>> >>>>>> In my opinion, disabling blink, when brightness change requested >>>>>> doesn't >>>>>> sound like the right thing to do. There could be cases in which the >>>>>> brightness of the blinking LED needs to be changed. >>>>> >>>>> It could be accomplished with following sequence: >>>>> >>>>> $ echo LED_FULL > brightness //set brightness >>>>> $ echo "timer" > trigger //enable blinking with >>>>> brightness=LED_FULL >>>>> $ echo 1 > brightness //stop blinking and set brightness to 1 >>>>> $ echo "timer" > trigger //enable blinking with brightness=1 >>>>> >>>>> The only drawback here would be interfered blinking rhythm while >>>>> resetting blink brightness. Most drivers that implement blink_set >>>>> op observe what documentation says and disable blinking when >>>>> new brightness is set. Unfortunately, led_set_brightness() after >>>>> modifications doesn't take into account drivers that implement >>>>> blink_set op. It needs to be fixed, i.e. made compatible with >>>>> the docs. >>>>> >>>>>> Maybe we can let the >>>>>> hardware driver deal with the blink request if it has implemented >>>>>> brightness_set? The change below seem to work. >>>>>> >>>>>> >>>>>> Subject: [PATCH] led-core: Use hardware blink when available >>>>>> >>>>>> At present hardware implemented brightness is not called >>>>>> if blink delays are set. >>>>>> >>>>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>>>>> --- >>>>>> drivers/leds/led-core.c | 4 +++- >>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>>> index 19e1e60d..02dd0f6 100644 >>>>>> --- a/drivers/leds/led-core.c >>>>>> +++ b/drivers/leds/led-core.c >>>>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>>>>> *led_cdev, >>>>>> /* >>>>>> * In case blinking is on delay brightness setting >>>>>> * until the next timer tick. >>>>>> + * Or if brightness_set is defined, use the associated >>>>>> implementation. >>>>>> */ >>>>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>>> + if ((!led_cdev->brightness_set) && >>>>> >>>>> s/brightness_set/blink_set/ AFAICT >>>>> >>>>> It wouldn't cover all cases as the fact that a driver implements >>>>> blink_set doesn't necessarily mean that hardware blinking is used >>>>> for current blinking parameters. There are drivers that resort to >>>>> using software fallback in case the LED controller device doesn't >>>>> support requested blink intervals. >>>>> >>>>> I'm planning on addition of a LED_BLINKING_HW flag, that would >>>>> be set after successful execution of blink_set op. It would allow to >>>>> distinguish between hardware and software blinking modes reliably. >>>>> >>>> >>>> Did you mean something like >>>> >>>> >>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>> index 19e1e60d..4a8b46d 100644 >>>> --- a/drivers/leds/led-core.c >>>> +++ b/drivers/leds/led-core.c >>>> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev >>>> *led_cdev, >>>> * until the next timer tick. >>>> */ >>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>> + if (led_cdev->brightness_set) >>>> + led_set_brightness_nosleep(led_cdev, >>>> brightness); >>> >>> brightness_set is always initialized, probably you meant blink_set. >> >> Your solution from your last comment below sounds better (so probably >> can skip reading this section). >> >> But no, It was not a mistake. Actually, copied from >> 'led_set_brightness_nopm' in case to protect from any drivers which >> doesn't define it. The change should follow existing architecture, and >> was hoping to work for both existing and new drivers. > > Right, brightness_set can remain uninitialized while > brightness_set_blocking is provided. > >> Existing Chip drivers: Note, the added brightness_set is called only >> when the blink is active. The flag LED_BLINKING_HW won't be set, either >> because driver is not updated to include the flag, or driver wants >> software fallback to deal with it. The problems I can think of is >> >> - the software flow will also call brightness_set later when the task >> is serviced. But that is with the same values. So shouldn't make >> difference to the user. >> >> New Drivers with brightness support while blinking:- They set brightness >> and the flag. They wont need the software fallback. If for any reason >> brightness couldn't be set, flag is not set and normal procedure >> applies. Again problem I see here is >> >> - Additional responsibility on chip drivers to set the flag, if > > Ah, now I understand your approach. > >> successfully managed to set brightness while blink is active. This >> shouldn't be a problem on new drivers, as they might just set the flag >> every time brightness change is requested, irrespective of blink >> settings. If they don't set the flag, it falls back to the software flow >> as in existing architecture. >> >>> >>>> + >>>> + if (led_cdev->flags & LED_BLINKING_HW) { >>>> + led_cdev->flags &= ~LED_BLINKING_HW; >>>> + return; >>>> + } >>> >>> The dependencies are quite versatile if we wanted to properly implement >>> what documentation claims. Setting brightness to any value while >>> blinking is on should stop blinking. It was so before the commit: >>> >>> 76931edd5 ('leds: fix brightness changing when software blinking is >>> active') >>> >>> The problem was that timer trigger remained active after stopping >>> blinking, which led us to changing the semantics on new brightness >>> set, rather than solving the problem with unregistered trigger. >>> This was also against documentation claims, which was overlooked. >>> >>> I tried yesterday to deactivate trigger on brightness set >>> executed during blinking, but there are circular dependencies, >>> since led_set_brightness(led_cdev, LED_OFF) is called on deactivation. >>> It is also called from led_trigger_set in the trigger removal path. >>> Generally it seems non-trivial to enforce current documentation claims >>> in case of software blinking. >>> >>> The easiest thing to do now would be changing the semantics, so that >>> only setting brightness to LED_OFF would disable the trigger, which >>> in fact is true since few releases. The problem is that many drivers >>> that implement hardware blinking resets it on any brightness change. >>> We would have to left them intact, but apply a new semantics in the >>> LED core, that would allow for new drivers to just update hardware >>> blinking brightness upon updating the brightness. >>> >>> If we followed this path then the LED_BLINKING_HW flag would have to >>> be set in led_blink_setup() after blink_set op returns 0. Thanks to >>> that, we could distinguish in led_set_brightness whether hardware >>> or software blinking is enabled. For !LED_BLINKING_HW case we would >>> proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag >>> and defer brightness setting until next timer tick. For the opposite >>> case we wouldn't do anything and let the led_set_brightness_nosleep() >>> to call the appropriate brightness_set/brightness_set_blocking op. >>> Old drivers would proceed as currently, by disabling blinking >>> on brightness change, and new ones could apply new semantics by >>> changing brightness but leaving hardware blinking active. >>> >> >> This sounds better as we do not have to rely on drivers to set the flag >> and does not have the problems mentioned above. I tried the following >> and works for my set up :) . >> >> >> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >> index 19e1e60d..3698b67 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev >> *led_cdev, >> { >> if (!(led_cdev->flags & LED_BLINK_ONESHOT) && >> led_cdev->blink_set && >> - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) >> + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ >> + led_cdev->flags |= LED_BLINKING_HW; >> return; >> + } >> >> /* blink with 1 Hz as default if nothing specified */ >> if (!*delay_on && !*delay_off) >> @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev >> *led_cdev, >> * In case blinking is on delay brightness setting >> * until the next timer tick. >> */ >> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >> + if (!(led_cdev->flags & LED_BLINKING_HW) && >> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >> /* >> * If we need to disable soft blinking delegate this to >> the >> * work queue task to avoid problems in case we are >> called > > We would have to also clear the flag upon blink deactivation, i.e. > when brightness to be set equals LED_OFF. Existing drivers that > implement blink_set op and deactivate blinking on any brightness set > would have to be modified to clear the LED_BLINKING_HW flag in their > brightness_{set|set_blocking} ops. > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 19e1e60d..7a15035 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -98,6 +98,9 @@ static void set_brightness_delayed(struct work_struct *ws) led_cdev->delayed_set_value); else ret = -ENOTSUPP; + + if (led_cdev->delayed_set_value == LED_OFF) + led_cdev->flags &= ~LED_BLINKING_HW; if (ret < 0) dev_err(led_cdev->dev, "Setting an LED's brightness failed (%d)\n", ret); @@ -141,8 +144,10 @@ static void led_blink_setup(struct led_classdev *led_cdev, { if (!(led_cdev->flags & LED_BLINK_ONESHOT) && led_cdev->blink_set && - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ + led_cdev->flags |= LED_BLINKING_HW; return; + } /* blink with 1 Hz as default if nothing specified */ if (!*delay_on && !*delay_off) @@ -209,7 +214,8 @@ void led_set_brightness(struct led_classdev *led_cdev, * In case blinking is on delay brightness setting * until the next timer tick. */ - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { + if (!(led_cdev->flags & LED_BLINKING_HW) && + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { /* * If we need to disable soft blinking delegate this to the * work queue task to avoid problems in case we are called @@ -235,6 +241,8 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev, /* Use brightness_set op if available, it is guaranteed not to sleep */ if (led_cdev->brightness_set) { led_cdev->brightness_set(led_cdev, value); + if (value == LED_OFF) + led_cdev->flags &= ~LED_BLINKING_HW; return; } @@ -267,6 +275,9 @@ int led_set_brightness_sync(struct led_classdev *led_cdev, if (led_cdev->flags & LED_SUSPENDED) return 0; + if (value == LED_OFF) + led_cdev->flags &= ~LED_BLINKING_HW; + if (led_cdev->brightness_set_blocking) return led_cdev->brightness_set_blocking(led_cdev, led_cdev->brightness); diff --git a/include/linux/leds.h b/include/linux/leds.h index bc1476f..f5fa566 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -48,6 +48,7 @@ struct led_classdev { #define LED_BLINK_DISABLE (1 << 21) #define LED_SYSFS_DISABLE (1 << 22) #define LED_DEV_CAP_FLASH (1 << 23) +#define LED_BLINKING_HW (1 << 24) /* Set LED brightness level */ /* Must not sleep, use a workqueue if needed */ ~ >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index bc1476f..f5fa566 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -48,6 +48,7 @@ struct led_classdev { >> #define LED_BLINK_DISABLE (1 << 21) >> #define LED_SYSFS_DISABLE (1 << 22) >> #define LED_DEV_CAP_FLASH (1 << 23) >> +#define LED_BLINKING_HW (1 << 24) >> >> /* Set LED brightness level */ >> /* Must not sleep, use a workqueue if needed */ >> >>> I wonder if it would be safe to rely on timer_pending() instead of >>> introducing LED_BLINKING_HW flag... >>> >> How would that work? I am assuming this has something to do with >> software blink? Does it take hardware blink to account? > > This way we could distinguish between software and hardware blinking. > It wouldn't require modifications in drivers: > > - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { > + if (timer_pending(&led_cdev->blink_timer) && > + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { > /* > * If we need to disable soft blinking delegate this > > It'd must be verified however if it isn't possible that > led_set_brightness is called when timer is already expired, > between two ticks. if blink_set is successful, would work without problem, When blink_set successful : The timer wont be triggered resulting the function to return null all the time. --> No problem here Software blink : I do not have hardware to actually test this case. I tried simulating the case.But going through the code. Following are my understanding. Timer Active (Most of the time) : Work as normal. --> No problem here Timer Expired :led_set_brightness_nopm called. - Case in which brightness == LED_OFF, LED will be turned off. led_cdev->blink_delay_on and delay_off will retain its value. The timer will keep on running. So it will re-enable back blink. --> LED_OFF will be ignored. - Case in which brightness != LED_OFF, new brightness set and resume normal operation. --> No problem here > >>>> /* >>>> * If we need to disable soft blinking delegate >>>> this to >>>> the >>>> * work queue task to avoid problems in case we are >>>> called >>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>> index bc1476f..f5fa566 100644 >>>> --- a/include/linux/leds.h >>>> +++ b/include/linux/leds.h >>>> @@ -48,6 +48,7 @@ struct led_classdev { >>>> #define LED_BLINK_DISABLE (1 << 21) >>>> #define LED_SYSFS_DISABLE (1 << 22) >>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>> +#define LED_BLINKING_HW (1 << 24) >>>> >>>> /* Set LED brightness level */ >>>> /* Must not sleep, use a workqueue if needed */ >>>> >>>>> >>>>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>>>> /* >>>>>> * If we need to disable soft blinking delegate this to the >>>>>> * work queue task to avoid problems in case we are called >>>>> >>>>> >>>> >>>> >>> >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-leds" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > > ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-13 14:20 ` Tony Makkiel @ 2016-05-16 9:21 ` Jacek Anaszewski 2016-05-16 13:43 ` Tony Makkiel 0 siblings, 1 reply; 18+ messages in thread From: Jacek Anaszewski @ 2016-05-16 9:21 UTC (permalink / raw) To: Tony Makkiel; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml Hi Tony, On 05/13/2016 04:20 PM, Tony Makkiel wrote: > > > On 12/05/16 11:26, Jacek Anaszewski wrote: >> On 05/11/2016 03:42 PM, Tony Makkiel wrote: >>> >>> >>> On 11/05/16 10:41, Jacek Anaszewski wrote: >>>> On 05/10/2016 06:55 PM, Tony Makkiel wrote: >>>>> >>>>> >>>>> On 10/05/16 14:26, Jacek Anaszewski wrote: >>>>>> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>>>>>> >>>>>>> >>>>>>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>>>>>> Hi Tony, >>>>>>>> >>>>>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>>>>>> Hi Jacek, >>>>>>>>> Thank you for getting back. I updated my kernel to 4.5 and >>>>>>>>> have >>>>>>>>> the >>>>>>>>> updated "led_set_brightness" now. >>>>>>>>> >>>>>>>>> It sets >>>>>>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>>>>>> led_cdev->blink_brightness = brightness; >>>>>>>>> >>>>>>>>> The new implementation requires hardware specific drivers to >>>>>>>>> poll >>>>>>>>> for flag change. Shouldn't the led-core driver be calling the >>>>>>>>> hardware >>>>>>>>> specific brightness_set (led_set_brightness_nosleep) >>>>>>>>> irrespective of >>>>>>>>> the >>>>>>>>> blink settings? >>>>>>>>> >>>>>>>>> Unfortunately, it place additional requirement on drivers, to >>>>>>>>> implement >>>>>>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>>>>>> brightness calls dependent on blink settings? >>>>>>>> >>>>>>>> If your question is still: >>>>>>>> >>>>>>>> "Is there a reason for rejecting brightness change requests when >>>>>>>> either of the blink_delays are set?" >>>>>>>> >>>>>>>> then the answer is: yes, brightness setting is deferred until >>>>>>>> the next timer tick to avoid avoid problems in case we are called >>>>>>>> from hard irq context. It should work fine for software blinking. >>>>>>> >>>>>>> >>>>>>> Sorry, was focused debugging 'hardware accelerated blink' on the >>>>>>> driver >>>>>>> I am working on, I missed the software blinking implementation. >>>>>>> >>>>>>>> >>>>>>>> Nonetheless, your question, made it obvious that we have a problem >>>>>>>> here in case of hardware accelerated blinking, i.e. drivers that >>>>>>>> implement blink_set op. Is this your use case? >>>>>>>> >>>>>>> >>>>>>> Yes, the brightness requests from user space >>>>>>> (/sys/class/leds/*/brightness) does not get passed to hardware >>>>>>> specific >>>>>>> driver via the blink_set implemented, unless led_cdev->flags is >>>>>>> polled. >>>>>>> >>>>>>>> Anyway, I've noticed a discrepancy between the LED core code and >>>>>>>> both Documentation/leds/leds-class.txt and comment over >>>>>>>> blink_set() op >>>>>>>> in include/linux/leds.h, which say that blinking is deactivated >>>>>>>> upon setting the brightness again. Many drivers apply this rule. >>>>>>>> >>>>>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>>>>>> and your question will be groundless, as changing the blink >>>>>>>> brightness should be impossible by design. >>>>>>>> >>>>>>> In my opinion, disabling blink, when brightness change requested >>>>>>> doesn't >>>>>>> sound like the right thing to do. There could be cases in which the >>>>>>> brightness of the blinking LED needs to be changed. >>>>>> >>>>>> It could be accomplished with following sequence: >>>>>> >>>>>> $ echo LED_FULL > brightness //set brightness >>>>>> $ echo "timer" > trigger //enable blinking with >>>>>> brightness=LED_FULL >>>>>> $ echo 1 > brightness //stop blinking and set brightness to 1 >>>>>> $ echo "timer" > trigger //enable blinking with brightness=1 >>>>>> >>>>>> The only drawback here would be interfered blinking rhythm while >>>>>> resetting blink brightness. Most drivers that implement blink_set >>>>>> op observe what documentation says and disable blinking when >>>>>> new brightness is set. Unfortunately, led_set_brightness() after >>>>>> modifications doesn't take into account drivers that implement >>>>>> blink_set op. It needs to be fixed, i.e. made compatible with >>>>>> the docs. >>>>>> >>>>>>> Maybe we can let the >>>>>>> hardware driver deal with the blink request if it has implemented >>>>>>> brightness_set? The change below seem to work. >>>>>>> >>>>>>> >>>>>>> Subject: [PATCH] led-core: Use hardware blink when available >>>>>>> >>>>>>> At present hardware implemented brightness is not called >>>>>>> if blink delays are set. >>>>>>> >>>>>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>>>>>> --- >>>>>>> drivers/leds/led-core.c | 4 +++- >>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>>>> index 19e1e60d..02dd0f6 100644 >>>>>>> --- a/drivers/leds/led-core.c >>>>>>> +++ b/drivers/leds/led-core.c >>>>>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>>>>>> *led_cdev, >>>>>>> /* >>>>>>> * In case blinking is on delay brightness setting >>>>>>> * until the next timer tick. >>>>>>> + * Or if brightness_set is defined, use the associated >>>>>>> implementation. >>>>>>> */ >>>>>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>>>> + if ((!led_cdev->brightness_set) && >>>>>> >>>>>> s/brightness_set/blink_set/ AFAICT >>>>>> >>>>>> It wouldn't cover all cases as the fact that a driver implements >>>>>> blink_set doesn't necessarily mean that hardware blinking is used >>>>>> for current blinking parameters. There are drivers that resort to >>>>>> using software fallback in case the LED controller device doesn't >>>>>> support requested blink intervals. >>>>>> >>>>>> I'm planning on addition of a LED_BLINKING_HW flag, that would >>>>>> be set after successful execution of blink_set op. It would allow to >>>>>> distinguish between hardware and software blinking modes reliably. >>>>>> >>>>> >>>>> Did you mean something like >>>>> >>>>> >>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>> index 19e1e60d..4a8b46d 100644 >>>>> --- a/drivers/leds/led-core.c >>>>> +++ b/drivers/leds/led-core.c >>>>> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev >>>>> *led_cdev, >>>>> * until the next timer tick. >>>>> */ >>>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>> + if (led_cdev->brightness_set) >>>>> + led_set_brightness_nosleep(led_cdev, >>>>> brightness); >>>> >>>> brightness_set is always initialized, probably you meant blink_set. >>> >>> Your solution from your last comment below sounds better (so probably >>> can skip reading this section). >>> >>> But no, It was not a mistake. Actually, copied from >>> 'led_set_brightness_nopm' in case to protect from any drivers which >>> doesn't define it. The change should follow existing architecture, and >>> was hoping to work for both existing and new drivers. >> >> Right, brightness_set can remain uninitialized while >> brightness_set_blocking is provided. >> >>> Existing Chip drivers: Note, the added brightness_set is called only >>> when the blink is active. The flag LED_BLINKING_HW won't be set, either >>> because driver is not updated to include the flag, or driver wants >>> software fallback to deal with it. The problems I can think of is >>> >>> - the software flow will also call brightness_set later when the task >>> is serviced. But that is with the same values. So shouldn't make >>> difference to the user. >>> >>> New Drivers with brightness support while blinking:- They set brightness >>> and the flag. They wont need the software fallback. If for any reason >>> brightness couldn't be set, flag is not set and normal procedure >>> applies. Again problem I see here is >>> >>> - Additional responsibility on chip drivers to set the flag, if >> >> Ah, now I understand your approach. >> >>> successfully managed to set brightness while blink is active. This >>> shouldn't be a problem on new drivers, as they might just set the flag >>> every time brightness change is requested, irrespective of blink >>> settings. If they don't set the flag, it falls back to the software flow >>> as in existing architecture. >>> >>>> >>>>> + >>>>> + if (led_cdev->flags & LED_BLINKING_HW) { >>>>> + led_cdev->flags &= ~LED_BLINKING_HW; >>>>> + return; >>>>> + } >>>> >>>> The dependencies are quite versatile if we wanted to properly implement >>>> what documentation claims. Setting brightness to any value while >>>> blinking is on should stop blinking. It was so before the commit: >>>> >>>> 76931edd5 ('leds: fix brightness changing when software blinking is >>>> active') >>>> >>>> The problem was that timer trigger remained active after stopping >>>> blinking, which led us to changing the semantics on new brightness >>>> set, rather than solving the problem with unregistered trigger. >>>> This was also against documentation claims, which was overlooked. >>>> >>>> I tried yesterday to deactivate trigger on brightness set >>>> executed during blinking, but there are circular dependencies, >>>> since led_set_brightness(led_cdev, LED_OFF) is called on deactivation. >>>> It is also called from led_trigger_set in the trigger removal path. >>>> Generally it seems non-trivial to enforce current documentation claims >>>> in case of software blinking. >>>> >>>> The easiest thing to do now would be changing the semantics, so that >>>> only setting brightness to LED_OFF would disable the trigger, which >>>> in fact is true since few releases. The problem is that many drivers >>>> that implement hardware blinking resets it on any brightness change. >>>> We would have to left them intact, but apply a new semantics in the >>>> LED core, that would allow for new drivers to just update hardware >>>> blinking brightness upon updating the brightness. >>>> >>>> If we followed this path then the LED_BLINKING_HW flag would have to >>>> be set in led_blink_setup() after blink_set op returns 0. Thanks to >>>> that, we could distinguish in led_set_brightness whether hardware >>>> or software blinking is enabled. For !LED_BLINKING_HW case we would >>>> proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag >>>> and defer brightness setting until next timer tick. For the opposite >>>> case we wouldn't do anything and let the led_set_brightness_nosleep() >>>> to call the appropriate brightness_set/brightness_set_blocking op. >>>> Old drivers would proceed as currently, by disabling blinking >>>> on brightness change, and new ones could apply new semantics by >>>> changing brightness but leaving hardware blinking active. >>>> >>> >>> This sounds better as we do not have to rely on drivers to set the flag >>> and does not have the problems mentioned above. I tried the following >>> and works for my set up :) . >>> >>> >>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>> index 19e1e60d..3698b67 100644 >>> --- a/drivers/leds/led-core.c >>> +++ b/drivers/leds/led-core.c >>> @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev >>> *led_cdev, >>> { >>> if (!(led_cdev->flags & LED_BLINK_ONESHOT) && >>> led_cdev->blink_set && >>> - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) >>> + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ >>> + led_cdev->flags |= LED_BLINKING_HW; >>> return; >>> + } >>> >>> /* blink with 1 Hz as default if nothing specified */ >>> if (!*delay_on && !*delay_off) >>> @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev >>> *led_cdev, >>> * In case blinking is on delay brightness setting >>> * until the next timer tick. >>> */ >>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>> + if (!(led_cdev->flags & LED_BLINKING_HW) && >>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>> /* >>> * If we need to disable soft blinking delegate this to >>> the >>> * work queue task to avoid problems in case we are >>> called >> >> We would have to also clear the flag upon blink deactivation, i.e. >> when brightness to be set equals LED_OFF. Existing drivers that >> implement blink_set op and deactivate blinking on any brightness set >> would have to be modified to clear the LED_BLINKING_HW flag in their >> brightness_{set|set_blocking} ops. >> > > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 19e1e60d..7a15035 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -98,6 +98,9 @@ static void set_brightness_delayed(struct work_struct > *ws) > > led_cdev->delayed_set_value); > else > ret = -ENOTSUPP; > + > + if (led_cdev->delayed_set_value == LED_OFF) > + led_cdev->flags &= ~LED_BLINKING_HW; > if (ret < 0) > dev_err(led_cdev->dev, > "Setting an LED's brightness failed (%d)\n", ret); > @@ -141,8 +144,10 @@ static void led_blink_setup(struct led_classdev > *led_cdev, > { > if (!(led_cdev->flags & LED_BLINK_ONESHOT) && > led_cdev->blink_set && > - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) > + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ > + led_cdev->flags |= LED_BLINKING_HW; > return; > + } > > /* blink with 1 Hz as default if nothing specified */ > if (!*delay_on && !*delay_off) > @@ -209,7 +214,8 @@ void led_set_brightness(struct led_classdev *led_cdev, > * In case blinking is on delay brightness setting > * until the next timer tick. > */ > - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { > + if (!(led_cdev->flags & LED_BLINKING_HW) && > + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { > /* > * If we need to disable soft blinking delegate this to > the > * work queue task to avoid problems in case we are called > @@ -235,6 +241,8 @@ void led_set_brightness_nopm(struct led_classdev > *led_cdev, > /* Use brightness_set op if available, it is guaranteed not to > sleep */ > if (led_cdev->brightness_set) { > led_cdev->brightness_set(led_cdev, value); > + if (value == LED_OFF) > + led_cdev->flags &= ~LED_BLINKING_HW; > return; > } > > @@ -267,6 +275,9 @@ int led_set_brightness_sync(struct led_classdev > *led_cdev, > if (led_cdev->flags & LED_SUSPENDED) > return 0; > > + if (value == LED_OFF) > + led_cdev->flags &= ~LED_BLINKING_HW; > + > if (led_cdev->brightness_set_blocking) > return led_cdev->brightness_set_blocking(led_cdev, > > led_cdev->brightness); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index bc1476f..f5fa566 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -48,6 +48,7 @@ struct led_classdev { > #define LED_BLINK_DISABLE (1 << 21) > #define LED_SYSFS_DISABLE (1 << 22) > #define LED_DEV_CAP_FLASH (1 << 23) > +#define LED_BLINKING_HW (1 << 24) > > /* Set LED brightness level */ > /* Must not sleep, use a workqueue if needed */ > ~ > > >>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>> index bc1476f..f5fa566 100644 >>> --- a/include/linux/leds.h >>> +++ b/include/linux/leds.h >>> @@ -48,6 +48,7 @@ struct led_classdev { >>> #define LED_BLINK_DISABLE (1 << 21) >>> #define LED_SYSFS_DISABLE (1 << 22) >>> #define LED_DEV_CAP_FLASH (1 << 23) >>> +#define LED_BLINKING_HW (1 << 24) >>> >>> /* Set LED brightness level */ >>> /* Must not sleep, use a workqueue if needed */ >>> >>>> I wonder if it would be safe to rely on timer_pending() instead of >>>> introducing LED_BLINKING_HW flag... >>>> >>> How would that work? I am assuming this has something to do with >>> software blink? Does it take hardware blink to account? >> >> This way we could distinguish between software and hardware blinking. >> It wouldn't require modifications in drivers: >> >> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >> + if (timer_pending(&led_cdev->blink_timer) && >> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >> /* >> * If we need to disable soft blinking delegate this >> >> It'd must be verified however if it isn't possible that >> led_set_brightness is called when timer is already expired, >> between two ticks. > > if blink_set is successful, would work without problem, > > When blink_set successful : The timer wont be triggered resulting the > function to return null all the time. --> No problem here > > Software blink : I do not have hardware to actually test this case. I > tried simulating the case.But going through the code. Following are my > understanding. You can test it by leaving blink_set op uninitialized in your driver. > > Timer Active (Most of the time) : Work as normal. --> No problem here > > Timer Expired :led_set_brightness_nopm called. > - Case in which brightness == LED_OFF, LED will be turned off. > led_cdev->blink_delay_on and delay_off will retain its value. The timer > will keep on running. So it will re-enable back blink. --> LED_OFF will > be ignored. > > - Case in which brightness != LED_OFF, new brightness set and resume > normal operation. --> No problem here > Thanks for this analysis. I have a new idea - wouldn't it be more robust if we added LED_BLINKING_SW flag and set it in led_set_software_blink()? The line if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) in led_set_brightness() could be then changed to if (led_cdev->flags & LED_BLINK_SW) . LED_BLINK_SW flag would have to be cleared in led_stop_software_blink() and in the first two conditions in the led_timer_function(). > >> >>>>> /* >>>>> * If we need to disable soft blinking delegate >>>>> this to >>>>> the >>>>> * work queue task to avoid problems in case we are >>>>> called >>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>>> index bc1476f..f5fa566 100644 >>>>> --- a/include/linux/leds.h >>>>> +++ b/include/linux/leds.h >>>>> @@ -48,6 +48,7 @@ struct led_classdev { >>>>> #define LED_BLINK_DISABLE (1 << 21) >>>>> #define LED_SYSFS_DISABLE (1 << 22) >>>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>>> +#define LED_BLINKING_HW (1 << 24) >>>>> >>>>> /* Set LED brightness level */ >>>>> /* Must not sleep, use a workqueue if needed */ >>>>> >>>>>> >>>>>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>>>>> /* >>>>>>> * If we need to disable soft blinking delegate this to >>>>>>> the >>>>>>> * work queue task to avoid problems in case we are called >>>>>> >>>>>> >>>>> >>>>> >>>> >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-leds" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >> >> > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-16 9:21 ` Jacek Anaszewski @ 2016-05-16 13:43 ` Tony Makkiel 2016-05-16 14:23 ` Jacek Anaszewski 0 siblings, 1 reply; 18+ messages in thread From: Tony Makkiel @ 2016-05-16 13:43 UTC (permalink / raw) To: Jacek Anaszewski; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 16/05/16 10:21, Jacek Anaszewski wrote: > Hi Tony, > > On 05/13/2016 04:20 PM, Tony Makkiel wrote: >> >> >> On 12/05/16 11:26, Jacek Anaszewski wrote: >>> On 05/11/2016 03:42 PM, Tony Makkiel wrote: >>>> >>>> >>>> On 11/05/16 10:41, Jacek Anaszewski wrote: >>>>> On 05/10/2016 06:55 PM, Tony Makkiel wrote: >>>>>> >>>>>> >>>>>> On 10/05/16 14:26, Jacek Anaszewski wrote: >>>>>>> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>>>>>>> Hi Tony, >>>>>>>>> >>>>>>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>>>>>>> Hi Jacek, >>>>>>>>>> Thank you for getting back. I updated my kernel to 4.5 and >>>>>>>>>> have >>>>>>>>>> the >>>>>>>>>> updated "led_set_brightness" now. >>>>>>>>>> >>>>>>>>>> It sets >>>>>>>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>>>>>>> led_cdev->blink_brightness = brightness; >>>>>>>>>> >>>>>>>>>> The new implementation requires hardware specific drivers to >>>>>>>>>> poll >>>>>>>>>> for flag change. Shouldn't the led-core driver be calling the >>>>>>>>>> hardware >>>>>>>>>> specific brightness_set (led_set_brightness_nosleep) >>>>>>>>>> irrespective of >>>>>>>>>> the >>>>>>>>>> blink settings? >>>>>>>>>> >>>>>>>>>> Unfortunately, it place additional requirement on drivers, to >>>>>>>>>> implement >>>>>>>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>>>>>>> brightness calls dependent on blink settings? >>>>>>>>> >>>>>>>>> If your question is still: >>>>>>>>> >>>>>>>>> "Is there a reason for rejecting brightness change requests when >>>>>>>>> either of the blink_delays are set?" >>>>>>>>> >>>>>>>>> then the answer is: yes, brightness setting is deferred until >>>>>>>>> the next timer tick to avoid avoid problems in case we are called >>>>>>>>> from hard irq context. It should work fine for software blinking. >>>>>>>> >>>>>>>> >>>>>>>> Sorry, was focused debugging 'hardware accelerated blink' on the >>>>>>>> driver >>>>>>>> I am working on, I missed the software blinking implementation. >>>>>>>> >>>>>>>>> >>>>>>>>> Nonetheless, your question, made it obvious that we have a problem >>>>>>>>> here in case of hardware accelerated blinking, i.e. drivers that >>>>>>>>> implement blink_set op. Is this your use case? >>>>>>>>> >>>>>>>> >>>>>>>> Yes, the brightness requests from user space >>>>>>>> (/sys/class/leds/*/brightness) does not get passed to hardware >>>>>>>> specific >>>>>>>> driver via the blink_set implemented, unless led_cdev->flags is >>>>>>>> polled. >>>>>>>> >>>>>>>>> Anyway, I've noticed a discrepancy between the LED core code and >>>>>>>>> both Documentation/leds/leds-class.txt and comment over >>>>>>>>> blink_set() op >>>>>>>>> in include/linux/leds.h, which say that blinking is deactivated >>>>>>>>> upon setting the brightness again. Many drivers apply this rule. >>>>>>>>> >>>>>>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>>>>>>> and your question will be groundless, as changing the blink >>>>>>>>> brightness should be impossible by design. >>>>>>>>> >>>>>>>> In my opinion, disabling blink, when brightness change requested >>>>>>>> doesn't >>>>>>>> sound like the right thing to do. There could be cases in which the >>>>>>>> brightness of the blinking LED needs to be changed. >>>>>>> >>>>>>> It could be accomplished with following sequence: >>>>>>> >>>>>>> $ echo LED_FULL > brightness //set brightness >>>>>>> $ echo "timer" > trigger //enable blinking with >>>>>>> brightness=LED_FULL >>>>>>> $ echo 1 > brightness //stop blinking and set brightness to 1 >>>>>>> $ echo "timer" > trigger //enable blinking with brightness=1 >>>>>>> >>>>>>> The only drawback here would be interfered blinking rhythm while >>>>>>> resetting blink brightness. Most drivers that implement blink_set >>>>>>> op observe what documentation says and disable blinking when >>>>>>> new brightness is set. Unfortunately, led_set_brightness() after >>>>>>> modifications doesn't take into account drivers that implement >>>>>>> blink_set op. It needs to be fixed, i.e. made compatible with >>>>>>> the docs. >>>>>>> >>>>>>>> Maybe we can let the >>>>>>>> hardware driver deal with the blink request if it has implemented >>>>>>>> brightness_set? The change below seem to work. >>>>>>>> >>>>>>>> >>>>>>>> Subject: [PATCH] led-core: Use hardware blink when available >>>>>>>> >>>>>>>> At present hardware implemented brightness is not called >>>>>>>> if blink delays are set. >>>>>>>> >>>>>>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>>>>>>> --- >>>>>>>> drivers/leds/led-core.c | 4 +++- >>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>>>>> index 19e1e60d..02dd0f6 100644 >>>>>>>> --- a/drivers/leds/led-core.c >>>>>>>> +++ b/drivers/leds/led-core.c >>>>>>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>>>>>>> *led_cdev, >>>>>>>> /* >>>>>>>> * In case blinking is on delay brightness setting >>>>>>>> * until the next timer tick. >>>>>>>> + * Or if brightness_set is defined, use the associated >>>>>>>> implementation. >>>>>>>> */ >>>>>>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>>>>> + if ((!led_cdev->brightness_set) && >>>>>>> >>>>>>> s/brightness_set/blink_set/ AFAICT >>>>>>> >>>>>>> It wouldn't cover all cases as the fact that a driver implements >>>>>>> blink_set doesn't necessarily mean that hardware blinking is used >>>>>>> for current blinking parameters. There are drivers that resort to >>>>>>> using software fallback in case the LED controller device doesn't >>>>>>> support requested blink intervals. >>>>>>> >>>>>>> I'm planning on addition of a LED_BLINKING_HW flag, that would >>>>>>> be set after successful execution of blink_set op. It would allow to >>>>>>> distinguish between hardware and software blinking modes reliably. >>>>>>> >>>>>> >>>>>> Did you mean something like >>>>>> >>>>>> >>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>>> index 19e1e60d..4a8b46d 100644 >>>>>> --- a/drivers/leds/led-core.c >>>>>> +++ b/drivers/leds/led-core.c >>>>>> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev >>>>>> *led_cdev, >>>>>> * until the next timer tick. >>>>>> */ >>>>>> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>>> + if (led_cdev->brightness_set) >>>>>> + led_set_brightness_nosleep(led_cdev, >>>>>> brightness); >>>>> >>>>> brightness_set is always initialized, probably you meant blink_set. >>>> >>>> Your solution from your last comment below sounds better (so probably >>>> can skip reading this section). >>>> >>>> But no, It was not a mistake. Actually, copied from >>>> 'led_set_brightness_nopm' in case to protect from any drivers which >>>> doesn't define it. The change should follow existing architecture, and >>>> was hoping to work for both existing and new drivers. >>> >>> Right, brightness_set can remain uninitialized while >>> brightness_set_blocking is provided. >>> >>>> Existing Chip drivers: Note, the added brightness_set is called only >>>> when the blink is active. The flag LED_BLINKING_HW won't be set, either >>>> because driver is not updated to include the flag, or driver wants >>>> software fallback to deal with it. The problems I can think of is >>>> >>>> - the software flow will also call brightness_set later when the task >>>> is serviced. But that is with the same values. So shouldn't make >>>> difference to the user. >>>> >>>> New Drivers with brightness support while blinking:- They set >>>> brightness >>>> and the flag. They wont need the software fallback. If for any reason >>>> brightness couldn't be set, flag is not set and normal procedure >>>> applies. Again problem I see here is >>>> >>>> - Additional responsibility on chip drivers to set the flag, if >>> >>> Ah, now I understand your approach. >>> >>>> successfully managed to set brightness while blink is active. This >>>> shouldn't be a problem on new drivers, as they might just set the flag >>>> every time brightness change is requested, irrespective of blink >>>> settings. If they don't set the flag, it falls back to the software >>>> flow >>>> as in existing architecture. >>>> >>>>> >>>>>> + >>>>>> + if (led_cdev->flags & LED_BLINKING_HW) { >>>>>> + led_cdev->flags &= ~LED_BLINKING_HW; >>>>>> + return; >>>>>> + } >>>>> >>>>> The dependencies are quite versatile if we wanted to properly >>>>> implement >>>>> what documentation claims. Setting brightness to any value while >>>>> blinking is on should stop blinking. It was so before the commit: >>>>> >>>>> 76931edd5 ('leds: fix brightness changing when software blinking is >>>>> active') >>>>> >>>>> The problem was that timer trigger remained active after stopping >>>>> blinking, which led us to changing the semantics on new brightness >>>>> set, rather than solving the problem with unregistered trigger. >>>>> This was also against documentation claims, which was overlooked. >>>>> >>>>> I tried yesterday to deactivate trigger on brightness set >>>>> executed during blinking, but there are circular dependencies, >>>>> since led_set_brightness(led_cdev, LED_OFF) is called on deactivation. >>>>> It is also called from led_trigger_set in the trigger removal path. >>>>> Generally it seems non-trivial to enforce current documentation claims >>>>> in case of software blinking. >>>>> >>>>> The easiest thing to do now would be changing the semantics, so that >>>>> only setting brightness to LED_OFF would disable the trigger, which >>>>> in fact is true since few releases. The problem is that many drivers >>>>> that implement hardware blinking resets it on any brightness change. >>>>> We would have to left them intact, but apply a new semantics in the >>>>> LED core, that would allow for new drivers to just update hardware >>>>> blinking brightness upon updating the brightness. >>>>> >>>>> If we followed this path then the LED_BLINKING_HW flag would have to >>>>> be set in led_blink_setup() after blink_set op returns 0. Thanks to >>>>> that, we could distinguish in led_set_brightness whether hardware >>>>> or software blinking is enabled. For !LED_BLINKING_HW case we would >>>>> proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag >>>>> and defer brightness setting until next timer tick. For the opposite >>>>> case we wouldn't do anything and let the led_set_brightness_nosleep() >>>>> to call the appropriate brightness_set/brightness_set_blocking op. >>>>> Old drivers would proceed as currently, by disabling blinking >>>>> on brightness change, and new ones could apply new semantics by >>>>> changing brightness but leaving hardware blinking active. >>>>> >>>> >>>> This sounds better as we do not have to rely on drivers to set the flag >>>> and does not have the problems mentioned above. I tried the following >>>> and works for my set up :) . >>>> >>>> >>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>> index 19e1e60d..3698b67 100644 >>>> --- a/drivers/leds/led-core.c >>>> +++ b/drivers/leds/led-core.c >>>> @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev >>>> *led_cdev, >>>> { >>>> if (!(led_cdev->flags & LED_BLINK_ONESHOT) && >>>> led_cdev->blink_set && >>>> - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) >>>> + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ >>>> + led_cdev->flags |= LED_BLINKING_HW; >>>> return; >>>> + } >>>> >>>> /* blink with 1 Hz as default if nothing specified */ >>>> if (!*delay_on && !*delay_off) >>>> @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev >>>> *led_cdev, >>>> * In case blinking is on delay brightness setting >>>> * until the next timer tick. >>>> */ >>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>> + if (!(led_cdev->flags & LED_BLINKING_HW) && >>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>> /* >>>> * If we need to disable soft blinking delegate >>>> this to >>>> the >>>> * work queue task to avoid problems in case we are >>>> called >>> >>> We would have to also clear the flag upon blink deactivation, i.e. >>> when brightness to be set equals LED_OFF. Existing drivers that >>> implement blink_set op and deactivate blinking on any brightness set >>> would have to be modified to clear the LED_BLINKING_HW flag in their >>> brightness_{set|set_blocking} ops. >>> >> >> >> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >> index 19e1e60d..7a15035 100644 >> --- a/drivers/leds/led-core.c >> +++ b/drivers/leds/led-core.c >> @@ -98,6 +98,9 @@ static void set_brightness_delayed(struct work_struct >> *ws) >> >> led_cdev->delayed_set_value); >> else >> ret = -ENOTSUPP; >> + >> + if (led_cdev->delayed_set_value == LED_OFF) >> + led_cdev->flags &= ~LED_BLINKING_HW; >> if (ret < 0) >> dev_err(led_cdev->dev, >> "Setting an LED's brightness failed (%d)\n", >> ret); >> @@ -141,8 +144,10 @@ static void led_blink_setup(struct led_classdev >> *led_cdev, >> { >> if (!(led_cdev->flags & LED_BLINK_ONESHOT) && >> led_cdev->blink_set && >> - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) >> + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ >> + led_cdev->flags |= LED_BLINKING_HW; >> return; >> + } >> >> /* blink with 1 Hz as default if nothing specified */ >> if (!*delay_on && !*delay_off) >> @@ -209,7 +214,8 @@ void led_set_brightness(struct led_classdev >> *led_cdev, >> * In case blinking is on delay brightness setting >> * until the next timer tick. >> */ >> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >> + if (!(led_cdev->flags & LED_BLINKING_HW) && >> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >> /* >> * If we need to disable soft blinking delegate this to >> the >> * work queue task to avoid problems in case we are >> called >> @@ -235,6 +241,8 @@ void led_set_brightness_nopm(struct led_classdev >> *led_cdev, >> /* Use brightness_set op if available, it is guaranteed not to >> sleep */ >> if (led_cdev->brightness_set) { >> led_cdev->brightness_set(led_cdev, value); >> + if (value == LED_OFF) >> + led_cdev->flags &= ~LED_BLINKING_HW; >> return; >> } >> >> @@ -267,6 +275,9 @@ int led_set_brightness_sync(struct led_classdev >> *led_cdev, >> if (led_cdev->flags & LED_SUSPENDED) >> return 0; >> >> + if (value == LED_OFF) >> + led_cdev->flags &= ~LED_BLINKING_HW; >> + >> if (led_cdev->brightness_set_blocking) >> return led_cdev->brightness_set_blocking(led_cdev, >> >> led_cdev->brightness); >> diff --git a/include/linux/leds.h b/include/linux/leds.h >> index bc1476f..f5fa566 100644 >> --- a/include/linux/leds.h >> +++ b/include/linux/leds.h >> @@ -48,6 +48,7 @@ struct led_classdev { >> #define LED_BLINK_DISABLE (1 << 21) >> #define LED_SYSFS_DISABLE (1 << 22) >> #define LED_DEV_CAP_FLASH (1 << 23) >> +#define LED_BLINKING_HW (1 << 24) >> >> /* Set LED brightness level */ >> /* Must not sleep, use a workqueue if needed */ >> ~ >> >> >>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>> index bc1476f..f5fa566 100644 >>>> --- a/include/linux/leds.h >>>> +++ b/include/linux/leds.h >>>> @@ -48,6 +48,7 @@ struct led_classdev { >>>> #define LED_BLINK_DISABLE (1 << 21) >>>> #define LED_SYSFS_DISABLE (1 << 22) >>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>> +#define LED_BLINKING_HW (1 << 24) >>>> >>>> /* Set LED brightness level */ >>>> /* Must not sleep, use a workqueue if needed */ >>>> >>>>> I wonder if it would be safe to rely on timer_pending() instead of >>>>> introducing LED_BLINKING_HW flag... >>>>> >>>> How would that work? I am assuming this has something to do with >>>> software blink? Does it take hardware blink to account? >>> >>> This way we could distinguish between software and hardware blinking. >>> It wouldn't require modifications in drivers: >>> >>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>> + if (timer_pending(&led_cdev->blink_timer) && >>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>> /* >>> * If we need to disable soft blinking delegate this >>> >>> It'd must be verified however if it isn't possible that >>> led_set_brightness is called when timer is already expired, >>> between two ticks. >> >> if blink_set is successful, would work without problem, >> >> When blink_set successful : The timer wont be triggered resulting the >> function to return null all the time. --> No problem here >> >> Software blink : I do not have hardware to actually test this case. I >> tried simulating the case.But going through the code. Following are my >> understanding. > > You can test it by leaving blink_set op uninitialized in your driver. > >> >> Timer Active (Most of the time) : Work as normal. --> No problem here >> >> Timer Expired :led_set_brightness_nopm called. >> - Case in which brightness == LED_OFF, LED will be turned off. >> led_cdev->blink_delay_on and delay_off will retain its value. The timer >> will keep on running. So it will re-enable back blink. --> LED_OFF will >> be ignored. >> >> - Case in which brightness != LED_OFF, new brightness set and resume >> normal operation. --> No problem here >> > > Thanks for this analysis. I have a new idea - wouldn't it be more robust > if we added LED_BLINKING_SW flag and set it in led_set_software_blink()? > > The line > > if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) > > in led_set_brightness() could be then changed to > > if (led_cdev->flags & LED_BLINK_SW) . > > LED_BLINK_SW flag would have to be cleared in led_stop_software_blink() > and in the first two conditions in the led_timer_function(). > Yes, that will do with minimal changes. I tested the following, and works. Date: Mon, 16 May 2016 14:18:42 +0100 Subject: [PATCH 1/1] Allow chip-driver to set brightness if, software blink not used. If software blink were active any brightness change request were not sent to the chip driver. Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> --- drivers/leds/led-core.c | 7 +++++-- include/linux/leds.h | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c index 19e1e60d..376b5ea 100644 --- a/drivers/leds/led-core.c +++ b/drivers/leds/led-core.c @@ -33,11 +33,12 @@ static void led_timer_function(unsigned long data) if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { led_set_brightness_nosleep(led_cdev, LED_OFF); + led_cdev->flags &= ~LED_BLINKING_SW; return; } if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) { - led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP; + led_cdev->flags &= ~(LED_BLINK_ONESHOT_STOP | LED_BLINKING_SW); return; } @@ -131,6 +132,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev, return; } + led_cdev->flags |= LED_BLINKING_SW; mod_timer(&led_cdev->blink_timer, jiffies + 1); } @@ -199,6 +201,7 @@ void led_stop_software_blink(struct led_classdev *led_cdev) del_timer_sync(&led_cdev->blink_timer); led_cdev->blink_delay_on = 0; led_cdev->blink_delay_off = 0; + led_cdev->flags &= ~LED_BLINKING_SW; } EXPORT_SYMBOL_GPL(led_stop_software_blink); @@ -209,7 +212,7 @@ void led_set_brightness(struct led_classdev *led_cdev, * In case blinking is on delay brightness setting * until the next timer tick. */ - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { + if (led_cdev->flags & LED_BLINKING_SW) { /* * If we need to disable soft blinking delegate this to the * work queue task to avoid problems in case we are called diff --git a/include/linux/leds.h b/include/linux/leds.h index bc1476f..08ef6f4 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -48,6 +48,7 @@ struct led_classdev { #define LED_BLINK_DISABLE (1 << 21) #define LED_SYSFS_DISABLE (1 << 22) #define LED_DEV_CAP_FLASH (1 << 23) +#define LED_BLINKING_SW (1 << 24) /* Set LED brightness level */ /* Must not sleep, use a workqueue if needed */ -- 1.9.1 >> >>> >>>>>> /* >>>>>> * If we need to disable soft blinking delegate >>>>>> this to >>>>>> the >>>>>> * work queue task to avoid problems in case we are >>>>>> called >>>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>>>> index bc1476f..f5fa566 100644 >>>>>> --- a/include/linux/leds.h >>>>>> +++ b/include/linux/leds.h >>>>>> @@ -48,6 +48,7 @@ struct led_classdev { >>>>>> #define LED_BLINK_DISABLE (1 << 21) >>>>>> #define LED_SYSFS_DISABLE (1 << 22) >>>>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>>>> +#define LED_BLINKING_HW (1 << 24) >>>>>> >>>>>> /* Set LED brightness level */ >>>>>> /* Must not sleep, use a workqueue if needed */ >>>>>> >>>>>>> >>>>>>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>>>>>> /* >>>>>>>> * If we need to disable soft blinking delegate this to >>>>>>>> the >>>>>>>> * work queue task to avoid problems in case we are >>>>>>>> called >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe >>>> linux-leds" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>> >>> >> >> > > ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-16 13:43 ` Tony Makkiel @ 2016-05-16 14:23 ` Jacek Anaszewski 2016-05-16 14:32 ` Jacek Anaszewski 0 siblings, 1 reply; 18+ messages in thread From: Jacek Anaszewski @ 2016-05-16 14:23 UTC (permalink / raw) To: Tony Makkiel; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 05/16/2016 03:43 PM, Tony Makkiel wrote: > > > On 16/05/16 10:21, Jacek Anaszewski wrote: >> Hi Tony, >> >> On 05/13/2016 04:20 PM, Tony Makkiel wrote: >>> >>> >>> On 12/05/16 11:26, Jacek Anaszewski wrote: >>>> On 05/11/2016 03:42 PM, Tony Makkiel wrote: >>>>> >>>>> >>>>> On 11/05/16 10:41, Jacek Anaszewski wrote: >>>>>> On 05/10/2016 06:55 PM, Tony Makkiel wrote: >>>>>>> >>>>>>> >>>>>>> On 10/05/16 14:26, Jacek Anaszewski wrote: >>>>>>>> On 05/10/2016 11:36 AM, Tony Makkiel wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 09/05/16 15:45, Jacek Anaszewski wrote: >>>>>>>>>> Hi Tony, >>>>>>>>>> >>>>>>>>>> On 05/09/2016 03:27 PM, Tony Makkiel wrote: >>>>>>>>>>> Hi Jacek, >>>>>>>>>>> Thank you for getting back. I updated my kernel to 4.5 and >>>>>>>>>>> have >>>>>>>>>>> the >>>>>>>>>>> updated "led_set_brightness" now. >>>>>>>>>>> >>>>>>>>>>> It sets >>>>>>>>>>> led_cdev->flags |= LED_BLINK_BRIGHTNESS_CHANGE; >>>>>>>>>>> led_cdev->blink_brightness = brightness; >>>>>>>>>>> >>>>>>>>>>> The new implementation requires hardware specific >>>>>>>>>>> drivers to >>>>>>>>>>> poll >>>>>>>>>>> for flag change. Shouldn't the led-core driver be calling the >>>>>>>>>>> hardware >>>>>>>>>>> specific brightness_set (led_set_brightness_nosleep) >>>>>>>>>>> irrespective of >>>>>>>>>>> the >>>>>>>>>>> blink settings? >>>>>>>>>>> >>>>>>>>>>> Unfortunately, it place additional requirement on drivers, to >>>>>>>>>>> implement >>>>>>>>>>> a polling mechanism which won't be needed otherwise. Why are the >>>>>>>>>>> brightness calls dependent on blink settings? >>>>>>>>>> >>>>>>>>>> If your question is still: >>>>>>>>>> >>>>>>>>>> "Is there a reason for rejecting brightness change requests when >>>>>>>>>> either of the blink_delays are set?" >>>>>>>>>> >>>>>>>>>> then the answer is: yes, brightness setting is deferred until >>>>>>>>>> the next timer tick to avoid avoid problems in case we are called >>>>>>>>>> from hard irq context. It should work fine for software blinking. >>>>>>>>> >>>>>>>>> >>>>>>>>> Sorry, was focused debugging 'hardware accelerated blink' on the >>>>>>>>> driver >>>>>>>>> I am working on, I missed the software blinking implementation. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Nonetheless, your question, made it obvious that we have a >>>>>>>>>> problem >>>>>>>>>> here in case of hardware accelerated blinking, i.e. drivers that >>>>>>>>>> implement blink_set op. Is this your use case? >>>>>>>>>> >>>>>>>>> >>>>>>>>> Yes, the brightness requests from user space >>>>>>>>> (/sys/class/leds/*/brightness) does not get passed to hardware >>>>>>>>> specific >>>>>>>>> driver via the blink_set implemented, unless led_cdev->flags is >>>>>>>>> polled. >>>>>>>>> >>>>>>>>>> Anyway, I've noticed a discrepancy between the LED core code and >>>>>>>>>> both Documentation/leds/leds-class.txt and comment over >>>>>>>>>> blink_set() op >>>>>>>>>> in include/linux/leds.h, which say that blinking is deactivated >>>>>>>>>> upon setting the brightness again. Many drivers apply this rule. >>>>>>>>>> >>>>>>>>>> In effect, LED_BLINK_BRIGHTNESS_CHANGE will have to be removed, >>>>>>>>>> and your question will be groundless, as changing the blink >>>>>>>>>> brightness should be impossible by design. >>>>>>>>>> >>>>>>>>> In my opinion, disabling blink, when brightness change requested >>>>>>>>> doesn't >>>>>>>>> sound like the right thing to do. There could be cases in which >>>>>>>>> the >>>>>>>>> brightness of the blinking LED needs to be changed. >>>>>>>> >>>>>>>> It could be accomplished with following sequence: >>>>>>>> >>>>>>>> $ echo LED_FULL > brightness //set brightness >>>>>>>> $ echo "timer" > trigger //enable blinking with >>>>>>>> brightness=LED_FULL >>>>>>>> $ echo 1 > brightness //stop blinking and set brightness >>>>>>>> to 1 >>>>>>>> $ echo "timer" > trigger //enable blinking with brightness=1 >>>>>>>> >>>>>>>> The only drawback here would be interfered blinking rhythm while >>>>>>>> resetting blink brightness. Most drivers that implement blink_set >>>>>>>> op observe what documentation says and disable blinking when >>>>>>>> new brightness is set. Unfortunately, led_set_brightness() after >>>>>>>> modifications doesn't take into account drivers that implement >>>>>>>> blink_set op. It needs to be fixed, i.e. made compatible with >>>>>>>> the docs. >>>>>>>> >>>>>>>>> Maybe we can let the >>>>>>>>> hardware driver deal with the blink request if it has implemented >>>>>>>>> brightness_set? The change below seem to work. >>>>>>>>> >>>>>>>>> >>>>>>>>> Subject: [PATCH] led-core: Use hardware blink when available >>>>>>>>> >>>>>>>>> At present hardware implemented brightness is not called >>>>>>>>> if blink delays are set. >>>>>>>>> >>>>>>>>> Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> >>>>>>>>> --- >>>>>>>>> drivers/leds/led-core.c | 4 +++- >>>>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>>>>>> index 19e1e60d..02dd0f6 100644 >>>>>>>>> --- a/drivers/leds/led-core.c >>>>>>>>> +++ b/drivers/leds/led-core.c >>>>>>>>> @@ -208,8 +208,10 @@ void led_set_brightness(struct led_classdev >>>>>>>>> *led_cdev, >>>>>>>>> /* >>>>>>>>> * In case blinking is on delay brightness setting >>>>>>>>> * until the next timer tick. >>>>>>>>> + * Or if brightness_set is defined, use the associated >>>>>>>>> implementation. >>>>>>>>> */ >>>>>>>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>>>>>> + if ((!led_cdev->brightness_set) && >>>>>>>> >>>>>>>> s/brightness_set/blink_set/ AFAICT >>>>>>>> >>>>>>>> It wouldn't cover all cases as the fact that a driver implements >>>>>>>> blink_set doesn't necessarily mean that hardware blinking is used >>>>>>>> for current blinking parameters. There are drivers that resort to >>>>>>>> using software fallback in case the LED controller device doesn't >>>>>>>> support requested blink intervals. >>>>>>>> >>>>>>>> I'm planning on addition of a LED_BLINKING_HW flag, that would >>>>>>>> be set after successful execution of blink_set op. It would >>>>>>>> allow to >>>>>>>> distinguish between hardware and software blinking modes reliably. >>>>>>>> >>>>>>> >>>>>>> Did you mean something like >>>>>>> >>>>>>> >>>>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>>>> index 19e1e60d..4a8b46d 100644 >>>>>>> --- a/drivers/leds/led-core.c >>>>>>> +++ b/drivers/leds/led-core.c >>>>>>> @@ -210,6 +210,13 @@ void led_set_brightness(struct led_classdev >>>>>>> *led_cdev, >>>>>>> * until the next timer tick. >>>>>>> */ >>>>>>> if (led_cdev->blink_delay_on || >>>>>>> led_cdev->blink_delay_off) { >>>>>>> + if (led_cdev->brightness_set) >>>>>>> + led_set_brightness_nosleep(led_cdev, >>>>>>> brightness); >>>>>> >>>>>> brightness_set is always initialized, probably you meant blink_set. >>>>> >>>>> Your solution from your last comment below sounds better (so probably >>>>> can skip reading this section). >>>>> >>>>> But no, It was not a mistake. Actually, copied from >>>>> 'led_set_brightness_nopm' in case to protect from any drivers which >>>>> doesn't define it. The change should follow existing architecture, and >>>>> was hoping to work for both existing and new drivers. >>>> >>>> Right, brightness_set can remain uninitialized while >>>> brightness_set_blocking is provided. >>>> >>>>> Existing Chip drivers: Note, the added brightness_set is called only >>>>> when the blink is active. The flag LED_BLINKING_HW won't be set, >>>>> either >>>>> because driver is not updated to include the flag, or driver wants >>>>> software fallback to deal with it. The problems I can think of is >>>>> >>>>> - the software flow will also call brightness_set later when the task >>>>> is serviced. But that is with the same values. So shouldn't make >>>>> difference to the user. >>>>> >>>>> New Drivers with brightness support while blinking:- They set >>>>> brightness >>>>> and the flag. They wont need the software fallback. If for any reason >>>>> brightness couldn't be set, flag is not set and normal procedure >>>>> applies. Again problem I see here is >>>>> >>>>> - Additional responsibility on chip drivers to set the flag, if >>>> >>>> Ah, now I understand your approach. >>>> >>>>> successfully managed to set brightness while blink is active. This >>>>> shouldn't be a problem on new drivers, as they might just set the flag >>>>> every time brightness change is requested, irrespective of blink >>>>> settings. If they don't set the flag, it falls back to the software >>>>> flow >>>>> as in existing architecture. >>>>> >>>>>> >>>>>>> + >>>>>>> + if (led_cdev->flags & LED_BLINKING_HW) { >>>>>>> + led_cdev->flags &= ~LED_BLINKING_HW; >>>>>>> + return; >>>>>>> + } >>>>>> >>>>>> The dependencies are quite versatile if we wanted to properly >>>>>> implement >>>>>> what documentation claims. Setting brightness to any value while >>>>>> blinking is on should stop blinking. It was so before the commit: >>>>>> >>>>>> 76931edd5 ('leds: fix brightness changing when software blinking is >>>>>> active') >>>>>> >>>>>> The problem was that timer trigger remained active after stopping >>>>>> blinking, which led us to changing the semantics on new brightness >>>>>> set, rather than solving the problem with unregistered trigger. >>>>>> This was also against documentation claims, which was overlooked. >>>>>> >>>>>> I tried yesterday to deactivate trigger on brightness set >>>>>> executed during blinking, but there are circular dependencies, >>>>>> since led_set_brightness(led_cdev, LED_OFF) is called on >>>>>> deactivation. >>>>>> It is also called from led_trigger_set in the trigger removal path. >>>>>> Generally it seems non-trivial to enforce current documentation >>>>>> claims >>>>>> in case of software blinking. >>>>>> >>>>>> The easiest thing to do now would be changing the semantics, so that >>>>>> only setting brightness to LED_OFF would disable the trigger, which >>>>>> in fact is true since few releases. The problem is that many drivers >>>>>> that implement hardware blinking resets it on any brightness change. >>>>>> We would have to left them intact, but apply a new semantics in the >>>>>> LED core, that would allow for new drivers to just update hardware >>>>>> blinking brightness upon updating the brightness. >>>>>> >>>>>> If we followed this path then the LED_BLINKING_HW flag would have to >>>>>> be set in led_blink_setup() after blink_set op returns 0. Thanks to >>>>>> that, we could distinguish in led_set_brightness whether hardware >>>>>> or software blinking is enabled. For !LED_BLINKING_HW case we would >>>>>> proceed as currently, i.e. set the LED_BLINK_BRIGHTNESS_CHANGE flag >>>>>> and defer brightness setting until next timer tick. For the opposite >>>>>> case we wouldn't do anything and let the led_set_brightness_nosleep() >>>>>> to call the appropriate brightness_set/brightness_set_blocking op. >>>>>> Old drivers would proceed as currently, by disabling blinking >>>>>> on brightness change, and new ones could apply new semantics by >>>>>> changing brightness but leaving hardware blinking active. >>>>>> >>>>> >>>>> This sounds better as we do not have to rely on drivers to set the >>>>> flag >>>>> and does not have the problems mentioned above. I tried the following >>>>> and works for my set up :) . >>>>> >>>>> >>>>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>>>> index 19e1e60d..3698b67 100644 >>>>> --- a/drivers/leds/led-core.c >>>>> +++ b/drivers/leds/led-core.c >>>>> @@ -141,8 +141,10 @@ static void led_blink_setup(struct led_classdev >>>>> *led_cdev, >>>>> { >>>>> if (!(led_cdev->flags & LED_BLINK_ONESHOT) && >>>>> led_cdev->blink_set && >>>>> - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) >>>>> + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ >>>>> + led_cdev->flags |= LED_BLINKING_HW; >>>>> return; >>>>> + } >>>>> >>>>> /* blink with 1 Hz as default if nothing specified */ >>>>> if (!*delay_on && !*delay_off) >>>>> @@ -209,7 +211,8 @@ void led_set_brightness(struct led_classdev >>>>> *led_cdev, >>>>> * In case blinking is on delay brightness setting >>>>> * until the next timer tick. >>>>> */ >>>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>>> + if (!(led_cdev->flags & LED_BLINKING_HW) && >>>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>>> /* >>>>> * If we need to disable soft blinking delegate >>>>> this to >>>>> the >>>>> * work queue task to avoid problems in case we are >>>>> called >>>> >>>> We would have to also clear the flag upon blink deactivation, i.e. >>>> when brightness to be set equals LED_OFF. Existing drivers that >>>> implement blink_set op and deactivate blinking on any brightness set >>>> would have to be modified to clear the LED_BLINKING_HW flag in their >>>> brightness_{set|set_blocking} ops. >>>> >>> >>> >>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c >>> index 19e1e60d..7a15035 100644 >>> --- a/drivers/leds/led-core.c >>> +++ b/drivers/leds/led-core.c >>> @@ -98,6 +98,9 @@ static void set_brightness_delayed(struct work_struct >>> *ws) >>> >>> led_cdev->delayed_set_value); >>> else >>> ret = -ENOTSUPP; >>> + >>> + if (led_cdev->delayed_set_value == LED_OFF) >>> + led_cdev->flags &= ~LED_BLINKING_HW; >>> if (ret < 0) >>> dev_err(led_cdev->dev, >>> "Setting an LED's brightness failed (%d)\n", >>> ret); >>> @@ -141,8 +144,10 @@ static void led_blink_setup(struct led_classdev >>> *led_cdev, >>> { >>> if (!(led_cdev->flags & LED_BLINK_ONESHOT) && >>> led_cdev->blink_set && >>> - !led_cdev->blink_set(led_cdev, delay_on, delay_off)) >>> + !led_cdev->blink_set(led_cdev, delay_on, delay_off)){ >>> + led_cdev->flags |= LED_BLINKING_HW; >>> return; >>> + } >>> >>> /* blink with 1 Hz as default if nothing specified */ >>> if (!*delay_on && !*delay_off) >>> @@ -209,7 +214,8 @@ void led_set_brightness(struct led_classdev >>> *led_cdev, >>> * In case blinking is on delay brightness setting >>> * until the next timer tick. >>> */ >>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>> + if (!(led_cdev->flags & LED_BLINKING_HW) && >>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>> /* >>> * If we need to disable soft blinking delegate this to >>> the >>> * work queue task to avoid problems in case we are >>> called >>> @@ -235,6 +241,8 @@ void led_set_brightness_nopm(struct led_classdev >>> *led_cdev, >>> /* Use brightness_set op if available, it is guaranteed not to >>> sleep */ >>> if (led_cdev->brightness_set) { >>> led_cdev->brightness_set(led_cdev, value); >>> + if (value == LED_OFF) >>> + led_cdev->flags &= ~LED_BLINKING_HW; >>> return; >>> } >>> >>> @@ -267,6 +275,9 @@ int led_set_brightness_sync(struct led_classdev >>> *led_cdev, >>> if (led_cdev->flags & LED_SUSPENDED) >>> return 0; >>> >>> + if (value == LED_OFF) >>> + led_cdev->flags &= ~LED_BLINKING_HW; >>> + >>> if (led_cdev->brightness_set_blocking) >>> return led_cdev->brightness_set_blocking(led_cdev, >>> >>> led_cdev->brightness); >>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>> index bc1476f..f5fa566 100644 >>> --- a/include/linux/leds.h >>> +++ b/include/linux/leds.h >>> @@ -48,6 +48,7 @@ struct led_classdev { >>> #define LED_BLINK_DISABLE (1 << 21) >>> #define LED_SYSFS_DISABLE (1 << 22) >>> #define LED_DEV_CAP_FLASH (1 << 23) >>> +#define LED_BLINKING_HW (1 << 24) >>> >>> /* Set LED brightness level */ >>> /* Must not sleep, use a workqueue if needed */ >>> ~ >>> >>> >>>>> diff --git a/include/linux/leds.h b/include/linux/leds.h >>>>> index bc1476f..f5fa566 100644 >>>>> --- a/include/linux/leds.h >>>>> +++ b/include/linux/leds.h >>>>> @@ -48,6 +48,7 @@ struct led_classdev { >>>>> #define LED_BLINK_DISABLE (1 << 21) >>>>> #define LED_SYSFS_DISABLE (1 << 22) >>>>> #define LED_DEV_CAP_FLASH (1 << 23) >>>>> +#define LED_BLINKING_HW (1 << 24) >>>>> >>>>> /* Set LED brightness level */ >>>>> /* Must not sleep, use a workqueue if needed */ >>>>> >>>>>> I wonder if it would be safe to rely on timer_pending() instead of >>>>>> introducing LED_BLINKING_HW flag... >>>>>> >>>>> How would that work? I am assuming this has something to do with >>>>> software blink? Does it take hardware blink to account? >>>> >>>> This way we could distinguish between software and hardware blinking. >>>> It wouldn't require modifications in drivers: >>>> >>>> - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { >>>> + if (timer_pending(&led_cdev->blink_timer) && >>>> + (led_cdev->blink_delay_on || led_cdev->blink_delay_off)) { >>>> /* >>>> * If we need to disable soft blinking delegate this >>>> >>>> It'd must be verified however if it isn't possible that >>>> led_set_brightness is called when timer is already expired, >>>> between two ticks. >>> >>> if blink_set is successful, would work without problem, >>> >>> When blink_set successful : The timer wont be triggered resulting the >>> function to return null all the time. --> No problem here >>> >>> Software blink : I do not have hardware to actually test this case. I >>> tried simulating the case.But going through the code. Following are my >>> understanding. >> >> You can test it by leaving blink_set op uninitialized in your driver. >> >>> >>> Timer Active (Most of the time) : Work as normal. --> No problem here >>> >>> Timer Expired :led_set_brightness_nopm called. >>> - Case in which brightness == LED_OFF, LED will be turned off. >>> led_cdev->blink_delay_on and delay_off will retain its value. The timer >>> will keep on running. So it will re-enable back blink. --> LED_OFF will >>> be ignored. >>> >>> - Case in which brightness != LED_OFF, new brightness set and resume >>> normal operation. --> No problem here >>> >> >> Thanks for this analysis. I have a new idea - wouldn't it be more robust >> if we added LED_BLINKING_SW flag and set it in led_set_software_blink()? >> >> The line >> >> if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) >> >> in led_set_brightness() could be then changed to >> >> if (led_cdev->flags & LED_BLINK_SW) . >> >> LED_BLINK_SW flag would have to be cleared in led_stop_software_blink() >> and in the first two conditions in the led_timer_function(). >> > > Yes, that will do with minimal changes. I tested the following, and works. Fine, so could you please submit the patch officially? Before that, please rebase your code on top of LED tree or linux-next and change LED_BLINKING_SW to LED_BLINK_SW, to keep the same prefix for each blinking related definition. > Date: Mon, 16 May 2016 14:18:42 +0100 > Subject: [PATCH 1/1] Allow chip-driver to set brightness if, software > blink not used. > > If software blink were active any brightness change request > were not sent to the chip driver. > > Signed-off-by: Tony Makkiel <tony.makkiel@daqri.com> > --- > drivers/leds/led-core.c | 7 +++++-- > include/linux/leds.h | 1 + > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c > index 19e1e60d..376b5ea 100644 > --- a/drivers/leds/led-core.c > +++ b/drivers/leds/led-core.c > @@ -33,11 +33,12 @@ static void led_timer_function(unsigned long data) > > if (!led_cdev->blink_delay_on || !led_cdev->blink_delay_off) { > led_set_brightness_nosleep(led_cdev, LED_OFF); > + led_cdev->flags &= ~LED_BLINKING_SW; > return; > } > > if (led_cdev->flags & LED_BLINK_ONESHOT_STOP) { > - led_cdev->flags &= ~LED_BLINK_ONESHOT_STOP; > + led_cdev->flags &= ~(LED_BLINK_ONESHOT_STOP | LED_BLINKING_SW); > return; > } > > @@ -131,6 +132,7 @@ static void led_set_software_blink(struct > led_classdev *led_cdev, > return; > } > > + led_cdev->flags |= LED_BLINKING_SW; > mod_timer(&led_cdev->blink_timer, jiffies + 1); > } > > @@ -199,6 +201,7 @@ void led_stop_software_blink(struct led_classdev > *led_cdev) > del_timer_sync(&led_cdev->blink_timer); > led_cdev->blink_delay_on = 0; > led_cdev->blink_delay_off = 0; > + led_cdev->flags &= ~LED_BLINKING_SW; > } > EXPORT_SYMBOL_GPL(led_stop_software_blink); > > @@ -209,7 +212,7 @@ void led_set_brightness(struct led_classdev *led_cdev, > * In case blinking is on delay brightness setting > * until the next timer tick. > */ > - if (led_cdev->blink_delay_on || led_cdev->blink_delay_off) { > + if (led_cdev->flags & LED_BLINKING_SW) { > /* > * If we need to disable soft blinking delegate this to the > * work queue task to avoid problems in case we are called > diff --git a/include/linux/leds.h b/include/linux/leds.h > index bc1476f..08ef6f4 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -48,6 +48,7 @@ struct led_classdev { > #define LED_BLINK_DISABLE (1 << 21) > #define LED_SYSFS_DISABLE (1 << 22) > #define LED_DEV_CAP_FLASH (1 << 23) > +#define LED_BLINKING_SW (1 << 24) > > /* Set LED brightness level */ > /* Must not sleep, use a workqueue if needed */ The above line looks different in the recent code. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Brightness control irrespective of blink state. 2016-05-16 14:23 ` Jacek Anaszewski @ 2016-05-16 14:32 ` Jacek Anaszewski 0 siblings, 0 replies; 18+ messages in thread From: Jacek Anaszewski @ 2016-05-16 14:32 UTC (permalink / raw) To: Tony Makkiel; +Cc: Linux LED Subsystem, Stas Sergeev, Pavel Machek, lkml On 05/16/2016 04:23 PM, Jacek Anaszewski wrote: > On 05/16/2016 03:43 PM, Tony Makkiel wrote: if (led_cdev->flags & LED_BLINK_SW) . >>> >>> LED_BLINK_SW flag would have to be cleared in led_stop_software_blink() >>> and in the first two conditions in the led_timer_function(). >>> >> >> Yes, that will do with minimal changes. I tested the following, and >> works. > > Fine, so could you please submit the patch officially? > Before that, please rebase your code on top of LED tree or linux-next > and change LED_BLINKING_SW to LED_BLINK_SW, to keep the same prefix for > each blinking related definition. Also please put it before LED_BLINK_ONESHOT. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 2/3] devicetree: leds: Introduce "panic-indicator" optional property 2016-04-28 22:03 [PATCH v3 0/3] Extend the LED panic trigger Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic Ezequiel Garcia @ 2016-04-28 22:03 ` Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 3/3] leds: gpio: Support the "panic-indicator" firmware property Ezequiel Garcia ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: Ezequiel Garcia @ 2016-04-28 22:03 UTC (permalink / raw) To: linux-leds, linux-kernel, linux-arm-kernel, devicetree Cc: Richard Purdie, Jacek Anaszewski, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek, Ezequiel Garcia It's desirable to specify which LEDs are to be blinked on a kernel panic. Therefore, introduce a devicetree boolean property to mark which LEDs should be treated this way, if possible. Acked-by: Rob Herring <rob@kernel.org> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> --- Documentation/devicetree/bindings/leds/common.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 68419843e32f..af10678ea2f6 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -37,6 +37,9 @@ Optional properties for child nodes: property is mandatory for the LEDs in the non-flash modes (e.g. torch or indicator). +- panic-indicator : This property specifies that the LED should be used, + if at all possible, as a panic indicator. + Required properties for flash LED child nodes: - flash-max-microamp : Maximum flash LED supply current in microamperes. - flash-max-timeout-us : Maximum timeout in microseconds after which the flash -- 2.7.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 3/3] leds: gpio: Support the "panic-indicator" firmware property 2016-04-28 22:03 [PATCH v3 0/3] Extend the LED panic trigger Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 2/3] devicetree: leds: Introduce "panic-indicator" optional property Ezequiel Garcia @ 2016-04-28 22:03 ` Ezequiel Garcia 2016-05-03 16:53 ` Rob Herring 2016-04-28 22:22 ` [PATCH v3 0/3] Extend the LED panic trigger Pavel Machek 2016-04-29 18:57 ` Matthias Brugger 4 siblings, 1 reply; 18+ messages in thread From: Ezequiel Garcia @ 2016-04-28 22:03 UTC (permalink / raw) To: linux-leds, linux-kernel, linux-arm-kernel, devicetree Cc: Richard Purdie, Jacek Anaszewski, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring, Pavel Machek, Ezequiel Garcia Calling a GPIO LEDs is quite likely to work even if the kernel has paniced, so they are ideal to blink in this situation. This commit adds support for the new "panic-indicator" firmware property, allowing to mark a given LED to blink on a kernel panic. Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> --- Documentation/devicetree/bindings/leds/leds-gpio.txt | 2 ++ drivers/leds/leds-gpio.c | 4 ++++ include/linux/leds.h | 1 + 3 files changed, 7 insertions(+) diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt index fea1ebfe24a9..cbbeb1850910 100644 --- a/Documentation/devicetree/bindings/leds/leds-gpio.txt +++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt @@ -23,6 +23,8 @@ LED sub-node properties: property is not present. - retain-state-suspended: (optional) The suspend state can be retained.Such as charge-led gpio. +- panic-indicator : (optional) + see Documentation/devicetree/bindings/leds/common.txt Examples: diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index 61143f55597e..8229f063b483 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -127,6 +127,8 @@ static int create_gpio_led(const struct gpio_led *template, led_dat->cdev.brightness = state ? LED_FULL : LED_OFF; if (!template->retain_state_suspended) led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME; + if (template->panic_indicator) + led_dat->cdev.flags |= LED_PANIC_INDICATOR; ret = gpiod_direction_output(led_dat->gpiod, state); if (ret < 0) @@ -200,6 +202,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev) if (fwnode_property_present(child, "retain-state-suspended")) led.retain_state_suspended = 1; + if (fwnode_property_present(child, "panic-indicator")) + led.panic_indicator = 1; ret = create_gpio_led(&led, &priv->leds[priv->num_leds], dev, NULL); diff --git a/include/linux/leds.h b/include/linux/leds.h index 7e9fb00e15e8..d2b13066e781 100644 --- a/include/linux/leds.h +++ b/include/linux/leds.h @@ -365,6 +365,7 @@ struct gpio_led { unsigned gpio; unsigned active_low : 1; unsigned retain_state_suspended : 1; + unsigned panic_indicator : 1; unsigned default_state : 2; /* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */ struct gpio_desc *gpiod; -- 2.7.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 3/3] leds: gpio: Support the "panic-indicator" firmware property 2016-04-28 22:03 ` [PATCH v3 3/3] leds: gpio: Support the "panic-indicator" firmware property Ezequiel Garcia @ 2016-05-03 16:53 ` Rob Herring 0 siblings, 0 replies; 18+ messages in thread From: Rob Herring @ 2016-05-03 16:53 UTC (permalink / raw) To: Ezequiel Garcia Cc: linux-leds, linux-kernel, linux-arm-kernel, devicetree, Richard Purdie, Jacek Anaszewski, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Pavel Machek On Thu, Apr 28, 2016 at 07:03:40PM -0300, Ezequiel Garcia wrote: > Calling a GPIO LEDs is quite likely to work even if the kernel > has paniced, so they are ideal to blink in this situation. > This commit adds support for the new "panic-indicator" > firmware property, allowing to mark a given LED to blink on > a kernel panic. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > Documentation/devicetree/bindings/leds/leds-gpio.txt | 2 ++ Acked-by: Rob Herring <robh@kernel.org> > drivers/leds/leds-gpio.c | 4 ++++ > include/linux/leds.h | 1 + > 3 files changed, 7 insertions(+) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/3] Extend the LED panic trigger 2016-04-28 22:03 [PATCH v3 0/3] Extend the LED panic trigger Ezequiel Garcia ` (2 preceding siblings ...) 2016-04-28 22:03 ` [PATCH v3 3/3] leds: gpio: Support the "panic-indicator" firmware property Ezequiel Garcia @ 2016-04-28 22:22 ` Pavel Machek 2016-04-29 18:57 ` Matthias Brugger 4 siblings, 0 replies; 18+ messages in thread From: Pavel Machek @ 2016-04-28 22:22 UTC (permalink / raw) To: Ezequiel Garcia Cc: linux-leds, linux-kernel, linux-arm-kernel, devicetree, Richard Purdie, Jacek Anaszewski, Kumar Gala, Ian Campbell, Mark Rutland, Pawel Moll, Rob Herring On Thu 2016-04-28 19:03:37, Ezequiel Garcia wrote: > As per commit 916fe619951f ("leds: trigger: Introduce a kernel > panic LED trigger"), the kernel now supports a new LED trigger > to hook on the panic blink. > > However, the only way of using this is to dedicate a LED device, > making it rather useless. > > To overcome this limitation, the present series introduces the > capability to switch the LED trigger of certain LED devices upon > a kernel panic (using the panic notifier). > > The decision of which LEDs should be switched to the panic trigger > is left to each LED device driver. As an example, a devicetree > boolean property is introduced and used in the leds-gpio driver. > > The big change in this v3 is that I've moved the panic trigger > switching away from the core code and it's now part of > ledtrig-panic.c. Pavel, Jacek: How does it look? Seems better now. Thanks for doing that. For the series: Acked-by: Pavel Machek <pavel@ucw.cz> Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 0/3] Extend the LED panic trigger 2016-04-28 22:03 [PATCH v3 0/3] Extend the LED panic trigger Ezequiel Garcia ` (3 preceding siblings ...) 2016-04-28 22:22 ` [PATCH v3 0/3] Extend the LED panic trigger Pavel Machek @ 2016-04-29 18:57 ` Matthias Brugger 4 siblings, 0 replies; 18+ messages in thread From: Matthias Brugger @ 2016-04-29 18:57 UTC (permalink / raw) To: Ezequiel Garcia, linux-leds, linux-kernel, linux-arm-kernel, devicetree Cc: Mark Rutland, Pawel Moll, Ian Campbell, Rob Herring, Richard Purdie, Pavel Machek, Kumar Gala, Jacek Anaszewski On 29/04/16 00:03, Ezequiel Garcia wrote: > As per commit 916fe619951f ("leds: trigger: Introduce a kernel > panic LED trigger"), the kernel now supports a new LED trigger > to hook on the panic blink. > > However, the only way of using this is to dedicate a LED device, > making it rather useless. > > To overcome this limitation, the present series introduces the > capability to switch the LED trigger of certain LED devices upon > a kernel panic (using the panic notifier). > > The decision of which LEDs should be switched to the panic trigger > is left to each LED device driver. As an example, a devicetree > boolean property is introduced and used in the leds-gpio driver. > > The big change in this v3 is that I've moved the panic trigger > switching away from the core code and it's now part of > ledtrig-panic.c. Pavel, Jacek: How does it look? > > Changes from v2: > > * Added Rob's "panic-indicator" devicetree property Acked-by. > > * Fix typo, as pointed out by Robin Murphy. > > * Documented "panic-indicator" in bindings/leds/leds-gpio.txt. > > * Moved the panic trigger switching from the trigger core code, > to ledtrig-panic.c. > > Changes from v1: > > * Dropped the led_trigger_event_nosleep API, and instead just > clear the blink_delay_{on, off} when the panic is notified. > This results in less changes. > > * Changed the flag to LED_PANIC_INDICATOR, as requested by Jacek. > > * Changed the firmware property name to "panic-indicator", as > requested by Jacek. > > Ezequiel Garcia (3): > leds: triggers: Allow to switch the trigger to "panic" on a kernel > panic > devicetree: leds: Introduce "panic-indicator" optional property > leds: gpio: Support the "panic-indicator" firmware property > > Documentation/devicetree/bindings/leds/common.txt | 3 ++ > .../devicetree/bindings/leds/leds-gpio.txt | 2 + > drivers/leds/led-triggers.c | 2 +- > drivers/leds/leds-gpio.c | 4 ++ > drivers/leds/leds.h | 1 + > drivers/leds/trigger/Kconfig | 3 ++ > drivers/leds/trigger/ledtrig-panic.c | 47 ++++++++++++++++++++++ > include/linux/leds.h | 2 + > 8 files changed, 63 insertions(+), 1 deletion(-) > Looks good to me. Reviewed-by: Matthias Brugger <mbrugger@suse.com> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-05-16 14:32 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-28 22:03 [PATCH v3 0/3] Extend the LED panic trigger Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 1/3] leds: triggers: Allow to switch the trigger to "panic" on a kernel panic Ezequiel Garcia 2016-04-29 7:20 ` Jacek Anaszewski 2016-05-06 9:03 ` Jacek Anaszewski 2016-05-06 13:05 ` Ezequiel Garcia [not found] ` <572CE1B0.8040001@daqri.com> [not found] ` <572CE715.6060504@gmail.com> [not found] ` <57309039.3060305@daqri.com> [not found] ` <5730A293.9050209@samsung.com> [not found] ` <5731ABB6.10607@daqri.com> [not found] ` <5731E194.1010004@samsung.com> [not found] ` <57321299.8090603@daqri.com> 2016-05-11 9:41 ` Brightness control irrespective of blink state Jacek Anaszewski 2016-05-11 13:42 ` Tony Makkiel 2016-05-12 10:26 ` Jacek Anaszewski 2016-05-13 14:20 ` Tony Makkiel 2016-05-16 9:21 ` Jacek Anaszewski 2016-05-16 13:43 ` Tony Makkiel 2016-05-16 14:23 ` Jacek Anaszewski 2016-05-16 14:32 ` Jacek Anaszewski 2016-04-28 22:03 ` [PATCH v3 2/3] devicetree: leds: Introduce "panic-indicator" optional property Ezequiel Garcia 2016-04-28 22:03 ` [PATCH v3 3/3] leds: gpio: Support the "panic-indicator" firmware property Ezequiel Garcia 2016-05-03 16:53 ` Rob Herring 2016-04-28 22:22 ` [PATCH v3 0/3] Extend the LED panic trigger Pavel Machek 2016-04-29 18:57 ` Matthias Brugger
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).