Kay,

Patch attached corrects all the brokenness with the led class (encoding
some attributes in the device handle).

 Documentation/leds-class.txt    |   13 --------
 drivers/leds/led-class.c        |   62 ++++++++++++++++++++++++++++++++++++++--
 drivers/leds/leds-ams-delta.c   |   18 +++++++----
 drivers/leds/leds-corgi.c       |    8 +++--
 drivers/leds/leds-h1940.c       |   12 +++++--
 drivers/leds/leds-locomo.c      |    8 +++--
 drivers/leds/leds-net48xx.c     |    3 +
 drivers/leds/leds-spitz.c       |    8 +++--
 drivers/leds/leds-tosa.c        |    8 +++--
 drivers/leds/leds-wrap.c        |    6 ++-
 drivers/macintosh/via-pmu-led.c |    1 
 drivers/misc/asus-laptop.c      |    3 +
 include/linux/leds.h            |    2 +
 13 files changed, 115 insertions(+), 37 deletions(-)

Signed-off-by: Richard Hughes <richard@hughsie.com>

Please review attached patch, thanks.

[ message continues ]

For the benefit of LKML, there has been some discussion of this
elsewhere. My arguments do not particularly come across in Richard's
choice of quoting and I'm a little annoyed he's chosen to do things this
way, particularly before any further feedback from Greg/Kay but so be
it.

Greg said that the LEDs class should export any property data as a class
attribute rather than this just being available in the device name. I
added the patch in
http://git.o-hand.com/?p=linux-rpurdie-leds;a=shortlog;h=for-mm to deal
with this, without breaking the existing LED naming and documented API.
Greg said that the only requirement on bus id naming was that it needed
to be unique so this should therefore be compatible. HAL could then go
ahead and ignore the device name, all the attributes are there in the
fashion it needs which was the original complaint.

I accept this is basically out of my hands now. Greg/Kay have the
appropriate emails and if they'll let me know which approach they want
to take, I'll apply an appropriate patch. I'd suggest not applying this
patch directly as it introduces a race in the error path it alters.

Richard


-

[ message continues ]

To be honest, I didn't want to do this, but you would not accept my
argument - and you wanted to add _more_ properties into the device

Yes, but as you'll notice with my patch, lots of the users that use the
led class do not use the handle:colour name, and so stripping off

But when I was investigating adding led class support to thinkpad_acpi I
couldn't use the existing LED class, as some of the LED's did not have a
pre-defined colour, but did have a description. Again, adding support
into HAL was made more difficult until you proposed screenscaping the

Sure, I really wanted to convert the class to struct device (which I'm
more familiar with) but just tried to do minimal changes. What changes
need to be made to avoid a race? Thanks.

Richard.


-

[ message continues ]

It has nothing to do with the struct device vs. class devices, you need
to think more carefully about the placement of the list_del().

My other concerns with this patch are that it exports incorrect
information on spitz (which I had warned about and I will get bug
reports about) and that "description" is not really a suitable name for
a sysfs attribute, "function" might give a better idea of what it
represents to developers.

I still question whether either colour or function properties are
actually particularly useful to userspace other than for naming purposes
which is why they were part of the device name.

Anyhow, I'm trying not to say too much more as it will just go in
circles. I'll await a reply from Greg.

Regards,

Richard


-

[ message continues ]

Hm, I thought I made my opinion pretty clear in the beginning.

Why not just do a simple:
	led01
	led02
	led03
	...

and so on?

And use the 'name' field to put the name of your device (disk,
bluetooth, etc.)  This is the way all other busses and devices work, and
I don't think that LEDs are anything more "special" than anything else
in the kernel, right?

So the original patch in this thread is close, but not quite there.

thanks,

greg k-h
-

[ message continues ]

If we do this, my point is we're exporting something to userspace which
is totally devoid of information. 

sysfs is for userspace and "led01" means absolutely nothing to it. The
most you can summarise from it is it happens to have registered first
and its an LED. Since its in class/leds/ we can summarise the latter
without help from the prefix. I'd hate to think userspace cares about

Since the "busid" field means absolutely nothing, why not give it a
sensible id and save the overhead of a separate name?

If kernel policy is that we should have useless data in sysfs, so be it,
I just make sure that is on record and then break the defined LED class
API.


<serious mode>
Ok, so I make the point above with a sledge hammer. There are more
subtle issues here too. People are asking for a ton of extra attributes
for the LED class. We can have a name, a colour, an LED "function", a
location on the device and probably 101 other things.

As I understand it, sysfs was put there to pass information *the kernel
knows* to userspace. The kernel doesn't actually care about the location
of an LED or its colour. All the kernel cares is we have an LED and it
has a certain brightness.

Yes, we can teach the kernel all this extra info but it is simply to
pass to userspace. Why should my kernel be bloated with all that extra
information which it doesn't need itself?

My conclusion is that there should be something in userspace
supplementing this information from the kernel. Going back the the
problem at hand, HAL is already equipped to do this, all it needs is
some kind of unique ID so it can identify the LED. This would be the
busid.

