Re: [PATCH] powerpc: Update Warp to use leds-gpio driver

Re: [PATCH] powerpc: Update Warp to use leds-gpio driver

[Trent Piepho]

On Mon, 6 Apr 2009, Sean MacLennan wrote:
> Now that leds-gpio is a proper OF platform driver, the Warp can use
> the leds-gpio driver rather than the old out-of-kernel driver.
>
> One side-effect is the leds-gpio driver always turns the leds off
> while the old driver left them alone. So we have to set them back to
> the correct settings.

Originally, I had the OF bindings support this feature, see
http://article.gmane.org/gmane.linux.kernel/749094

Maybe this would be a better way to do it?  It avoids the glitch in the
leds, is less code overall, and can be use by other devices that might want
this same behavior.

Re: [PATCH] powerpc: Update Warp to use leds-gpio driver

[Sean MacLennan]

On Fri, 17 Apr 2009 20:12:50 -0700 (PDT)
"Trent Piepho" <xyzzy@speakeasy.org> wrote:

> On Mon, 6 Apr 2009, Sean MacLennan wrote:
> > Now that leds-gpio is a proper OF platform driver, the Warp can use
> > the leds-gpio driver rather than the old out-of-kernel driver.
> >
> > One side-effect is the leds-gpio driver always turns the leds off
> > while the old driver left them alone. So we have to set them back to
> > the correct settings.  
> 
> Originally, I had the OF bindings support this feature, see
> http://article.gmane.org/gmane.linux.kernel/749094
> 
> Maybe this would be a better way to do it?  It avoids the glitch in
> the leds, is less code overall, and can be use by other devices that
> might want this same behavior.

Yes, that is a cleaner way to handle the LEDs. Do you know why this
patch wasn't accepted at the time? A quick google shows that Grant
Likely acked it.

The patch will no longer apply since default state does not exist.

Cheers,
   Sean

Re: [PATCH] powerpc: Update Warp to use leds-gpio driver

[Grant Likely]

On Fri, Apr 17, 2009 at 10:37 PM, Sean MacLennan
<smaclennan@pikatech.com> wrote:
> On Fri, 17 Apr 2009 20:12:50 -0700 (PDT)
> "Trent Piepho" <xyzzy@speakeasy.org> wrote:
>
>> On Mon, 6 Apr 2009, Sean MacLennan wrote:
>> > Now that leds-gpio is a proper OF platform driver, the Warp can use
>> > the leds-gpio driver rather than the old out-of-kernel driver.
>> >
>> > One side-effect is the leds-gpio driver always turns the leds off
>> > while the old driver left them alone. So we have to set them back to
>> > the correct settings.
>>
>> Originally, I had the OF bindings support this feature, see
>> http://article.gmane.org/gmane.linux.kernel/749094
>>
>> Maybe this would be a better way to do it? =A0It avoids the glitch in
>> the leds, is less code overall, and can be use by other devices that
>> might want this same behavior.
>
> Yes, that is a cleaner way to handle the LEDs. Do you know why this
> patch wasn't accepted at the time? A quick google shows that Grant
> Likely acked it.
>
> The patch will no longer apply since default state does not exist.

It got left here:

On Sun, Jan 11, 2009 at 7:39 AM, Richard Purdie <rpurdie@rpsys.net> wrote:
> On Sun, 2009-01-11 at 04:58 -0800, Trent Piepho wrote:
>> It doesn't seem right to merge someone's patches together, make a very
>> small change, and then no longer credit them as the author.  Seems like =
it
>> defeats the purpose of the SOB lines for tracing the train of custody to=
o.
>> If someone looks to see where the code came from, it will look like you
>> wrote it.  Maybe Freescale will say Intel stole our code?  Without the S=
OB,
>> what record is there in git that Freescale gave permission to put the co=
de
>> in the kernel?
>>
>> I also put some significant effort into writing informative commit
>> messages, which have been lost.  Along with Grant's acks for my patches.
>
> It also doesn't make sense to make three changes adding different
> interfaces and rearranging the same section of code three different
> times. I'm dropping the patch, please send me a merged version of those
> patches with a commit message you're happy with. If you want Acked-by
> lines, we'll have to wait for them on the new patch as I'm going to do
> this exactly by the book regardless of time pressures now. Please
> indicate who you want Ack-ed by lines from so I know who to wait for.
> Also, you'd better exclude the suspend/resume change and credit me for
> the bitfield change, just to be 100% sure this is all legally accurate.

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

