Re: [PATCH 4/4] leds: Let GPIO LEDs keep their current state
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.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/4] leds: Let GPIO LEDs keep their current state
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;
unsignedgpio;
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
--
Richard Purdie
Intel Open Source Technology Centre
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/4] leds: Let GPIO LEDs keep their current state
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;
unsignedgpio;
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.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/4] leds: Let GPIO LEDs keep their current state
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;
unsignedgpio;
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.
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;
unsignedgpio;
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_KEEP2
struct gpio_led_platform_data {
int num_leds;
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/4] leds: Let GPIO LEDs keep their current state
On Fri, 2008-10-24 at 16:09 -0700, Trent Piepho wrote:
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 data for the platform device binding gets a new field
'keep_state' which turns this on. keep_state overrides default_state.
For the OpenFirmware bindings, the default-state property gains a new
allowable setting keep.
Signed-off-by: Trent Piepho [EMAIL PROTECTED]
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 0dbad87..bb2a234 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -92,10 +92,13 @@ 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 = template-default_state ? LED_FULL : LED_OFF;
+ if (template-keep_state)
+ state = !!gpio_get_value(led_dat-gpio) ^ led_dat-active_low;
+ else
+ state = template-default_state;
+ led_dat-cdev.brightness = state ? LED_FULL : LED_OFF;
- gpio_direction_output(led_dat-gpio,
- led_dat-active_low ^ template-default_state);
+ gpio_direction_output(led_dat-gpio, led_dat-active_low ^ state);
INIT_WORK(led_dat-work, gpio_led_work);
@@ -266,6 +269,7 @@ static int __devinit of_gpio_leds_probe(struct of_device
*ofdev,
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);
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 caa3987..c51b625 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -138,7 +138,8 @@ struct gpio_led {
const char *default_trigger;
unsignedgpio;
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.
Regards,
Richard
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
[PATCH 4/4] leds: Let GPIO LEDs keep their current state
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 data for the platform device binding gets a new field
'keep_state' which turns this on. keep_state overrides default_state.
For the OpenFirmware bindings, the default-state property gains a new
allowable setting keep.
Signed-off-by: Trent Piepho [EMAIL PROTECTED]
---
Documentation/powerpc/dts-bindings/gpio/led.txt | 16
drivers/leds/leds-gpio.c| 12
include/linux/leds.h|3 ++-
3 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt
b/Documentation/powerpc/dts-bindings/gpio/led.txt
index 544ded7..918bf9f 100644
--- a/Documentation/powerpc/dts-bindings/gpio/led.txt
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -21,10 +21,12 @@ LED sub-node properties:
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.
+ 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:
@@ -35,6 +37,12 @@ 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 {
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 0dbad87..bb2a234 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;
ret = gpio_request(template-gpio, template-name);
if (ret 0)
@@ -92,10 +92,13 @@ 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 = template-default_state ? LED_FULL : LED_OFF;
+ if (template-keep_state)
+ state = !!gpio_get_value(led_dat-gpio) ^ led_dat-active_low;
+ else
+ state = template-default_state;
+ led_dat-cdev.brightness = state ? LED_FULL : LED_OFF;
- gpio_direction_output(led_dat-gpio,
- led_dat-active_low ^ template-default_state);
+ gpio_direction_output(led_dat-gpio, led_dat-active_low ^ state);
INIT_WORK(led_dat-work, gpio_led_work);
@@ -266,6 +269,7 @@ static int __devinit of_gpio_leds_probe(struct of_device
*ofdev,
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);
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 caa3987..c51b625 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -138,7 +138,8 @@ struct gpio_led {
const char *default_trigger;
unsignedgpio;
u8 active_low;
- u8 default_state;
+ u8 default_state; /* 0 = off, 1 = on */
+ u8 keep_state; /* overrides default_state */
};
struct gpio_led_platform_data {
--
1.5.4.3
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 4/4] leds: Let GPIO LEDs keep their current state
On Fri, Oct 24, 2008 at 5:09 PM, Trent Piepho [EMAIL PROTECTED] wrote:
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 data for the platform device binding gets a new field
'keep_state' which turns this on. keep_state overrides default_state.
For the OpenFirmware bindings, the default-state property gains a new
allowable setting keep.
Hmmm... I'd prefer firmware to actually update the device tree if the
LED state needs to be preserved. In fact, it might be better use the
name current-state instead of default-state similar to the
current-speed property used in serial devices. However, I understand
that there are practical implications with this were not all firmware
is device tree aware. I don't see any reason not to support this
approach, so:
Acked-by: Grant Likely [EMAIL PROTECTED]
(But consider changing the property name)
This is a good patch series, thanks.
g.
Signed-off-by: Trent Piepho [EMAIL PROTECTED]
---
Documentation/powerpc/dts-bindings/gpio/led.txt | 16
drivers/leds/leds-gpio.c| 12
include/linux/leds.h|3 ++-
3 files changed, 22 insertions(+), 9 deletions(-)
diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt
b/Documentation/powerpc/dts-bindings/gpio/led.txt
index 544ded7..918bf9f 100644
--- a/Documentation/powerpc/dts-bindings/gpio/led.txt
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -21,10 +21,12 @@ LED sub-node properties:
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.
+ 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:
@@ -35,6 +37,12 @@ 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 {
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 0dbad87..bb2a234 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;
ret = gpio_request(template-gpio, template-name);
if (ret 0)
@@ -92,10 +92,13 @@ 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 = template-default_state ? LED_FULL :
LED_OFF;
+ if (template-keep_state)
+ state = !!gpio_get_value(led_dat-gpio) ^ led_dat-active_low;
+ else
+ state = template-default_state;
+ led_dat-cdev.brightness = state ? LED_FULL : LED_OFF;
- gpio_direction_output(led_dat-gpio,
- led_dat-active_low ^ template-default_state);
+ gpio_direction_output(led_dat-gpio, led_dat-active_low ^ state);
INIT_WORK(led_dat-work, gpio_led_work);
@@ -266,6 +269,7 @@ static int __devinit of_gpio_leds_probe(struct of_device
*ofdev,
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);
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 caa3987..c51b625 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -138,7 +138,8 @@ struct
