[PATCH 0/12] LED Class, Triggers and Drivers

[PATCH 0/12] LED Class, Triggers and Drivers

[Richard Purdie]

This is an updated version of the LED class/subsystem. The main change
is the renamed API - I've settled on led_device -> led_classdev. Other
minor issues like the error cases in the timer trigger were also fixed.

I've not received any feed back on what the kconfig name for the class
should be (something better than NEW_LEDS is needed but arm is using
LEDS).

I've included a modified ide-disk trigger at the end of the series along
with a patch which fixes the IDE system so it calls a driver's
end_request function in all cases. This fix has yet to be confirmed by
the IDE developers. An alternative is to add the hooks into ide-io.c
start/end_request instead.

To quote my previous introduction:

The LED class/subsystem takes John Lenz's work and extends and alters it
to give what I think should be a fairly universal LED implementation.

The series consists of several logical units:

* LED Core + Class implementation
* LED Trigger Core implementation
* LED timer trigger (example of a complex trigger)
* LED device drivers for corgi, spitz and tosa Zaurus models
* LED device driver for locomo LEDs
* LED device driver for ARM ixp4xx LEDs
* Zaurus charging LED trigger
* IDE disk activity LED trigger
* NAND MTD activity LED trigger


Why?
====

LEDs are really simple devices usually amounting to a GPIO that can be
turned on and off so why do we need all this code? On handheld or
embedded devices they're an important part of an often limited user
interface. Both users and developers want to be able to control and
configure what the LED does and the number of different things they'd
potentially want the LED to show is large. 

A subsystem is needed to try and provide all this different
functionality in an architecture independent, simple but complete,
generic and scalable manner.

The alternative is for everyone to implement just what they need hidden
away in different corners of the kernel source tree and to provide an
inconsistent interface to userspace.


Other Implementations
=====================

I'm aware of the existing arm led implementation. Currently the new
subsystem and the arm code can coexist quite happily. Its up to the arm
community to decide whether this new interface is acceptable to them. As
far as I can see, the new interface can do everything the existing arm
implementation can with the advantage that the new code is architecture
independent and much more generic, configurable and scalable.

I'm prepared to make the conversion to the LED subsystem (or assist with
it) if appropriate.


Implementation Details
======================

I've stripped a lot of code out of John's original LED class. Colours
were removed as LED colour is now part of the device name. Multiple
colours are to be handled as multiple led devices. This means you get
full control over each colour. I also removed the LED hardware timer
code as the generic timer isn't going to add much overhead and is just
as useful. I also decided to have the LED core track the current LED
status (to ease suspend/resume handling) removing the need for
brightness_get implementations in the LED drivers.

An underlying design philosophy is simplicity. The aim is to keep a
small amount of code giving as much functionality as possible.

The major new idea is the led "trigger". A trigger is a source of led
events. Triggers can either be simple or complex. A simple trigger isn't
configurable and is designed to slot into existing subsystems with
minimal additional code. Examples are the ide-disk, nand-disk and
zaurus-charging triggers. With leds disabled, the code optimises away.
Examples are nand-disk and ide-disk.

Complex triggers whilst available to all LEDs have LED specific
parameters and work on a per LED basis. The timer trigger is an example.

You can change triggers in a similar manner to the way an IO scheduler
is chosen (via /sys/class/leds/somedevice/trigger).

So far there are only a handful of examples but it should easy to add
further LED triggers without too much interference into other
subsystems.


Known Issues
============

The LED Trigger core cannot be a module as the simple trigger functions
would cause nightmare dependency issues. I see this as a minor issue
compared to the benefits the simple trigger functionality brings. The
rest of the LED subsystem can be modular.

Some leds can be programmed to flash in hardware. As this isn't a
generic LED device property, I think this should be exported as a device
specific sysfs attribute rather than part of the class if this
functionality is required (eg. to keep the led flashing whilst the
device is suspended).


Future Development
==================

At the moment, a trigger can't be created specifically for a single LED.
There are a number of cases where a trigger might only be mappable to a
particular LED. The addition of triggers provided by the LED driver
should cover this option and be possible to add without breaking the
current interface.

A cpu activity trigger similar to that found in the arm led
implementation should be trivial to add.


Richard

RE: [PATCH 0/12] LED Class, Triggers and Drivers

[John Bowler]

From: Richard Purdie [mailto:rpurdie@rpsys.net]
>This is an updated version of the LED class/subsystem. The main change
>is the renamed API - I've settled on led_device -> led_classdev. Other
>minor issues like the error cases in the timer trigger were also fixed.

In the previous version 'frequency' for the timer trigger was actually
half the period of the oscillation - the time in ms between each (on/off)
state transition.

In this version 'frequency' is the frequency of the oscillation in Hz.

That creates a big problem for me because the value is parsed as an
integer and I can no longer achieve slow flash rates (<1Hz).  Since I
have to be able to do this I made a fairly crude patch to store the
frequency in mHz, not Hz, and to handle a decimal point in the value.

Possible fixes:

1) Use 'period' not 'frequency' and accept a value in ms (as before,
   but with the off-by-2 error corrected.)
2) Use 'mark' and 'space' or 'time_on', 'time_off' or something similar
   and remove 'duty' (I *do* need flashing with duty cycle != 0.5)
3) Accept fractional frequency (as in my patch).
4) Provide both period and frequency as integers and accept that
   long period flashs will come out as frequency '1' (but the period
   would still need to be in ms, because periods about 1500ms are of
   significant utility.)

John Bowler <jbowler@acm.org>

Re: [PATCH 0/12] LED Class, Triggers and Drivers

[Pavel Machek]

Hi!

Seems okay to me. It would be nice if it could be merged to -mm -- I
think it is ready, and it is possible to improve it further there...

								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 0/12] LED Class, Triggers and Drivers

[Jiri Slaby]

Richard Purdie wrote:
>This is an updated version of the LED class/subsystem. The main change
>is the renamed API - I've settled on led_device -> led_classdev. Other
>minor issues like the error cases in the timer trigger were also fixed.
Seems good, but some issues:
Use more macros
	struct led_classdev *led_cdev = dev->class_data;
	pdev->resource[i].start, pdev->resource...
__init and __exit
	static int __devinit ixp4xxgpioled_init(void)
	static void ixp4xxgpioled_exit(void)
	static void nand_base_exit(void)
	tosaled_{init,exit}, corgiled_{init,exit}, spitzled_{init,exit}
coding style
	static void __exit timer_trig_exit (void)
	static inline void led_set_brightness(struct led_classdev *led_cdev, enum led_brightness value) -- more than 80 columns
+			led_cdev->trigger->deactivate(led_cdev);
+
+	}
	ixp4xxgpioled_brightness_set -- a little bit ugly