[PATCH] leds: Add options to have GPIO LEDs start on or keep their state

[Trent Piepho]

There already is a "default-on" trigger but there are problems with it.

For one, it's a inefficient way to do it and requires led trigger support
to be compiled in.

But the real reason is that is produces a glitch on the LED.  The GPIO is
allocate with the LED *off*, then *later* when the trigger runs it is
turned back on.  If the LED was already on via the GPIO's reset default or
action of the firmware, this produces a glitch where the LED goes from on
to off to on.  While normally this is fast enough that it wouldn't be
noticeable to a human observer, there are still serious problems.

One is that there may be something else on the GPIO line, like a hardware
alarm or watchdog, that is fast enough to notice the glitch.

Another is that the kernel may panic before the LED is turned back on, thus
hanging with the LED in the wrong state.  This is not just speculation, but
actually happened to me with an embedded system that has an LED which
should turn off when the kernel finishes booting, which was left in the
incorrect state due to a bug in the OF LED binding code.

We also let GPIO LEDs get their initial value from whatever the current
state of the GPIO line is.  On some systems the LEDs are put into some
state by the firmware or hardware before Linux boots, and it is desired to
have them keep this state which is otherwise unknown to Linux.

This requires that the underlying GPIO driver support reading the value of
output GPIOs.  Some drivers support this and some do not.

The platform device binding gains a field in the platform data
"default_state" that controls this.  There are three constants defined to
select from on, off, or keeping the current state.  The OpenFirmware
binding uses a property named "default-state" that can be set to "on",
"off", or "keep".  The default if the property isn't present is off.

Signed-off-by: Trent Piepho <xyzzy@speakeasy.org>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
---
 Documentation/powerpc/dts-bindings/gpio/led.txt |   17 ++++++++++++++++-
 drivers/leds/leds-gpio.c                        |   21 ++++++++++++++++++---
 include/linux/leds.h                            |    9 +++++++--
 3 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
index 4fe14de..064db92 100644
--- a/Documentation/powerpc/dts-bindings/gpio/led.txt
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -16,10 +16,17 @@ LED sub-node properties:
   string defining the trigger assigned to the LED.  Current triggers are:
     "backlight" - LED will act as a back-light, controlled by the framebuffer
 		  system
-    "default-on" - LED will turn on
+    "default-on" - LED will turn on, but see "default-state" below
     "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", "off", and "keep".  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 "keep" setting will keep the LED at whatever its current
+  state is, without producing a glitch.  The default is off if this
+  property is not present.
 
 Examples:
 
@@ -30,14 +37,22 @@ leds {
 		gpios = <&mcu_pio 0 1>; /* Active low */
 		linux,default-trigger = "ide-disk";
 	};
