2004-09-02 20:35:36

by John Lenz

[permalink] [raw]
Subject: [PATCH 2.6.8.1 0/2] leds: new class for led devices

This is an attempt to provide an alternative to the current arm
specific led interface. This arm interface does not integrate well
with the device model and sysfs.

We create a new class that drivers can register a leds_properties
structure with. Each led that is registered is assigned a number, and
three attributes are exported to sysfs in /sys/class/leds/1/, /sys/
class/leds/2, etc.

color : a read only string attribute that shows the available colors of
this led. If it is a multi-color led, then the colors are seperated by
a "/" (for example "green/blue").

function : a read/write attribute that sets the current function of
this led. The available options are

timer : the led blinks on and off on a timer
idle : the led turns off when the system is idle and on when not idle
power : the led represents the current power state
user : the led is controlled by user space

light : a read/write attribute that allows userspace to see the current
status of the led. If function="user" then writing to this attribute
will change the led on or off. If function != "user" then writing has
no effect.
light is an integer, where 0 means off, 1 means on first color, 2
means on second color, etc. (for example, if color="green/blue" then
light=1 means turn led on to green and if light=2 means turn led on to
blue.

A device and driver symlink point to the actual driver and such, so any
extra attributes can also be registered.

Signed-off-by: John Lenz <[email protected]>


2004-09-02 20:40:11

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 2/2] leds: new class for led devices

This is an example driver for the leds on the Sharp Zaurus using the
locomo bus.

Signed-off-by: John Lenz <[email protected]>


Attachments:
(No filename) (128.00 B)
locomo_led.patch (3.22 kB)
Download all attachments

2004-09-03 03:01:58

by John Lenz

[permalink] [raw]

2004-09-03 04:54:17

by Kalin KOZHUHAROV

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

John Lenz wrote:
> This is an attempt to provide an alternative to the current arm
> specific led interface. This arm interface does not integrate well
> with the device model and sysfs.
I am just curious, but what specific hardware devices can be controlled with this?

[snip]

> function : a read/write attribute that sets the current function of
> this led. The available options are
>
> timer : the led blinks on and off on a timer
> idle : the led turns off when the system is idle and on when not idle
> power : the led represents the current power state
> user : the led is controlled by user space

Please put the power comment in the source too, e.g. :

+enum led_functions {
+ leds_user = 0, /* user has control of this led through sysfs */
+ leds_timer, /* led blinks on a timer */
+ leds_idle, /* led is on when the system is not idle */
+ leds_power, /* led is on when ?????????????????????? */
+};

To be honest, I don't get the meaning of any of the led_functions except leds_user...

