From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rv-out-0506.google.com (rv-out-0506.google.com [209.85.198.229]) by ozlabs.org (Postfix) with ESMTP id C5036DDE06 for ; Sat, 25 Oct 2008 10:59:41 +1100 (EST) Received: by rv-out-0506.google.com with SMTP id f6so1040365rvb.9 for ; Fri, 24 Oct 2008 16:59:40 -0700 (PDT) Message-ID: Date: Fri, 24 Oct 2008 17:59:40 -0600 From: "Grant Likely" To: "Trent Piepho" Subject: Re: [PATCH 3/4] leds: Add option to have GPIO LEDs start on In-Reply-To: <1224889741-4167-3-git-send-email-tpiepho@freescale.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 References: <1224889741-4167-3-git-send-email-tpiepho@freescale.com> Cc: linux-kernel@vger.kernel.org, linuxppc-dev@ozlabs.org, Richard Purdie , Sean MacLennan List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On Fri, Oct 24, 2008 at 5:09 PM, Trent Piepho wrote: > Yes, there is the "default-on" trigger but there are problems with that. [...] > The platform device binding gains a field in the platform data "default_state" > that controls this. The OpenFirmware binding uses a property named > "default-state" that can be set to "on" or "off". The default the property > isn't present is off. This look much preferred to setting the the default-on trigger. Why not remove the default-on trigger entirely from the binding documentation so there is no confusion? Also, my preference would be an empty "led-default-on" property instead of "default-state" with a value that needs to be parsed, but I'm not concerned about it enough to argue. Otherwise: Acked-by: Grant Likely > > Signed-off-by: Trent Piepho > --- > Documentation/powerpc/dts-bindings/gpio/led.txt | 7 +++++++ > drivers/leds/leds-gpio.c | 8 ++++++-- > include/linux/leds.h | 1 + > 3 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt > index 9f969c2..544ded7 100644 > --- a/Documentation/powerpc/dts-bindings/gpio/led.txt > +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt > @@ -20,6 +20,11 @@ LED sub-node properties: > "heartbeat" - LED "double" flashes at a load average based rate > "ide-disk" - LED indicates disk activity > "timer" - LED flashes at a fixed, configurable rate > +- default-state: (optional) The initial state of the LED. Valid > + values are "on" and "off". If the LED is already on or off and the > + default-state property is set the to same value, then no glitch > + should be produced where the LED momentarily turns off (or on). > + The default is off if this property is not present. > > Examples: > > @@ -36,8 +41,10 @@ run-control { > compatible = "gpio-leds"; > red { > gpios = <&mpc8572 6 0>; > + default-state = "off"; > }; > green { > gpios = <&mpc8572 7 0>; > + default-state = "on"; > }; > } > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c > index f41b841..0dbad87 100644 > --- a/drivers/leds/leds-gpio.c > +++ b/drivers/leds/leds-gpio.c > @@ -92,9 +92,10 @@ static int __devinit create_gpio_led(const struct gpio_led *template, > led_dat->cdev.blink_set = gpio_blink_set; > } > led_dat->cdev.brightness_set = gpio_led_set; > - led_dat->cdev.brightness = LED_OFF; > + led_dat->cdev.brightness = template->default_state ? LED_FULL : LED_OFF; > > - gpio_direction_output(led_dat->gpio, led_dat->active_low); > + gpio_direction_output(led_dat->gpio, > + led_dat->active_low ^ template->default_state); > > INIT_WORK(&led_dat->work, gpio_led_work); > > @@ -256,12 +257,15 @@ static int __devinit of_gpio_leds_probe(struct of_device *ofdev, > memset(&led, 0, sizeof(led)); > for_each_child_of_node(np, child) { > unsigned int flags; > + const char *state; > > led.gpio = of_get_gpio(child, 0, &flags); > led.active_low = flags & OF_GPIO_ACTIVE_LOW; > led.name = of_get_property(child, "label", NULL) ? : child->name; > led.default_trigger = > of_get_property(child, "linux,default-trigger", NULL); > + state = of_get_property(child, "default-state", NULL); > + led.default_state = state && !strcmp(state, "on"); > > ret = create_gpio_led(&led, &pdata->led_data[pdata->num_leds++], > &ofdev->dev, NULL); > diff --git a/include/linux/leds.h b/include/linux/leds.h > index d3a73f5..caa3987 100644 > --- a/include/linux/leds.h > +++ b/include/linux/leds.h > @@ -138,6 +138,7 @@ struct gpio_led { > const char *default_trigger; > unsigned gpio; > u8 active_low; > + u8 default_state; > }; > > struct gpio_led_platform_data { > -- > 1.5.4.3 > > -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd.