* [GIT PULL] LED updates
@ 2008-02-07 10:35 Richard Purdie
2008-02-07 21:38 ` Henrique de Moraes Holschuh
0 siblings, 1 reply; 17+ messages in thread
From: Richard Purdie @ 2008-02-07 10:35 UTC (permalink / raw)
To: Linus Torvalds; +Cc: LKML
Linus,
Could you please pull from:
git://git.o-hand.com/linux-rpurdie-leds for-linus
for the LED tree updates for 2.6.25. This includes a couple of new
drivers, removal of a now superseded driver, some extensions to some
existing drivers and some LED class enhancements/bugfixes. Most of this
has been testing in -mm for quite a while.
Thanks, Richard
Documentation/leds-class.txt | 29 +++-
arch/arm/mach-ixp4xx/dsmg600-setup.c | 4
arch/arm/mach-ixp4xx/nas100d-setup.c | 6
arch/arm/mach-ixp4xx/nslu2-setup.c | 8 -
drivers/hwmon/applesmc.c | 2
drivers/input/misc/wistron_btns.c | 4
drivers/leds/Kconfig | 48 ++++++-
drivers/leds/Makefile | 3
drivers/leds/leds-ams-delta.c | 12 -
drivers/leds/leds-clevo-mail.c | 219 +++++++++++++++++++++++++++++++++++
drivers/leds/leds-corgi.c | 4
drivers/leds/leds-gpio.c | 2
drivers/leds/leds-hp6xx.c | 120 +++++++++++++++++++
drivers/leds/leds-ixp4xx-gpio.c | 214 ----------------------------------
drivers/leds/leds-locomo.c | 4
drivers/leds/leds-net48xx.c | 2
drivers/leds/leds-spitz.c | 8 -
drivers/leds/leds-tosa.c | 4
drivers/leds/leds-wrap.c | 47 ++++++-
drivers/leds/ledtrig-timer.c | 41 +++++-
drivers/misc/asus-laptop.c | 2
drivers/net/wireless/b43/leds.c | 8 -
include/linux/leds.h | 5
23 files changed, 519 insertions(+), 277 deletions(-)
Kristoffer Ericson (1):
leds: Add HP Jornada 6xx driver
Michael Loeffler (1):
leds: Add power LED to the wrap driver
Márton Németh (3):
leds: Add clevo notebook LED driver
leds: Add support for hardware accelerated LED flashing
leds: hw acceleration for Clevo mail LED driver
Raphael Assenat (1):
leds: Fix led-gpio active_low default brightness
Richard Purdie (1):
leds: Standardise LED naming scheme
Rod Whitby (1):
leds: Remove the now uneeded ixp4xx driver
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [GIT PULL] LED updates 2008-02-07 10:35 [GIT PULL] LED updates Richard Purdie @ 2008-02-07 21:38 ` Henrique de Moraes Holschuh 2008-02-07 22:13 ` Richard Purdie 0 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-02-07 21:38 UTC (permalink / raw) To: Richard Purdie, Márton Németh; +Cc: LKML On Thu, 07 Feb 2008, Richard Purdie wrote: > Márton Németh: > leds: Add support for hardware accelerated LED flashing > leds: hw acceleration for Clevo mail LED driver This one has a loose end: when you call brightness_set on a led with hardware flash acceleration, you will leave the trigger armed, BUT the led won't blink anymore. That's just wrong. Either we should always remove *any* (hardware accelerated or not!) active trigger when a write to brightness_set is done, or the stuff about "calling brightness_set will disable the hardware accelerated blink" has to go. I personally prefer that we would always remove any active trigger if brightness_set is to be called. IMHO, it is neater, and it is also the least-surprise-behaviour from an user perspective with the LED_OFF:LED_FULL triggers we have right now. Which one will be? If it is "remove any active trigger", I'd not mind writing the patch. > Richard Purdie: > leds: Standardise LED naming scheme This one causes trouble (at least on 2.6.23 -- I backported the patch) due to the 20-byte length limit on sysfs names. I had to use "tp::<somecrap>" instead of "thinkpad::<somecrap>" to name LEDs, and still had to reduce ultrabase_battery to ultrabase_batt :-) Anyway, IMHO, the LED function should come first, and we should not even need the led driver name anywhere. In case of clashes in the class sysfs dir, just tack a .# to the end or somesuch. The device the LED is tied to already differentiates them. That would save a lot of chars for something much more useful (the function). -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-02-07 21:38 ` Henrique de Moraes Holschuh @ 2008-02-07 22:13 ` Richard Purdie 2008-02-07 23:23 ` [PATCH] leds: disable triggers on brightness set Henrique de Moraes Holschuh ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Richard Purdie @ 2008-02-07 22:13 UTC (permalink / raw) To: Henrique de Moraes Holschuh; +Cc: Márton Németh, LKML On Thu, 2008-02-07 at 19:38 -0200, Henrique de Moraes Holschuh wrote: > On Thu, 07 Feb 2008, Richard Purdie wrote: > > Márton Németh: > > leds: Add support for hardware accelerated LED flashing > > leds: hw acceleration for Clevo mail LED driver > > This one has a loose end: when you call brightness_set on a led with > hardware flash acceleration, you will leave the trigger armed, BUT the led > won't blink anymore. That's just wrong. Agreed. > Either we should always remove *any* (hardware accelerated or not!) active > trigger when a write to brightness_set is done, or the stuff about "calling > brightness_set will disable the hardware accelerated blink" has to go. > > I personally prefer that we would always remove any active trigger if > brightness_set is to be called. IMHO, it is neater, and it is also the > least-surprise-behaviour from an user perspective with the LED_OFF:LED_FULL > triggers we have right now. Even without the hardware acceleration, a user write to set_brightness leaves any active trigger active and isn't really intuitive or right either. > Which one will be? If it is "remove any active trigger", I'd not mind > writing the patch. I'll accept a patch for that :). > > Richard Purdie: > > leds: Standardise LED naming scheme > > This one causes trouble (at least on 2.6.23 -- I backported the patch) due > to the 20-byte length limit on sysfs names. I had to use "tp::<somecrap>" > instead of "thinkpad::<somecrap>" to name LEDs, and still had to reduce > ultrabase_battery to ultrabase_batt :-) > > Anyway, IMHO, the LED function should come first, and we should not even > need the led driver name anywhere. In case of clashes in the class sysfs > dir, just tack a .# to the end or somesuch. The device the LED is tied to > already differentiates them. That would save a lot of chars for something > much more useful (the function). Ouch, I'm looking into this. I wish I'd known about it earlier. I agree function is more important but didn't want to break the existing convention. I guess this limitation comes from the kobjects involved... Richard ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] leds: disable triggers on brightness set 2008-02-07 22:13 ` Richard Purdie @ 2008-02-07 23:23 ` Henrique de Moraes Holschuh 2008-02-10 11:52 ` Németh Márton 2008-02-08 7:03 ` [GIT PULL] LED updates Németh Márton 2008-03-16 18:18 ` Henrique de Moraes Holschuh 2 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-02-07 23:23 UTC (permalink / raw) To: Richard Purdie; +Cc: Márton Németh, LKML Disable any active triggers when someone writes to the brightness attribute. This fixes also a misbehaviour with hardware accelerated flashing. Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> Cc: Márton Németh <nm127@freemail.hu> Cc: Richard Purdie <rpurdie@rpsys.net> --- drivers/leds/led-class.c | 2 ++ drivers/leds/led-triggers.c | 12 +++++++++--- drivers/leds/leds.h | 2 ++ 3 files changed, 13 insertions(+), 3 deletions(-) The led class is not even close to checkpath-clean, so I have followed the led class style instead of the kernel style. You'll probably have to fix the style of the entire stuff under drivers/leds/ eventually. diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 64c66b3..835f939 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -51,6 +51,8 @@ static ssize_t led_brightness_store(struct device *dev, if (count == size) { ret = count; + + led_trigger_remove(led_cdev); led_set_brightness(led_cdev, state); } diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index 13c9026..21dd969 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -45,9 +45,7 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, trigger_name[len - 1] = '\0'; if (!strcmp(trigger_name, "none")) { - down_write(&led_cdev->trigger_lock); - led_trigger_set(led_cdev, NULL); - up_write(&led_cdev->trigger_lock); + led_trigger_remove(led_cdev); return count; } @@ -139,6 +137,13 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger) led_cdev->trigger = trigger; } +void led_trigger_remove(struct led_classdev *led_cdev) +{ + down_write(&led_cdev->trigger_lock); + led_trigger_set(led_cdev, NULL); + up_write(&led_cdev->trigger_lock); +} + void led_trigger_set_default(struct led_classdev *led_cdev) { struct led_trigger *trig; @@ -231,6 +236,7 @@ void led_trigger_unregister_simple(struct led_trigger *trigger) /* Used by LED Class */ EXPORT_SYMBOL_GPL(led_trigger_set); +EXPORT_SYMBOL_GPL(led_trigger_remove); EXPORT_SYMBOL_GPL(led_trigger_set_default); EXPORT_SYMBOL_GPL(led_trigger_show); EXPORT_SYMBOL_GPL(led_trigger_store); diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h index 12b6fe9..b618256 100644 --- a/drivers/leds/leds.h +++ b/drivers/leds/leds.h @@ -34,9 +34,11 @@ extern struct list_head leds_list; void led_trigger_set_default(struct led_classdev *led_cdev); void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger); +void led_trigger_remove(struct led_classdev *led_cdev); #else #define led_trigger_set_default(x) do {} while(0) #define led_trigger_set(x, y) do {} while(0) +#define led_trigger_remove(x) do {} while(0) #endif ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, -- 1.5.3.8 -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] leds: disable triggers on brightness set 2008-02-07 23:23 ` [PATCH] leds: disable triggers on brightness set Henrique de Moraes Holschuh @ 2008-02-10 11:52 ` Németh Márton 2008-02-17 23:30 ` Richard Purdie 0 siblings, 1 reply; 17+ messages in thread From: Németh Márton @ 2008-02-10 11:52 UTC (permalink / raw) To: Henrique de Moraes Holschuh; +Cc: Richard Purdie, LKML Disable any active triggers when the brightness attribute is set to zero. Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> Signed-off-by: Márton Németh <nm127@freemail.hu> Cc: Richard Purdie <rpurdie@rpsys.net> --- diff --git a/Documentation/leds-class.txt b/Documentation/leds-class.txt index 56757c7..d2aebfb 100644 --- a/Documentation/leds-class.txt +++ b/Documentation/leds-class.txt @@ -19,6 +19,12 @@ optimises away. Complex triggers whilst available to all LEDs have LED specific parameters and work on a per LED basis. The timer trigger is an example. +The timer trigger will periodically change the LED brightness between +LED_OFF and the current brightness setting. The "on" and "off" time can +be specified via /sys/class/leds/<device>/delay_{on,off} in milliseconds. +You can change the brightness value of a LED independently of the timer +trigger. However, if you set the brightness value to LED_OFF it will +also disable the timer trigger. You can change triggers in a similar manner to the way an IO scheduler is chosen (via /sys/class/leds/<device>/trigger). Trigger specific @@ -63,9 +69,9 @@ value if it is called with *delay_on==0 && *delay_off==0 parameters. In this case the driver should give back the chosen value through delay_on and delay_off parameters to the leds subsystem. -Any call to the brightness_set() callback function should cancel the -previously programmed hardware blinking function so setting the brightness -to 0 can also cancel the blinking of the LED. +Setting the brightness to zero with brightness_set() callback function +should completely turn off the LED and cancel the previously programmed +hardware blinking function, if any. Known Issues diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c index 4a93878..e6c5b98 100644 --- a/drivers/leds/led-class.c +++ b/drivers/leds/led-class.c @@ -51,6 +51,9 @@ static ssize_t led_brightness_store(struct device *dev, if (count == size) { ret = count; + + if (state == LED_OFF) + led_trigger_remove(led_cdev); led_set_brightness(led_cdev, state); } diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c index 13c9026..21dd969 100644 --- a/drivers/leds/led-triggers.c +++ b/drivers/leds/led-triggers.c @@ -45,9 +45,7 @@ ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, trigger_name[len - 1] = '\0'; if (!strcmp(trigger_name, "none")) { - down_write(&led_cdev->trigger_lock); - led_trigger_set(led_cdev, NULL); - up_write(&led_cdev->trigger_lock); + led_trigger_remove(led_cdev); return count; } @@ -139,6 +137,13 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger) led_cdev->trigger = trigger; } +void led_trigger_remove(struct led_classdev *led_cdev) +{ + down_write(&led_cdev->trigger_lock); + led_trigger_set(led_cdev, NULL); + up_write(&led_cdev->trigger_lock); +} + void led_trigger_set_default(struct led_classdev *led_cdev) { struct led_trigger *trig; @@ -231,6 +236,7 @@ void led_trigger_unregister_simple(struct led_trigger *trigger) /* Used by LED Class */ EXPORT_SYMBOL_GPL(led_trigger_set); +EXPORT_SYMBOL_GPL(led_trigger_remove); EXPORT_SYMBOL_GPL(led_trigger_set_default); EXPORT_SYMBOL_GPL(led_trigger_show); EXPORT_SYMBOL_GPL(led_trigger_store); diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h index 12b6fe9..0214799 100644 --- a/drivers/leds/leds.h +++ b/drivers/leds/leds.h @@ -27,6 +27,11 @@ static inline void led_set_brightness(struct led_classdev *led_cdev, led_cdev->brightness_set(led_cdev, value); } +static inline int led_get_brightness(struct led_classdev *led_cdev) +{ + return led_cdev->brightness; +} + extern struct rw_semaphore leds_list_lock; extern struct list_head leds_list; @@ -34,9 +39,11 @@ extern struct list_head leds_list; void led_trigger_set_default(struct led_classdev *led_cdev); void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger); +void led_trigger_remove(struct led_classdev *led_cdev); #else #define led_trigger_set_default(x) do {} while(0) #define led_trigger_set(x, y) do {} while(0) +#define led_trigger_remove(x) do {} while(0) #endif ssize_t led_trigger_store(struct device *dev, struct device_attribute *attr, diff --git a/drivers/leds/ledtrig-timer.c b/drivers/leds/ledtrig-timer.c index 82c55d6..7062977 100644 --- a/drivers/leds/ledtrig-timer.c +++ b/drivers/leds/ledtrig-timer.c @@ -25,6 +25,9 @@ #include "leds.h" struct timer_trig_data { + int brightness_on; /* LED brightness during "on" period. + * (LED_OFF < brightness_on <= LED_FULL) + */ unsigned long delay_on; /* milliseconds on */ unsigned long delay_off; /* milliseconds off */ struct timer_list timer; @@ -34,17 +37,26 @@ static void led_timer_function(unsigned long data) { struct led_classdev *led_cdev = (struct led_classdev *) data; struct timer_trig_data *timer_data = led_cdev->trigger_data; - unsigned long brightness = LED_OFF; - unsigned long delay = timer_data->delay_off; + unsigned long brightness; + unsigned long delay; if (!timer_data->delay_on || !timer_data->delay_off) { led_set_brightness(led_cdev, LED_OFF); return; } - if (!led_cdev->brightness) { - brightness = LED_FULL; + brightness = led_get_brightness(led_cdev); + if (!brightness) { + /* Time to switch the LED on. */ + brightness = timer_data->brightness_on; delay = timer_data->delay_on; + } else { + /* Store the current brightness value to be able + * to restore it when the delay_off period is over. + */ + timer_data->brightness_on = brightness; + brightness = LED_OFF; + delay = timer_data->delay_off; } led_set_brightness(led_cdev, brightness); @@ -156,6 +168,9 @@ static void timer_trig_activate(struct led_classdev *led_cdev) if (!timer_data) return; + timer_data->brightness_on = led_get_brightness(led_cdev); + if (timer_data->brightness_on == LED_OFF) + timer_data->brightness_on = LED_FULL; led_cdev->trigger_data = timer_data; init_timer(&timer_data->timer); ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] leds: disable triggers on brightness set 2008-02-10 11:52 ` Németh Márton @ 2008-02-17 23:30 ` Richard Purdie 2008-02-18 1:59 ` Henrique de Moraes Holschuh 0 siblings, 1 reply; 17+ messages in thread From: Richard Purdie @ 2008-02-17 23:30 UTC (permalink / raw) To: Németh Márton; +Cc: Henrique de Moraes Holschuh, LKML On Sun, 2008-02-10 at 12:52 +0100, Németh Márton wrote: > Disable any active triggers when the brightness attribute is > set to zero. I agree with this approach and will merge this as a clarification of the interface, thanks. I'll also merge your other two patches into the LED queue. Cheers, Richard ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] leds: disable triggers on brightness set 2008-02-17 23:30 ` Richard Purdie @ 2008-02-18 1:59 ` Henrique de Moraes Holschuh 0 siblings, 0 replies; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-02-18 1:59 UTC (permalink / raw) To: Richard Purdie; +Cc: Németh Márton, LKML On Sun, 17 Feb 2008, Richard Purdie wrote: > On Sun, 2008-02-10 at 12:52 +0100, Németh Márton wrote: > > Disable any active triggers when the brightness attribute is > > set to zero. > > I agree with this approach and will merge this as a clarification of the > interface, thanks. I'll also merge your other two patches into the LED > queue. Where can I get the queue? I'd like to backport the patches (as merged by you) to my various thinkpad-acpi backport trees, so that I can push the thinkpad-acpi LED code to those trees for early testing... I'd *really* recommend explicitly stating in the header file whether the callbacks can or cannot be called from an interrupt context (i.e. "can they sleep?"). And I sure hope it is "they can sleep", because otherwise, it will be *hell* to implement them on any ACPI driver :-) -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-02-07 22:13 ` Richard Purdie 2008-02-07 23:23 ` [PATCH] leds: disable triggers on brightness set Henrique de Moraes Holschuh @ 2008-02-08 7:03 ` Németh Márton 2008-02-08 11:20 ` Henrique de Moraes Holschuh 2008-03-16 18:18 ` Henrique de Moraes Holschuh 2 siblings, 1 reply; 17+ messages in thread From: Németh Márton @ 2008-02-08 7:03 UTC (permalink / raw) To: Richard Purdie, Henrique de Moraes Holschuh; +Cc: LKML Richard Purdie wrote: > On Thu, 2008-02-07 at 19:38 -0200, Henrique de Moraes Holschuh wrote: >> On Thu, 07 Feb 2008, Richard Purdie wrote: >>> Márton Németh: >>> leds: Add support for hardware accelerated LED flashing >>> leds: hw acceleration for Clevo mail LED driver >> This one has a loose end: when you call brightness_set on a led with >> hardware flash acceleration, you will leave the trigger armed, BUT the led >> won't blink anymore. That's just wrong. > > Agreed. My only question is that do you know any LED hardware which can blink _and_ can set the brightness independently? If there would be such a LED I could imagine that the brightness can be changed while the LED remains blinking at some low frequency. For example a simple LED with brightness set possibility and blinking directed by software is an example where the blinking and the brightness setting are completely independent. I agree, however, that if the brightness is set to LED_OFF, the trigger should be also removed. >> Either we should always remove *any* (hardware accelerated or not!) active >> trigger when a write to brightness_set is done, or the stuff about "calling >> brightness_set will disable the hardware accelerated blink" has to go. >> >> I personally prefer that we would always remove any active trigger if >> brightness_set is to be called. IMHO, it is neater, and it is also the >> least-surprise-behaviour from an user perspective with the LED_OFF:LED_FULL >> triggers we have right now. > > Even without the hardware acceleration, a user write to set_brightness > leaves any active trigger active and isn't really intuitive or right > either. > >> Which one will be? If it is "remove any active trigger", I'd not mind >> writing the patch. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-02-08 7:03 ` [GIT PULL] LED updates Németh Márton @ 2008-02-08 11:20 ` Henrique de Moraes Holschuh 2008-02-10 11:52 ` Németh Márton 0 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-02-08 11:20 UTC (permalink / raw) To: Németh Márton; +Cc: Richard Purdie, LKML On Fri, 08 Feb 2008, Németh Márton wrote: > Richard Purdie wrote: > >>> leds: Add support for hardware accelerated LED flashing > >> This one has a loose end: when you call brightness_set on a led with > >> hardware flash acceleration, you will leave the trigger armed, BUT the led > >> won't blink anymore. That's just wrong. > > > > Agreed. > > My only question is that do you know any LED hardware which can blink _and_ > can set the brightness independently? If there would be such a LED I could Several, but none on laptops or other stuff that runs Linux. That behaviour is not common on indicator LEDs. I have seen standby LEDs on laptops which "blink" by slowly fading from full to off, and then back to full, though. > imagine that the brightness can be changed while the LED remains blinking at > some low frequency. For example a simple LED with brightness set possibility and > blinking directed by software is an example where the blinking and the brightness > setting are completely independent. Sure, it is perfectly possible. I am not sure it is *desireable*, though. The way we have triggers work make what you describe impossible right now, the software triggers are LED_OFF:LED_FULL, not LED_OFF:old-brightness. And so are the common hardware triggers on laptops, for that matter. If we go and fix every trigger to use the current brightness (as long as it is non-zero) as the "turn LED on" trigger event, then the documentation has to be changed accordingly to do what you said above, and we would stop the trigger only by setting brightness to zero or by explicitly removing it. I don't think it is worth the hassle, though. But we better decide that *now*, because all this changing of the LED class ABI (even if it is, IMHO, a big improvement) is not a good idea. We better do it all during the 2.6.25 cycle. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-02-08 11:20 ` Henrique de Moraes Holschuh @ 2008-02-10 11:52 ` Németh Márton 0 siblings, 0 replies; 17+ messages in thread From: Németh Márton @ 2008-02-10 11:52 UTC (permalink / raw) To: Henrique de Moraes Holschuh; +Cc: Richard Purdie, LKML Henrique de Moraes Holschuh wrote: > On Fri, 08 Feb 2008, Németh Márton wrote: >> Richard Purdie wrote: >>>>> leds: Add support for hardware accelerated LED flashing >>>> This one has a loose end: when you call brightness_set on a led with >>>> hardware flash acceleration, you will leave the trigger armed, BUT the led >>>> won't blink anymore. That's just wrong. >>> Agreed. >> My only question is that do you know any LED hardware which can blink _and_ >> can set the brightness independently? If there would be such a LED I could > > Several, but none on laptops or other stuff that runs Linux. That behaviour > is not common on indicator LEDs. I have seen standby LEDs on laptops which > "blink" by slowly fading from full to off, and then back to full, though. > >> imagine that the brightness can be changed while the LED remains blinking at >> some low frequency. For example a simple LED with brightness set possibility and >> blinking directed by software is an example where the blinking and the brightness >> setting are completely independent. > > Sure, it is perfectly possible. I am not sure it is *desireable*, though. > The way we have triggers work make what you describe impossible right now, > the software triggers are LED_OFF:LED_FULL, not LED_OFF:old-brightness. > > And so are the common hardware triggers on laptops, for that matter. > > If we go and fix every trigger to use the current brightness (as long as it > is non-zero) as the "turn LED on" trigger event, then the documentation has > to be changed accordingly to do what you said above, and we would stop the > trigger only by setting brightness to zero or by explicitly removing it. > > I don't think it is worth the hassle, though. But we better decide that > *now*, because all this changing of the LED class ABI (even if it is, IMHO, > a big improvement) is not a good idea. We better do it all during the > 2.6.25 cycle. I investigated what would have to be changed if we decide that the brightness and the blinking parameters can be set independently. There are not much too change I think, please have a look at my next mail. Márton Németh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-02-07 22:13 ` Richard Purdie 2008-02-07 23:23 ` [PATCH] leds: disable triggers on brightness set Henrique de Moraes Holschuh 2008-02-08 7:03 ` [GIT PULL] LED updates Németh Márton @ 2008-03-16 18:18 ` Henrique de Moraes Holschuh 2008-03-16 19:29 ` Richard Purdie 2 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-03-16 18:18 UTC (permalink / raw) To: Richard Purdie; +Cc: Márton Németh, LKML On Thu, 07 Feb 2008, Richard Purdie wrote: > > > Richard Purdie: > > > leds: Standardise LED naming scheme > > > > This one causes trouble (at least on 2.6.23 -- I backported the patch) due > > to the 20-byte length limit on sysfs names. I had to use "tp::<somecrap>" > > instead of "thinkpad::<somecrap>" to name LEDs, and still had to reduce > > ultrabase_battery to ultrabase_batt :-) > > > > Anyway, IMHO, the LED function should come first, and we should not even > > need the led driver name anywhere. In case of clashes in the class sysfs > > dir, just tack a .# to the end or somesuch. The device the LED is tied to > > already differentiates them. That would save a lot of chars for something > > much more useful (the function). > > Ouch, I'm looking into this. I wish I'd known about it earlier. I agree > function is more important but didn't want to break the existing > convention. I guess this limitation comes from the kobjects involved... Richard, any ideas for that? It *is* still time to change this for 2.6.25, if required. If you changed it once already, changing it again won't cause further damage. I need to know if the current naming scheme will hold or not, I do NOT want an ABI issue on thinkpad-acpi, and I know for a fact at least Debian will want to use the thinkpad-acpi LED interface as soon as I deploy it. I want to send the thinkpad-acpi LED interface patches to the users as soon as possible. Sincerely? I think you should make it <function>:[color][.instance] and drop device name compleley, ASAP. I will write the patches for mainline and your for-mm branch, if that would speed up things. But I need to know what you have decided, first. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-03-16 18:18 ` Henrique de Moraes Holschuh @ 2008-03-16 19:29 ` Richard Purdie 2008-03-16 19:48 ` Henrique de Moraes Holschuh 0 siblings, 1 reply; 17+ messages in thread From: Richard Purdie @ 2008-03-16 19:29 UTC (permalink / raw) To: Henrique de Moraes Holschuh; +Cc: Márton Németh, LKML On Sun, 2008-03-16 at 15:18 -0300, Henrique de Moraes Holschuh wrote: > On Thu, 07 Feb 2008, Richard Purdie wrote: > > > This one causes trouble (at least on 2.6.23 -- I backported the patch) due > > > to the 20-byte length limit on sysfs names. I had to use "tp::<somecrap>" > > > instead of "thinkpad::<somecrap>" to name LEDs, and still had to reduce > > > ultrabase_battery to ultrabase_batt :-) > > > > > > Anyway, IMHO, the LED function should come first, and we should not even > > > need the led driver name anywhere. In case of clashes in the class sysfs > > > dir, just tack a .# to the end or somesuch. The device the LED is tied to > > > already differentiates them. That would save a lot of chars for something > > > much more useful (the function). > > > > Ouch, I'm looking into this. I wish I'd known about it earlier. I agree > > function is more important but didn't want to break the existing > > convention. I guess this limitation comes from the kobjects involved... > > Richard, any ideas for that? It *is* still time to change this for 2.6.25, > if required. If you changed it once already, changing it again won't cause > further damage. > > I need to know if the current naming scheme will hold or not, I do NOT want > an ABI issue on thinkpad-acpi, and I know for a fact at least Debian will > want to use the thinkpad-acpi LED interface as soon as I deploy it. I want > to send the thinkpad-acpi LED interface patches to the users as soon as > possible. > > Sincerely? I think you should make it <function>:[color][.instance] and > drop device name compleley, ASAP. > > I will write the patches for mainline and your for-mm branch, if that would > speed up things. But I need to know what you have decided, first. As I understand it 2.6.26 will lose the limitation on the name size entirely so the problem will go away soon. I don't want to change the existing ABI so changing to what you describe above isn't possible. You could leave the devicename empty if you wish although I'd prefer you not to. Keeping it short might be the best option for 2.6.25. Your other patch looks ok btw although I'm not sure why you sent it twice. I'll queue that tomorrow. Cheers, Richard ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [GIT PULL] LED updates 2008-03-16 19:29 ` Richard Purdie @ 2008-03-16 19:48 ` Henrique de Moraes Holschuh 2008-03-17 3:34 ` LED naming standard for LED class Henrique de Moraes Holschuh 0 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-03-16 19:48 UTC (permalink / raw) To: Richard Purdie; +Cc: Márton Németh, LKML On Sun, 16 Mar 2008, Richard Purdie wrote: > As I understand it 2.6.26 will lose the limitation on the name size > entirely so the problem will go away soon. I don't want to change the > existing ABI so changing to what you describe above isn't possible. You > could leave the devicename empty if you wish although I'd prefer you not > to. Keeping it short might be the best option for 2.6.25. Hmm, that means I will have to keep the short names, since I can't very much have two different ABIs across several kernel versions... or I will have to backport whatever is needed to expand this name size restriction (and who knows if that's doable... I better start digging). This won't be easy on my side :( Well, might as well claim tpacpi as a thinkpad-acpi short handle and use it on workqueue and kthread naming (and document it) as well as the led class. Yuck. > Your other patch looks ok btw although I'm not sure why you sent it > twice. I'll queue that tomorrow. I screwed up the prefix on the first one (used LED instead of leds). The patches are the same. I wrote the reason for the resend after the first "---" separator. Anyway, thanks for the fast reply. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* LED naming standard for LED class 2008-03-16 19:48 ` Henrique de Moraes Holschuh @ 2008-03-17 3:34 ` Henrique de Moraes Holschuh 2008-03-17 9:51 ` Richard Purdie 0 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-03-17 3:34 UTC (permalink / raw) To: Richard Purdie; +Cc: LKML (changing the subject. Newcomers, the context is given by commit 6c152beefbf90579d21afc4f7e075b1f801f9a75). On Sun, 16 Mar 2008, Henrique de Moraes Holschuh wrote: > On Sun, 16 Mar 2008, Richard Purdie wrote: > > As I understand it 2.6.26 will lose the limitation on the name size > > entirely so the problem will go away soon. I don't want to change the > > existing ABI so changing to what you describe above isn't possible. You > > could leave the devicename empty if you wish although I'd prefer you not > > to. Keeping it short might be the best option for 2.6.25. > > Hmm, that means I will have to keep the short names, since I can't very > much have two different ABIs across several kernel versions... or I will Ok, I looked at it from various angles, and did some heavy thinking about it. Richard, isn't there *any* way to change this new ABI? It has not been in any stable mainline release yet, so it is not too late if we do it now. Even if the lenght limit for sysfs entries (19 chars + NULL) is lifted in 2.6.26, the current proposal that would become stable in 2.6.25 is still ugly, and not nearly close to the best we could do, IMHO. The only technical reason I can see for the "devicename" part of the LED name is to avoid clashes (or as a placeholder when you have no function). However, it is the least important information for any sort of generic LED tool (which is the only reason to even bother with a LED naming standard to begin with), so it certainly should not come first in the name. And there are other ways to avoid clashes that waste far less real state than the device name. Can't we have "function[:color].<number>" instead? And allow for a choice of "devicename" or simply "led" instead of "function" for leds with no defined or implied function? It places fields in order of importance (thus it is far more user-friendly), it wastes less real state (typically just two chars) to avoid clashes, and it also follows what we already have on other sysfs subsystems (except for the :color part, and I agree with you that it is a desireable thing to have in there). The clash-avoiding ".%d" postfix would be taken care of by the class. If two devices are registered with a "battery:green" name, you'd end up with "battery:green.0" and "battery:green.1". If just one device tries to register "battery:yellow", you'd get "battery:yellow.0". Later (for 2.6.26) I'd also suggest adding the color notion to the *class* itself, including the notion of "undefined" color. The class would take care to add the ":color" part of the node name (when it is not "undefined") and *also* export the color as an attribute. I can write this patch too, if you want. Would you take patches to implement the "function[:color].<ordinal>" naming scheme, and try to merge them into 2.6.25? Avoiding a bad ABI becoming stable *is* an acceptable reason for a late merge. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: LED naming standard for LED class 2008-03-17 3:34 ` LED naming standard for LED class Henrique de Moraes Holschuh @ 2008-03-17 9:51 ` Richard Purdie 2008-03-18 3:35 ` Henrique de Moraes Holschuh 0 siblings, 1 reply; 17+ messages in thread From: Richard Purdie @ 2008-03-17 9:51 UTC (permalink / raw) To: Henrique de Moraes Holschuh; +Cc: LKML On Mon, 2008-03-17 at 00:34 -0300, Henrique de Moraes Holschuh wrote: > Richard, isn't there *any* way to change this new ABI? It has not been in > any stable mainline release yet, so it is not too late if we do it now. It is not an new ABI, its an extension as per the way it was documented to work. > Even if the lenght limit for sysfs entries (19 chars + NULL) is lifted in > 2.6.26, the current proposal that would become stable in 2.6.25 is still > ugly, and not nearly close to the best we could do, IMHO. > > The only technical reason I can see for the "devicename" part of the LED > name is to avoid clashes (or as a placeholder when you have no function). > However, it is the least important information for any sort of generic LED > tool (which is the only reason to even bother with a LED naming standard to > begin with), so it certainly should not come first in the name. And there > are other ways to avoid clashes that waste far less real state than the > device name. > > Can't we have "function[:color].<number>" instead? And allow for a choice > of "devicename" or simply "led" instead of "function" for leds with no > defined or implied function? > > It places fields in order of importance (thus it is far more user-friendly), > it wastes less real state (typically just two chars) to avoid clashes, and > it also follows what we already have on other sysfs subsystems (except for > the :color part, and I agree with you that it is a desireable thing to have > in there). > > The clash-avoiding ".%d" postfix would be taken care of by the class. If > two devices are registered with a "battery:green" name, you'd end up with > "battery:green.0" and "battery:green.1". If just one device tries to > register "battery:yellow", you'd get "battery:yellow.0". > > Later (for 2.6.26) I'd also suggest adding the color notion to the *class* > itself, including the notion of "undefined" color. The class would take > care to add the ":color" part of the node name (when it is not "undefined") > and *also* export the color as an attribute. I can write this patch too, if > you want. > > Would you take patches to implement the "function[:color].<ordinal>" naming > scheme, and try to merge them into 2.6.25? Avoiding a bad ABI becoming > stable *is* an acceptable reason for a late merge. The problem is the "bad" ABI has existed in that form since somewhere around 2.6.15 and it is therefore too late to drop things from it or rearrange it. The ABI described additions being allowed so we're taking advantage of that to gain something which should have really been there originally. Short term there is some complexity with some limits but they're going away so there is no problem. The whole point of the naming was so at a glance you can tell which LEDs are which and whilst the device name isn't important in the things you're considering, in general it does make sense, particularly if we see LEDs attached to removable hardware for example. The alternative is we throw the whole thing away and go to random numbers for the different LEDs and attributes. To work out which LED is which you then have to read up to 3 files per LED before you can even decide whether its the one you want. I don't like that idea... Cheers, Richard ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: LED naming standard for LED class 2008-03-17 9:51 ` Richard Purdie @ 2008-03-18 3:35 ` Henrique de Moraes Holschuh 2008-03-18 4:55 ` Henrique de Moraes Holschuh 0 siblings, 1 reply; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-03-18 3:35 UTC (permalink / raw) To: Richard Purdie; +Cc: LKML On Mon, 17 Mar 2008, Richard Purdie wrote: > On Mon, 2008-03-17 at 00:34 -0300, Henrique de Moraes Holschuh wrote: > > Richard, isn't there *any* way to change this new ABI? It has not been in > > any stable mainline release yet, so it is not too late if we do it now. > > It is not an new ABI, its an extension as per the way it was documented > to work. It is a full ABI break for any drivers you change a LED name. It is not an ABI break for drivers which *add* new LEDs with the new names, but keep the old naming on older leds. And almost all of the changed drivers had a stable release already. There is no way we can pass a sysfs node name change like that as an extension of the ABI, as it breaks all userspace scripts that used the old node name. Looking at the commit, the following drivers had ABI breaks to go from "function" to "driver:color:function": nas100d, nslu2, wistron. These can be considered bug-fixing, since "function" is clash-prone. The following drivers had ABI breaks to add the middle ":" for the color field: applesmc, ams-delta, net48, wrap, asus, b43. The following drivers had ABI breaks to go from driver to driver:color:function : corgi, locomo, spitz, tosa. Looks like it was a hideous mess to me, and IMO we can pretty much say that the naming guidelines were nothing but a dream before the commit. Anyway, you did break ABI in 13 drivers, and only 3 could be considered a bug fix. And if you're going to break the ABI (I am *not* speaking against it in this case), let's make the best of it, please. > The problem is the "bad" ABI has existed in that form since somewhere > around 2.6.15 and it is therefore too late to drop things from it or LED drivers did whatever they wanted, no matter what docs said. Each LED driver had its own ABI, and only a few of those followed what was described in the led class docs. > rearrange it. The ABI described additions being allowed so we're taking > advantage of that to gain something which should have really been there > originally. Short term there is some complexity with some limits but > they're going away so there is no problem. Now that we have it clear that a widespread ABI break took place, the above reasoning doesn't hold anymore, and there is no reason to fix the "past mistake" in the led class documentation. > The alternative is we throw the whole thing away and go to random > numbers for the different LEDs and attributes. To work out which LED is > which you then have to read up to 3 files per LED before you can even > decide whether its the one you want. I don't like that idea... I didn't suggest that. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: LED naming standard for LED class 2008-03-18 3:35 ` Henrique de Moraes Holschuh @ 2008-03-18 4:55 ` Henrique de Moraes Holschuh 0 siblings, 0 replies; 17+ messages in thread From: Henrique de Moraes Holschuh @ 2008-03-18 4:55 UTC (permalink / raw) To: Richard Purdie; +Cc: LKML On Tue, 18 Mar 2008, Henrique de Moraes Holschuh wrote: > Now that we have it clear that a widespread ABI break took place, the > above reasoning doesn't hold anymore, and there is no reason to fix the > "past mistake" in the led class documentation. ... there is no reason to NOT fix the ..., I mean. -- "One disk to rule them all, One disk to find them. One disk to bring them all and in the darkness grind them. In the Land of Redmond where the shadows lie." -- The Silicon Valley Tarot Henrique Holschuh ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2008-03-18 4:55 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2008-02-07 10:35 [GIT PULL] LED updates Richard Purdie 2008-02-07 21:38 ` Henrique de Moraes Holschuh 2008-02-07 22:13 ` Richard Purdie 2008-02-07 23:23 ` [PATCH] leds: disable triggers on brightness set Henrique de Moraes Holschuh 2008-02-10 11:52 ` Németh Márton 2008-02-17 23:30 ` Richard Purdie 2008-02-18 1:59 ` Henrique de Moraes Holschuh 2008-02-08 7:03 ` [GIT PULL] LED updates Németh Márton 2008-02-08 11:20 ` Henrique de Moraes Holschuh 2008-02-10 11:52 ` Németh Márton 2008-03-16 18:18 ` Henrique de Moraes Holschuh 2008-03-16 19:29 ` Richard Purdie 2008-03-16 19:48 ` Henrique de Moraes Holschuh 2008-03-17 3:34 ` LED naming standard for LED class Henrique de Moraes Holschuh 2008-03-17 9:51 ` Richard Purdie 2008-03-18 3:35 ` Henrique de Moraes Holschuh 2008-03-18 4:55 ` Henrique de Moraes Holschuh
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.