This series of patches adds support for OpenFirmware bindings for GPIO based
LEDs.
I last posted a version of this in July but discussion of the OF binding
details didn't seem to be going anywhere.
I've since been contacted by some people who are actually using the previous
patches and have been motivated to try again.
All the users of this code are satisfied with the current version and it does
everything they need it to do.
The first patch extends the of_get_gpio() function to provide the gpio flags.
The way this already works (i.e., this is not something I'm adding, it's
what's already there) is that the OF gpio specifier is an opaque sequence of
words. The GPIO controller driver (of which only one currently exists)
provides an ->xlate() method that turns this into a Linux gpiolib gpio number.
All current gpio controllers have flags in their gpio specifier, but there is
no way to get them. So I extend the xlate method to also produce the flags in
a Linux format, the same way it produces a Linux gpio number.
The second patch adds OF bindings to the gpio-leds driver. The existing code
is refactored so that almost all of the common code is shared between the two
binding methods. Either or both bindings can be selected via Kconfig. The
bindings do have one "linux," property, but no one has been able to come up
with something close to workable that avoids this and still satisfies the
*requirement* that the default trigger be settable from the OF bindings.
The second and third patches add some additional capabilities for gpio leds
that some users need.
One is to have a LED start in the on state when it's made known to Linux.
There is already a "default-on" trigger that does this, but it produces a
glitch where an LED that is already on will turn off then back on. My (tiny)
patch avoids this problem.
The next lets an LED stay, without glitches, in whatever state it was already
in when it's made known to Linux. It may have been put into this state by the
BIOS, firmware, bootloader, or the hardware itself.
The device binding spec for OF GPIOs defines a flags field, but there is
currently no way to get it. This patch adds a parameter to of_get_gpio()
where the flags will be returned if non-NULL. of_get_gpio() in turn passes
the parameter to the of_gpio_chip's xlate method, which can extract any
flags present from the gpio_spec.
The default (and currently only) of_gpio_chip xlate method,
of_gpio_simple_xlate(), is modified to do this.
Signed-off-by: Trent Piepho <[email protected]>
---
drivers/mtd/nand/fsl_upm.c | 2 +-
drivers/net/fs_enet/fs_enet-main.c | 2 +-
drivers/net/phy/mdio-ofgpio.c | 4 ++--
drivers/of/gpio.c | 13 ++++++++++---
drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
include/linux/of_gpio.h | 17 +++++++++++++----
6 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 024e3ff..a25d962 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -218,7 +218,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
}
fun->upm_cmd_offset = *prop;
- fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
+ fun->rnb_gpio = of_get_gpio(ofdev->node, 0, NULL);
if (fun->rnb_gpio >= 0) {
ret = gpio_request(fun->rnb_gpio, ofdev->dev.bus_id);
if (ret) {
diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index cb51c1f..5a3c7ee 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -994,7 +994,7 @@ static int __devinit find_phy(struct device_node *np,
goto out_put_phy;
}
- bus_id = of_get_gpio(mdionode, 0);
+ bus_id = of_get_gpio(mdionode, 0, NULL);
if (bus_id < 0) {
struct resource res;
ret = of_address_to_resource(mdionode, 0, &res);
diff --git a/drivers/net/phy/mdio-ofgpio.c b/drivers/net/phy/mdio-ofgpio.c
index 2ff9775..e3757c6 100644
--- a/drivers/net/phy/mdio-ofgpio.c
+++ b/drivers/net/phy/mdio-ofgpio.c
@@ -78,8 +78,8 @@ static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
{
struct mdio_gpio_info *bitbang = bus->priv;
- bitbang->mdc = of_get_gpio(np, 0);
- bitbang->mdio = of_get_gpio(np, 1);
+ bitbang->mdc = of_get_gpio(np, 0, NULL);
+ bitbang->mdio = of_get_gpio(np, 1, NULL);
if (bitbang->mdc < 0 || bitbang->mdio < 0)
return -ENODEV;
diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 7cd7301..2123517 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -22,11 +22,12 @@
* of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
* @np: device node to get GPIO from
* @index: index of the GPIO
+ * @flags: GPIO's flags are returned here if non-NULL
*
* Returns GPIO number to use with Linux generic GPIO API, or one of the errno
* value on the error condition.
*/
-int of_get_gpio(struct device_node *np, int index)
+int of_get_gpio(struct device_node *np, int index, unsigned int *flags)
{
int ret;
struct device_node *gc;
@@ -59,7 +60,9 @@ int of_get_gpio(struct device_node *np, int index)
goto err1;
}
- ret = of_gc->xlate(of_gc, np, gpio_spec);
+ if (flags)
+ *flags = 0;
+ ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
if (ret < 0)
goto err1;
@@ -77,19 +80,23 @@ EXPORT_SYMBOL(of_get_gpio);
* @of_gc: pointer to the of_gpio_chip structure
* @np: device node of the GPIO chip
* @gpio_spec: gpio specifier as found in the device tree
+ * @flags: if non-NUll flags are returned here
*
* This is simple translation function, suitable for the most 1:1 mapped
* gpio chips. This function performs only one sanity check: whether gpio
* is less than ngpios (that is specified in the gpio_chip).
*/
int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
- const void *gpio_spec)
+ const void *gpio_spec, unsigned int *flags)
{
const u32 *gpio = gpio_spec;
if (*gpio > of_gc->gc.ngpio)
return -EINVAL;
+ if (flags && of_gc->gpio_cells > 1)
+ *flags = gpio[1];
+
return *gpio;
}
EXPORT_SYMBOL(of_gpio_simple_xlate);
diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index bde4b4b..7835cd4 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -1094,7 +1094,7 @@ static int cpm_uart_init_port(struct device_node *np,
}
for (i = 0; i < NUM_GPIOS; i++)
- pinfo->gpios[i] = of_get_gpio(np, i);
+ pinfo->gpios[i] = of_get_gpio(np, i, NULL);
return cpm_uart_request_port(&pinfo->port);
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 67db101..0d332bf 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -26,7 +26,7 @@ struct of_gpio_chip {
struct gpio_chip gc;
int gpio_cells;
int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
- const void *gpio_spec);
+ const void *gpio_spec, unsigned int *flags);
};
static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
@@ -35,6 +35,14 @@ static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
}
/*
+ * Flags as returned by OF GPIO chip's xlate function.
+ * These do not need to be the same as the flags in the GPIO specifier in the
+ * OF device tree, but it's convenient if they are. The mm chip OF GPIO
+ * driver works this way.
+ */
+#define OF_GPIO_ACTIVE_LOW 1
+
+/*
* OF GPIO chip for memory mapped banks
*/
struct of_mm_gpio_chip {
@@ -50,16 +58,17 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
}
-extern int of_get_gpio(struct device_node *np, int index);
+extern int of_get_gpio(struct device_node *np, int index, unsigned int *flags);
extern int of_mm_gpiochip_add(struct device_node *np,
struct of_mm_gpio_chip *mm_gc);
extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
struct device_node *np,
- const void *gpio_spec);
+ const void *gpio_spec, unsigned int *flags);
#else
/* Drivers may not strictly depend on the GPIO support, so let them link. */
-static inline int of_get_gpio(struct device_node *np, int index)
+static inline int of_get_gpio(struct device_node *np, int index,
+ unsigned int *flags)
{
return -ENOSYS;
}
--
1.5.4.3
Add bindings to support LEDs defined as of_platform devices in addition to
the existing bindings for platform devices.
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.
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 | 46 ++++-
drivers/leds/Kconfig | 21 ++-
drivers/leds/leds-gpio.c | 230 ++++++++++++++++++-----
3 files changed, 243 insertions(+), 54 deletions(-)
diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
index ff51f4c..9f969c2 100644
--- a/Documentation/powerpc/dts-bindings/gpio/led.txt
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -1,15 +1,43 @@
-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
+- compatible : should be "gpio-leds".
+
+Each LED is represented as a sub-node of the gpio-leds device. Each
+node's name represents the name of the corresponding LED.
+
+LED sub-node properties:
+- gpios : Should specify the LED's GPIO, see "Specifying GPIO information
+ for devices" in Documentation/powerpc/booting-without-of.txt. Active
+ low LEDs should be indicated using flags in the GPIO specifier.
+- 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.
+- linux,default-trigger : (optional) This parameter, if present, is a
+ 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
+ "heartbeat" - LED "double" flashes at a load average based rate
+ "ide-disk" - LED indicates disk activity
+ "timer" - LED flashes at a fixed, configurable rate
-Example:
+Examples:
-led@0 {
- compatible = "gpio-led";
- label = "hdd";
- gpios = <&mcu_pio 0 1>;
+leds {
+ compatible = "gpio-leds";
+ hdd {
+ label = "IDE Activity";
+ gpios = <&mcu_pio 0 1>; /* Active low */
+ linux,default-trigger = "ide-disk";
+ };
};
+
+run-control {
+ compatible = "gpio-leds";
+ red {
+ gpios = <&mpc8572 6 0>;
+ };
+ green {
+ gpios = <&mpc8572 7 0>;
+ };
+}
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index e7fb7d2..6c6dc96 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -111,7 +111,26 @@ config LEDS_GPIO
help
This option enables support for the LEDs connected to GPIO
outputs. To be useful the particular board must have LEDs
- and they must be connected to the GPIO lines.
+ and they must be connected to the GPIO lines. The LEDs must be
+ defined as platform devices and/or OpenFirmware platform devices.
+ The code to use these bindings can be selected below.
+
+config LEDS_GPIO_PLATFORM
+ bool "Platform device bindings for GPIO LEDs"
+ depends on LEDS_GPIO
+ default y
+ help
+ Let the leds-gpio driver drive LEDs which have been defined as
+ platform devices. If you don't know what this means, say yes.
+
+config LEDS_GPIO_OF
+ bool "OpenFirmware platform device bindings for GPIO LEDs"
+ depends on LEDS_GPIO && OF_DEVICE
+ default y
+ help
+ Let the leds-gpio driver drive LEDs which have been defined as
+ of_platform devices. For instance, LEDs which are listed in a "dts"
+ file.
config LEDS_HP_DISK
tristate "LED Support for disk protection LED on HP notebooks"
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index b13bd29..f41b841 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -3,6 +3,7 @@
*
* Copyright (C) 2007 8D Technologies inc.
* Raphael Assenat <[email protected]>
+ * Copyright (C) 2008 Freescale Semiconductor, Inc.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
@@ -71,50 +72,67 @@ static int gpio_blink_set(struct led_classdev *led_cdev,
return led_dat->platform_gpio_blink_set(led_dat->gpio, delay_on, delay_off);
}
-static int gpio_led_probe(struct platform_device *pdev)
+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;
+
+ ret = gpio_request(template->gpio, template->name);
+ if (ret < 0)
+ return ret;
+
+ led_dat->cdev.name = template->name;
+ led_dat->cdev.default_trigger = template->default_trigger;
+ led_dat->gpio = template->gpio;
+ led_dat->can_sleep = gpio_cansleep(template->gpio);
+ led_dat->active_low = template->active_low;
+ if (blink_set) {
+ led_dat->platform_gpio_blink_set = blink_set;
+ led_dat->cdev.blink_set = gpio_blink_set;
+ }
+ led_dat->cdev.brightness_set = gpio_led_set;
+ led_dat->cdev.brightness = LED_OFF;
+
+ gpio_direction_output(led_dat->gpio, led_dat->active_low);
+
+ INIT_WORK(&led_dat->work, gpio_led_work);
+
+ ret = led_classdev_register(parent, &led_dat->cdev);
+ if (ret < 0)
+ gpio_free(led_dat->gpio);
+
+ return ret;
+}
+
+static void delete_gpio_led(struct gpio_led_data *led)
+{
+ led_classdev_unregister(&led->cdev);
+ cancel_work_sync(&led->work);
+ gpio_free(led->gpio);
+}
+
+/* Code to creates LEDs from platform devices */
+#ifdef CONFIG_LEDS_GPIO_PLATFORM
+static int __devinit gpio_led_probe(struct platform_device *pdev)
{
struct gpio_led_platform_data *pdata = pdev->dev.platform_data;
- struct gpio_led *cur_led;
- struct gpio_led_data *leds_data, *led_dat;
+ struct gpio_led_data *leds_data;
int i, ret = 0;
if (!pdata)
return -EBUSY;
leds_data = kzalloc(sizeof(struct gpio_led_data) * pdata->num_leds,
- GFP_KERNEL);
+ GFP_KERNEL);
if (!leds_data)
return -ENOMEM;
for (i = 0; i < pdata->num_leds; i++) {
- cur_led = &pdata->leds[i];
- led_dat = &leds_data[i];
-
- ret = gpio_request(cur_led->gpio, cur_led->name);
+ ret = create_gpio_led(&pdata->leds[i], &leds_data[i],
+ &pdev->dev, pdata->gpio_blink_set);
if (ret < 0)
goto err;
-
- led_dat->cdev.name = cur_led->name;
- led_dat->cdev.default_trigger = cur_led->default_trigger;
- led_dat->gpio = cur_led->gpio;
- led_dat->can_sleep = gpio_cansleep(cur_led->gpio);
- led_dat->active_low = cur_led->active_low;
- if (pdata->gpio_blink_set) {
- led_dat->platform_gpio_blink_set = pdata->gpio_blink_set;
- led_dat->cdev.blink_set = gpio_blink_set;
- }
- led_dat->cdev.brightness_set = gpio_led_set;
- led_dat->cdev.brightness = LED_OFF;
-
- gpio_direction_output(led_dat->gpio, led_dat->active_low);
-
- INIT_WORK(&led_dat->work, gpio_led_work);
-
- ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
- if (ret < 0) {
- gpio_free(led_dat->gpio);
- goto err;
- }
}
platform_set_drvdata(pdev, leds_data);
@@ -122,13 +140,8 @@ static int gpio_led_probe(struct platform_device *pdev)
return 0;
err:
- if (i > 0) {
- for (i = i - 1; i >= 0; i--) {
- led_classdev_unregister(&leds_data[i].cdev);
- cancel_work_sync(&leds_data[i].work);
- gpio_free(leds_data[i].gpio);
- }
- }
+ for (i = i - 1; i >= 0; i--)
+ delete_gpio_led(&leds_data[i]);
kfree(leds_data);
@@ -143,11 +156,8 @@ static int __devexit gpio_led_remove(struct platform_device *pdev)
leds_data = platform_get_drvdata(pdev);
- for (i = 0; i < pdata->num_leds; i++) {
- led_classdev_unregister(&leds_data[i].cdev);
- cancel_work_sync(&leds_data[i].work);
- gpio_free(leds_data[i].gpio);
- }
+ for (i = 0; i < pdata->num_leds; i++)
+ delete_gpio_led(&leds_data[i]);
kfree(leds_data);
@@ -211,7 +221,139 @@ static void __exit gpio_led_exit(void)
module_init(gpio_led_init);
module_exit(gpio_led_exit);
-MODULE_AUTHOR("Raphael Assenat <[email protected]>");
+MODULE_ALIAS("platform:leds-gpio");
+#endif /* CONFIG_LEDS_GPIO_PLATFORM */
+
+/* Code to create from OpenFirmware platform devices */
+#ifdef CONFIG_LEDS_GPIO_OF
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+
+struct gpio_led_of_platform_data {
+ int num_leds;
+ struct gpio_led_data led_data[];
+};
+
+static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ struct device_node *np = ofdev->node, *child;
+ struct gpio_led led;
+ struct gpio_led_of_platform_data *pdata;
+ int count = 0, ret;
+
+ /* count LEDs defined by this device, so we know how much to allocate */
+ for_each_child_of_node(np, child)
+ count++;
+ if (!count)
+ return 0; /* or ENODEV? */
+
+ pdata = kzalloc(sizeof(*pdata) + sizeof(struct gpio_led_data) * count,
+ GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ memset(&led, 0, sizeof(led));
+ for_each_child_of_node(np, child) {
+ unsigned int flags;
+
+ 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);
+
+ ret = create_gpio_led(&led, &pdata->led_data[pdata->num_leds++],
+ &ofdev->dev, NULL);
+ if (ret < 0)
+ goto err;
+ }
+
+ dev_set_drvdata(&ofdev->dev, pdata);
+
+ return 0;
+
+err:
+ for (count = pdata->num_leds - 2; count >= 0; count--)
+ delete_gpio_led(&pdata->led_data[count]);
+
+ kfree(pdata);
+
+ return ret;
+}
+
+static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
+{
+ struct gpio_led_of_platform_data *pdata = dev_get_drvdata(&ofdev->dev);
+ int i;
+
+ for (i = 0; i < pdata->num_leds; i++)
+ delete_gpio_led(&pdata->led_data[i]);
+
+ kfree(pdata);
+
+ dev_set_drvdata(&ofdev->dev, NULL);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
+{
+ struct gpio_led_of_platform_data *pdata = dev_get_drvdata(&ofdev->dev);
+ int i;
+
+ for (i = 0; i < pdata->num_leds; i++)
+ led_classdev_suspend(&pdata->leds_data[i].cdev);
+
+ return 0;
+}
+
+static int of_gpio_led_resume(struct of_device *ofdev)
+{
+ struct gpio_led_of_platform_data *pdata = dev_get_drvdata(&ofdev->dev);
+ int i;
+
+ for (i = 0; i < pdata->num_leds; i++)
+ led_classdev_resume(&pdata->leds_data[i].cdev);
+
+ return 0;
+}
+#else
+#define of_gpio_led_suspend NULL
+#define of_gpio_led_resume NULL
+#endif /* CONFIG_PM */
+
+static const struct of_device_id of_gpio_leds_match[] = {
+ { .compatible = "gpio-leds", },
+ {},
+};
+
+static struct of_platform_driver of_gpio_leds_driver = {
+ .driver = {
+ .name = "of_gpio_leds",
+ .owner = THIS_MODULE,
+ },
+ .match_table = of_gpio_leds_match,
+ .probe = of_gpio_leds_probe,
+ .remove = __devexit_p(of_gpio_leds_remove),
+ .suspend = of_gpio_led_suspend,
+ .resume = of_gpio_led_resume,
+};
+
+static int __init of_gpio_leds_init(void)
+{
+ return of_register_platform_driver(&of_gpio_leds_driver);
+}
+module_init(of_gpio_leds_init);
+
+static void __exit of_gpio_leds_exit(void)
+{
+ of_unregister_platform_driver(&of_gpio_leds_driver);
+}
+module_exit(of_gpio_leds_exit);
+#endif
+
+MODULE_AUTHOR("Raphael Assenat <[email protected]>, Trent Piepho <[email protected]>");
MODULE_DESCRIPTION("GPIO LED driver");
MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:leds-gpio");
--
1.5.4.3
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;
unsigned gpio;
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
Yes, there is the "default-on" trigger but there are problems with that.
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 then 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.
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.
Signed-off-by: Trent Piepho <[email protected]>
---
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
On Fri, Oct 24, 2008 at 5:08 PM, Trent Piepho <[email protected]> wrote:
> The device binding spec for OF GPIOs defines a flags field, but there is
> currently no way to get it. This patch adds a parameter to of_get_gpio()
> where the flags will be returned if non-NULL. of_get_gpio() in turn passes
> the parameter to the of_gpio_chip's xlate method, which can extract any
> flags present from the gpio_spec.
>
> The default (and currently only) of_gpio_chip xlate method,
> of_gpio_simple_xlate(), is modified to do this.
>
> Signed-off-by: Trent Piepho <[email protected]>
This looks sane to me
Acked-by: Grant Likely <[email protected]>
> ---
> drivers/mtd/nand/fsl_upm.c | 2 +-
> drivers/net/fs_enet/fs_enet-main.c | 2 +-
> drivers/net/phy/mdio-ofgpio.c | 4 ++--
> drivers/of/gpio.c | 13 ++++++++++---
> drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
> include/linux/of_gpio.h | 17 +++++++++++++----
> 6 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
> index 024e3ff..a25d962 100644
> --- a/drivers/mtd/nand/fsl_upm.c
> +++ b/drivers/mtd/nand/fsl_upm.c
> @@ -218,7 +218,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
> }
> fun->upm_cmd_offset = *prop;
>
> - fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
> + fun->rnb_gpio = of_get_gpio(ofdev->node, 0, NULL);
> if (fun->rnb_gpio >= 0) {
> ret = gpio_request(fun->rnb_gpio, ofdev->dev.bus_id);
> if (ret) {
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
> index cb51c1f..5a3c7ee 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -994,7 +994,7 @@ static int __devinit find_phy(struct device_node *np,
> goto out_put_phy;
> }
>
> - bus_id = of_get_gpio(mdionode, 0);
> + bus_id = of_get_gpio(mdionode, 0, NULL);
> if (bus_id < 0) {
> struct resource res;
> ret = of_address_to_resource(mdionode, 0, &res);
> diff --git a/drivers/net/phy/mdio-ofgpio.c b/drivers/net/phy/mdio-ofgpio.c
> index 2ff9775..e3757c6 100644
> --- a/drivers/net/phy/mdio-ofgpio.c
> +++ b/drivers/net/phy/mdio-ofgpio.c
> @@ -78,8 +78,8 @@ static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
> {
> struct mdio_gpio_info *bitbang = bus->priv;
>
> - bitbang->mdc = of_get_gpio(np, 0);
> - bitbang->mdio = of_get_gpio(np, 1);
> + bitbang->mdc = of_get_gpio(np, 0, NULL);
> + bitbang->mdio = of_get_gpio(np, 1, NULL);
>
> if (bitbang->mdc < 0 || bitbang->mdio < 0)
> return -ENODEV;
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 7cd7301..2123517 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -22,11 +22,12 @@
> * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
> * @np: device node to get GPIO from
> * @index: index of the GPIO
> + * @flags: GPIO's flags are returned here if non-NULL
> *
> * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
> * value on the error condition.
> */
> -int of_get_gpio(struct device_node *np, int index)
> +int of_get_gpio(struct device_node *np, int index, unsigned int *flags)
> {
> int ret;
> struct device_node *gc;
> @@ -59,7 +60,9 @@ int of_get_gpio(struct device_node *np, int index)
> goto err1;
> }
>
> - ret = of_gc->xlate(of_gc, np, gpio_spec);
> + if (flags)
> + *flags = 0;
> + ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
> if (ret < 0)
> goto err1;
>
> @@ -77,19 +80,23 @@ EXPORT_SYMBOL(of_get_gpio);
> * @of_gc: pointer to the of_gpio_chip structure
> * @np: device node of the GPIO chip
> * @gpio_spec: gpio specifier as found in the device tree
> + * @flags: if non-NUll flags are returned here
> *
> * This is simple translation function, suitable for the most 1:1 mapped
> * gpio chips. This function performs only one sanity check: whether gpio
> * is less than ngpios (that is specified in the gpio_chip).
> */
> int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> - const void *gpio_spec)
> + const void *gpio_spec, unsigned int *flags)
> {
> const u32 *gpio = gpio_spec;
>
> if (*gpio > of_gc->gc.ngpio)
> return -EINVAL;
>
> + if (flags && of_gc->gpio_cells > 1)
> + *flags = gpio[1];
> +
> return *gpio;
> }
> EXPORT_SYMBOL(of_gpio_simple_xlate);
> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
> index bde4b4b..7835cd4 100644
> --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> @@ -1094,7 +1094,7 @@ static int cpm_uart_init_port(struct device_node *np,
> }
>
> for (i = 0; i < NUM_GPIOS; i++)
> - pinfo->gpios[i] = of_get_gpio(np, i);
> + pinfo->gpios[i] = of_get_gpio(np, i, NULL);
>
> return cpm_uart_request_port(&pinfo->port);
>
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 67db101..0d332bf 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -26,7 +26,7 @@ struct of_gpio_chip {
> struct gpio_chip gc;
> int gpio_cells;
> int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
> - const void *gpio_spec);
> + const void *gpio_spec, unsigned int *flags);
> };
>
> static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
> @@ -35,6 +35,14 @@ static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
> }
>
> /*
> + * Flags as returned by OF GPIO chip's xlate function.
> + * These do not need to be the same as the flags in the GPIO specifier in the
> + * OF device tree, but it's convenient if they are. The mm chip OF GPIO
> + * driver works this way.
> + */
> +#define OF_GPIO_ACTIVE_LOW 1
> +
> +/*
> * OF GPIO chip for memory mapped banks
> */
> struct of_mm_gpio_chip {
> @@ -50,16 +58,17 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
> return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
> }
>
> -extern int of_get_gpio(struct device_node *np, int index);
> +extern int of_get_gpio(struct device_node *np, int index, unsigned int *flags);
> extern int of_mm_gpiochip_add(struct device_node *np,
> struct of_mm_gpio_chip *mm_gc);
> extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
> struct device_node *np,
> - const void *gpio_spec);
> + const void *gpio_spec, unsigned int *flags);
> #else
>
> /* Drivers may not strictly depend on the GPIO support, so let them link. */
> -static inline int of_get_gpio(struct device_node *np, int index)
> +static inline int of_get_gpio(struct device_node *np, int index,
> + unsigned int *flags)
> {
> return -ENOSYS;
> }
> --
> 1.5.4.3
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Fri, Oct 24, 2008 at 5:08 PM, Trent Piepho <[email protected]> wrote:
> Add bindings to support LEDs defined as of_platform devices in addition to
> the existing bindings for platform devices.
>
> 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.
>
> 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]>
This also looks sane. However, since this modifies a binding, can you
please repost to the [email protected] mailing list?
Also, one comment below.
Acked-by: Grant Likely <[email protected]>
> diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
> index ff51f4c..9f969c2 100644
> --- a/Documentation/powerpc/dts-bindings/gpio/led.txt
> +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
> @@ -1,15 +1,43 @@
> -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
> +- compatible : should be "gpio-leds".
> +
> +Each LED is represented as a sub-node of the gpio-leds device. Each
> +node's name represents the name of the corresponding LED.
> +
> +LED sub-node properties:
> +- gpios : Should specify the LED's GPIO, see "Specifying GPIO information
> + for devices" in Documentation/powerpc/booting-without-of.txt. Active
> + low LEDs should be indicated using flags in the GPIO specifier.
> +- 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.
> +- linux,default-trigger : (optional) This parameter, if present, is a
> + 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
> + "heartbeat" - LED "double" flashes at a load average based rate
> + "ide-disk" - LED indicates disk activity
> + "timer" - LED flashes at a fixed, configurable rate
As I'm sure you can predict, I'm not thrilled with this; but it *is* a
very linux-specific sort of thing, it *is* a necessary bit of data.
:-) My biggest worry is that the Linux internal strings will change
in the future which will force the gpio driver to insert a translation
between these names and some future internal Linux name...
... I suppose another option is to just codify them here right now
(maybe named something like "led-usage") and if Linux changes then the
driver just needs to keep up with the new trigger names (translating
the old).
Either way, I'm not going to oppose this patch over this issue.
Nice patch.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Fri, Oct 24, 2008 at 5:09 PM, Trent Piepho <[email protected]> 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 <[email protected]>
>
> Signed-off-by: Trent Piepho <[email protected]>
> ---
> 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.
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 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 */
> };
>
> struct gpio_led_platform_data {
> --
> 1.5.4.3
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Fri, Oct 24, 2008 at 04:08:58PM -0700, Trent Piepho wrote:
> The device binding spec for OF GPIOs defines a flags field, but there is
> currently no way to get it. This patch adds a parameter to of_get_gpio()
> where the flags will be returned if non-NULL. of_get_gpio() in turn passes
> the parameter to the of_gpio_chip's xlate method, which can extract any
> flags present from the gpio_spec.
>
> The default (and currently only) of_gpio_chip xlate method,
> of_gpio_simple_xlate(), is modified to do this.
Looks good. Few comments below.
> Signed-off-by: Trent Piepho <[email protected]>
> ---
> drivers/mtd/nand/fsl_upm.c | 2 +-
> drivers/net/fs_enet/fs_enet-main.c | 2 +-
> drivers/net/phy/mdio-ofgpio.c | 4 ++--
> drivers/of/gpio.c | 13 ++++++++++---
> drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
> include/linux/of_gpio.h | 17 +++++++++++++----
> 6 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
> index 024e3ff..a25d962 100644
> --- a/drivers/mtd/nand/fsl_upm.c
> +++ b/drivers/mtd/nand/fsl_upm.c
[...]
> @@ -59,7 +60,9 @@ int of_get_gpio(struct device_node *np, int index)
> goto err1;
> }
>
> - ret = of_gc->xlate(of_gc, np, gpio_spec);
> + if (flags)
> + *flags = 0;
> + ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
> if (ret < 0)
> goto err1;
>
> @@ -77,19 +80,23 @@ EXPORT_SYMBOL(of_get_gpio);
> * @of_gc: pointer to the of_gpio_chip structure
> * @np: device node of the GPIO chip
> * @gpio_spec: gpio specifier as found in the device tree
> + * @flags: if non-NUll flags are returned here
NULL, not NUll.
> *
> * This is simple translation function, suitable for the most 1:1 mapped
> * gpio chips. This function performs only one sanity check: whether gpio
> * is less than ngpios (that is specified in the gpio_chip).
> */
> int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> - const void *gpio_spec)
> + const void *gpio_spec, unsigned int *flags)
Why you made it unsigned int? In my original patch, I used
named enum, which is self-documenting type.
> {
> const u32 *gpio = gpio_spec;
>
> if (*gpio > of_gc->gc.ngpio)
> return -EINVAL;
>
> + if (flags && of_gc->gpio_cells > 1)
> + *flags = gpio[1];
> +
> return *gpio;
> }
> EXPORT_SYMBOL(of_gpio_simple_xlate);
> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
> index bde4b4b..7835cd4 100644
> --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> @@ -1094,7 +1094,7 @@ static int cpm_uart_init_port(struct device_node *np,
> }
>
> for (i = 0; i < NUM_GPIOS; i++)
> - pinfo->gpios[i] = of_get_gpio(np, i);
> + pinfo->gpios[i] = of_get_gpio(np, i, NULL);
>
> return cpm_uart_request_port(&pinfo->port);
>
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 67db101..0d332bf 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -26,7 +26,7 @@ struct of_gpio_chip {
> struct gpio_chip gc;
> int gpio_cells;
> int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
> - const void *gpio_spec);
> + const void *gpio_spec, unsigned int *flags);
> };
>
> static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
> @@ -35,6 +35,14 @@ static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
> }
>
> /*
> + * Flags as returned by OF GPIO chip's xlate function.
> + * These do not need to be the same as the flags in the GPIO specifier in the
> + * OF device tree, but it's convenient if they are. The mm chip OF GPIO
> + * driver works this way.
This is not of_mm_gpio_chip specific.
> + */
> +#define OF_GPIO_ACTIVE_LOW 1
> +
> +/*
> * OF GPIO chip for memory mapped banks
> */
> struct of_mm_gpio_chip {
> @@ -50,16 +58,17 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
> return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
> }
>
> -extern int of_get_gpio(struct device_node *np, int index);
> +extern int of_get_gpio(struct device_node *np, int index, unsigned int *flags);
> extern int of_mm_gpiochip_add(struct device_node *np,
> struct of_mm_gpio_chip *mm_gc);
> extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
> struct device_node *np,
> - const void *gpio_spec);
> + const void *gpio_spec, unsigned int *flags);
> #else
>
> /* Drivers may not strictly depend on the GPIO support, so let them link. */
> -static inline int of_get_gpio(struct device_node *np, int index)
> +static inline int of_get_gpio(struct device_node *np, int index,
> + unsigned int *flags)
> {
> return -ENOSYS;
> }
> --
> 1.5.4.3
Can you repost a fixed version with my Ack and Cc: Andrew Morton,
Benjamin Herrenschmidt?
I think this change should go into the 2.6.28, so that we can
write new code on top of new API. Otherwise this change will cause
issues in the next merge window.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Tue, Oct 28, 2008 at 8:39 AM, Anton Vorontsov
<[email protected]> wrote:
> Can you repost a fixed version with my Ack and Cc: Andrew Morton,
> Benjamin Herrenschmidt?
>
> I think this change should go into the 2.6.28, so that we can
> write new code on top of new API. Otherwise this change will cause
> issues in the next merge window.
While I like this series, it should not be pushed for .28. The window
is closed and we are in bug-fix-only mode. Focus on getting it into
Ben's -next tree instead. That will deal with any merge window
conflicts. I'd be happy to pull it through my 5200 tree when you post
the next version.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Tue, Oct 28, 2008 at 08:53:11AM -0600, Grant Likely wrote:
> On Tue, Oct 28, 2008 at 8:39 AM, Anton Vorontsov
> <[email protected]> wrote:
> > Can you repost a fixed version with my Ack and Cc: Andrew Morton,
> > Benjamin Herrenschmidt?
> >
> > I think this change should go into the 2.6.28, so that we can
> > write new code on top of new API. Otherwise this change will cause
> > issues in the next merge window.
>
> While I like this series, it should not be pushed for .28. The window
> is closed and we are in bug-fix-only mode.
IIRC, we approve API changes in the early -rcs, exactly to avoid build
break issues for the new code (though the API changes themselves are
discouraged).
> Focus on getting it into Ben's -next tree instead.
Would Benjamin apply the USB FHCI driver then? I doubt it.
So, there are options:
1. Either this patch go in into the .28;
2. Or this patch go in _after_ USB FHCI driver, and Trent takes care
to convert it to the new API. (I'm trying to push the driver since
2007, I think. Firstly it has GPIO framework dependency, QE GPIO,
QE Pinmux, FSL GTM dependency, bindings, e.t.c. And now that.
No, no deal. I quite tired of the dependencies for this driver.)
3. Or we apply my original patch that won't break the API.
http://lkml.org/lkml/2008/7/25/236
I still like it better than Trent's patch, exactly because it
can't break anything.
> That will deal with any merge window
> conflicts.
This won't deal with build breakage because of API changes, which
means I can't submit FHCI driver to Greg, I have to wait for the
API changes applied.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Tue, Oct 28, 2008 at 9:16 AM, Anton Vorontsov
<[email protected]> wrote:
> On Tue, Oct 28, 2008 at 08:53:11AM -0600, Grant Likely wrote:
>> That will deal with any merge window
>> conflicts.
>
> This won't deal with build breakage because of API changes, which
> means I can't submit FHCI driver to Greg, I have to wait for the
> API changes applied.
Then we coordinate with Greg and make sure the fixup gets put in place
before the window opens; either by bringing the patch in Ben's tree or
by getting Greg to pull Ben's tree into -next. This API change should
not block Greg picking up the FHCI driver. This is not an
insurmountable problem.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Tue, Oct 28, 2008 at 09:42:36AM -0600, Grant Likely wrote:
> On Tue, Oct 28, 2008 at 9:16 AM, Anton Vorontsov
> <[email protected]> wrote:
> > On Tue, Oct 28, 2008 at 08:53:11AM -0600, Grant Likely wrote:
> >> That will deal with any merge window
> >> conflicts.
> >
> > This won't deal with build breakage because of API changes, which
> > means I can't submit FHCI driver to Greg, I have to wait for the
> > API changes applied.
>
> Then we coordinate with Greg and make sure the fixup gets put in place
> before the window opens; either by bringing the patch in Ben's tree or
> by getting Greg to pull Ben's tree into -next. This API change should
> not block Greg picking up the FHCI driver. This is not an
> insurmountable problem.
Sounds complicated.
Ok, but if something will go wrong with the FHCI vs. of_get_gpio()
change, I'll just attribute the blame to you. :-))
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Tue, Oct 28, 2008 at 10:56 AM, Anton Vorontsov
<[email protected]> wrote:
> On Tue, Oct 28, 2008 at 09:42:36AM -0600, Grant Likely wrote:
>> Then we coordinate with Greg and make sure the fixup gets put in place
>> before the window opens; either by bringing the patch in Ben's tree or
>> by getting Greg to pull Ben's tree into -next. This API change should
>> not block Greg picking up the FHCI driver. This is not an
>> insurmountable problem.
>
> Sounds complicated.
>
> Ok, but if something will go wrong with the FHCI vs. of_get_gpio()
> change, I'll just attribute the blame to you. :-))
Bring it on. :-)
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Tue, 28 Oct 2008, Anton Vorontsov wrote:
> On Fri, Oct 24, 2008 at 04:08:58PM -0700, Trent Piepho wrote:
>> + * @flags: if non-NUll flags are returned here
>
> NULL, not NUll.
Thanks, fixed.
>> + const void *gpio_spec, unsigned int *flags)
>
> Why you made it unsigned int? In my original patch, I used
> named enum, which is self-documenting type.
I started writing this patch before you posted yours, and I didn't think of
the enum. But you have a good point so I'll switch to an enum.
>> + * Flags as returned by OF GPIO chip's xlate function.
>> + * These do not need to be the same as the flags in the GPIO specifier in the
>> + * OF device tree, but it's convenient if they are. The mm chip OF GPIO
>> + * driver works this way.
>
> This is not of_mm_gpio_chip specific.
of_mm_gpio_chip was just an example of a driver that uses the same flag format
for Linux and the OF binding. I'll clarify the comment.
WRT changing the interface, Linux doesn't provide a stable kernel API.
Functions that have been around far longer than of_get_gpio() and have far
more users have changed. Yes, it is slightly annoying now. But providing
backward compatibility for every single interface change will produce a
bloated and redundant API that will be around forever.
> Can you repost a fixed version with my Ack and Cc: Andrew Morton,
> Benjamin Herrenschmidt?
>
> I think this change should go into the 2.6.28, so that we can
> write new code on top of new API. Otherwise this change will cause
> issues in the next merge window.
If you can get your patch into Ben's -next tree before the high .28-RCs come
out, I can just rebase my patch to that tree and make the changes to any new
callers of of_get_gpio() that are there.
On Wed, Oct 29, 2008 at 07:21:24PM -0700, Trent Piepho wrote:
[...]
> > Can you repost a fixed version with my Ack and Cc: Andrew Morton,
> > Benjamin Herrenschmidt?
> >
> > I think this change should go into the 2.6.28, so that we can
> > write new code on top of new API. Otherwise this change will cause
> > issues in the next merge window.
>
> If you can get your patch into Ben's -next tree before the high .28-RCs come
> out,
I can't, that's the point. Big USB patches don't go through the
powerpc tree. ;-)
> I can just rebase my patch to that tree and make the changes to any new
> callers of of_get_gpio() that are there.
Never mind, just make sure your patch goes into the powerpc-next
early, thus we could start using the new API.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
The device binding spec for OF GPIOs defines a flags field, but there is
currently no way to get it. This patch adds a parameter to of_get_gpio()
where the flags will be returned if non-NULL. of_get_gpio() in turn passes
the parameter to the of_gpio_chip's xlate method, which can extract any
flags present from the gpio_spec.
The default (and currently only) of_gpio_chip xlate method,
of_gpio_simple_xlate(), is modified to do this.
Signed-off-by: Trent Piepho <[email protected]>
Acked-by: Grant Likely <[email protected]>
Acked-by: Anton Vorontsov <[email protected]>
---
Since this patch changes an API, it would be nice to get it into powerpc-next
soon so that people who have new patches that use this API, like Anton, can
base off it.
drivers/mtd/nand/fsl_upm.c | 2 +-
drivers/net/fs_enet/fs_enet-main.c | 2 +-
drivers/net/phy/mdio-ofgpio.c | 4 ++--
drivers/of/gpio.c | 13 ++++++++++---
drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
include/linux/of_gpio.h | 21 +++++++++++++++++----
6 files changed, 32 insertions(+), 12 deletions(-)
diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 024e3ff..a25d962 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -218,7 +218,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
}
fun->upm_cmd_offset = *prop;
- fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
+ fun->rnb_gpio = of_get_gpio(ofdev->node, 0, NULL);
if (fun->rnb_gpio >= 0) {
ret = gpio_request(fun->rnb_gpio, ofdev->dev.bus_id);
if (ret) {
diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index cb51c1f..5a3c7ee 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -994,7 +994,7 @@ static int __devinit find_phy(struct device_node *np,
goto out_put_phy;
}
- bus_id = of_get_gpio(mdionode, 0);
+ bus_id = of_get_gpio(mdionode, 0, NULL);
if (bus_id < 0) {
struct resource res;
ret = of_address_to_resource(mdionode, 0, &res);
diff --git a/drivers/net/phy/mdio-ofgpio.c b/drivers/net/phy/mdio-ofgpio.c
index 2ff9775..e3757c6 100644
--- a/drivers/net/phy/mdio-ofgpio.c
+++ b/drivers/net/phy/mdio-ofgpio.c
@@ -78,8 +78,8 @@ static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
{
struct mdio_gpio_info *bitbang = bus->priv;
- bitbang->mdc = of_get_gpio(np, 0);
- bitbang->mdio = of_get_gpio(np, 1);
+ bitbang->mdc = of_get_gpio(np, 0, NULL);
+ bitbang->mdio = of_get_gpio(np, 1, NULL);
if (bitbang->mdc < 0 || bitbang->mdio < 0)
return -ENODEV;
diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 7cd7301..ae14215 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -22,11 +22,12 @@
* of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
* @np: device node to get GPIO from
* @index: index of the GPIO
+ * @flags: GPIO's flags are returned here if non-NULL
*
* Returns GPIO number to use with Linux generic GPIO API, or one of the errno
* value on the error condition.
*/
-int of_get_gpio(struct device_node *np, int index)
+int of_get_gpio(struct device_node *np, int index, enum of_gpio_flags *flags)
{
int ret;
struct device_node *gc;
@@ -59,7 +60,9 @@ int of_get_gpio(struct device_node *np, int index)
goto err1;
}
- ret = of_gc->xlate(of_gc, np, gpio_spec);
+ if (flags)
+ *flags = 0;
+ ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
if (ret < 0)
goto err1;
@@ -77,19 +80,23 @@ EXPORT_SYMBOL(of_get_gpio);
* @of_gc: pointer to the of_gpio_chip structure
* @np: device node of the GPIO chip
* @gpio_spec: gpio specifier as found in the device tree
+ * @flags: if non-NULL flags are returned here
*
* This is simple translation function, suitable for the most 1:1 mapped
* gpio chips. This function performs only one sanity check: whether gpio
* is less than ngpios (that is specified in the gpio_chip).
*/
int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
- const void *gpio_spec)
+ const void *gpio_spec, enum of_gpio_flags *flags)
{
const u32 *gpio = gpio_spec;
if (*gpio > of_gc->gc.ngpio)
return -EINVAL;
+ if (flags && of_gc->gpio_cells > 1)
+ *flags = gpio[1];
+
return *gpio;
}
EXPORT_SYMBOL(of_gpio_simple_xlate);
diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index bde4b4b..7835cd4 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -1094,7 +1094,7 @@ static int cpm_uart_init_port(struct device_node *np,
}
for (i = 0; i < NUM_GPIOS; i++)
- pinfo->gpios[i] = of_get_gpio(np, i);
+ pinfo->gpios[i] = of_get_gpio(np, i, NULL);
return cpm_uart_request_port(&pinfo->port);
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 67db101..d611746 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -20,13 +20,23 @@
#ifdef CONFIG_OF_GPIO
/*
+ * Flags as returned by OF GPIO chip's xlate method.
+ * The GPIO specifier in the OF device tree does not need to use this
+ * same format for its flags, but it's convenient if it does. For
+ * example, the of_mm_gpio_chip driver works this way.
+ */
+enum of_gpio_flags {
+ OF_GPIO_ACTIVE_LOW = 1,
+};
+
+/*
* Generic OF GPIO chip
*/
struct of_gpio_chip {
struct gpio_chip gc;
int gpio_cells;
int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
- const void *gpio_spec);
+ const void *gpio_spec, enum of_gpio_flags *flags);
};
static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
@@ -50,16 +60,19 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
}
-extern int of_get_gpio(struct device_node *np, int index);
+extern int of_get_gpio(struct device_node *np, int index,
+ enum of_gpio_flags *flags);
extern int of_mm_gpiochip_add(struct device_node *np,
struct of_mm_gpio_chip *mm_gc);
extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
struct device_node *np,
- const void *gpio_spec);
+ const void *gpio_spec,
+ enum of_gpio_flags *flags);
#else
/* Drivers may not strictly depend on the GPIO support, so let them link. */
-static inline int of_get_gpio(struct device_node *np, int index)
+static inline int of_get_gpio(struct device_node *np, int index,
+ enum of_gpio_flags *flags)
{
return -ENOSYS;
}
--
1.5.4.3
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;
> 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.
Regards,
Richard
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.
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;
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.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Oct 30, 2008 at 07:03:09PM -0700, Trent Piepho wrote:
> The device binding spec for OF GPIOs defines a flags field, but there is
> currently no way to get it. This patch adds a parameter to of_get_gpio()
> where the flags will be returned if non-NULL. of_get_gpio() in turn passes
> the parameter to the of_gpio_chip's xlate method, which can extract any
> flags present from the gpio_spec.
>
> The default (and currently only) of_gpio_chip xlate method,
> of_gpio_simple_xlate(), is modified to do this.
>
> Signed-off-by: Trent Piepho <[email protected]>
> Acked-by: Grant Likely <[email protected]>
> Acked-by: Anton Vorontsov <[email protected]>
> ---
> Since this patch changes an API, it would be nice to get it into powerpc-next
> soon so that people who have new patches that use this API, like Anton, can
> base off it.
Can we apply it? Paul, Benjamin?
The patchwork url for this patch is:
http://patchwork.ozlabs.org/patch/6650/
Thanks!
> drivers/mtd/nand/fsl_upm.c | 2 +-
> drivers/net/fs_enet/fs_enet-main.c | 2 +-
> drivers/net/phy/mdio-ofgpio.c | 4 ++--
> drivers/of/gpio.c | 13 ++++++++++---
> drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
> include/linux/of_gpio.h | 21 +++++++++++++++++----
> 6 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
> index 024e3ff..a25d962 100644
> --- a/drivers/mtd/nand/fsl_upm.c
> +++ b/drivers/mtd/nand/fsl_upm.c
> @@ -218,7 +218,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
> }
> fun->upm_cmd_offset = *prop;
>
> - fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
> + fun->rnb_gpio = of_get_gpio(ofdev->node, 0, NULL);
> if (fun->rnb_gpio >= 0) {
> ret = gpio_request(fun->rnb_gpio, ofdev->dev.bus_id);
> if (ret) {
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
> index cb51c1f..5a3c7ee 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -994,7 +994,7 @@ static int __devinit find_phy(struct device_node *np,
> goto out_put_phy;
> }
>
> - bus_id = of_get_gpio(mdionode, 0);
> + bus_id = of_get_gpio(mdionode, 0, NULL);
> if (bus_id < 0) {
> struct resource res;
> ret = of_address_to_resource(mdionode, 0, &res);
> diff --git a/drivers/net/phy/mdio-ofgpio.c b/drivers/net/phy/mdio-ofgpio.c
> index 2ff9775..e3757c6 100644
> --- a/drivers/net/phy/mdio-ofgpio.c
> +++ b/drivers/net/phy/mdio-ofgpio.c
> @@ -78,8 +78,8 @@ static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
> {
> struct mdio_gpio_info *bitbang = bus->priv;
>
> - bitbang->mdc = of_get_gpio(np, 0);
> - bitbang->mdio = of_get_gpio(np, 1);
> + bitbang->mdc = of_get_gpio(np, 0, NULL);
> + bitbang->mdio = of_get_gpio(np, 1, NULL);
>
> if (bitbang->mdc < 0 || bitbang->mdio < 0)
> return -ENODEV;
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 7cd7301..ae14215 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -22,11 +22,12 @@
> * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
> * @np: device node to get GPIO from
> * @index: index of the GPIO
> + * @flags: GPIO's flags are returned here if non-NULL
> *
> * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
> * value on the error condition.
> */
> -int of_get_gpio(struct device_node *np, int index)
> +int of_get_gpio(struct device_node *np, int index, enum of_gpio_flags *flags)
> {
> int ret;
> struct device_node *gc;
> @@ -59,7 +60,9 @@ int of_get_gpio(struct device_node *np, int index)
> goto err1;
> }
>
> - ret = of_gc->xlate(of_gc, np, gpio_spec);
> + if (flags)
> + *flags = 0;
> + ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
> if (ret < 0)
> goto err1;
>
> @@ -77,19 +80,23 @@ EXPORT_SYMBOL(of_get_gpio);
> * @of_gc: pointer to the of_gpio_chip structure
> * @np: device node of the GPIO chip
> * @gpio_spec: gpio specifier as found in the device tree
> + * @flags: if non-NULL flags are returned here
> *
> * This is simple translation function, suitable for the most 1:1 mapped
> * gpio chips. This function performs only one sanity check: whether gpio
> * is less than ngpios (that is specified in the gpio_chip).
> */
> int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> - const void *gpio_spec)
> + const void *gpio_spec, enum of_gpio_flags *flags)
> {
> const u32 *gpio = gpio_spec;
>
> if (*gpio > of_gc->gc.ngpio)
> return -EINVAL;
>
> + if (flags && of_gc->gpio_cells > 1)
> + *flags = gpio[1];
> +
> return *gpio;
> }
> EXPORT_SYMBOL(of_gpio_simple_xlate);
> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
> index bde4b4b..7835cd4 100644
> --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> @@ -1094,7 +1094,7 @@ static int cpm_uart_init_port(struct device_node *np,
> }
>
> for (i = 0; i < NUM_GPIOS; i++)
> - pinfo->gpios[i] = of_get_gpio(np, i);
> + pinfo->gpios[i] = of_get_gpio(np, i, NULL);
>
> return cpm_uart_request_port(&pinfo->port);
>
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 67db101..d611746 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -20,13 +20,23 @@
> #ifdef CONFIG_OF_GPIO
>
> /*
> + * Flags as returned by OF GPIO chip's xlate method.
> + * The GPIO specifier in the OF device tree does not need to use this
> + * same format for its flags, but it's convenient if it does. For
> + * example, the of_mm_gpio_chip driver works this way.
> + */
> +enum of_gpio_flags {
> + OF_GPIO_ACTIVE_LOW = 1,
> +};
> +
> +/*
> * Generic OF GPIO chip
> */
> struct of_gpio_chip {
> struct gpio_chip gc;
> int gpio_cells;
> int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
> - const void *gpio_spec);
> + const void *gpio_spec, enum of_gpio_flags *flags);
> };
>
> static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
> @@ -50,16 +60,19 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
> return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
> }
>
> -extern int of_get_gpio(struct device_node *np, int index);
> +extern int of_get_gpio(struct device_node *np, int index,
> + enum of_gpio_flags *flags);
> extern int of_mm_gpiochip_add(struct device_node *np,
> struct of_mm_gpio_chip *mm_gc);
> extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
> struct device_node *np,
> - const void *gpio_spec);
> + const void *gpio_spec,
> + enum of_gpio_flags *flags);
> #else
>
> /* Drivers may not strictly depend on the GPIO support, so let them link. */
> -static inline int of_get_gpio(struct device_node *np, int index)
> +static inline int of_get_gpio(struct device_node *np, int index,
> + enum of_gpio_flags *flags)
> {
> return -ENOSYS;
> }
> --
> 1.5.4.3
>
Anton Vorontsov writes:
> Can we apply it? Paul, Benjamin?
>
> The patchwork url for this patch is:
> http://patchwork.ozlabs.org/patch/6650/
>
>
> Thanks!
>
> > drivers/mtd/nand/fsl_upm.c | 2 +-
> > drivers/net/fs_enet/fs_enet-main.c | 2 +-
> > drivers/net/phy/mdio-ofgpio.c | 4 ++--
> > drivers/of/gpio.c | 13 ++++++++++---
> > drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
> > include/linux/of_gpio.h | 21 +++++++++++++++++----
> > 6 files changed, 32 insertions(+), 12 deletions(-)
That would need acks from Jeff Garzik and David Woodhouse.
Alternatively you could add a new function (called, for instance,
of_get_gpio_flags) with the extra parameter to eliminate the need to
change any drivers at this stage, since they all seem to pass NULL for
the flags argument.
Paul.
On Thu, Nov 27, 2008 at 08:38:51AM +1100, Paul Mackerras wrote:
[...]
> > > drivers/mtd/nand/fsl_upm.c | 2 +-
> > > drivers/net/fs_enet/fs_enet-main.c | 2 +-
> > > drivers/net/phy/mdio-ofgpio.c | 4 ++--
> > > drivers/of/gpio.c | 13 ++++++++++---
> > > drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
> > > include/linux/of_gpio.h | 21 +++++++++++++++++----
> > > 6 files changed, 32 insertions(+), 12 deletions(-)
>
> That would need acks from Jeff Garzik and David Woodhouse.
>
> Alternatively you could add a new function (called, for instance,
> of_get_gpio_flags) with the extra parameter to eliminate the need to
> change any drivers at this stage, since they all seem to pass NULL for
> the flags argument.
:-))
This was _exactly_ my initial approach, but Trent and Grant thought
that changing API would be easy. Hah-ha!
Here is my initial patch implementing of_get_gpio_flags():
http://lkml.org/lkml/2008/7/25/236
Trent, are you going to push your patch through all the
maintainers, or should we, after all, revert to my approach
instead?
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Thu, 27 Nov 2008, Paul Mackerras wrote:
> Anton Vorontsov writes:
>
>> Can we apply it? Paul, Benjamin?
>>
>> The patchwork url for this patch is:
>> http://patchwork.ozlabs.org/patch/6650/
>>
>>
>> Thanks!
>>
>>> drivers/mtd/nand/fsl_upm.c | 2 +-
>>> drivers/net/fs_enet/fs_enet-main.c | 2 +-
>>> drivers/net/phy/mdio-ofgpio.c | 4 ++--
>>> drivers/of/gpio.c | 13 ++++++++++---
>>> drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
>>> include/linux/of_gpio.h | 21 +++++++++++++++++----
>>> 6 files changed, 32 insertions(+), 12 deletions(-)
>
> That would need acks from Jeff Garzik and David Woodhouse.
>
> Alternatively you could add a new function (called, for instance,
> of_get_gpio_flags) with the extra parameter to eliminate the need to
> change any drivers at this stage, since they all seem to pass NULL for
> the flags argument.
But if we did this every time any exported function needs to change, think
how bloated the API would be with cruft.
On Wed, Nov 26, 2008 at 02:35:54PM -0800, Trent Piepho wrote:
> On Thu, 27 Nov 2008, Paul Mackerras wrote:
> > Anton Vorontsov writes:
> >
> >> Can we apply it? Paul, Benjamin?
> >>
> >> The patchwork url for this patch is:
> >> http://patchwork.ozlabs.org/patch/6650/
> >>
> >>
> >> Thanks!
> >>
> >>> drivers/mtd/nand/fsl_upm.c | 2 +-
> >>> drivers/net/fs_enet/fs_enet-main.c | 2 +-
> >>> drivers/net/phy/mdio-ofgpio.c | 4 ++--
> >>> drivers/of/gpio.c | 13 ++++++++++---
> >>> drivers/serial/cpm_uart/cpm_uart_core.c | 2 +-
> >>> include/linux/of_gpio.h | 21 +++++++++++++++++----
> >>> 6 files changed, 32 insertions(+), 12 deletions(-)
> >
> > That would need acks from Jeff Garzik and David Woodhouse.
> >
> > Alternatively you could add a new function (called, for instance,
> > of_get_gpio_flags) with the extra parameter to eliminate the need to
> > change any drivers at this stage, since they all seem to pass NULL for
> > the flags argument.
>
> But if we did this every time any exported function needs to change, think
> how bloated the API would be with cruft.
Stable API is nonsense, yes. But we tend to change the API evolutionary,
not revolutionary. That is,
1. Implement of_get_gpio_flags();
2. Now we can start using it (no stall in development, see?);
3. Then somebody comes with the _cleanup_ patch:
"[PATCH] Merge of_get_gpio_flags() and of_get_gpio(), convert users"
^^ That patch is trivial and could be applied at any appropriate
moment (i.e. when there are no of_*_gpio*() patches queued in the
-next trees). And as time goes by, the patch collects all the needed
Acks, no need to hurry -- it's trivial cleanup.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
Trent Piepho writes:
> > Alternatively you could add a new function (called, for instance,
> > of_get_gpio_flags) with the extra parameter to eliminate the need to
> > change any drivers at this stage, since they all seem to pass NULL for
> > the flags argument.
>
> But if we did this every time any exported function needs to change, think
> how bloated the API would be with cruft.
I don't buy the argument that we can't add one thing because if we
added a hundred that would be too much. You could add
of_get_gpio_flags, get that upstream, then get the driver patches
upstream, then submit a patch to remove of_get_gpio. Alternatively
you could make of_get_gpio a macro or inline function in of_gpio.h.
If you really want to change everything in one hit you'll have to get
acks + agreement for the change to go upstream via my tree from all
the relevant driver maintainers first. I don't see any particular
advantage to doing it that way, though.
Paul.
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
--
Richard Purdie
Intel Open Source Technology Centre
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.