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

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

[John Lenz]

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 <lenz@cs.wisc.edu>

Re: [PATCH 2.6.8.1 1/2] leds: new class for led devices

[John Lenz]

[-- Attachment #1: Type: text/plain, Size: 88 bytes --]

This provides the base ledscore.c support.

Signed-off-by: John Lenz <lenz@cs.wisc.edu>

[-- Attachment #2: leds_sysfs.patch --]
[-- Type: text/x-patch, Size: 10908 bytes --]


#
# Patch managed by http://www.holgerschurig.de/patcher.html
#

--- /dev/null
+++ linux/drivers/leds/ledscore.c
@@ -0,0 +1,314 @@
+/*
+ * linux/drivers/leds/ledscore.c
+ *
+ *   Copyright (C) 2004 John Lenz <lenz@cs.wisc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/sysdev.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+
+#define LEDS_TIMER_INTERVAL (HZ * 5)
+
+struct led_device {
+	/* This protects the props field.*/
+	spinlock_t lock;
+	/* If props is NULL, the driver that registered this device has been unloaded */
+	struct led_properties *props;
+	struct class_device class_dev;
+	struct list_head list;
+};
+
+#define to_led_device(d) container_of(d, struct led_device, class_dev)
+
+static rwlock_t leds_list_lock = RW_LOCK_UNLOCKED;
+static LIST_HEAD(leds_list);
+static atomic_t leds_count = ATOMIC_INIT(0);
+
+struct leds_function_name {
+	const char *		name;
+	enum led_functions	function;
+};
+static const struct leds_function_name leds_function_names[] = {
+	{ "user",	leds_user },
+	{ "timer",	leds_timer },
+	{ "idle",	leds_idle },
+	{ "power",	leds_power },
+};
+
+static void leds_class_release(struct class_device *dev)
+{
+	struct led_device *d = to_led_device(dev);
+
+	write_lock(&leds_list_lock);
+		list_del(&d->list);
+	write_unlock(&leds_list_lock);
+	
+	kfree(d);
+}
+
+static struct class leds_class = {
+	.name		= "leds",
+	.release	= leds_class_release,
+};
+
+static ssize_t leds_show_color(struct class_device *dev, char *buf)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	ssize_t ret = 0;
+	
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		sprintf(buf, "%s\n", led_dev->props->color);
+		ret = strlen(buf) + 1;
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static CLASS_DEVICE_ATTR(color, 0444, leds_show_color, NULL);
+
+static ssize_t leds_show_light(struct class_device *dev, char *buf)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	ssize_t ret = 0;
+
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		sprintf(buf, "%lu\n", led_dev->props->light_state);
+		ret = strlen(buf) + 1;
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static ssize_t leds_store_light(struct class_device *dev, const char *buf, size_t size)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	int ret = -EINVAL;
+	char *after;
+
+	unsigned long state = simple_strtoul(buf, &after, 10);
+	if (after - buf > 0) {
+		ret = after - buf;
+		spin_lock(&led_dev->lock);
+		if (likely(led_dev->props)) {
+			if (led_dev->props->function == leds_user) {
+				led_dev->props->light_state = state;
+				if (led_dev->props->light) 
+					led_dev->props->light(led_dev->class_dev.dev, led_dev->props);
+			}
+		}
+		spin_unlock(&led_dev->lock);
+	}
+
+	return ret;
+}
+
+static CLASS_DEVICE_ATTR(light, 0644, leds_show_light, leds_store_light);
+
+static ssize_t leds_show_function(struct class_device *dev, char *buf)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	ssize_t ret = 0;
+
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		sprintf(buf, "%s\n", leds_function_names[(int)led_dev->props->function].name);
+		ret = strlen(buf) + 1;
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static ssize_t leds_store_function(struct class_device *dev, const char *buf, size_t size)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	int ret = -EINVAL;
+	int i;
+	
+	for (i = 0; i < ARRAY_SIZE(leds_function_names); i++) {
+		if (strncmp(buf, leds_function_names[i].name, strlen(leds_function_names[i].name)) == 0) {
+			ret = size;
+			spin_lock(&led_dev->lock);
+			if (likely(led_dev->props)) {
+				led_dev->props->function = leds_function_names[i].function;
+			}
+			spin_unlock(&led_dev->lock);
+		}
+	}
+
+	return ret;
+}
+
+static CLASS_DEVICE_ATTR(function, 0644, leds_show_function, leds_store_function);
+
+/**
+ * leds_device_register - register a new object of led_device class.
+ * @dev: The device to register.
+ * @prop: the led properties structure for this device.
+ */
+int leds_device_register(struct device *dev, struct led_properties *props)
+{
+	int rc, num;
+	struct led_device *new_led;
+
+	new_led = kmalloc (sizeof (struct led_device), GFP_KERNEL);
+	if (unlikely (!new_led))
+		return -ENOMEM;
+
+	spin_lock_init(&new_led->lock);
+	new_led->props = props;
+	props->led_dev = new_led;
+
+	memset (&new_led->class_dev, 0, sizeof (new_led->class_dev));
+	new_led->class_dev.class = &leds_class;
+	new_led->class_dev.dev = dev;
+
+	/* assign this led a number */
+	num = atomic_add_return(1, &leds_count);
+	sprintf(new_led->class_dev.class_id, "%i", num);
+
+	rc = class_device_register (&new_led->class_dev);
+	if (unlikely (rc)) {
+		kfree (new_led);
+		return rc;
+	}
+
+	/* register the attributes */
+	class_device_create_file(&new_led->class_dev, &class_device_attr_color);
+	class_device_create_file(&new_led->class_dev, &class_device_attr_light);
+	class_device_create_file(&new_led->class_dev, &class_device_attr_function);
+
+	/* add to the list of leds */
+	write_lock(&leds_list_lock);
+		list_add_tail(&new_led->list, &leds_list);
+	write_unlock(&leds_list_lock);
+
+	printk(KERN_INFO "Registered led device: number=%s, color=%s\n", new_led->class_dev.class_id, props->color);
+
+	return 0;
+}
+EXPORT_SYMBOL(leds_device_register);
+
+/**
+ * led_device_unregister - unregisters a object of led_properties class.
+ * @props: the property to unreigister
+ *
+ * Unregisters a previously registered via led_device_register object.
+ */
+void leds_device_unregister(struct led_properties *props)
+{
+	struct led_device *led_dev;
+	if (!props)
+		return;
+
+	pr_debug("led_device_unregister: color=%s\n", props->color);
+
+	led_dev = props->led_dev;
+
+	class_device_remove_file (&led_dev->class_dev, &class_device_attr_function);
+	class_device_remove_file (&led_dev->class_dev, &class_device_attr_light);
+	class_device_remove_file (&led_dev->class_dev, &class_device_attr_color);
+
+	spin_lock(&led_dev->lock);
+	led_dev->props = NULL;
+	props->led_dev = NULL;
+	spin_unlock(&led_dev->lock);
+
+	class_device_unregister(&led_dev->class_dev);
+}
+EXPORT_SYMBOL(leds_device_unregister);
+
+/* set up a kernel timer */
+static struct timer_list	leds_ktimer;
+static atomic_t 		leds_stop_timer = ATOMIC_INIT(0);
+
+static void leds_timer_function(unsigned long data)
+{
+	struct led_device *led_dev;
+
+	read_lock(&leds_list_lock);
+	list_for_each_entry(led_dev, &leds_list, list) {
+		spin_lock(&led_dev->lock);
+		if (likely(led_dev->props)) {
+			if (led_dev->props->function == leds_timer) {
+				led_dev->props->light_state = !led_dev->props->light_state;
+				if (led_dev->props->light) 
+					led_dev->props->light(led_dev->class_dev.dev, led_dev->props);
+			}
+		}
+		spin_unlock(&led_dev->lock);
+	}
+	read_unlock(&leds_list_lock);
+
+	if (!atomic_read(&leds_stop_timer))
+		mod_timer(&leds_ktimer, jiffies + LEDS_TIMER_INTERVAL);
+}
+
+/* arch code should call this function from the idle thread */
+void leds_idle_function(int state)
+{
+	struct led_device *led_dev;
+
+	read_lock(&leds_list_lock);
+	list_for_each_entry(led_dev, &leds_list, list) {
+		spin_lock(&led_dev->lock);
+		if (likely(led_dev->props)) {
+			if (led_dev->props->function == leds_idle) {
+				led_dev->props->light_state = state;
+				if (led_dev->props->light) 
+					led_dev->props->light(led_dev->class_dev.dev, led_dev->props);
+			}
+		}
+		spin_unlock(&led_dev->lock);
+	}
+	read_unlock(&leds_list_lock);
+}
+
+static int __init leds_init(void)
+{
+	int ret = 0;
+
+	/* initialize the class device */
+	class_register(&leds_class);
+
+	/* register the timer */
+	init_timer(&leds_ktimer);
+	leds_ktimer.function = leds_timer_function;
+	leds_ktimer.data = 0;
+	leds_ktimer.expires = jiffies + LEDS_TIMER_INTERVAL;
+	add_timer(&leds_ktimer);
+	
+	return ret;
+}
+subsys_initcall(leds_init);
+
+static void __exit leds_exit(void)
+{
+	class_unregister(&leds_class);
+
+	atomic_set(&leds_stop_timer, 1);
+	
+	del_timer_sync(&leds_ktimer);
+}
+module_exit(leds_exit);
+
+MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("LED core class interface");
+
--- /dev/null
+++ linux/include/linux/leds.h
@@ -0,0 +1,53 @@
+/*
+ *  linux/include/leds.h
+ *
+ *  Copyright (C) 2004 John Lenz <lenz@cs.wisc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver model for leds
+ */
+#ifndef ASM_ARM_LEDS_H
+#define ASM_ARM_LEDS_H
+
+#include <linux/device.h>
+
+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,
+};
+
+struct led_device;
+
+struct led_properties {
+	struct module *owner;
+	
+	/* Color of the led.  For multiple color leds, the color names should
+	 * be seperated by a "/".  For example, "amber/green".
+	 */
+	char *color;
+	
+	/* current state of this led.
+	 * 0 = off, 1,2,3,... = on, where the number is the posision in the list
+	 * of colors given above.
+	 */
+	unsigned long light_state;
+
+	/* current function of this led */
+	enum led_functions function;
+
+	/* This function is called after the light_state property is changed. */
+	void (*light)(struct device *, struct led_properties *props);
+	
+	/* private structure */
+	struct led_device *led_dev;
+};
+
+int leds_device_register(struct device *dev, struct led_properties *props);
+void leds_device_unregister(struct led_properties *props);
+
+#endif
--- /dev/null
+++ linux/drivers/leds/Kconfig
@@ -0,0 +1,11 @@
+
+menu "LED devices"
+
+config CLASS_LEDS
+	tristate "LED support"
+	help
+	  This option provides the generic support for the leds class.
+	  LEDs can be accessed from /sys/class/leds.  It will also allow you
+	  to select individual drivers for LED devices.  If unsure, say N.
+
+endmenu
--- /dev/null
+++ linux/drivers/leds/Makefile
@@ -0,0 +1,3 @@
+
+# Core functionality.
+obj-$(CONFIG_CLASS_LEDS)		+= ledscore.o
--- linux/drivers/Kconfig~leds_sysfs
+++ linux/drivers/Kconfig
@@ -48,6 +48,8 @@
 
 source "drivers/media/Kconfig"
 
