diff mbox

[4/4] leds: Let GPIO LEDs keep their current state

Message ID Pine.LNX.4.64.0811201640570.11673@t2.domain.actdsltmp (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Trent Piepho Nov. 21, 2008, 1:05 a.m. UTC 
On Mon, 17 Nov 2008, Richard Purdie wrote:
> On Fri, 2008-10-24 at 16:09 -0700, Trent Piepho wrote:
>> +	if (template->keep_state)
>> +		state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
>> +	else
>> +		state = template->default_state;
>>
>>  		state = of_get_property(child, "default-state", NULL);
>>  		led.default_state = state && !strcmp(state, "on");
>> +		led.keep_state = state && !strcmp(state, "keep");
>>
>> +++ b/include/linux/leds.h
>> @@ -138,7 +138,8 @@ struct gpio_led {
>>  	const char *default_trigger;
>>  	unsigned 	gpio;
>>  	u8 		active_low;
>> -	u8		default_state;
>> +	u8		default_state;	/* 0 = off, 1 = on */
>> +	u8		keep_state; /* overrides default_state */
>>  };
>
> How about something simpler here, just make default state have three
> different values - "keep", "on" and "off"? I'm not keen on having two
> different state variables like this.

I thought of that, but it ends up being more complex.  Instead of just
using:
static const struct gpio_led myled = {
 	.name = "something",
 	.keep_state = 1,
}

You'd do something like this:
 	.default_state = LEDS_GPIO_DEFSTATE_KEEP,

Is that better?  The constants for on/off/keep are one more thing you have
to look-up and remember when defining leds.  The code in the leds-gpio
driver ends up getting more complex to deal with one tristate vs two
booleans.

This is a patch to change to a tristate.  I don't think it's an
improvement.  More symbols defined, more code, extra stuff to remember
when defining leds, and removing the field from struct gpio_led doesn't
make it smaller due to padding.

Comments

Pavel Machek Nov. 23, 2008, 12:31 p.m. UTC | #1 
On Thu 2008-11-20 17:05:56, Trent Piepho wrote:
> On Mon, 17 Nov 2008, Richard Purdie wrote:
> > On Fri, 2008-10-24 at 16:09 -0700, Trent Piepho wrote:
> >> +	if (template->keep_state)
> >> +		state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
> >> +	else
> >> +		state = template->default_state;
> >>
> >>  		state = of_get_property(child, "default-state", NULL);
> >>  		led.default_state = state && !strcmp(state, "on");
> >> +		led.keep_state = state && !strcmp(state, "keep");
> >>
> >> +++ b/include/linux/leds.h
> >> @@ -138,7 +138,8 @@ struct gpio_led {
> >>  	const char *default_trigger;
> >>  	unsigned 	gpio;
> >>  	u8 		active_low;
> >> -	u8		default_state;
> >> +	u8		default_state;	/* 0 = off, 1 = on */
> >> +	u8		keep_state; /* overrides default_state */
> >>  };
> >
> > How about something simpler here, just make default state have three
> > different values - "keep", "on" and "off"? I'm not keen on having two
> > different state variables like this.
> 
> I thought of that, but it ends up being more complex.  Instead of just
> using:
> static const struct gpio_led myled = {
>  	.name = "something",
>  	.keep_state = 1,
> }
> 
> You'd do something like this:
>  	.default_state = LEDS_GPIO_DEFSTATE_KEEP,
> 
> Is that better?

Yes.
Richard Purdie Dec. 3, 2008, 10:04 a.m. UTC | #2 
On Sun, 2008-11-23 at 13:31 +0100, Pavel Machek wrote:
> On Thu 2008-11-20 17:05:56, Trent Piepho wrote:
> > On Mon, 17 Nov 2008, Richard Purdie wrote:
> > > On Fri, 2008-10-24 at 16:09 -0700, Trent Piepho wrote:
> > >> +	if (template->keep_state)
> > >> +		state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
> > >> +	else
> > >> +		state = template->default_state;
> > >>
> > >>  		state = of_get_property(child, "default-state", NULL);
> > >>  		led.default_state = state && !strcmp(state, "on");
> > >> +		led.keep_state = state && !strcmp(state, "keep");
> > >>
> > >> +++ b/include/linux/leds.h
> > >> @@ -138,7 +138,8 @@ struct gpio_led {
> > >>  	const char *default_trigger;
> > >>  	unsigned 	gpio;
> > >>  	u8 		active_low;
> > >> -	u8		default_state;
> > >> +	u8		default_state;	/* 0 = off, 1 = on */
> > >> +	u8		keep_state; /* overrides default_state */
> > >>  };
> > >
> > > How about something simpler here, just make default state have three
> > > different values - "keep", "on" and "off"? I'm not keen on having two
> > > different state variables like this.
> > 
> > I thought of that, but it ends up being more complex.  Instead of just
> > using:
> > static const struct gpio_led myled = {
> >  	.name = "something",
> >  	.keep_state = 1,
> > }
> > 
> > You'd do something like this:
> >  	.default_state = LEDS_GPIO_DEFSTATE_KEEP,
> > 
> > Is that better?
> 
> Yes.

Yes, agreed, much better.

Richard
Trent Piepho Dec. 10, 2008, 4:33 a.m. UTC | #3 
On Wed, 3 Dec 2008, Richard Purdie wrote:
> On Sun, 2008-11-23 at 13:31 +0100, Pavel Machek wrote:
>> On Thu 2008-11-20 17:05:56, Trent Piepho wrote:
>>> I thought of that, but it ends up being more complex.  Instead of just
>>> using:
>>> static const struct gpio_led myled = {
>>>  	.name = "something",
>>>  	.keep_state = 1,
>>> }
>>>
>>> You'd do something like this:
>>>  	.default_state = LEDS_GPIO_DEFSTATE_KEEP,
>>>
>>> Is that better?
>>
>> Yes.
>
> Yes, agreed, much better.

Oh very well, I'll change it.  But I reserve the right to make a sarcastic
commit message.
diff mbox

Patch

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index bb2a234..8a7303c 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -92,10 +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;
-	if (template->keep_state)
+	if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP)
  		state = !!gpio_get_value(led_dat->gpio) ^ led_dat->active_low;
  	else
-		state = template->default_state;
+		state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
  	led_dat->cdev.brightness = state ? LED_FULL : LED_OFF;

  	gpio_direction_output(led_dat->gpio, led_dat->active_low ^ state);
@@ -268,8 +268,15 @@  static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
  		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");
-		led.keep_state = state && !strcmp(state, "keep");
+		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 c51b625..f4a125c 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -138,9 +138,12 @@  struct gpio_led {
  	const char *default_trigger;
  	unsigned 	gpio;
  	u8 		active_low;
-	u8		default_state;	/* 0 = off, 1 = on */
-	u8		keep_state; /* overrides default_state */
+	u8		default_state;
+	/* 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;