Re: [PATCH 2/2] leds: Support OpenFirmware led bindings

On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]

quoted

quoted

quoted

+- function :  (optional) This parameter, if present, is a string
+  defining the function of the LED.  It can be used to put the LED
+  under software control, e.g. Linux LED triggers like "heartbeat",
+  "ide-disk", and "timer".  Or it could be used to attach a hardware
+  signal to the LED, e.g. a SoC that can configured to put a SATA
+  activity signal on a GPIO line.

This makes me nervous.  It exposes Linux internal implementation details
into the device tree data.  If you want to have a property that
describes the LED usage, then the possible values and meanings should be
documented here.

Should it be a linux specific property then?  I could list all the current
linux triggers, but enumerating every possible function someone might want
to assign to an LED seems hopeless.

I don't like adding Linux specific properties to the device tree if at
all possible, and I really don't like encoding Linux internal details
(like trigger names).  They can change between kernel versions and
breaking compatibility with older device trees is strongly avoided.
That's why so much effort goes into getting bindings correct the first
time.

I'd rather see the device tree provide 'hints' toward the expected usage
and if a platform needs something specific, then the platform specific
code should setup the trigger.

Regardless, any hints provided by the binding must be documented.  In
most cases the gpio-leds driver should be able to figure out which trigger
to bind without platform code intervention.

Maybe we can encode leds into devices themselves, via phandles?

E.g.

sata@101 {
	compatible = "fsl,sata";
	leds = <&red_led>;
};

And then the OF GPIO LEDs driver could do something like:

char *ide_disk_trigger_compatibles[] = {
	"fsl,sata",
	"ide-generic",
	...
};

for_each_node_with_leds_property(node, led_phandle) {
	if (if_ide_disk_compatible(node)) {
		struct gpio_led *led = phandle_to_led(led_phandle);

		led->default_trigger = "ide-disk";
	}
}

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2