+source "drivers/leds/Kconfig"
+
 source "drivers/video/Kconfig"
 
 source "sound/Kconfig"
--- linux/drivers/Makefile~leds_sysfs
+++ linux/drivers/Makefile
@@ -50,4 +50,5 @@
 obj-$(CONFIG_MCA)		+= mca/
 obj-$(CONFIG_EISA)		+= eisa/
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq/
+obj-$(CONFIG_CLASS_LEDS)	+= leds/
 obj-y				+= firmware/

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

[John Lenz]

[-- Attachment #1: Type: text/plain, Size: 128 bytes --]

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

Signed-off-by: John Lenz <lenz@cs.wisc.edu>

[-- Attachment #2: locomo_led.patch --]
[-- Type: text/x-patch, Size: 3301 bytes --]


#
# Patch managed by http://www.holgerschurig.de/patcher.html
#

--- /dev/null
+++ linux/drivers/leds/locomo.c
@@ -0,0 +1,112 @@
+/*
+ * linux/drivers/leds/locomo.c
+ *
+ * Copyright (C) 2004 John Lenz <lenz@cs.wisc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/config.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/leds.h>
+
+#include <asm/hardware.h>
+#include <asm/hardware/locomo.h>
+
+struct locomoled_data {
+	unsigned long		offset;
+	int			registered;
+	struct led_properties	props;
+};
+#define to_locomoled_data(d) container_of(d, struct locomoled_data, props)
+
+void locomoled_light(struct device *dev, struct led_properties *props)
+{
+	struct locomo_dev *locomo_dev = LOCOMO_DEV(dev);
+	struct locomoled_data *data = to_locomoled_data(props);
+	
+	unsigned long flags;
+
+	local_irq_save(flags);
+	if (props->light_state)
+		locomo_writel(LOCOMO_LPT_TOFH, locomo_dev->mapbase + data->offset);
+	else
+		locomo_writel(LOCOMO_LPT_TOFL, locomo_dev->mapbase + data->offset);
+	local_irq_restore(flags);
+}
+
+static struct locomoled_data leds[] = {
+	{
+		.offset	= LOCOMO_LPT0,
+		.props	= {
+			.owner		= THIS_MODULE,
+			.color		= "amber",
+			.light_state	= 0,
+	
+			.light		= locomoled_light,
+
+			.function	= leds_timer,
+		}
+	},
+	{
+		.offset	= LOCOMO_LPT1,
+		.props	= {
+			.owner		= THIS_MODULE,
+			.color		= "green",
+			.light_state	= 0,
+
+			.light		= locomoled_light,
+
+			.function	= leds_idle,
+		}
+	},
+};
+
+static int locomoled_probe(struct locomo_dev *dev)
+{
+	int i, ret = 0;
+	
+	for (i = 0; i < ARRAY_SIZE(leds); i++) {
+		ret = leds_device_register(&dev->dev, &leds[i].props);
+		leds[i].registered = 1;
+		if (unlikely(ret)) {
+			printk(KERN_WARNING "Unable to register locomo led %s\n", leds[i].props.color);
+			leds[i].registered = 0;
+		}
+	}
+	
+	return ret;
+}
+
+static int locomoled_remove(struct locomo_dev *dev) {
+	int i;
+	
+	for (i = 0; i < ARRAY_SIZE(leds); i++) {
+		if (leds[i].registered) {
+			leds_device_unregister(&leds[i].props);
+		}
+	}
+	return 0;
+}
+
+static struct locomo_driver locomoled_driver = {
+	.drv = {
+		.name = "locomoled"
+	},
+	.devid	= LOCOMO_DEVID_LED,
+	.probe	= locomoled_probe,
+	.remove	= locomoled_remove,
+};
+
+static int __init locomoled_init(void) {
+	return locomo_driver_register(&locomoled_driver);
+}
+module_init(locomoled_init);
+
+MODULE_AUTHOR("John Lenz <lenz@cs.wisc.edu>");
+MODULE_DESCRIPTION("Locomo LED driver");
+MODULE_LICENSE("GPL");
--- linux/drivers/leds/Kconfig~locomo_led
+++ linux/drivers/leds/Kconfig
@@ -15,4 +15,11 @@
 	  This option enables support for the old leds_event interface used by
 	  the arm port.  This driver can not be compiled as a module.
 
+config LEDS_LOCOMO
+	tristate "LED Support for Locomo device"
+	depends CLASS_LEDS && SHARP_LOCOMO
+	help
+	  This option enables support for the LEDs on Sharp Locomo.
+
+
 endmenu
--- linux/drivers/leds/Makefile~locomo_led
+++ linux/drivers/leds/Makefile
@@ -1,3 +1,6 @@
 
 # Core functionality.
 obj-$(CONFIG_CLASS_LEDS)		+= ledscore.o
+
+# drivers
+obj-$(CONFIG_LEDS_LOCOMO)		+= locomo.o

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

[Kalin KOZHUHAROV]

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/ (  )
 || ______________________ ||

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

[Oliver Neukum]

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

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

[Geert Uytterhoeven]

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 -- geert@linux-m68k.org

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

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

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On Fri, 2004-09-03 13:54:04 +0900, Kalin KOZHUHAROV <kalin@thinrope.net>
wrote in message <ch8tdd$1uf$1@sea.gmane.org>:
> 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       jbglaw@lug-owl.de    . +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));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Robert Schwebel]

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

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

[Robert Schwebel]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

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

[-- Attachment #2: gpio.diff --]
[-- Type: text/plain, Size: 15969 bytes --]

Index: kernel/gpio.c
===================================================================
--- a/kernel/gpio.c	(revision 0)
+++ b/kernel/gpio.c	(revision 205)
@@ -0,0 +1,459 @@
+/*
+ * linux/kernel/gpio.c
+ *
+ * (C) 2004 Robert Schwebel, Pengutronix
+ * 
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/sysdev.h>
+#include <linux/timer.h>
+#include <linux/proc_fs.h>
+#include <linux/gpio.h>
+
+#define DRIVER_NAME "gpio"
+
+static char mapping[255] = "";
+
+MODULE_AUTHOR("John Lenz, Robert Schwebel");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Generic GPIO Infrastructure");
+module_param_string(mapping, mapping, sizeof(mapping), 0);
+MODULE_PARM_DESC(mapping,
+"period delimited options string to map GPIO pins to userland:\n"
+"\n"
+"	<pin>:[out|in][hi|lo]\n"
+"\n"
+"	example: mapping=5:out:hi.8:in\n"	
+);
+
+static ssize_t gpio_show_level(struct class_device *dev, char *buf);
+static ssize_t gpio_store_level(struct class_device *dev, const char *buf, size_t size);
+static ssize_t gpio_show_policy(struct class_device *dev, char *buf);
+
+struct gpio_properties {
+	unsigned int       pin_nr;
+	unsigned char      policy;	/* GPIO_xxx */
+	char               pin_level;	/* -1=tristate, 0, 1 */
+	char               owner[20];
+	struct gpio_device *gpio_dev;
+};
+
+struct gpio_device {
+	spinlock_t lock; 		/* protects the props field */
+	struct gpio_properties props;
+	struct class_device class_dev;
+	struct list_head list;
+};
+#define to_gpio_device(d) container_of(d, struct gpio_device, class_dev)
+
+static LIST_HEAD(gpio_list);
+static rwlock_t gpio_list_lock = RW_LOCK_UNLOCKED;
+
+/* gpio_device is static, so we don't have to free it here */
+static void gpio_class_release(struct class_device *dev)
+{
+	return;
+}
+
+static struct class gpio_class = {
+	.name		= "gpio",
+	.release	= gpio_class_release,
+};
+
+
+/* 
+ * Attribute: /sys/class/gpio/gpioX/level 
+ */
+static struct class_device_attribute attr_gpio_level = {
+	.attr = { .name = "level", .mode = 0644, .owner = THIS_MODULE },
+	.show = gpio_show_level,
+	.store = gpio_store_level,
+};
+
+static ssize_t gpio_show_level(struct class_device *dev, char *buf)
+{
+	struct gpio_device *gpio_dev = to_gpio_device(dev);
+	ssize_t ret_size = 0;
+
+	if (gpio_dev->props.policy & GPIO_INPUT)
+		gpio_dev->props.pin_level = gpio_get_pin(gpio_dev->props.pin_nr);
+	
+	spin_lock(&gpio_dev->lock);
+	ret_size += sprintf(buf, "%i\n", gpio_dev->props.pin_level);
+	spin_unlock(&gpio_dev->lock);
+
+	return ret_size;
+}
+
+static ssize_t gpio_store_level(struct class_device *dev, const char *buf, size_t size)
+{
+	struct gpio_device *gpio_dev = to_gpio_device(dev);
+	long value;
+
+	if (gpio_dev->props.policy & GPIO_INPUT) 
+		return -EINVAL;
+	
+	value = simple_strtol(buf, NULL, 10);
+	if ((value != 0) && (value != 1)) 
+		return -EINVAL;
+	gpio_dev->props.pin_level = value;
+
+	/* set real hardware */
+	switch (value) {
+		case 0:  gpio_clear_pin(gpio_dev->props.pin_nr); 
+			 gpio_dir_output(gpio_dev->props.pin_nr); 
+			 break;
+		case 1:  gpio_set_pin(gpio_dev->props.pin_nr); 
+			 gpio_dir_output(gpio_dev->props.pin_nr);
+			 break;
+		default: break;
+	}
+	return size;
+}
+
+/* 
+ * Attribute: /sys/class/gpio/gpioX/policy 
+ */
+static struct class_device_attribute attr_gpio_policy = {
+	.attr = { .name = "policy", .mode = 0444, .owner = THIS_MODULE },
+	.show = gpio_show_policy,
+	.store = NULL, 
+};
+
+static ssize_t gpio_show_policy(struct class_device *dev, char *buf)
+{
+	struct gpio_device *gpio_dev = to_gpio_device(dev);
+	ssize_t ret_size = 0;
+	
+	spin_lock(&gpio_dev->lock);
+	if (gpio_dev->props.policy & GPIO_USER)
+		ret_size += sprintf(buf,"userspace\n");
+	if (gpio_dev->props.policy & GPIO_KERNEL)
+		ret_size += sprintf(buf,"kernel\n");
+	spin_unlock(&gpio_dev->lock);
+
+	return ret_size;
+}
+
+static int gpio_read_proc(char *page, char **start, off_t off,
+			  int count, int *eof, void *data)
+{
+	char *p = page;
+	int size = 0;
+	struct gpio_device *gpio_dev;
+	struct list_head *lact, *ltmp;
+
+	if (off != 0)
+		goto end;
+
+	p += sprintf(p, "GPIO   POLICY       DIRECTION    OWNER\n");
+	list_for_each_safe(lact, ltmp, &gpio_list) {
+		gpio_dev = list_entry(lact, struct gpio_device, list);
+		p += sprintf(p, "%3i:   ", gpio_dev->props.pin_nr);
+		if (gpio_dev->props.policy & GPIO_KERNEL)
+			p += sprintf(p, "kernel       ");
+		if (gpio_dev->props.policy & GPIO_USER)
+			p += sprintf(p, "user space   ");
+		if (gpio_dev->props.policy & GPIO_INPUT)
+			p += sprintf(p, "input        ");
+		if (gpio_dev->props.policy & GPIO_OUTPUT)
+			p += sprintf(p, "output       ");
+		p += sprintf(p, "%s\n", gpio_dev->props.owner);
+	}
+end:
+	size = (p - page);
+	if (size <= off + count)
+		*eof = 1;
+	*start = page + off;
+	size -= off;
+	if (size > count)
+		size = count;
+	if (size < 0)
+		size = 0;
+
+	return size;
+}
+
+/** 
+ * request_gpio - register a new object of gpio_device class.  
+ *
+ * @pin_nr:     GPIO pin which is registered
+ * @owner:      name of the driver that owns this pin
+ * @policy:     set policy for this pin, which is one of these: 
+ * 		- GPIO_USER or GPIO_KERNEL
+ * 		- GPIO_INPUT or GPIO_OUTPUT
+ * 		For user space registered pins a sysfs entry is added. 
+ * @init_level: initially configured pin level
+ */
+int request_gpio(unsigned int pin_nr, const char *owner,
+		 unsigned char policy, unsigned char init_level)
+{
+	int rc;
+	struct gpio_device *gpio_dev;
+	struct list_head *lact, *ltmp;
+
+	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;
+		}
+	}
+
+	/* value checks - FIXME: is there a logical xor in C? */
+	if ( ( (policy & GPIO_USER) &&  (policy & GPIO_KERNEL)) ||
+	     (!(policy & GPIO_USER) && !(policy & GPIO_KERNEL))) {
+		printk(KERN_ERR "%s: policy has to be one of GPIO_KERNEL, GPIO_USER\n", DRIVER_NAME); 
+		return -EINVAL;
+	}
+	if ( ( (policy & GPIO_INPUT) &&  (policy & GPIO_OUTPUT)) ||
+	     (!(policy & GPIO_INPUT) && !(policy & GPIO_OUTPUT))) {
+		printk(KERN_ERR "%s: policy has to be one of GPIO_INPUT, GPIO_OUTPUT\n", DRIVER_NAME); 
+		return -EINVAL;
+	}
+
+	gpio_dev = kmalloc(sizeof(struct gpio_device), GFP_KERNEL);
+	
+	if (unlikely(!gpio_dev)) {
+		printk(KERN_ERR "%s: couldn't allocate memory\n", DRIVER_NAME);
+		return -ENOMEM;
+	}
+
+	spin_lock_init(&gpio_dev->lock);
+	gpio_dev->props.pin_nr = pin_nr;
+	gpio_dev->props.policy = policy;
+	gpio_dev->props.pin_level = init_level;
+	gpio_dev->props.gpio_dev = gpio_dev;
+	strncpy(gpio_dev->props.owner, owner, 20);
+
+	memset(&gpio_dev->class_dev, 0, sizeof(gpio_dev->class_dev));
+	gpio_dev->class_dev.class = &gpio_class;
+	snprintf(gpio_dev->class_dev.class_id, BUS_ID_SIZE, "gpio%i", pin_nr);
+
+	rc = class_device_register(&gpio_dev->class_dev);
+	if (unlikely(rc)) {
+		printk(KERN_ERR "%s: class registering failed\n", DRIVER_NAME);
+		kfree(gpio_dev);
+		return rc;
+	}
+
+	INIT_LIST_HEAD(&gpio_dev->list);
+
+	/* register the attributes */
+	if (policy & GPIO_USER)
+		class_device_create_file(&gpio_dev->class_dev, &attr_gpio_level);
+	
+	class_device_create_file(&gpio_dev->class_dev, &attr_gpio_policy);
+
+	/* set real hardware */
+	if (policy & GPIO_OUTPUT) {
+		switch (init_level) {
+			case 0: gpio_clear_pin(pin_nr); break;
+			case 1: gpio_set_pin(pin_nr); break;
+			default: break; 
+		}
+		gpio_dir_output(pin_nr); 
+	}
+
+	write_lock(&gpio_list_lock);
+	list_add_tail(&gpio_dev->list, &gpio_list);
+	write_unlock(&gpio_list_lock);
+	
+	printk(KERN_INFO "registered gpio%i\n", pin_nr);
+
+	return 0;
+}
+EXPORT_SYMBOL(request_gpio);
+
+/**
+ * free_gpio - unregisters a object of gpio_properties class.
+ *
+ * @pin_nr: pin number to free. 
+ *
+ * Unregisters a previously registered via request_gpio object.
+ */
+void free_gpio(unsigned int pin_nr)
+{
+	struct gpio_device *gpio_dev;
+	struct list_head *lact, *ltmp;
+
+	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_INFO "unregistering gpio pin %i\n", pin_nr);
+
+			/* unregister attributes */
+			if (gpio_dev->props.policy & GPIO_USER)
+				class_device_remove_file(&gpio_dev->class_dev,
+							 &attr_gpio_level);
+
+			class_device_remove_file(&gpio_dev->class_dev, &attr_gpio_level);
+
+			class_device_unregister(&gpio_dev->class_dev);
+			write_lock(&gpio_list_lock);
+			list_del(&gpio_dev->list);
+			write_unlock(&gpio_list_lock);
+			kfree(gpio_dev);
+			return;
+		}
+	}
+}
+EXPORT_SYMBOL(free_gpio);
+
+void free_all_gpios(void)
+{
+	struct gpio_device *gpio_dev;
+	struct list_head *lact, *ltmp;
+
+	list_for_each_safe(lact, ltmp, &gpio_list) {
+		gpio_dev = list_entry(lact, struct gpio_device, list);
+
+		printk(KERN_INFO "unregistering gpio pin %i\n", gpio_dev->props.pin_nr);
+
+		/* unregister attributes */
+		if (gpio_dev->props.policy & GPIO_USER)
+			class_device_remove_file(&gpio_dev->class_dev,
+						 &attr_gpio_level);
+
+		class_device_remove_file(&gpio_dev->class_dev, &attr_gpio_level);
+
+		class_device_unregister(&gpio_dev->class_dev);
+		write_lock(&gpio_list_lock);
+		list_del(&gpio_dev->list);
+		write_unlock(&gpio_list_lock);
+		kfree(gpio_dev);
+		return;
+	}
+}
+EXPORT_SYMBOL(free_all_gpios);
+
+static struct sysdev_class gpio_sysclass = {
+	set_kset_name("gpio"),
+};
+
+static struct sys_device gpio_sys_device = {
+	.id		= 0,
+	.cls		= &gpio_sysclass,
+};
+
+static int gpio_setup(char *s)
+{
+	char *p, *q = NULL;
+	int pin_nr;
+	unsigned char policy;
+	unsigned char init_level = 0;
+
+	while ((p = strsep(&s, ".,")) != NULL) {
+
+		/* end of command reached? -> next command */
+		if (*p == '\0')
+			continue;
+
+		/* process one command */
+		pin_nr = -1;
+		policy = 0;
+		q = strsep(&p, ":");
+		if (*q == '\0') {
+			printk("%s: invalid token (scanning pin_nr): %s\n", DRIVER_NAME, p); 
+			continue;
+		}
+		pin_nr = simple_strtoul(q, NULL, 0);	/* FIXME: this doesn't detect if no number is given! */
+		q = strsep(&p, ":");
+		if (*q == '\0') {
+			printk("%s: invalid token (scanning direction): %s\n", DRIVER_NAME, p); 
+			continue;
+		}
+		policy |= strncmp(q, "in", strlen(q)) == 0 ? GPIO_INPUT : 0;
+		policy |= strncmp(q, "out", strlen(q)) == 0 ? GPIO_OUTPUT : 0;
+		if (policy == 0) {
+			printk("%s: invalid token (scanning policy): %s\n", DRIVER_NAME, p); 
+			continue;
+		}
+		if (policy & GPIO_OUTPUT) {
+			q = strsep(&p, ":");
+			if (*q == '\0') {
+				printk("%s: invalid token (scanning init_level): %s\n", DRIVER_NAME, p); 
+				continue;
+			}
+			init_level = 2;
+			if (strncmp(q, "hi", strlen(q)) == 0)
+				init_level = 1;
+			if (strncmp(q, "lo", strlen(q)) == 0)
+				init_level = 0;
+			if (init_level == 2) {
+				printk("%s: invalid token (scanning level_value): %s\n", DRIVER_NAME, p); 
+				continue;
+			}
+		}
+		policy |= GPIO_USER;
+		if (request_gpio(pin_nr, "kernel", policy, init_level) != 0) {
+			printk(KERN_ERR
+			       "%s: could not register GPIO pins from commandline!\n",
+			       DRIVER_NAME);
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
+static int __init gpio_init(void)
+{
+	int ret;
+
+	printk(KERN_INFO "Initialising gpio device class.\n");
+
+	class_register(&gpio_class);
+
+	ret = sysdev_class_register(&gpio_sysclass);
+	if (ret) {
+		printk(KERN_ERR "%s: couldn't register sysdev class, exiting\n", DRIVER_NAME);
+		goto out_sysdev_class;
+	}
+
+	ret = sysdev_register(&gpio_sys_device);
+	if (ret) {
+		printk(KERN_ERR "%s: couldn't register sysdev, exiting\n", DRIVER_NAME);
+		goto out_sysdev_register;
+	}
+
+        if (!create_proc_read_entry ("gpio", 0, 0, gpio_read_proc, NULL)) {
+		printk(KERN_ERR "%s: couldn't register proc entry, exiting\n", DRIVER_NAME);
+		goto out_proc;
+	}
+
+	/* parse command line parameters */
+	printk(KERN_INFO "%s: mapping=", DRIVER_NAME);
+	if (strcmp(mapping,"")) {
+		printk("%s\n", mapping);
+		if (gpio_setup(mapping) != 0) {
+			printk(KERN_ERR "%s: could not register ('mapping=...'), exiting\n", DRIVER_NAME);
+		}
+	} else {
+		printk("EMPTY\n"); 
+	};
+
+	return ret;
+
+out_proc:
+	sysdev_unregister(&gpio_sys_device);
+out_sysdev_register:
+	sysdev_class_unregister(&gpio_sysclass);
+out_sysdev_class:
+	class_unregister(&gpio_class);
+	return ret;
+}
+
+module_init(gpio_init);
Index: kernel/Makefile
===================================================================
--- a/kernel/Makefile	(.../vanilla/linux-2.6.8-rc2)	(revision 205)
+++ b/kernel/Makefile	(.../linux-pxa/trunks/linux-2.6.8-rc2-trunk)	(revision 205)
@@ -23,6 +23,7 @@
 obj-$(CONFIG_STOP_MACHINE) += stop_machine.o
 obj-$(CONFIG_AUDIT) += audit.o
 obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
+obj-$(CONFIG_GPIO) += gpio.o
 
 ifneq ($(CONFIG_IA64),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
Index: include/asm-arm/arch-pxa/gpio.h
===================================================================
--- a/include/asm-arm/arch-pxa/gpio.h	(revision 0)
+++ b/include/asm-arm/arch-pxa/gpio.h	(revision 205)
@@ -0,0 +1,49 @@
+/*
+ * linux/include/asm-arm/arch-pxa/gpio.h
+ *
+ * Copyright (C) 2004 Robert Schwebel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __ARM_PXA_GPIO_H
+#define __ARM_PXA_GPIO_H
+
+#include <linux/kernel.h>
+#include <asm/arch/hardware.h>
+
+#define DEBUG 1
+
+static inline void gpio_set_pin(int gpio_nr)
+{
+	GPSR(gpio_nr) |= GPIO_bit(gpio_nr);
+	return;
+}
+
+static inline void gpio_clear_pin(int gpio_nr)
+{
+	GPCR(gpio_nr) |= GPIO_bit(gpio_nr);
+	return;
+}
+
+static inline void gpio_dir_input(int gpio_nr)
+{
+	GPDR(gpio_nr) &= ~GPIO_bit(gpio_nr);
+	return;
+}
+
+static inline void gpio_dir_output(int gpio_nr)
+{
+	GPDR(gpio_nr) |= GPIO_bit(gpio_nr);
+	return;
+}
+
+static inline int gpio_get_pin(int gpio_nr)
+{
+	return GPLR(gpio_nr) & GPIO_bit(gpio_nr) ? 1:0;
+}
+
+
+#endif
Index: include/linux/gpio.h
===================================================================
--- a/include/linux/gpio.h	(revision 0)
+++ b/include/linux/gpio.h	(revision 205)
@@ -0,0 +1,27 @@
+/*
+ * include/linux/gpio.h
+ *
+ * Copyright (C) 2004 Robert Schwebel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __GPIO_H
+#define __GPIO_H
+
+#include "asm/arch/gpio.h"
+
+/* Values for policy */
+#define GPIO_KERNEL     (1<<0)
+#define GPIO_USER       (1<<1)
+#define GPIO_INPUT      (1<<2)
+#define GPIO_OUTPUT     (1<<3)
+
+int request_gpio(unsigned int pin_nr, const char *owner,
+		 unsigned char policy, unsigned char init_level);
+
+void free_gpio(unsigned int pin_nr);
+
+#endif
Index: init/Kconfig
===================================================================
--- a/init/Kconfig	(.../vanilla/linux-2.6.8-rc2)	(revision 205)
+++ b/init/Kconfig	(.../linux-pxa/trunks/linux-2.6.8-rc2-trunk)	(revision 205)
@@ -294,6 +294,18 @@
 
 	  If unsure, say N.
 
+config GPIO
+	bool "GPIO pin support"
+	default y if ARM || PPC
+	default n
+	help
+	  Enabling this option adds support for generic GPIO pins. Most
+	  System-on-Chip processors have this kind of pins.
+
+	  FIXME: write more documentation. 
+
+	  If unsure, say N. 
+
 endmenu		# General setup
 
 

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

[Pavel Machek]

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?

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

[John Lenz]

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

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

[John Lenz]

On 09/03/04 07:06:34, Jan-Benedict Glaw wrote:
> On Fri, 2004-09-03 13:54:04 +0900, Kalin KOZHUHAROV <kalin@thinrope.net>
> wrote in message <ch8tdd$1uf$1@sea.gmane.org>:
> > 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

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

[Russell King]

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

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

[John Lenz]

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

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

[Russell King]

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

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

[John Lenz]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

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 <lenz@cs.wisc.edu>

[-- Attachment #2: leds_sysfs.patch --]
[-- Type: text/x-patch, Size: 9728 bytes --]


#
# Patch managed by http://www.holgerschurig.de/patcher.html
#

--- /dev/null
+++ linux/drivers/leds/ledscore.c
@@ -0,0 +1,281 @@
+/*
+ * linux/drivers/leds/ledscore.c
+ *
+ *   Copyright (C) 2004 John Lenz <lenz@cs.wisc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/device.h>
+#include <linux/sysdev.h>
+#include <linux/timer.h>
+#include <linux/leds.h>
+
+#define LEDS_TIMER_INTERVAL (HZ * 5)
+
+struct led_device {
+	/* This protects the props field.*/
+	spinlock_t lock;
+	/* If props is NULL, the driver that registered this device has been unloaded */
+	struct led_properties *props;
+	struct class_device class_dev;
+	struct list_head list;
+};
+
+#define to_led_device(d) container_of(d, struct led_device, class_dev)
+
+static rwlock_t leds_list_lock = RW_LOCK_UNLOCKED;
+static LIST_HEAD(leds_list);
+static atomic_t leds_count = ATOMIC_INIT(0);
+
+static void leds_class_release(struct class_device *dev)
+{
+	struct led_device *d = to_led_device(dev);
+
+	write_lock(&leds_list_lock);
+		list_del(&d->list);
+	write_unlock(&leds_list_lock);
+	
+	kfree(d);
+}
+
+static struct class leds_class = {
+	.name		= "leds",
+	.release	= leds_class_release,
+};
+
+static ssize_t leds_show_color(struct class_device *dev, char *buf)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	ssize_t ret = 0;
+	
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		sprintf(buf, "%s\n", led_dev->props->color);
+		ret = strlen(buf) + 1;
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static CLASS_DEVICE_ATTR(color, 0444, leds_show_color, NULL);
+
+static ssize_t leds_show_light(struct class_device *dev, char *buf)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	ssize_t ret = 0;
+
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		sprintf(buf, "%lu\n", led_dev->props->light_state);
+		ret = strlen(buf) + 1;
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static ssize_t leds_store_light(struct class_device *dev, const char *buf, size_t size)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	int ret = -EINVAL;
+	char *after;
+
+	unsigned long state = simple_strtoul(buf, &after, 10);
+	if (after - buf > 0) {
+		ret = after - buf;
+		spin_lock(&led_dev->lock);
+		if (likely(led_dev->props)) {
+			if (!led_dev->props->heartbeat_enabled) {
+				led_dev->props->light_state = state;
+				if (led_dev->props->light) 
+					led_dev->props->light(led_dev->class_dev.dev, led_dev->props);
+			}
+		}
+		spin_unlock(&led_dev->lock);
+	}
+
+	return ret;
+}
+
+static CLASS_DEVICE_ATTR(light, 0644, leds_show_light, leds_store_light);
+
+static ssize_t leds_show_heartbeat(struct class_device *dev, char *buf)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	ssize_t ret = 0;
+
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		sprintf(buf, "%lu\n", led_dev->props->heartbeat_enabled);
+		ret = strlen(buf) + 1;
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static ssize_t leds_store_heartbeat(struct class_device *dev, const char *buf, size_t size)
+{
+	struct led_device *led_dev = to_led_device(dev);
+	int ret = -EINVAL;
+
+	spin_lock(&led_dev->lock);
+	if (likely(led_dev->props)) {
+		if (buf[0] == '1') {
+			led_dev->props->heartbeat_enabled = 1;
+		} else {
+			led_dev->props->heartbeat_enabled = 0;
+		}
+	}
+	spin_unlock(&led_dev->lock);
+
+	return ret;
+}
+
+static CLASS_DEVICE_ATTR(heartbeat_enabled, 0644, leds_show_heartbeat, leds_store_heartbeat);
+
+/**
+ * leds_device_register - register a new object of led_device class.
+ * @dev: The device to register.
+ * @prop: the led properties structure for this device.
+ */
+int leds_device_register(struct device *dev, struct led_properties *props)
+{
+	int rc, num;
+	struct led_device *new_led;
+
+	new_led = kmalloc (sizeof (struct led_device), GFP_KERNEL);
+	if (unlikely (!new_led))
+		return -ENOMEM;
+
+	spin_lock_init(&new_led->lock);
+	new_led->props = props;
+	props->led_dev = new_led;
+
+	memset (&new_led->class_dev, 0, sizeof (new_led->class_dev));
+	new_led->class_dev.class = &leds_class;
+	new_led->class_dev.dev = dev;
+
+	/* assign this led a number */
+	num = atomic_add_return(1, &leds_count);
+	sprintf(new_led->class_dev.class_id, "%i", num);
+
+	rc = class_device_register (&new_led->class_dev);
+	if (unlikely (rc)) {
+		kfree (new_led);
+		return rc;
+	}
+
+	/* register the attributes */
+	class_device_create_file(&new_led->class_dev, &class_device_attr_color);
+	class_device_create_file(&new_led->class_dev, &class_device_attr_light);
+	class_device_create_file(&new_led->class_dev, &class_device_attr_heartbeat_enabled);
+
+	/* add to the list of leds */
+	write_lock(&leds_list_lock);
+		list_add_tail(&new_led->list, &leds_list);
+	write_unlock(&leds_list_lock);
+
+	printk(KERN_INFO "Registered led device: number=%s, color=%s\n", new_led->class_dev.class_id, props->color);
+
+	return 0;
+}
+EXPORT_SYMBOL(leds_device_register);
+
+/**
+ * led_device_unregister - unregisters a object of led_properties class.
+ * @props: the property to unreigister
+ *
+ * Unregisters a previously registered via led_device_register object.
+ */
+void leds_device_unregister(struct led_properties *props)
+{
+	struct led_device *led_dev;
+	if (!props)
+		return;
+
+	pr_debug("led_device_unregister: color=%s\n", props->color);
+
+	led_dev = props->led_dev;
+
+	class_device_remove_file (&led_dev->class_dev, &class_device_attr_heartbeat_enabled);
+	class_device_remove_file (&led_dev->class_dev, &class_device_attr_light);
+	class_device_remove_file (&led_dev->class_dev, &class_device_attr_color);
+
+	spin_lock(&led_dev->lock);
+	led_dev->props = NULL;
+	props->led_dev = NULL;
+	spin_unlock(&led_dev->lock);
+
+	class_device_unregister(&led_dev->class_dev);
+}
+EXPORT_SYMBOL(leds_device_unregister);
+
+/* set up a kernel timer */
+static struct timer_list	leds_ktimer;
+static atomic_t 		leds_stop_timer = ATOMIC_INIT(0);
+
+static void leds_timer_function(unsigned long data)
+{
+	struct led_device *led_dev;
+
+	read_lock(&leds_list_lock);
+	list_for_each_entry(led_dev, &leds_list, list) {
+		spin_lock(&led_dev->lock);
+		if (likely(led_dev->props)) {
+			if (led_dev->props->heartbeat_enabled) {
+				led_dev->props->light_state = !led_dev->props->light_state;
+				if (led_dev->props->light) 
+					led_dev->props->light(led_dev->class_dev.dev, led_dev->props);
+			}
+		}
+		spin_unlock(&led_dev->lock);
+	}
+	read_unlock(&leds_list_lock);
+
+	if (!atomic_read(&leds_stop_timer))
+		mod_timer(&leds_ktimer, jiffies + LEDS_TIMER_INTERVAL);
+}
+
+static int __init leds_init(void)
+{
+	int ret = 0;
+
+	/* initialize the class device */
+	class_register(&leds_class);
+
+	/* register the timer */
+	init_timer(&leds_ktimer);
+	leds_ktimer.function = leds_timer_function;
+	leds_ktimer.data = 0;
+	leds_ktimer.expires = jiffies + LEDS_TIMER_INTERVAL;
+	add_timer(&leds_ktimer);
+	
+	return ret;
+}
+subsys_initcall(leds_init);
+
+static void __exit leds_exit(void)
+{
+	class_unregister(&leds_class);
+
+	atomic_set(&leds_stop_timer, 1);
+	
+	del_timer_sync(&leds_ktimer);
+}
+module_exit(leds_exit);
+
+MODULE_AUTHOR("John Lenz");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("LED core class interface");
+
--- /dev/null
+++ linux/include/linux/leds.h
@@ -0,0 +1,46 @@
+/*
+ *  linux/include/leds.h
+ *
+ *  Copyright (C) 2004 John Lenz <lenz@cs.wisc.edu>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver model for leds
+ */
+#ifndef ASM_ARM_LEDS_H
+#define ASM_ARM_LEDS_H
+
+#include <linux/device.h>
+
+struct led_device;
+
+struct led_properties {
+	struct module *owner;
+	
+	/* Color of the led.  For multiple color leds, the color names should
+	 * be seperated by a "/".  For example, "amber/green".
+	 */
+	char *color;
+	
+	/* current state of this led.
+	 * 0 = off, 1,2,3,... = on, where the number is the posision in the list
+	 * of colors given above.
+	 */
+	unsigned long light_state;
+
+	/* heartbeat enabled on this led */
+	int heartbeat_enabled;
+
+	/* This function is called after the light_state property is changed. */
+	void (*light)(struct device *, struct led_properties *props);
+	
+	/* private structure */
+	struct led_device *led_dev;
+};
+
+int leds_device_register(struct device *dev, struct led_properties *props);
+void leds_device_unregister(struct led_properties *props);
+
+#endif
--- /dev/null
+++ linux/drivers/leds/Kconfig
@@ -0,0 +1,11 @@
+
+menu "LED devices"
+
+config CLASS_LEDS
+	tristate "LED support"
+	help
+	  This option provides the generic support for the leds class.
+	  LEDs can be accessed from /sys/class/leds.  It will also allow you
+	  to select individual drivers for LED devices.  If unsure, say N.
+
+endmenu
--- /dev/null
+++ linux/drivers/leds/Makefile
@@ -0,0 +1,3 @@
+
+# Core functionality.
+obj-$(CONFIG_CLASS_LEDS)		+= ledscore.o
--- linux/drivers/Kconfig~leds_sysfs
+++ linux/drivers/Kconfig
@@ -48,6 +48,8 @@
 
 source "drivers/media/Kconfig"
 
+source "drivers/leds/Kconfig"
+
 source "drivers/video/Kconfig"
 
 source "sound/Kconfig"
--- linux/drivers/Makefile~leds_sysfs
+++ linux/drivers/Makefile
@@ -50,4 +50,5 @@
 obj-$(CONFIG_MCA)		+= mca/
 obj-$(CONFIG_EISA)		+= eisa/
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq/
+obj-$(CONFIG_CLASS_LEDS)	+= leds/
 obj-y				+= firmware/

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

[Russell King]

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

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

[John Lenz]

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

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

[Pavel Machek]

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

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

[Pavel Machek]

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

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

[Russell King]

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

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

[Pavel Machek]

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

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

[John Lenz]

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