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

[Anton Vorontsov]

On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]
> > >> +- 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@xxxxxxxxx
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/