[GIT PULL] LED updates

[GIT PULL] LED updates

[Richard Purdie]

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

Re: [GIT PULL] LED updates

[Henrique de Moraes Holschuh]

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

Re: [GIT PULL] LED updates

[Richard Purdie]

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

[PATCH] leds: disable triggers on brightness set

[Henrique de Moraes Holschuh]

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

Re: [PATCH] leds: disable triggers on brightness set

[Németh Márton]

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

Re: [PATCH] leds: disable triggers on brightness set

[Richard Purdie]

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

Re: [PATCH] leds: disable triggers on brightness set

[Henrique de Moraes Holschuh]

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

Re: [GIT PULL] LED updates

[Németh Márton]

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.

Re: [GIT PULL] LED updates

[Henrique de Moraes Holschuh]

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

Re: [GIT PULL] LED updates

[Németh Márton]

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

Re: [GIT PULL] LED updates

[Henrique de Moraes Holschuh]

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

Re: [GIT PULL] LED updates

[Richard Purdie]

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

Re: [GIT PULL] LED updates

[Henrique de Moraes Holschuh]

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

LED naming standard for LED class

[Henrique de Moraes Holschuh]

(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

Re: LED naming standard for LED class

[Richard Purdie]

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

Re: LED naming standard for LED class

[Henrique de Moraes Holschuh]

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

Re: LED naming standard for LED class

[Henrique de Moraes Holschuh]

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