LED devices

[Richard Hughes]

On Fri, 2007-06-01 at 13:18 +0100, Richard Purdie wrote:
> On Fri, 2007-06-01 at 13:04 +0100, Richard Hughes wrote:
> > On Fri, 2007-06-01 at 12:56 +0100, Richard Purdie wrote:
> > > > Richard, would you let me write a patch to fix this or are you already
> > > > doing this? Thanks.
> > > 
> > > I'd started something, I've finished it off and added it to the LEDs
> > > tree:
> > > 
> > > http://git.o-hand.com/?p=linux-rpurdie-leds;a=shortlog;h=for-mm
> [...]
> > Why not just add to the API to do:
> > 
> > static struct led_classdev standby_led = {
> > 	.name			= "thinkpad_standby",
> > 	.color			= "green",
> > 	.default_trigger	= "none",
> > 	.brightness_set		= led_standby_set,
> > };
> 
> Several reasons:
> 
> * I want to keep the names descriptive (which is allowed afaict)

Then we can use apple_power_on, thinkpad_charging, logitech_low_power.
The colour of the led is not always the best descriptor.

> * If we remove colour from the names we break the existing API as
> documented.

No, we don't have to break API, we just say that in future you don't
have to put the colour appended to the device name. You can continue to
use spitz:red if you want, but we shouldn't assume everybody wants to do
this.

In linus right now we have this:

static struct led_classdev pmu_led = {
	.name = "pmu-front-led",
	.default_trigger = "ide-disk",
	.brightness_set = pmu_led_set,
};

and for asus-laptop we have:

ASUS_LED(mled, "mail");
ASUS_LED(tled, "touchpad");
ASUS_LED(rled, "record");
ASUS_LED(pled, "phone");
ASUS_LED(gled, "gaming");

And these were the two main led class users I could find in drivers.

Your argument about breaking name:colour API looks a little weak, as it
appears nearly nothing in-tree actually uses it.

> * Drivers should only specify the colour in one place.

Yup. Agree, which to me suggests that a descriptor to the led
"discharging" is better than "amber", and then if the led colour ever
changes from model to model (like the thinklight) then we only have to
change one property and not the device name.

> * With this approach we only need to add a method to the core, not
> change every LED driver.

.colour is optional.

I've attached a completely untested patch against linus that changes the
colour to be an attribute, and not scraped from the device name.

Richard

-------------- next part --------------
A non-text attachment was scrubbed...
Name: leds-add-colour.patch
Type: text/x-patch
Size: 3632 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/hal/attachments/20070601/d1c3a518/attachment-0001.bin