LED devices

[Richard Hughes]

On Wed, 2007-05-30 at 15:46 +0100, Richard Purdie wrote:
> On Wed, 2007-05-30 at 14:59 +0100, Richard Hughes wrote:
> > On Wed, 2007-05-30 at 14:31 +0100, Richard Purdie wrote:
> > > To be honest, the LED device names are a lot more meaningful than
> > > "batt1", "batt2" and "batt3" which really do tell you absolutely
> > > nothing.
> > 
> > That's not the point. We shouldn't be scanning for magic tokens in the
> > device name in sysfs.
> 
> Why not? I'm sure HAL can cope with this.

Yes, but my point is it shouldn't _have_ to when we have attributes on
devices.

> > > > If we have to parse the values in sysfs device names then we might as
> > > > well go back to the horrors that are /proc. This is bad. One value per
> > > > file, simple as that.
> > > 
> > > This doesn't break that rule. We just encode some information into the
> > > device name. You don't need that information to control the LED and it
> > > is machine extractable.
> > 
> > Yes, but so is all the stuff in /proc - but this broke everytime the
> > interface changed. Scanning for tokens is wrong.
> 
> Entries under /sys/class/ get to define their interface. I don't see why
> that interface can't include a naming convention for the devices which
> is all this is.

But there is already a way to expose properties in the sysfs API - why
re-invent the wheel and put attributes in the device title?

> > If you have the led device with extra parameters you can add parameters
> > without breaking API, for instance, adding a color attribute would only
> > be added to devices that have a colour.
> >
> > Similarly, if the device (like the thinkpad leds) supports hardware
> > flashing, then an attribute could be added to define what duty cycle the
> > flash exhibits. Maybe a silly idea, but helps explain my point.
> >
> > If the LED has a meaningful descriptor, then this too can be exposed,
> > without breaking API, adding yet another ":" entry.
> 
> Adding another : does not break the API. Its certainly implied if not
> explicitly specified that the name field is ':' delimited and you should
> be ignoring any 3rd field or greater.

That doesn't match what I would expect of sysfs, and I'm guessing this
will be the case for most of the guys who are going to write userspace
glue to this.

> > Using:delimited:names:in:the:title is not standard, and I've only seen
> > it used in limited numeric form with SCSI devices and PCI devices, not
> > freeform text descriptors.
> 
> If the interface defines it to work like that, it is a standard.

Okay then, in my opinion, the standard used in the leds class is broken
and non-optimal and will break userspace everytime the standard changes
as new properties are added.

> > > >  I'm surprised this hasn't been ripped to pieces on LKML already.
> > > 
> > > It was discussed, no objections were raised.
> > 
> > But what actual userspace code actually uses the led class at the
> > moment?
> 
> Why does that matter?

What are the consumers of the led class: userspace code that wants to
read and write LED values without echoing random bytes into an ec
address space or doing horrible hacks in /proc that involve parsing
strings and numbers from vendor specific interfaces.

At the moment we are going to have to special case the HAL code for the
led class to read one (or two, depending on kernel version) string
properties out of a title. Not cool.

Richard.