> light : a read/write attribute that allows userspace to see the current
> status of the led. If function="user" then writing to this attribute
> will change the led on or off. If function != "user" then writing has
> no effect.
> light is an integer, where 0 means off, 1 means on first color, 2
> means on second color, etc. (for example, if color="green/blue" then
> light=1 means turn led on to green and if light=2 means turn led on to
> blue.

Is something like max_colors or num_colors necessary?
Might be handy for changing to the "last" color or something,
but it may as well be done by userspace app.


--
|| ~~~~~~~~~~~~~~~~~~~~~~ ||
( ) http://ThinRope.net/ ( )
|| ______________________ ||

2004-09-03 07:18:00

by Robert Schwebel

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

John,

On Thu, Sep 02, 2004 at 08:33:10PM +0000, John Lenz wrote:
> This is an attempt to provide an alternative to the current arm
> specific led interface. This arm interface does not integrate well
> with the device model and sysfs.

I have written a GPIO device class driver for the same purpose; before
this goes into the kernel I think we should try to merge your attempts
with mine.

On embedded systems you normally have several things which are similar
to LEDs; embedded processors have quite a lot of general purpose I/O
pins. Normally you want some of the pins being used from userspace (like
'echo 1 > /sys/class/gpio/gpio5/level'), others are used from device
drivers. My framework offers a request_gpio() function similar to
request_gpio(), so the kernel can administrate these ressources.

I suppose it is not too difficult to unify our drivers in a way that the
base mechanism is more abstract then LEDs (a GPIO pin can also represent
a power switch or whatever) and still can support your LED levels.

I'll pull the gpio patch out of my working tree and post it here for
discussion.

Robert
--
Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
Pengutronix - Linux Solutions for Science and Industry
Handelsregister: Amtsgericht Hildesheim, HRA 2686
Hornemannstra?e 12, 31137 Hildesheim, Germany
Phone: +49-5121-28619-0 | Fax: +49-5121-28619-4

2004-09-03 07:34:59

by Robert Schwebel

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Fri, Sep 03, 2004 at 03:17:15PM +0200, Robert Schwebel wrote:
> I'll pull the gpio patch out of my working tree and post it here for
> discussion.

Attached. Parts of it have been taken from your LEDs patch anyway, so it
should be not too difficult to reunify them.

Robert
--
Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
Pengutronix - Linux Solutions for Science and Industry
Handelsregister: Amtsgericht Hildesheim, HRA 2686
Hornemannstra?e 12, 31137 Hildesheim, Germany
Phone: +49-5121-28619-0 | Fax: +49-5121-28619-4


Attachments:
(No filename) (554.00 B)
gpio.diff (15.59 kB)
Download all attachments

2004-09-03 07:58:44

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

Am Freitag, 3. September 2004 15:31 schrieb Robert Schwebel:
> On Fri, Sep 03, 2004 at 03:17:15PM +0200, Robert Schwebel wrote:
> > I'll pull the gpio patch out of my working tree and post it here for
> > discussion.
>
> Attached. Parts of it have been taken from your LEDs patch anyway, so it
> should be not too difficult to reunify them.
>
> Robert
+ list_for_each_safe(lact, ltmp, &gpio_list) {
+ gpio_dev = list_entry(lact, struct gpio_device, list);
+ if (pin_nr == gpio_dev->props.pin_nr) {
+ printk(KERN_ERR "gpio pin %i is already used by %s\n",
+ pin_nr, gpio_dev->props.owner);
+ return -EBUSY;
+ }
+ }

[..]
+ spin_lock_init(&gpio_dev->lock);
+ gpio_dev->props.pin_nr = pin_nr;

This looks like a race condition. You need to check and reserve under
a lock.

Regards
Oliver

2004-09-03 11:32:25

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Fri, 3 Sep 2004, Kalin KOZHUHAROV wrote:
> > function : a read/write attribute that sets the current function of
> > this led. The available options are
> >
> > timer : the led blinks on and off on a timer
> > idle : the led turns off when the system is idle and on when not idle
> > power : the led represents the current power state
> > user : the led is controlled by user space
>
> Please put the power comment in the source too, e.g. :
>
> +enum led_functions {
> + leds_user = 0, /* user has control of this led through sysfs */
> + leds_timer, /* led blinks on a timer */
> + leds_idle, /* led is on when the system is not idle */
> + leds_power, /* led is on when ?????????????????????? */
> +};
>
> To be honest, I don't get the meaning of any of the led_functions except leds_user...

leds_timer means something like a heartbeat (cfr. PPC and m68k)? Or do you need
a separate `heartbeat' option for that?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2004-09-03 12:09:12

by Jan-Benedict Glaw

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Fri, 2004-09-03 13:54:04 +0900, Kalin KOZHUHAROV <[email protected]>
wrote in message <[email protected]>:
> John Lenz wrote:
> >This is an attempt to provide an alternative to the current arm
> >specific led interface. This arm interface does not integrate well
> >with the device model and sysfs.
> I am just curious, but what specific hardware devices can be controlled
> with this?

For example, the smaller VAX computers offer 8 LEDs which show system
status during IPL. After boot finished, the OS may use them...

> >function : a read/write attribute that sets the current function of
> > timer : the led blinks on and off on a timer
> > idle : the led turns off when the system is idle and on when not idle
> > power : the led represents the current power state
> > user : the led is controlled by user space

Is idle/timer/power hardware-controlled (eg. by a secondary processor,
direct chipset implementation) or is switching on/off controlled by
kernel (eg. heartbeat, IO and ethernet for the LEDs you can find on some
PA-RISC machines)?

MfG, JBG

--
Jan-Benedict Glaw [email protected] . +49-172-7608481 _ O _
"Eine Freie Meinung in einem Freien Kopf | Gegen Zensur | Gegen Krieg _ _ O
fuer einen Freien Staat voll Freier B?rger" | im Internet! | im Irak! O O O
ret = do_actions((curr | FREE_SPEECH) & ~(NEW_COPYRIGHT_LAW | DRM | TCPA));


Attachments:
(No filename) (1.37 kB)
signature.asc (189.00 B)
Digital signature
Download all attachments

2004-09-03 15:12:18

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

Hi!

> This is an attempt to provide an alternative to the current arm
> specific led interface. This arm interface does not integrate well
> with the device model and sysfs.
>
> We create a new class that drivers can register a leds_properties
> structure with. Each led that is registered is assigned a number, and
> three attributes are exported to sysfs in /sys/class/leds/1/, /sys/
> class/leds/2, etc.
>
> color : a read only string attribute that shows the available colors of
> this led. If it is a multi-color led, then the colors are seperated by
> a "/" (for example "green/blue").
>
> function : a read/write attribute that sets the current function of
> this led. The available options are
>
> timer : the led blinks on and off on a timer
> idle : the led turns off when the system is idle and on when not idle
> power : the led represents the current power state
> user : the led is controlled by user space

I'm afraid this is really good idea. It seems quite overengineered to
me (and I'd be afraid that idle part slows machine). Perhaps having
only "user" mode is better idea?

> light : a read/write attribute that allows userspace to see the current
> status of the led. If function="user" then writing to this attribute
> will change the led on or off. If function != "user" then writing has
> no effect.
> light is an integer, where 0 means off, 1 means on first color, 2
> means on second color, etc. (for example, if color="green/blue" then
> light=1 means turn led on to green and if light=2 means turn led on to
> blue.

Is there ways to turn on both?
Pavel

--
When do you have heart between your knees?

2004-09-03 18:42:22

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On 09/03/04 08:51:03, Pavel Machek wrote:
> > function : a read/write attribute that sets the current function of
> > this led. The available options are
> >
> > timer : the led blinks on and off on a timer
> > idle : the led turns off when the system is idle and on when not idle
> > power : the led represents the current power state
> > user : the led is controlled by user space
>
> I'm afraid this is really good idea. It seems quite overengineered to
> me (and I'd be afraid that idle part slows machine). Perhaps having
> only "user" mode is better idea?

I was only mimicing the support currently in the arm led code.
After thinking about it from a broader perspective of including GPIOs,
we should probably get rid of this function thing entirely. Just let user
space do everything... userspace can monitor sysfs and hotplug and have the
led represent power or idle or whatever.

>
> > light : a read/write attribute that allows userspace to see the current
>
> > status of the led. If function="user" then writing to this attribute
> > will change the led on or off. If function != "user" then writing has
> > no effect.
> > light is an integer, where 0 means off, 1 means on first color, 2
> > means on second color, etc. (for example, if color="green/blue" then
> > light=1 means turn led on to green and if light=2 means turn led on to
> > blue.
>
> Is there ways to turn on both?

depends on the hardware...

The led class does not really inforce any policy, it just passes this
number along to the actual driver that is registered. So say you had
a led that could be red, green, or both red and green at the same time
(not sure how that would work hardware wise, but ok). The driver could
do something like set color="red/green/red&green" and then use 0,1,2,3.
The kernel is just reflecting the possible states the hardware could be
in.

John



2004-09-03 18:55:12

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On 09/03/04 07:06:34, Jan-Benedict Glaw wrote:
> On Fri, 2004-09-03 13:54:04 +0900, Kalin KOZHUHAROV <[email protected]>
> wrote in message <[email protected]>:
> > John Lenz wrote:
> > >This is an attempt to provide an alternative to the current arm
> > >specific led interface. This arm interface does not integrate well
> > >with the device model and sysfs.
> > I am just curious, but what specific hardware devices can be controlled
> > with this?
>
> For example, the smaller VAX computers offer 8 LEDs which show system
> status during IPL. After boot finished, the OS may use them...
>
> > >function : a read/write attribute that sets the current function of
> > > timer : the led blinks on and off on a timer
> > > idle : the led turns off when the system is idle and on when not idle
> > > power : the led represents the current power state
> > > user : the led is controlled by user space
>
> Is idle/timer/power hardware-controlled (eg. by a secondary processor,
> direct chipset implementation) or is switching on/off controlled by
> kernel (eg. heartbeat, IO and ethernet for the LEDs you can find on some
> PA-RISC machines)?

Right now the kernel is in sole control. The device I am testing this on
is a Sharp Zaurus SL5500, which has two leds that by default are used to
light when new mail arrives and if the power is plugged in.

A read-only interface should probably be added?

The function stuff was intended to be controlled by the kernel, but I think
now that I should just remove this and let userspace do it. If we just
allow userspace an easy way to turn the leds on and off (and stuff like
power state is available elsewhere in sysfs) a simple shell script could
turn the led to show the power state.

John


2004-09-03 18:58:21

by Russell King

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Fri, Sep 03, 2004 at 06:38:06PM +0000, John Lenz wrote:
> On 09/03/04 08:51:03, Pavel Machek wrote:
> > > function : a read/write attribute that sets the current function of
> > > this led. The available options are
> > >
> > > timer : the led blinks on and off on a timer
> > > idle : the led turns off when the system is idle and on when not idle
> > > power : the led represents the current power state
> > > user : the led is controlled by user space
> >
> > I'm afraid this is really good idea. It seems quite overengineered to
> > me (and I'd be afraid that idle part slows machine). Perhaps having
> > only "user" mode is better idea?
>
> I was only mimicing the support currently in the arm led code.
> After thinking about it from a broader perspective of including GPIOs,
> we should probably get rid of this function thing entirely. Just let user
> space do everything... userspace can monitor sysfs and hotplug and have the
> led represent power or idle or whatever.

Bear in mind that my _original_ reason for implementing some of this
was to tell what's going on with the kernel on my machines. I'm fairly
opposed to shifting it to userspace just because someone wants a bloated
sysfs interface to drive some tiddly little LEDs and then having to
losing the benefits of the existing implementation.

> The led class does not really inforce any policy, it just passes this
> number along to the actual driver that is registered. So say you had
> a led that could be red, green, or both red and green at the same time
> (not sure how that would work hardware wise, but ok).

See Netwinders. They have a bi-colour LED which has independent red
and green LEDs in the same package. When they're both on it's yellow.

It's _VERY_ useful to see the green LED flashing and know that the
headless machine is running, or that the red LED being on means that
either it hasn't booted a kernel yet or the system has successfully
shut down.

It means I don't have to fsck around with monitor cables before pulling
the power.

And no, doing that from userspace won't work because userspace is dead
_before_ the system has finished shutting down (see drive cache
flushing on shutdown.)

It's also _VERY_ useful to know whether the kernel has managed to get
far enough through the boot that the heartbeat LED is flashing but
maybe not sufficiently to bring up the serial console as well -
again another thing that a userspace implementation will never be
able to support.

So sorry... I'm definitely opposed.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core

2004-09-03 19:12:18

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On 09/03/04 13:55:55, Russell King wrote:
> On Fri, Sep 03, 2004 at 06:38:06PM +0000, John Lenz wrote:
> > On 09/03/04 08:51:03, Pavel Machek wrote:
> > > > function : a read/write attribute that sets the current function of
> > > > this led. The available options are
> > > >
> > > > timer : the led blinks on and off on a timer
> > > > idle : the led turns off when the system is idle and on when not
> idle
> > > > power : the led represents the current power state
> > > > user : the led is controlled by user space
> > >
> > > I'm afraid this is really good idea. It seems quite overengineered to
> > > me (and I'd be afraid that idle part slows machine). Perhaps having
> > > only "user" mode is better idea?
> >
> > I was only mimicing the support currently in the arm led code.
> > After thinking about it from a broader perspective of including GPIOs,
> > we should probably get rid of this function thing entirely. Just let
> user
> > space do everything... userspace can monitor sysfs and hotplug and have
> the
> > led represent power or idle or whatever.
>
> Bear in mind that my _original_ reason for implementing some of this
> was to tell what's going on with the kernel on my machines. I'm fairly
> opposed to shifting it to userspace just because someone wants a bloated
> sysfs interface to drive some tiddly little LEDs and then having to
> losing the benefits of the existing implementation.

Yea, but there are other uses than developing and debugging. For example,
handheld environments use the leds to display when new mail comes, could
also turn on the led based on an entry in the calandar to notify the user
that some event is coming up, etc.

It's not really intended for debugging, because you can't control the leds
until after module_init/device_initcall.

>
> > The led class does not really inforce any policy, it just passes this
> > number along to the actual driver that is registered. So say you had
> > a led that could be red, green, or both red and green at the same time
> > (not sure how that would work hardware wise, but ok).
>
> See Netwinders. They have a bi-colour LED which has independent red
> and green LEDs in the same package. When they're both on it's yellow.
>
> It's _VERY_ useful to see the green LED flashing and know that the
> headless machine is running, or that the red LED being on means that
> either it hasn't booted a kernel yet or the system has successfully
> shut down.
>
> It means I don't have to fsck around with monitor cables before pulling
> the power.
>
> And no, doing that from userspace won't work because userspace is dead
> _before_ the system has finished shutting down (see drive cache
> flushing on shutdown.)
>
> It's also _VERY_ useful to know whether the kernel has managed to get
> far enough through the boot that the heartbeat LED is flashing but
> maybe not sufficiently to bring up the serial console as well -
> again another thing that a userspace implementation will never be
> able to support.

Hey I agree! I use the led as a debugging tool on the Sharp Zaurus.
Before I had the framebuffer or the serial port working, being able to see
the led told me something was working :) This patch isn't really intended
for debugging support, so perhaps some other interface could be added.

On the other hand, perhaps we can just do a #ifdef LEDS_DEBUGGING that
would toggle the led on a timer.

John


2004-09-03 19:54:21

by Russell King

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Fri, Sep 03, 2004 at 07:09:59PM +0000, John Lenz wrote:
> > > The led class does not really inforce any policy, it just passes this
> > > number along to the actual driver that is registered. So say you had
> > > a led that could be red, green, or both red and green at the same time
> > > (not sure how that would work hardware wise, but ok).
> >
> > See Netwinders. They have a bi-colour LED which has independent red
> > and green LEDs in the same package. When they're both on it's yellow.
> >
> > It's _VERY_ useful to see the green LED flashing and know that the
> > headless machine is running, or that the red LED being on means that
> > either it hasn't booted a kernel yet or the system has successfully
> > shut down.
> >
> > It means I don't have to fsck around with monitor cables before pulling
> > the power.
> >
> > And no, doing that from userspace won't work because userspace is dead
> > _before_ the system has finished shutting down (see drive cache
> > flushing on shutdown.)
> >
> > It's also _VERY_ useful to know whether the kernel has managed to get
> > far enough through the boot that the heartbeat LED is flashing but
> > maybe not sufficiently to bring up the serial console as well -
> > again another thing that a userspace implementation will never be
> > able to support.
>
> Hey I agree! I use the led as a debugging tool on the Sharp Zaurus.
> Before I had the framebuffer or the serial port working, being able to see
> the led told me something was working :) This patch isn't really intended
> for debugging support, so perhaps some other interface could be added.
>
> On the other hand, perhaps we can just do a #ifdef LEDS_DEBUGGING that
> would toggle the led on a timer.

You missed the point. The above scenarios I was describing are _not_
debugging.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core

2004-09-03 20:42:57

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On 09/03/04 14:51:15, Russell King wrote:

>
> You missed the point. The above scenarios I was describing are _not_
> debugging.

Ok, so we get rid of the function thing, but still provide a
heartbeat_enabled entry in sysfs. Something like this patch

Signed-off-by: John Lenz <[email protected]>


Attachments:
(No filename) (301.00 B)
leds_sysfs.patch (9.50 kB)
Download all attachments

2004-09-03 22:25:26

by Russell King

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Fri, Sep 03, 2004 at 06:47:23PM +0000, John Lenz wrote:
> On 09/03/04 07:06:34, Jan-Benedict Glaw wrote:
> > Is idle/timer/power hardware-controlled (eg. by a secondary processor,
> > direct chipset implementation) or is switching on/off controlled by
> > kernel (eg. heartbeat, IO and ethernet for the LEDs you can find on some
> > PA-RISC machines)?
>
> Right now the kernel is in sole control. The device I am testing this on
> is a Sharp Zaurus SL5500, which has two leds that by default are used to
> light when new mail arrives and if the power is plugged in.

The kernel is NOT in sole control today on ARM platforms:

echo claim > /sys/devices/system/leds/leds0/event
echo red on > /sys/devices/system/leds/leds0/event
echo green on > /sys/devices/system/leds/leds0/event
echo red off > /sys/devices/system/leds/leds0/event
echo release > /sys/devices/system/leds/leds0/event

etc

Sure, we have a weird naming scheme (red, green, amber, blue) but
that came around because that's what people were dealing with.
There's nothing really stopping us from having any name for a LED
in the existing scheme.

I just don't buy the "we must have one sysfs node for every LED"
argument.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core

2004-09-03 23:20:32

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On 09/03/04 17:25:07, Russell King wrote:
> On Fri, Sep 03, 2004 at 06:47:23PM +0000, John Lenz wrote:
> > On 09/03/04 07:06:34, Jan-Benedict Glaw wrote:
> > > Is idle/timer/power hardware-controlled (eg. by a secondary
> processor,
> > > direct chipset implementation) or is switching on/off controlled by
> > > kernel (eg. heartbeat, IO and ethernet for the LEDs you can find on
> some
> > > PA-RISC machines)?
> >
> > Right now the kernel is in sole control. The device I am testing this
> on
> > is a Sharp Zaurus SL5500, which has two leds that by default are used
> to
>
> > light when new mail arrives and if the power is plugged in.
>
> The kernel is NOT in sole control today on ARM platforms:
>
> echo claim > /sys/devices/system/leds/leds0/event
> echo red on > /sys/devices/system/leds/leds0/event
> echo green on > /sys/devices/system/leds/leds0/event
> echo red off > /sys/devices/system/leds/leds0/event
> echo release > /sys/devices/system/leds/leds0/event
>
> etc
>
> Sure, we have a weird naming scheme (red, green, amber, blue) but
> that came around because that's what people were dealing with.
> There's nothing really stopping us from having any name for a LED
> in the existing scheme.

1) This is arm specific. Do any other arches want to provide access to
leds? Why should they implement some different api?

2) What happens when you have more than four leds? or 3 dual color leds?

3) no way to read the current status of the led. (could be added to read
from /sys/devices/system/leds/leds0 or something)

4) really wierd in-kernel api... The led is associated with a machine,
when that sometimes doesn't make sense; code duplication for the same
harware drivers. As an example, the led device on the Sharp Zauruses are
all the same... exact same code to control the led. Except one model is
sa1100 and two are pxa based, so leds-collie.c and leds-poodle.c are
exactly the same, except to be registered in the associated leds.c for that
mach. This model, instead of associating leds with a specific machine make
it a device/driver concept like every other device in the kernel.

We can easily provide backwards compatibility with the arm led code. I
have a semi-working driver to register with ledsbase.c and call the
leds_event function. As well, the old /sys/devices/system/leds/leds0/event
thing can also be emulated.

> I just don't buy the "we must have one sysfs node for every LED"
> argument.

well, we could just create one attribute per led in /sys/devices/system/
leds/leds0/, but that breaks conceptally what sysfs is trying to provide...
one device per attribute? 4 devices controlled by ONE attribute?

John


2004-09-04 11:10:06

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

Hi!

> >> function : a read/write attribute that sets the current function of
> >> this led. The available options are
> >>
> >> timer : the led blinks on and off on a timer
> >> idle : the led turns off when the system is idle and on when not idle
> >> power : the led represents the current power state
> >> user : the led is controlled by user space
> >
> >I'm afraid this is really good idea. It seems quite overengineered to
> >me (and I'd be afraid that idle part slows machine). Perhaps having
> >only "user" mode is better idea?
>
> I was only mimicing the support currently in the arm led code.
> After thinking about it from a broader perspective of including GPIOs,
> we should probably get rid of this function thing entirely. Just let user
> space do everything... userspace can monitor sysfs and hotplug and have the
> led represent power or idle or whatever.

Well, it might be okay than.

Another possibility would be to use hard-coded behaviour while system
is booting/stopping and /sys-controlled behaviour while userspace is
alive.

Pavel

2004-09-04 11:15:57

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

Hi!

> The kernel is NOT in sole control today on ARM platforms:
>
> echo claim > /sys/devices/system/leds/leds0/event
> echo red on > /sys/devices/system/leds/leds0/event
> echo green on > /sys/devices/system/leds/leds0/event
> echo red off > /sys/devices/system/leds/leds0/event
> echo release > /sys/devices/system/leds/leds0/event
>
> etc
>
> Sure, we have a weird naming scheme (red, green, amber, blue) but
> that came around because that's what people were dealing with.
> There's nothing really stopping us from having any name for a LED
> in the existing scheme.
>
> I just don't buy the "we must have one sysfs node for every LED"
> argument.

sysfs is one-file-one-value. We do not want to end up with /proc-like
mess.

Pavel

2004-09-04 20:53:41

by Russell King

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On Sat, Sep 04, 2004 at 01:12:02PM +0200, Pavel Machek wrote:
> Hi!
>
> > The kernel is NOT in sole control today on ARM platforms:
> >
> > echo claim > /sys/devices/system/leds/leds0/event
> > echo red on > /sys/devices/system/leds/leds0/event
> > echo green on > /sys/devices/system/leds/leds0/event
> > echo red off > /sys/devices/system/leds/leds0/event
> > echo release > /sys/devices/system/leds/leds0/event
> >
> > etc
> >
> > Sure, we have a weird naming scheme (red, green, amber, blue) but
> > that came around because that's what people were dealing with.
> > There's nothing really stopping us from having any name for a LED
> > in the existing scheme.
> >
> > I just don't buy the "we must have one sysfs node for every LED"
> > argument.
>
> sysfs is one-file-one-value. We do not want to end up with /proc-like
> mess.

In which case I protest in the strongest terms that having one device
node plus attributes _PER_ _LED_ is just fscking stupid. What if you
have an embedded machine with 32 LEDs? Do you _really_ need all that
extra data just to support sysfs so that maybe you can control them
from userspace?

What next? One sysfs node plus attributes per GPIO line? How about
we do one sysfs node per virtual memory bit so people can control
anything in their system on a bit granularity without needing mmap
or any other interfaces? When does this madness stop?

It comes down to this:
- is a single LED in one package one device?
- is a set of two LEDs in one package one device?
- is a set of three LEDs in one package a device?
- what about a bank of 8 LEDs grouped together?
- what about 4 banks of 8 LEDs grouped together?
- what about 7 segment numeric LED displays?
- what about 14 segment alphanumeric LED displays?

All of these are LED devices. Does one sysfs node per individual LED
element _really_ make sense for all these cases? I think not.

--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core

2004-09-04 21:41:17

by Pavel Machek

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

Hi!

> > sysfs is one-file-one-value. We do not want to end up with /proc-like
> > mess.
>
> In which case I protest in the strongest terms that having one device
> node plus attributes _PER_ _LED_ is just fscking stupid. What if you
> have an embedded machine with 32 LEDs? Do you _really_ need all that
> extra data just to support sysfs so that maybe you can control them
> from userspace?

If you do not need that 32 files on that embedded system, do not
enable that in config... I do not see what is wrong with two or three
files per led.

> What next? One sysfs node plus attributes per GPIO line? How about
> we do one sysfs node per virtual memory bit so people can control
> anything in their system on a bit granularity without needing mmap
> or any other interfaces? When does this madness stop?

GPIO lines are obviously system specific, I guess they could go to
/proc or be controlled via ioctl()... But that was attempt at
universal LED interface, right?
Pavel

2004-09-06 07:36:37

by John Lenz

[permalink] [raw]
Subject: Re: [PATCH 2.6.8.1 0/2] leds: new class for led devices

On 09/04/04 16:41:11, Pavel Machek wrote:
> > What next? One sysfs node plus attributes per GPIO line? How about
> > we do one sysfs node per virtual memory bit so people can control
> > anything in their system on a bit granularity without needing mmap
> > or any other interfaces? When does this madness stop?
>
> GPIO lines are obviously system specific, I guess they could go to
> /proc or be controlled via ioctl()... But that was attempt at
> universal LED interface, right?

Yep. As well, the GPIO interface proposed by Robert Schwebel should be
kept seperate... I don't see a reason or anything useful from unifying
them.

John