On Fri, Jul 25, 2008 at 02:01:45PM -0700, Trent Piepho wrote:
> Add bindings to support LEDs defined as of_platform devices in addition to
> the existing bindings for platform devices.
(adding [EMAIL PROTECTED] to the cc list)
> New options in Kconfig allow the platform binding code and/or the
> of_platform code to be turned on. The of_platform code is of course only
> available on archs that have OF support.
>
> The existing probe and remove methods are refactored to use new functions
> create_gpio_led(), to create and register one led, and delete_gpio_led(),
> to unregister and free one led. The new probe and remove methods for the
> of_platform driver can then share most of the common probe and remove code
> with the platform driver.
>
> The suspend and resume methods aren't shared, but they are very short. The
> actual led driving code is the same for LEDs created by either binding.
I like this approach. I think it is a good pattern.
> The OF bindings are based on patch by Anton Vorontsov
> <[EMAIL PROTECTED]>. They have been extended to allow multiple LEDs
> per device.
>
> Signed-off-by: Trent Piepho <[EMAIL PROTECTED]>
> ---
> Documentation/powerpc/dts-bindings/gpio/led.txt | 44 ++++-
> drivers/leds/Kconfig | 21 ++-
> drivers/leds/leds-gpio.c | 225
> ++++++++++++++++++-----
> 3 files changed, 236 insertions(+), 54 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt
> b/Documentation/powerpc/dts-bindings/gpio/led.txt
> index ff51f4c..ed01297 100644
> --- a/Documentation/powerpc/dts-bindings/gpio/led.txt
> +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
> @@ -1,15 +1,39 @@
> -LED connected to GPIO
> +LEDs connected to GPIO lines
>
> Required properties:
> -- compatible : should be "gpio-led".
> -- label : (optional) the label for this LED. If omitted, the label is
> - taken from the node name (excluding the unit address).
> -- gpios : should specify LED GPIO.
> +- compatible : should be "gpio-leds".
>
> -Example:
> +Each LED is represented as a sub-node of the gpio-leds device. Each
> +node's name represents the name of the corresponding LED.
>
> [EMAIL PROTECTED] {
> - compatible = "gpio-led";
> - label = "hdd";
> - gpios = <&mcu_pio 0 1>;
> +LED node properties:
> +- gpios : Should specify the LED GPIO.
Question: it is possible/desirable for a single LED to be assigned
multiple GPIO pins? Say, for a tri-color LED? (I'm fishing for
opinions; I really don't know if it would be a good idea or not)
> +- label : (optional) The label for this LED. If omitted, the label
> + is taken from the node name (excluding the unit address).
> +- 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.
> + memset(&led, 0, sizeof(led));
> + for_each_child_of_node(np, child) {
> + led.gpio = of_get_gpio(child, 0);
> + led.name = of_get_property(child, "label", NULL) ? :
> child->name;
> + led.default_trigger =
> + of_get_property(child, "linux,default-trigger", NULL);
This isn't in the documented binding. I assume that you mean 'function'
here?
Otherwise, the code looks pretty good to me.
g.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