+static int apply_to_all_leds(struct platform_device *pdev,
+	void (*operation)(struct led_classdev *pled)) {
{ on the next line

regards,
--
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
B67499670407CE62ACC8 22A032CC55C339D47A7E

RE: [PATCH 4/12] LED: Add LED Timer Trigger

[Richard Purdie]

As mentioned on LKML by John Bowler, using frequency and duty as
attributes is a bad idea as it restricts the potential usage of this
trigger. Therefore, instead export the two delay parameters directly.
Also remove unneeded locking and use class_get_devdata().

Signed-off-by: Richard Purdie <rpurdie@rpsys.net>

Index: linux-2.6.15/drivers/leds/ledtrig-timer.c
===================================================================
--- linux-2.6.15.orig/drivers/leds/ledtrig-timer.c	2006-02-08 01:42:26.000000000 +0000
+++ linux-2.6.15/drivers/leds/ledtrig-timer.c	2006-02-08 02:03:20.000000000 +0000
@@ -24,8 +24,6 @@
 #include "leds.h"
 
 struct timer_trig_data {
-	unsigned long duty;		/* duty cycle, as a percentage */
-	unsigned long frequency;	/* frequency of blinking, in Hz */
 	unsigned long delay_on;		/* milliseconds on */
 	unsigned long delay_off;	/* milliseconds off */
 	struct timer_list timer;
@@ -38,9 +36,8 @@
 	unsigned long brightness = LED_OFF;
 	unsigned long delay = timer_data->delay_off;
 
-	write_lock(&led_cdev->lock);
-
-	if (!timer_data->frequency) {
+	if (!timer_data->delay_on || !timer_data->delay_off) {
+		write_lock(&led_cdev->lock);
 		led_set_brightness(led_cdev, LED_OFF);
 		write_unlock(&led_cdev->lock);
 		return;
@@ -51,107 +48,73 @@
 		delay = timer_data->delay_on;
 	}
 
+	write_lock(&led_cdev->lock);
 	led_set_brightness(led_cdev, brightness);
+	write_unlock(&led_cdev->lock);
 
 	mod_timer(&timer_data->timer, jiffies + msecs_to_jiffies(delay));
-	write_unlock(&led_cdev->lock);
 }
 
-/* led_cdev write lock needs to be held */
-static int led_timer_setdata(struct led_classdev *led_cdev, unsigned long duty,
-				unsigned long frequency)
+static ssize_t led_delay_on_show(struct class_device *dev, char *buf)
 {
+	struct led_classdev *led_cdev = class_get_devdata(dev);
 	struct timer_trig_data *timer_data = led_cdev->trigger_data;
 
-	if (frequency > 500)
-		return -EINVAL;
-
-	if (duty > 100)
-		return -EINVAL;
-
-	timer_data->duty = duty;
-	timer_data->frequency = frequency;
-	if (frequency != 0) {
-		timer_data->delay_on = duty * 1000 / 50 / frequency / 2;
-		timer_data->delay_off = (100 - duty)*1000 / 50 / frequency / 2;
-	}
-
-	mod_timer(&timer_data->timer, jiffies + 1);
-
-	return 0;
-}
-
-static ssize_t led_duty_show(struct class_device *dev, char *buf)
-{
-	struct led_classdev *led_cdev = dev->class_data;
-	struct timer_trig_data *timer_data;
-
-	read_lock(&led_cdev->lock);
-	timer_data = led_cdev->trigger_data;
-	sprintf(buf, "%lu\n", timer_data->duty);
-	read_unlock(&led_cdev->lock);
+	sprintf(buf, "%lu\n", timer_data->delay_on);
 
 	return strlen(buf) + 1;
 }
 
-static ssize_t led_duty_store(struct class_device *dev, const char *buf,
+static ssize_t led_delay_on_store(struct class_device *dev, const char *buf,
 				size_t size)
 {
-	struct led_classdev *led_cdev = dev->class_data;
-	struct timer_trig_data *timer_data;
+	struct led_classdev *led_cdev = class_get_devdata(dev);
+	struct timer_trig_data *timer_data = led_cdev->trigger_data;
 	int ret = -EINVAL;
 	char *after;
-
 	unsigned long state = simple_strtoul(buf, &after, 10);
+
 	if (after - buf > 0) {
-		write_lock(&led_cdev->lock);
-		timer_data = led_cdev->trigger_data;
-		ret = led_timer_setdata(led_cdev, state, timer_data->frequency);
-		if (!ret)
-			ret = after - buf;			
-		write_unlock(&led_cdev->lock);
+		timer_data->delay_on = state;
+		mod_timer(&timer_data->timer, jiffies + 1);
+		ret = after - buf;
 	}
 
 	return ret;
 }
 
-static ssize_t led_frequency_show(struct class_device *dev, char *buf)
+static ssize_t led_delay_off_show(struct class_device *dev, char *buf)
 {
-	struct led_classdev *led_cdev = dev->class_data;
-	struct timer_trig_data *timer_data;
+	struct led_classdev *led_cdev = class_get_devdata(dev);
+	struct timer_trig_data *timer_data = led_cdev->trigger_data;
 
-	read_lock(&led_cdev->lock);
-	timer_data = led_cdev->trigger_data;
-	sprintf(buf, "%lu\n", timer_data->frequency);
-	read_unlock(&led_cdev->lock);
+	sprintf(buf, "%lu\n", timer_data->delay_off);
 
 	return strlen(buf) + 1;
 }
 
-static ssize_t led_frequency_store(struct class_device *dev, const char *buf,
+static ssize_t led_delay_off_store(struct class_device *dev, const char *buf,
 				size_t size)
 {
-	struct led_classdev *led_cdev = dev->class_data;
-	struct timer_trig_data *timer_data;
+	struct led_classdev *led_cdev = class_get_devdata(dev);
+	struct timer_trig_data *timer_data = led_cdev->trigger_data;
 	int ret = -EINVAL;
 	char *after;
 	unsigned long state = simple_strtoul(buf, &after, 10);
 
 	if (after - buf > 0) {
-		write_lock(&led_cdev->lock);
-		timer_data = led_cdev->trigger_data;
-		ret = led_timer_setdata(led_cdev, timer_data->duty, state);
-		if (!ret)
-			ret = after - buf;			
-		write_unlock(&led_cdev->lock);
+		timer_data->delay_off = state;
+		mod_timer(&timer_data->timer, jiffies + 1);
+		ret = after - buf;
 	}
 
 	return ret;
 }
 
-static CLASS_DEVICE_ATTR(duty, 0644, led_duty_show, led_duty_store);
-static CLASS_DEVICE_ATTR(frequency, 0644, led_frequency_show,
-			led_frequency_store);
+static CLASS_DEVICE_ATTR(delay_on, 0644, led_delay_on_show, 
+			led_delay_on_store);
+static CLASS_DEVICE_ATTR(delay_off, 0644, led_delay_off_show,
+			led_delay_off_store);
 
 static void timer_trig_activate(struct led_classdev *led_cdev)
 {
@@ -167,11 +130,10 @@
 	timer_data->timer.function = led_timer_function;
 	timer_data->timer.data = (unsigned long) led_cdev;
 
-	timer_data->duty = 50;
-
-	class_device_create_file(led_cdev->class_dev, &class_device_attr_duty);
+	class_device_create_file(led_cdev->class_dev, 
+				&class_device_attr_delay_on);
 	class_device_create_file(led_cdev->class_dev,
-				&class_device_attr_frequency);
+				&class_device_attr_delay_off);
 }
 
 static void timer_trig_deactivate(struct led_classdev *led_cdev)
@@ -180,9 +142,9 @@
 
 	if (timer_data) {
 		class_device_remove_file(led_cdev->class_dev,
-					&class_device_attr_duty);
+					&class_device_attr_delay_on);
 		class_device_remove_file(led_cdev->class_dev,
-					&class_device_attr_frequency);
+					&class_device_attr_delay_off);
 		del_timer_sync(&timer_data->timer);
 		kfree(timer_data);
 	}
@@ -199,7 +161,7 @@
 	return led_trigger_register(&timer_led_trigger);
 }
 
-static void __exit timer_trig_exit (void)
+static void __exit timer_trig_exit(void)
 {
 	led_trigger_unregister(&timer_led_trigger);
 }