So I do stand by what the LED class does as being sane. Yes, the class
defined a way of writing its busids which isn't like any other but there
is a good reason for it. We could make it machine readable to provide
hints to userspace and I never saw any reason why we shouldn't have
done. It was discussed on LKML and no objections were raised (it was ...
[ message continues ]

Just like all other subsystems?

No, the only information the device name is that it shows a UNIQUE name
at that point in time in the kernel.  Heck, we could just use random
numbers that are unique like some other people have suggested, it would

Wrong, the attributes in that directory mean everything.  The name means


Ok, then use a random number please.  Start with 00000000000001 and just
increment from there, or use the idr subsystem.

I was trying to be nice and at least give you a prefix that looked kind


Again, the bus id needs to be unique, for that specific class/bus that's
it.  The attributes in the directory let you figure out what specifics
are for the device.

Look at the PCI and USB devices.  There is some information you can
glean from those names, but they are primary a unique identifier, you
need to look at the attributes to get the real information about your
device.



If you know the color and location already, why not export that
information?  Unless you can determine it from userspace some other way

If there's no other way for userspace to determine it, then put it in


Sorry, I missed it with the other stuff on lkml, but now I'm trying to
get this fixed.  Just use an attribute like the whole rest of the





I know that LEDs are special and unique, just like everyone else, so
please work like everyone else does :)

thanks,

greg k-h
-

[ message continues ]

This whole mess is because the LED class tried to be nice with sane

Most of your argument seems to read that this shouldn't be done because
nobody else does it which isn't a particularly good one. Everyone else
is jumping off a bridge, I should as well? You then point out that PCI
and USB busids do have some meaning and a quick glance at sysfs shows

and as I've said several times, the LED scheme *is* unique meeting your

So the PCI and USB subsystems need adjusting to use random numbers since
they're also breaking the rules and might have meaningful information in


The kernel doesn't know this and it doesn't care about this.


Ok. The LED class can provide a name attribute which uniquely identifies
each LED. Userspace can then store and look up all the information it
wants against that "name".

So by this argument, colour, function and all these other attributes

I've never claimed that it was special and unique. Just because it does
something differently, that doesn't mean its wrong either. 

One last try to demonstrate this is grossly inefficient (and this kind
of inefficiency is one reason I'm beginning to dislike sysfs). On my
handheld, you can currently script something to light a specific LED
with something like:

echo "1" > /sys/class/leds/spitz\:green/brightness

With the busid changes, this becomes:

for busid in `ls /sys/class/leds`
do
  name=`cat /sys/class/leds/$busid/name`
  if [ "$name" = "spitz_green" ]; then
    break
  fi
done
echo "1" > /sys/class/leds/$busid/brightness

Even after accounting for my lack of l33t shell skills, the latter will
be much more complex. For what gain? It doesn't matter whether you do
this through shell or any other language. Even if you know the name of
the device you want to talk to you have to convert to some meaningless
busid first which is inefficient. Things like HAL are going to be forced
to read things after every boot. This has wider implications than the
LEDs which are just a tiny consideration in the ...
[ message continues ]

Thank you, I really appreciate it, and so does the people who write the
tools that parse sysfs (like HAL.)

greg k-h
-

[ message continues ]

There were some other opinions voiced including one from the person who
started this discussion.

So no, the people who write the tools that parse sysfs (like HAL.) don't
appreciate this.

People who write tools that parse sysfs like shell scripts don't
appreciate it either, as I illustrated.

You've yet to give any technical reason why we can't have meaningful
busids rather than random numbers. Your entire argument seems to be that
its wrong because its a bit different and nobody else does it...

Richard


-

[ message continues ]

'light_to_dance_the_fandango' and from a shell point of view it's
probably best for the latter. I think the point Greg tried to make is
that it shouldn't matter, and HAL shouldn't export (nor parse) the

If it's a trivial name then I think led_thinklight0 is perfectly okay, I
think Kay was talking more about the attribute vs. name-in-device
encoding.

Richard.


-

[ message continues ]

I see no problem to use the function name and add an enumeration number
to that name to be able to handle multiple instances with the same
function name. Like pointed out in earlier mail:
  http://lists.freedesktop.org/archives/hal/2007-May/008552.html

Thanks,
Kay

-

[ message continues ]

Can we keep the original naming? spitz:disk is as unique as led02, and
it is _way_ easier to use.

Come on, I want to use the led subsystem from the scripts... 
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
-

[ message continues ]

I don't see a problem with spitz_disk, which is just as easy to use in
scripts.

Richard.


-

[ message continues ]

Hi Richard!


It is the old problem with mV in battery_level_mV.  How do you know for sure
what is the designator, and what is the unit (or in this case, color)?  You
could take the last _[^_]+, but what if there is no unit/color? etc.

There is a BIIIIG thread about it (or more than one, actually) in the
archives, and I don't recall a consensus being reached.

-- 
  "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
-

[ message continues ]