+
+	fault {
+		gpios = <&mcu_pio 1 0>;
+		/* Keep LED on if BIOS detected hardware fault */
+		default-state = "keep";
+	};
 };
 
 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 d210905..b5fd008 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -76,7 +76,7 @@ static int __devinit create_gpio_led(const struct gpio_led *template,
 	struct gpio_led_data *led_dat, struct device *parent,
 	int (*blink_set)(unsigned, unsigned long *, unsigned long *))
 {
-	int ret;
+	int ret, state;
 
 	/* skip leds that aren't available */
 	if (!gpio_is_valid(template->gpio)) {
@@ -99,11 +99,15 @@ 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;
+	if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP)
+		state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
+	else
+		state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
+	led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;
 	if (!template->retain_state_suspended)
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 
-	ret = gpio_direction_output(led_dat->gpio, led_dat->active_low);
+	ret = gpio_direction_output(led_dat->gpio, led_dat->active_low ^ state);
 	if (ret < 0)
 		goto err;
 
@@ -223,12 +227,23 @@ static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
 	memset(&led, 0, sizeof(led));
 	for_each_child_of_node(np, child) {
 		enum of_gpio_flags flags;
+		const char *state;
 
 		led.gpio = of_get_gpio_flags(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);
+		if (state) {
+			if (!strcmp(state, "keep")) {
+				led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
+			} else if(!strcmp(state, "on")) {
+				led.default_state = LEDS_GPIO_DEFSTATE_ON;
+			} else {
+				led.default_state = LEDS_GPIO_DEFSTATE_OFF;
+			}
+		}
 
 		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 376fe07..66e7d75 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -141,9 +141,14 @@ struct gpio_led {
 	const char *name;
 	const char *default_trigger;
 	unsigned 	gpio;
-	u8 		active_low : 1;
-	u8		retain_state_suspended : 1;
+	unsigned	active_low : 1;
+	unsigned	retain_state_suspended : 1;
+	unsigned	default_state : 2;
+	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
 };
+#define LEDS_GPIO_DEFSTATE_OFF	0
+#define LEDS_GPIO_DEFSTATE_ON	1
+#define LEDS_GPIO_DEFSTATE_KEEP	2
 
 struct gpio_led_platform_data {
 	int 		num_leds;
-- 
1.5.4.1

Re: [PATCH] leds: Add options to have GPIO LEDs start on or keep their state

[Sean MacLennan]

On Tue, 12 May 2009 15:33:12 -0700
Trent Piepho <xyzzy@speakeasy.org> wrote:

> +			if (!strcmp(state, "keep")) {
> +				led.default_state =
> LEDS_GPIO_DEFSTATE_KEEP;
> +			} else if(!strcmp(state, "on")) {
> +				led.default_state =
> LEDS_GPIO_DEFSTATE_ON;
> +			} else {
> +				led.default_state =
> LEDS_GPIO_DEFSTATE_OFF;
> +			}

Just a nitpick, you don't need the {} braces here. Other than that:

Acked-by: Sean MacLennan <smaclennan@pikatech.com>
Tested-by: Sean MacLennan <smaclennan@pikatech.com>

Tested on a warp with the following:

			power-leds {
				compatible = "gpio-leds";
				green {
					gpios = <&GPIO1 0 0>;
					default-state = "on";
				};
				red {
					gpios = <&GPIO1 1 0>;
					default-state = "keep";
				};
			};

I tested with both the red LED initially off and initially on as set in
u-boot.

Cheers,
   Sean

Re: [PATCH] leds: Add options to have GPIO LEDs start on or keep their state

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

Hello Trent,

> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 376fe07..66e7d75 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -141,9 +141,14 @@ struct gpio_led {
>  	const char *name;
>  	const char *default_trigger;
>  	unsigned 	gpio;
> -	u8 		active_low : 1;
> -	u8		retain_state_suspended : 1;
> +	unsigned	active_low : 1;
> +	unsigned	retain_state_suspended : 1;
> +	unsigned	default_state : 2;
> +	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */

Any specific reason for the change from u8 to unsigned? Could be
mentioned in the patch description maybe. And what Sean mentioned :)

Other than that:

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] leds: Add options to have GPIO LEDs start on or keep their state

[Trent Piepho]

On Wed, 13 May 2009, Wolfram Sang wrote:
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 376fe07..66e7d75 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -141,9 +141,14 @@ struct gpio_led {
> >  	const char *name;
> >  	const char *default_trigger;
> >  	unsigned 	gpio;
> > -	u8 		active_low : 1;
> > -	u8		retain_state_suspended : 1;
> > +	unsigned	active_low : 1;
> > +	unsigned	retain_state_suspended : 1;
> > +	unsigned	default_state : 2;
> > +	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
>
> Any specific reason for the change from u8 to unsigned? Could be
> mentioned in the patch description maybe. And what Sean mentioned :)

I should have mentioned that in the description.  It didn't make sense to
me to declare a bit field with u8.  An eight bit type that is one bit
wide?  The field width overrides the type width, but I think it's better to
just use "unsigned" and only specify a width once.

>
> Other than that:
>
> Acked-by: Wolfram Sang <w.sang@pengutronix.de>
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>