Despite leds-gpio and leds-of-gpio similar names and purposes, there
is not much code can be shared between the two drivers (both are mostly
driver bindings anyway).
Signed-off-by: Anton Vorontsov <[email protected]>
---
Kumar, since the dts bindings are split now, please don't bother with
all-in-one documentation patch. I guess it would be better to do it
this way. I also hope "Documentation/powerpc/dts-bindings" is the final
location for the bindings and won't change.
Documentation/powerpc/dts-bindings/gpio/led.txt | 15 ++
drivers/leds/Kconfig | 8 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-of-gpio.c | 192 +++++++++++++++++++++++
4 files changed, 216 insertions(+), 0 deletions(-)
create mode 100644 Documentation/powerpc/dts-bindings/gpio/led.txt
create mode 100644 drivers/leds/leds-of-gpio.c
diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
new file mode 100644
index 0000000..7e9ce81
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -0,0 +1,15 @@
+LED connected to GPIO
+
+Required properties:
+- compatible : should be "gpio-led".
+- 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.
+
+Example:
+
+led@0 {
+ compatible = "gpio-led";
+ label = "hdd";
+ gpios = <&mcu_pio 0 0>;
+};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c35dfa..ad7eab3 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -119,6 +119,14 @@ config LEDS_GPIO
outputs. To be useful the particular board must have LEDs
and they must be connected to the GPIO lines.
+config LEDS_OF_GPIO
+ tristate "LED Support for GPIO connected LEDs (OpenFirmware bindings)"
+ depends on LEDS_CLASS && OF_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.
+
config LEDS_CM_X270
tristate "LED Support for the CM-X270 LEDs"
depends on LEDS_CLASS && MACH_ARMCORE
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 7156f99..593775c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o
obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o
obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o
obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
+obj-$(CONFIG_LEDS_OF_GPIO) += leds-of-gpio.o
obj-$(CONFIG_LEDS_CM_X270) += leds-cm-x270.o
obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o
obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o
diff --git a/drivers/leds/leds-of-gpio.c b/drivers/leds/leds-of-gpio.c
new file mode 100644
index 0000000..f8e2597
--- /dev/null
+++ b/drivers/leds/leds-of-gpio.c
@@ -0,0 +1,192 @@
+/*
+ * LEDs driver for GPIOs (OpenFirmware bindings)
+ *
+ * Copyright (C) 2007 8D Technologies inc.
+ * Raphael Assenat <[email protected]>
+ * Copyright (C) 2008 MontaVista Software, Inc.
+ * Anton Vorontsov <[email protected]>
+ *
+ * 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
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+
+struct of_gpio_led {
+ struct device_node *np;
+ struct led_classdev cdev;
+ unsigned int gpio;
+ struct work_struct work;
+ u8 new_level;
+ u8 can_sleep;
+};
+
+static void gpio_led_work(struct work_struct *work)
+{
+ struct of_gpio_led *led = container_of(work, struct of_gpio_led, work);
+
+ gpio_set_value_cansleep(led->gpio, led->new_level);
+}
+
+static void gpio_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct of_gpio_led *led = container_of(led_cdev, struct of_gpio_led,
+ cdev);
+ int level;
+
+ if (value == LED_OFF)
+ level = 0;
+ else
+ level = 1;
+
+ /* Setting GPIOs with I2C/etc requires a task context, and we don't
+ * seem to have a reliable way to know if we're already in one; so
+ * let's just assume the worst.
+ */
+ if (led->can_sleep) {
+ led->new_level = level;
+ schedule_work(&led->work);
+ } else {
+ gpio_set_value(led->gpio, level);
+ }
+}
+
+static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ int ret;
+ struct of_gpio_led *led;
+ struct device_node *np = ofdev->node;
+
+ led = kzalloc(sizeof(*led), GFP_KERNEL);
+ if (!led)
+ return -ENOMEM;
+
+ led->np = np;
+
+ ret = of_get_gpio(np, 0);
+ if (!gpio_is_valid(ret)) {
+ dev_err(&ofdev->dev, "gpio is invalid\n");
+ goto err_get_gpio;
+ }
+ led->gpio = ret;
+ led->can_sleep = gpio_cansleep(led->gpio);
+
+ led->cdev.name = of_get_property(np, "label", NULL);
+ if (!led->cdev.name)
+ led->cdev.name = ofdev->dev.bus_id;
+ led->cdev.brightness_set = gpio_led_set;
+
+ ret = gpio_request(led->gpio, ofdev->dev.bus_id);
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "could not request gpio, status is %d\n",
+ ret);
+ goto err_gpio;
+ }
+
+ ret = gpio_direction_output(led->gpio, 0);
+ if (ret) {
+ dev_err(&ofdev->dev, "gpio could not be an output, "
+ "status is %d\n", ret);
+ goto err_gpio;
+ }
+
+ INIT_WORK(&led->work, gpio_led_work);
+ dev_set_drvdata(&ofdev->dev, led);
+
+ ret = led_classdev_register(&ofdev->dev, &led->cdev);
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "could register led cdev, status is %d\n",
+ ret);
+ gpio_free(led->gpio);
+ goto err_reg_cdev;
+ }
+
+ return 0;
+
+err_reg_cdev:
+ cancel_work_sync(&led->work);
+err_gpio:
+ gpio_free(led->gpio);
+err_get_gpio:
+ kfree(led);
+ return ret;
+}
+
+static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_unregister(&led->cdev);
+ cancel_work_sync(&led->work);
+ gpio_free(led->gpio);
+ of_node_put(led->np);
+ kfree(led);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_suspend(&led->cdev);
+ return 0;
+}
+
+static int of_gpio_led_resume(struct of_device *ofdev)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_resume(&led->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-led", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
+
+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);
+
+MODULE_DESCRIPTION("OF GPIO LED driver");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.5.5.4
Hi Anton,
On Mon, 14 Jul 2008 20:41:14 +0400 Anton Vorontsov <[email protected]> wrote:
>
> +static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int ret;
> + struct of_gpio_led *led;
> + struct device_node *np = ofdev->node;
> +
> + led = kzalloc(sizeof(*led), GFP_KERNEL);
> + if (!led)
> + return -ENOMEM;
> +
> + led->np = np;
You need to take a reference if you are keeping a pointer to a
device_node, so:
led->np = of_node_get(np);
> + led->cdev.name = of_get_property(np, "label", NULL);
> + if (!led->cdev.name)
> + led->cdev.name = ofdev->dev.bus_id;
Please use dev_name() in new code:
led->cdev.name = dev_name(&ofdev->dev);
> + led->cdev.brightness_set = gpio_led_set;
> +
> + ret = gpio_request(led->gpio, ofdev->dev.bus_id);
dev_name() again.
> +err_get_gpio:
of_node_put(led->np);
> + kfree(led);
> + return ret;
> +}
> +
> +static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
> +{
> + struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
> +
> + led_classdev_unregister(&led->cdev);
> + cancel_work_sync(&led->work);
> + gpio_free(led->gpio);
> + of_node_put(led->np);
This was going to be unbalanced, but is now correct.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
Hello Stephen,
On Tue, Jul 15, 2008 at 01:10:04PM +1000, Stephen Rothwell wrote:
[...]
> > + led->np = np;
>
> You need to take a reference if you are keeping a pointer to a
> device_node, so:
> led->np = of_node_get(np);
>
> > + led->cdev.name = of_get_property(np, "label", NULL);
> > + if (!led->cdev.name)
> > + led->cdev.name = ofdev->dev.bus_id;
>
> Please use dev_name() in new code:
> led->cdev.name = dev_name(&ofdev->dev);
>
> > + led->cdev.brightness_set = gpio_led_set;
> > +
> > + ret = gpio_request(led->gpio, ofdev->dev.bus_id);
>
> dev_name() again.
>
> > +err_get_gpio:
>
> of_node_put(led->np);
>
> > + kfree(led);
> > + return ret;
> > +}
> > +
> > +static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
> > +{
> > + struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
> > +
> > + led_classdev_unregister(&led->cdev);
> > + cancel_work_sync(&led->work);
> > + gpio_free(led->gpio);
> > + of_node_put(led->np);
>
> This was going to be unbalanced, but is now correct.
Thank you so much for the review, corrected version follows.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
Despite leds-gpio and leds-of-gpio similar names and purposes, there
is not much code can be shared between the two drivers (both are mostly
driver bindings anyway).
Signed-off-by: Anton Vorontsov <[email protected]>
---
Documentation/powerpc/dts-bindings/gpio/led.txt | 15 ++
drivers/leds/Kconfig | 8 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-of-gpio.c | 193 +++++++++++++++++++++++
4 files changed, 217 insertions(+), 0 deletions(-)
create mode 100644 Documentation/powerpc/dts-bindings/gpio/led.txt
create mode 100644 drivers/leds/leds-of-gpio.c
diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
new file mode 100644
index 0000000..7e9ce81
--- /dev/null
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -0,0 +1,15 @@
+LED connected to GPIO
+
+Required properties:
+- compatible : should be "gpio-led".
+- 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.
+
+Example:
+
+led@0 {
+ compatible = "gpio-led";
+ label = "hdd";
+ gpios = <&mcu_pio 0 0>;
+};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c35dfa..ad7eab3 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -119,6 +119,14 @@ config LEDS_GPIO
outputs. To be useful the particular board must have LEDs
and they must be connected to the GPIO lines.
+config LEDS_OF_GPIO
+ tristate "LED Support for GPIO connected LEDs (OpenFirmware bindings)"
+ depends on LEDS_CLASS && OF_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.
+
config LEDS_CM_X270
tristate "LED Support for the CM-X270 LEDs"
depends on LEDS_CLASS && MACH_ARMCORE
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 7156f99..593775c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o
obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o
obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o
obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
+obj-$(CONFIG_LEDS_OF_GPIO) += leds-of-gpio.o
obj-$(CONFIG_LEDS_CM_X270) += leds-cm-x270.o
obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o
obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o
diff --git a/drivers/leds/leds-of-gpio.c b/drivers/leds/leds-of-gpio.c
new file mode 100644
index 0000000..02c4290
--- /dev/null
+++ b/drivers/leds/leds-of-gpio.c
@@ -0,0 +1,193 @@
+/*
+ * LEDs driver for GPIOs (OpenFirmware bindings)
+ *
+ * Copyright (C) 2007 8D Technologies inc.
+ * Raphael Assenat <[email protected]>
+ * Copyright (C) 2008 MontaVista Software, Inc.
+ * Anton Vorontsov <[email protected]>
+ *
+ * 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
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+
+struct of_gpio_led {
+ struct device_node *np;
+ struct led_classdev cdev;
+ unsigned int gpio;
+ struct work_struct work;
+ u8 new_level;
+ u8 can_sleep;
+};
+
+static void gpio_led_work(struct work_struct *work)
+{
+ struct of_gpio_led *led = container_of(work, struct of_gpio_led, work);
+
+ gpio_set_value_cansleep(led->gpio, led->new_level);
+}
+
+static void gpio_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct of_gpio_led *led = container_of(led_cdev, struct of_gpio_led,
+ cdev);
+ int level;
+
+ if (value == LED_OFF)
+ level = 0;
+ else
+ level = 1;
+
+ /* Setting GPIOs with I2C/etc requires a task context, and we don't
+ * seem to have a reliable way to know if we're already in one; so
+ * let's just assume the worst.
+ */
+ if (led->can_sleep) {
+ led->new_level = level;
+ schedule_work(&led->work);
+ } else {
+ gpio_set_value(led->gpio, level);
+ }
+}
+
+static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ int ret;
+ struct of_gpio_led *led;
+ struct device_node *np = ofdev->node;
+
+ led = kzalloc(sizeof(*led), GFP_KERNEL);
+ if (!led)
+ return -ENOMEM;
+
+ led->np = of_node_get(np);
+
+ ret = of_get_gpio(np, 0);
+ if (!gpio_is_valid(ret)) {
+ dev_err(&ofdev->dev, "gpio is invalid\n");
+ goto err_get_gpio;
+ }
+ led->gpio = ret;
+ led->can_sleep = gpio_cansleep(led->gpio);
+
+ led->cdev.name = of_get_property(np, "label", NULL);
+ if (!led->cdev.name)
+ led->cdev.name = dev_name(&ofdev->dev);
+ led->cdev.brightness_set = gpio_led_set;
+
+ ret = gpio_request(led->gpio, dev_name(&ofdev->dev));
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "could not request gpio, status is %d\n",
+ ret);
+ goto err_gpio;
+ }
+
+ ret = gpio_direction_output(led->gpio, 0);
+ if (ret) {
+ dev_err(&ofdev->dev, "gpio could not be an output, "
+ "status is %d\n", ret);
+ goto err_gpio;
+ }
+
+ INIT_WORK(&led->work, gpio_led_work);
+ dev_set_drvdata(&ofdev->dev, led);
+
+ ret = led_classdev_register(&ofdev->dev, &led->cdev);
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "could register led cdev, status is %d\n",
+ ret);
+ gpio_free(led->gpio);
+ goto err_reg_cdev;
+ }
+
+ return 0;
+
+err_reg_cdev:
+ cancel_work_sync(&led->work);
+err_gpio:
+ gpio_free(led->gpio);
+err_get_gpio:
+ of_node_put(led->np);
+ kfree(led);
+ return ret;
+}
+
+static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_unregister(&led->cdev);
+ cancel_work_sync(&led->work);
+ gpio_free(led->gpio);
+ of_node_put(led->np);
+ kfree(led);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_suspend(&led->cdev);
+ return 0;
+}
+
+static int of_gpio_led_resume(struct of_device *ofdev)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_resume(&led->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-led", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
+
+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);
+
+MODULE_DESCRIPTION("OF GPIO LED driver");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.5.5.4
On Tue, 2008-07-15 at 16:40 +0400, Anton Vorontsov wrote:
> Despite leds-gpio and leds-of-gpio similar names and purposes, there
> is not much code can be shared between the two drivers (both are mostly
> driver bindings anyway).
I don't have any issue with the driver itself, just the name which is
going to confuse people no end.
Can we come up with a better name for this driver please?
"dts-bind-gpio"? "openfirmware-led"?
I'm mainly concerned with the more user visible bits like the name of
the .c file, the wording of the Kconfig option and the module
description. We need to play down the GPIO bit and play up the
openfirmware bindings bit.
As an example the Kconfig says "LED Support for GPIO connected LEDs"
which its not, the bit about openfirmware bindings is in brackets and
hence looks incidental.
Cheers,
Richard
On Tue, Jul 15, 2008 at 01:54:30PM +0100, Richard Purdie wrote:
> On Tue, 2008-07-15 at 16:40 +0400, Anton Vorontsov wrote:
> > Despite leds-gpio and leds-of-gpio similar names and purposes, there
> > is not much code can be shared between the two drivers (both are mostly
> > driver bindings anyway).
>
> I don't have any issue with the driver itself, just the name which is
> going to confuse people no end.
>
> Can we come up with a better name for this driver please?
> "dts-bind-gpio"?
Hm... I don't actually understand what this name implies.
> "openfirmware-led"?
And this would be wrong, since this driver is for GPIO LEDs only, not
for all LEDs that OF can describe. In future there could be OF PWM LEDs
or something like this.
> I'm mainly concerned with the more user visible bits like the name of
> the .c file, the wording of the Kconfig option and the module
> description. We need to play down the GPIO bit and play up the
> openfirmware bindings bit.
Hm... file name is leds-of-gpio.c, how could I play up the "of" bit more
than this? ;-)
> As an example the Kconfig says "LED Support for GPIO connected LEDs"
> which its not, the bit about openfirmware bindings is in brackets and
> hence looks incidental.
As for Kconfig, yeah.. probably I can improve the wording. How about
"OpenFirmware bindings for GPIO connected LEDs"? Would that work?
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Tue, 2008-07-15 at 17:24 +0400, Anton Vorontsov wrote:
> On Tue, Jul 15, 2008 at 01:54:30PM +0100, Richard Purdie wrote:
> > I don't have any issue with the driver itself, just the name which is
> > going to confuse people no end.
> >
> > Can we come up with a better name for this driver please?
[...]
> > "openfirmware-led"?
>
> And this would be wrong, since this driver is for GPIO LEDs only, not
> for all LEDs that OF can describe. In future there could be OF PWM LEDs
> or something like this.
Ok, will these be a separate driver or combined into the gpio driver?
> > I'm mainly concerned with the more user visible bits like the name of
> > the .c file, the wording of the Kconfig option and the module
> > description. We need to play down the GPIO bit and play up the
> > openfirmware bindings bit.
>
> Hm... file name is leds-of-gpio.c, how could I play up the "of" bit more
> than this? ;-)
Spell out openfirmware :). I initially had no idea "of == openfirmware"
and I suspect others won't either...
> > As an example the Kconfig says "LED Support for GPIO connected LEDs"
> > which its not, the bit about openfirmware bindings is in brackets and
> > hence looks incidental.
>
> As for Kconfig, yeah.. probably I can improve the wording. How about
> "OpenFirmware bindings for GPIO connected LEDs"? Would that work?
Yes, thats better. I think basically we need to spell out OF a bit more.
Its probably obvious to powerpc people but not everyone else.
Cheers,
Richard
On Tue, Jul 15, 2008 at 02:31:27PM +0100, Richard Purdie wrote:
> On Tue, 2008-07-15 at 17:24 +0400, Anton Vorontsov wrote:
> > On Tue, Jul 15, 2008 at 01:54:30PM +0100, Richard Purdie wrote:
> > > I don't have any issue with the driver itself, just the name which is
> > > going to confuse people no end.
> > >
> > > Can we come up with a better name for this driver please?
> [...]
> > > "openfirmware-led"?
> >
> > And this would be wrong, since this driver is for GPIO LEDs only, not
> > for all LEDs that OF can describe. In future there could be OF PWM LEDs
> > or something like this.
>
> Ok, will these be a separate driver or combined into the gpio driver?
I believe this must be a separate driver. The two drivers would have
nothing in common except prologue registration stuff.
> > > I'm mainly concerned with the more user visible bits like the name of
> > > the .c file, the wording of the Kconfig option and the module
> > > description. We need to play down the GPIO bit and play up the
> > > openfirmware bindings bit.
> >
> > Hm... file name is leds-of-gpio.c, how could I play up the "of" bit more
> > than this? ;-)
>
> Spell out openfirmware :). I initially had no idea "of == openfirmware"
> and I suspect others won't either...
This would be unusually long name, that is
$ find . -iname '*openfirmware*' | wc -l
0
And in contrast:
drivers/video/offb.c
drivers/video/nvidia/nv_of.c
drivers/usb/host/ohci-ppc-of.c
drivers/usb/host/ehci-ppc-of.c
drivers/serial/of_serial.c
drivers/mtd/maps/physmap_of.c
...
So, I think we should stick with the "of" for consistency, while
confused users may consult with Kconfig for disambiguation.
But if you really want the expanded name.. just repeat it, and I'll
change the name.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Tue, 2008-07-15 at 18:23 +0400, Anton Vorontsov wrote:
> > Spell out openfirmware :). I initially had no idea "of == openfirmware"
> > and I suspect others won't either...
>
> This would be unusually long name, that is
>
> $ find . -iname '*openfirmware*' | wc -l
> 0
>
> And in contrast:
>
> drivers/video/offb.c
> drivers/video/nvidia/nv_of.c
> drivers/usb/host/ohci-ppc-of.c
> drivers/usb/host/ehci-ppc-of.c
> drivers/serial/of_serial.c
> drivers/mtd/maps/physmap_of.c
> ...
>
> So, I think we should stick with the "of" for consistency, while
> confused users may consult with Kconfig for disambiguation.
The other cases don't have a gpio driver to confuse this new driver
with. Lets spell it out please, the filesystems can handle the extra
letters :).
Cheers,
Richard
Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
is not much code can be shared between the two drivers (both are mostly
driver bindings anyway).
Signed-off-by: Anton Vorontsov <[email protected]>
---
- dropped Documentation/powerpc/ changes, since Kumar applied them via
powerpc tree.
- renamed leds-of-gpio to leds-openfirmware-gpio
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-openfirmware-gpio.c | 194 +++++++++++++++++++++++++++++++++
3 files changed, 203 insertions(+), 0 deletions(-)
create mode 100644 drivers/leds/leds-openfirmware-gpio.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1c35dfa..a645e8d 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -119,6 +119,14 @@ config LEDS_GPIO
outputs. To be useful the particular board must have LEDs
and they must be connected to the GPIO lines.
+config LEDS_OPENFIRMWARE_GPIO
+ tristate "OpenFirmware bindings for GPIO connected LEDs"
+ depends on LEDS_CLASS && OF_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.
+
config LEDS_CM_X270
tristate "LED Support for the CM-X270 LEDs"
depends on LEDS_CLASS && MACH_ARMCORE
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 7156f99..0258ab7 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o
obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o
obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o
obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
+obj-$(CONFIG_LEDS_OPENFIRMWARE_GPIO) += leds-openfirmware-gpio.o
obj-$(CONFIG_LEDS_CM_X270) += leds-cm-x270.o
obj-$(CONFIG_LEDS_CLEVO_MAIL) += leds-clevo-mail.o
obj-$(CONFIG_LEDS_HP6XX) += leds-hp6xx.o
diff --git a/drivers/leds/leds-openfirmware-gpio.c b/drivers/leds/leds-openfirmware-gpio.c
new file mode 100644
index 0000000..ce2c3cd
--- /dev/null
+++ b/drivers/leds/leds-openfirmware-gpio.c
@@ -0,0 +1,194 @@
+/*
+ * OpenFirmware bindings for GPIO connected LEDs
+ *
+ * Copyright (C) 2007 8D Technologies inc.
+ * Raphael Assenat <[email protected]>
+ * Copyright (C) 2008 MontaVista Software, Inc.
+ * Anton Vorontsov <[email protected]>
+ *
+ * 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
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
+#include <asm/prom.h>
+
+struct of_gpio_led {
+ struct device_node *np;
+ struct led_classdev cdev;
+ unsigned int gpio;
+ struct work_struct work;
+ u8 new_level;
+ u8 can_sleep;
+};
+
+static void gpio_led_work(struct work_struct *work)
+{
+ struct of_gpio_led *led = container_of(work, struct of_gpio_led, work);
+
+ gpio_set_value_cansleep(led->gpio, led->new_level);
+}
+
+static void gpio_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct of_gpio_led *led = container_of(led_cdev, struct of_gpio_led,
+ cdev);
+ int level;
+
+ if (value == LED_OFF)
+ level = 0;
+ else
+ level = 1;
+
+ /* Setting GPIOs with I2C/etc requires a task context, and we don't
+ * seem to have a reliable way to know if we're already in one; so
+ * let's just assume the worst.
+ */
+ if (led->can_sleep) {
+ led->new_level = level;
+ schedule_work(&led->work);
+ } else {
+ gpio_set_value(led->gpio, level);
+ }
+}
+
+static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ int ret;
+ struct of_gpio_led *led;
+ struct device_node *np = ofdev->node;
+
+ led = kzalloc(sizeof(*led), GFP_KERNEL);
+ if (!led)
+ return -ENOMEM;
+
+ led->np = of_node_get(np);
+
+ ret = of_get_gpio(np, 0);
+ if (!gpio_is_valid(ret)) {
+ dev_err(&ofdev->dev, "gpio is invalid\n");
+ goto err_get_gpio;
+ }
+ led->gpio = ret;
+ led->can_sleep = gpio_cansleep(led->gpio);
+
+ led->cdev.name = of_get_property(np, "label", NULL);
+ if (!led->cdev.name)
+ led->cdev.name = dev_name(&ofdev->dev);
+ led->cdev.brightness_set = gpio_led_set;
+
+ ret = gpio_request(led->gpio, dev_name(&ofdev->dev));
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "could not request gpio, status is %d\n",
+ ret);
+ goto err_gpio;
+ }
+
+ ret = gpio_direction_output(led->gpio, 0);
+ if (ret) {
+ dev_err(&ofdev->dev, "gpio could not be an output, "
+ "status is %d\n", ret);
+ goto err_gpio;
+ }
+
+ INIT_WORK(&led->work, gpio_led_work);
+ dev_set_drvdata(&ofdev->dev, led);
+
+ ret = led_classdev_register(&ofdev->dev, &led->cdev);
+ if (ret < 0) {
+ dev_err(&ofdev->dev, "could register led cdev, status is %d\n",
+ ret);
+ gpio_free(led->gpio);
+ goto err_reg_cdev;
+ }
+
+ return 0;
+
+err_reg_cdev:
+ cancel_work_sync(&led->work);
+err_gpio:
+ gpio_free(led->gpio);
+err_get_gpio:
+ of_node_put(led->np);
+ kfree(led);
+ return ret;
+}
+
+static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_unregister(&led->cdev);
+ cancel_work_sync(&led->work);
+ gpio_free(led->gpio);
+ of_node_put(led->np);
+ kfree(led);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_suspend(&led->cdev);
+ return 0;
+}
+
+static int of_gpio_led_resume(struct of_device *ofdev)
+{
+ struct of_gpio_led *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_resume(&led->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-led", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_gpio_leds_match);
+
+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);
+
+MODULE_DESCRIPTION("OpenFirmware bindings for GPIO connected LEDs");
+MODULE_AUTHOR("Anton Vorontsov <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.5.5.4
On Tue, 15 Jul 2008, Anton Vorontsov wrote:
> Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
> is not much code can be shared between the two drivers (both are mostly
> driver bindings anyway).
Why can't this driver use the existing gpio-led driver? Basically, do
something like this:
of_gpio_leds_probe(...)
{
gpio = of_get_gpio(np, 0);
label = of_get_property(np, "label", NULL);
struct gpio_led led = {
.name = label,
.gpio = gpio,
};
pdev = platform_device_register_simple("leds-gpio", 0, NULL, 0);
platform_device_add_data(pdev, &led, sizeof(led));
}
On Tue, 15 Jul 2008, Richard Purdie wrote:
> On Tue, 2008-07-15 at 18:23 +0400, Anton Vorontsov wrote:
>>> Spell out openfirmware :). I initially had no idea "of == openfirmware"
>>> and I suspect others won't either...
>>
>> This would be unusually long name, that is
>>
>> $ find . -iname '*openfirmware*' | wc -l
>> 0
>>
>> And in contrast:
>>
>> drivers/video/offb.c
>> drivers/video/nvidia/nv_of.c
>> drivers/usb/host/ohci-ppc-of.c
>> drivers/usb/host/ehci-ppc-of.c
>> drivers/serial/of_serial.c
>> drivers/mtd/maps/physmap_of.c
>> ...
>>
>> So, I think we should stick with the "of" for consistency, while
>> confused users may consult with Kconfig for disambiguation.
>
> The other cases don't have a gpio driver to confuse this new driver
> with. Lets spell it out please, the filesystems can handle the extra
> letters :).
There's drivers/mtd/maps/physmap.c and drivers/mtd/maps/physmap_of.c,
drivers/serial/of_serial.c and drivers/serial/serial_core.c,
drivers/usb/host/ehci-ppc-soc.c and drivers/usb/host/ehci-ppc-of.c, etc.
On Wed, Jul 16, 2008 at 04:18:52PM -0700, Trent Piepho wrote:
> On Tue, 15 Jul 2008, Anton Vorontsov wrote:
> > Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
> > is not much code can be shared between the two drivers (both are mostly
> > driver bindings anyway).
>
> Why can't this driver use the existing gpio-led driver? Basically, do
> something like this:
>
> of_gpio_leds_probe(...)
> {
> gpio = of_get_gpio(np, 0);
> label = of_get_property(np, "label", NULL);
>
> struct gpio_led led = {
> .name = label,
> .gpio = gpio,
> };
>
> pdev = platform_device_register_simple("leds-gpio", 0, NULL, 0);
> platform_device_add_data(pdev, &led, sizeof(led));
> }
Ugh; that means registering *2* 'struct device' with the kernel instead of
one. One as a platform device and one as an of_platform device.
It's bad enough that the LED scheme we're using for OF bindings has a
separate registration for every single LED.
Now that it comes to it, I worry that this driver takes the wrong
approach. The number of resources dedicated per LED in this driver
seems pretty loony to me (one of_platform device per LED). The fact
that the binding specifies one node per LED makes of_platform not a very
efficient solution.
I think it would be better to have a module that scans the device tree
for LED nodes and registers a single leds-gpio platform device for the
whole lot.
Thoughts?
g.
on Wed, 16 Jul 2008, Grant Likely wrote:
> On Wed, Jul 16, 2008 at 04:18:52PM -0700, Trent Piepho wrote:
>> On Tue, 15 Jul 2008, Anton Vorontsov wrote:
>>> Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
>>> is not much code can be shared between the two drivers (both are mostly
>>> driver bindings anyway).
>>
>> Why can't this driver use the existing gpio-led driver? Basically, do
>> something like this:
>>
>
> Ugh; that means registering *2* 'struct device' with the kernel instead of
> one. One as a platform device and one as an of_platform device.
> It's bad enough that the LED scheme we're using for OF bindings has a
> separate registration for every single LED.
Ok, how about adding code the existing leds-gpio driver so that it can creates
LEDs from of_platform devices too?
I've made a patch to do this and it works ok. The code added to leds-gpio is
about half what was involved in Anton's new driver. What I did was re-factor
the existing platform device probe function to use a new function that creates
a single led. Then a new of_platform probe function can use it too. That way
most of the probe code is shared. remove, suspend and resume aren't shared,
but they're short. And the existing code to actually drive the led gets
reused as is.
There is still one of_platform device per led because of how the bindings work
(but that could be changed with new bindings), but there are zero extra
platform devices created.
Here's an example patch. It won't apply to the git kernel as is, but should
make it clear how this works.
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index a4a2838..12e681e 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -71,11 +71,45 @@ 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 create_gpio_led(struct gpio_led *cur_led,
+ struct gpio_led_data *led_dat, struct device *parent,
+ int (*blink_set)(unsigned, unsigned long *, unsigned long *))
+
+{
+ int ret;
+
+ ret = gpio_request(cur_led->gpio, cur_led->name);
+ if (ret < 0)
+ return ret;
+
+ 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 (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 = cur_led->start_on ? LED_FULL : LED_OFF;
+
+ gpio_direction_output(led_dat->gpio,
+ led_dat->active_low ^ cur_led->start_on);
+
+ 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 int 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)
@@ -87,36 +121,10 @@ static int gpio_led_probe(struct platform_device *pdev)
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 =
- cur_led->start_on ? LED_FULL : LED_OFF;
-
- gpio_direction_output(led_dat->gpio,
- led_dat->active_low ^ cur_led->start_on);
-
- 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);
@@ -217,3 +225,105 @@ MODULE_AUTHOR("Raphael Assenat <[email protected]>");
MODULE_DESCRIPTION("GPIO LED driver");
MODULE_LICENSE("GPL");
MODULE_ALIAS("platform:leds-gpio");
+
+
+/* #ifdef CONFIG_LEDS_GPIO_OF */
+/* OpenFirmware bindings */
+#include <linux/of_platform.h>
+
+/* crap for old kernel, ignore */
+static inline const char *dev_name(struct device *dev)
+{ return dev->bus_id; }
+int of_get_gpio(struct device_node *np, int index)
+{ const u32 *pp = of_get_property(np, "gpio", NULL); return pp ? *pp : -1; }
+
+static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ struct device_node *np = ofdev->node;
+ struct gpio_led led;
+ struct gpio_led_data *led_dat;
+ int ret;
+
+ led_dat = kzalloc(sizeof(*led_dat), GFP_KERNEL);
+ if (!led_dat)
+ return -ENOMEM;
+
+ memset(&led, 0, sizeof(led));
+ led.gpio = of_get_gpio(np, 0);
+ led.name = of_get_property(np, "label", NULL);
+ if (!led.name)
+ led.name = dev_name(&ofdev->dev);
+
+ ret = create_gpio_led(&led, led_dat, &ofdev->dev, NULL);
+ if (ret < 0) {
+ kfree(led_dat);
+ return ret;
+ }
+
+ dev_set_drvdata(&ofdev->dev, led_dat);
+
+ return 0;
+}
+
+static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
+{
+ struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_unregister(&led->cdev);
+ cancel_work_sync(&led->work);
+ gpio_free(led->gpio);
+ kfree(led);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
+{
+ struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_suspend(&led->cdev);
+ return 0;
+}
+
+static int of_gpio_led_resume(struct of_device *ofdev)
+{
+ struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
+
+ led_classdev_resume(&led->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-led", },
+ {},
+};
+
+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);
> diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt
> b/Documentation/powerpc/dts-bindings/gpio/led.txt
> new file mode 100644
> index 0000000..7e9ce81
> --- /dev/null
> +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
> @@ -0,0 +1,15 @@
> +LED connected to GPIO
> +
> +Required properties:
> +- compatible : should be "gpio-led".
This "compatible" name is a bit too generic. No, I don't know a
better name :-(
> +- label : (optional) the label for this LED. If omitted, the label is
> + taken from the node name (excluding the unit address).
What is a label? It should be described here. Also, its encoding
should be described ("a string" I guess).
> +- gpios : should specify LED GPIO.
> +
> +Example:
> +
> +led@0 {
> + compatible = "gpio-led";
> + label = "hdd";
> + gpios = <&mcu_pio 0 0>;
> +};
You show a unit address but have no "reg" value. This is
incorrect.
What would be the parent node of this, btw?
Segher
On Thu, Jul 17, 2008 at 07:59:03AM +0200, Segher Boessenkool wrote:
>> diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt
>> b/Documentation/powerpc/dts-bindings/gpio/led.txt
>> new file mode 100644
>> index 0000000..7e9ce81
>> --- /dev/null
>> +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
>> @@ -0,0 +1,15 @@
>> +LED connected to GPIO
>> +
>> +Required properties:
>> +- compatible : should be "gpio-led".
>
> This "compatible" name is a bit too generic. No, I don't know a
> better name :-(
>
>> +- label : (optional) the label for this LED. If omitted, the label is
>> + taken from the node name (excluding the unit address).
>
> What is a label?
The label that is written on the board for this particular LED, or
the label that hardware documentation refers to.
> It should be described here. Also, its encoding
> should be described ("a string" I guess).
Yes.
>> +- gpios : should specify LED GPIO.
>> +
>> +Example:
>> +
>> +led@0 {
>> + compatible = "gpio-led";
>> + label = "hdd";
>> + gpios = <&mcu_pio 0 0>;
>> +};
>
> You show a unit address but have no "reg" value. This is
> incorrect.
Hm.. how could I enumerate them then? Or should I just give them the
full names, i.e. "led-hdd" or something?
> What would be the parent node of this, btw?
This is tricky question. Personally I place them inside the gpio
controller node that is responsible for the LED. But I think placing the
led nodes at top level would be also fine (maybe with "leds { }" node as
a parent for all board's LEDs. What would you suggest for a "best
practice"?
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Wed, Jul 16, 2008 at 10:13:06PM -0700, Trent Piepho wrote:
> on Wed, 16 Jul 2008, Grant Likely wrote:
> > On Wed, Jul 16, 2008 at 04:18:52PM -0700, Trent Piepho wrote:
> >> On Tue, 15 Jul 2008, Anton Vorontsov wrote:
> >>> Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
> >>> is not much code can be shared between the two drivers (both are mostly
> >>> driver bindings anyway).
> >>
> >> Why can't this driver use the existing gpio-led driver? Basically, do
> >> something like this:
> >>
> >
> > Ugh; that means registering *2* 'struct device' with the kernel instead of
> > one. One as a platform device and one as an of_platform device.
> > It's bad enough that the LED scheme we're using for OF bindings has a
> > separate registration for every single LED.
>
> Ok, how about adding code the existing leds-gpio driver so that it can creates
> LEDs from of_platform devices too?
Few comments below.
> I've made a patch to do this and it works ok. The code added to leds-gpio is
> about half what was involved in Anton's new driver.
This isn't true.
> What I did was re-factor
> the existing platform device probe function to use a new function that creates
> a single led. Then a new of_platform probe function can use it too. That way
> most of the probe code is shared. remove, suspend and resume aren't shared,
> but they're short. And the existing code to actually drive the led gets
> reused as is.
>
> There is still one of_platform device per led because of how the bindings work
> (but that could be changed with new bindings), but there are zero extra
> platform devices created.
You didn't count extra platform driver. You can't #ifdef it. The only way
you can avoid this is creating leds-gpio-base.c or something, and place the
helper functions there.
> Here's an example patch. It won't apply to the git kernel as is, but should
> make it clear how this works.
>
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index a4a2838..12e681e 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -71,11 +71,45 @@ 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 create_gpio_led(struct gpio_led *cur_led,
The create_gpio_led() interface is also quite weird, since it implies that
we have to pass two GPIO LED "handles": struct gpio_led_data (that we
allocated) and temporary struct gpio_led. And this helper function will
just assign things from one struct to another, and then will register the
led.
With OF driver I don't need "struct gpio_led". Only the platform driver
need this so platforms could pass gpio led info through it, while with OF
we're getting all information from the device tree.
I'm not opposed to another struct used in the OF driver though, I'm rather
opposed to the indirectness it introduces. But I also believe that we
can refactor the code so it will look neat. I just don't want to do
all this churn to save mere 30 lines of code.
> + struct gpio_led_data *led_dat, struct device *parent,
> + int (*blink_set)(unsigned, unsigned long *, unsigned long *))
We don't need blink_set. Personally I think that accepting blink
support in leds-gpio driver was... um, not right. In OF driver we'll
never use it. We support just GPIO LEDs, not PWM LEDs.
> +
> +{
> + int ret;
> +
> + ret = gpio_request(cur_led->gpio, cur_led->name);
> + if (ret < 0)
> + return ret;
> +
> + 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 (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 = cur_led->start_on ? LED_FULL : LED_OFF;
> +
> + gpio_direction_output(led_dat->gpio,
> + led_dat->active_low ^ cur_led->start_on);
> +
> + 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 int 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)
> @@ -87,36 +121,10 @@ static int gpio_led_probe(struct platform_device *pdev)
> 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 =
> - cur_led->start_on ? LED_FULL : LED_OFF;
> -
> - gpio_direction_output(led_dat->gpio,
> - led_dat->active_low ^ cur_led->start_on);
> -
> - 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);
> @@ -217,3 +225,105 @@ MODULE_AUTHOR("Raphael Assenat <[email protected]>");
> MODULE_DESCRIPTION("GPIO LED driver");
> MODULE_LICENSE("GPL");
> MODULE_ALIAS("platform:leds-gpio");
> +
> +
> +/* #ifdef CONFIG_LEDS_GPIO_OF */
Heh.
> +/* OpenFirmware bindings */
> +#include <linux/of_platform.h>
> +
> +/* crap for old kernel, ignore */
> +static inline const char *dev_name(struct device *dev)
> +{ return dev->bus_id; }
> +int of_get_gpio(struct device_node *np, int index)
> +{ const u32 *pp = of_get_property(np, "gpio", NULL); return pp ? *pp : -1; }
> +
> +static int __devinit of_gpio_leds_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + struct device_node *np = ofdev->node;
> + struct gpio_led led;
> + struct gpio_led_data *led_dat;
> + int ret;
> +
> + led_dat = kzalloc(sizeof(*led_dat), GFP_KERNEL);
> + if (!led_dat)
> + return -ENOMEM;
> +
> + memset(&led, 0, sizeof(led));
> + led.gpio = of_get_gpio(np, 0);
> + led.name = of_get_property(np, "label", NULL);
> + if (!led.name)
> + led.name = dev_name(&ofdev->dev);
> +
> + ret = create_gpio_led(&led, led_dat, &ofdev->dev, NULL);
> + if (ret < 0) {
> + kfree(led_dat);
> + return ret;
> + }
> +
> + dev_set_drvdata(&ofdev->dev, led_dat);
> +
> + return 0;
> +}
> +
> +static int __devexit of_gpio_leds_remove(struct of_device *ofdev)
> +{
> + struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
> +
> + led_classdev_unregister(&led->cdev);
> + cancel_work_sync(&led->work);
> + gpio_free(led->gpio);
> + kfree(led);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int of_gpio_led_suspend(struct of_device *ofdev, pm_message_t state)
> +{
> + struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
> +
> + led_classdev_suspend(&led->cdev);
> + return 0;
> +}
> +
> +static int of_gpio_led_resume(struct of_device *ofdev)
> +{
> + struct gpio_led_data *led = dev_get_drvdata(&ofdev->dev);
> +
> + led_classdev_resume(&led->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-led", },
> + {},
> +};
> +
> +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);
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Wed, Jul 16, 2008 at 10:15:31PM -0600, Grant Likely wrote:
> On Wed, Jul 16, 2008 at 04:18:52PM -0700, Trent Piepho wrote:
> > On Tue, 15 Jul 2008, Anton Vorontsov wrote:
> > > Despite leds-gpio and leds-openfirmware-gpio similar purposes, there
> > > is not much code can be shared between the two drivers (both are mostly
> > > driver bindings anyway).
> >
> > Why can't this driver use the existing gpio-led driver? Basically, do
> > something like this:
> >
> > of_gpio_leds_probe(...)
> > {
> > gpio = of_get_gpio(np, 0);
> > label = of_get_property(np, "label", NULL);
> >
> > struct gpio_led led = {
> > .name = label,
> > .gpio = gpio,
> > };
> >
> > pdev = platform_device_register_simple("leds-gpio", 0, NULL, 0);
> > platform_device_add_data(pdev, &led, sizeof(led));
> > }
>
> Ugh; that means registering *2* 'struct device' with the kernel instead of
> one. One as a platform device and one as an of_platform device.
> It's bad enough that the LED scheme we're using for OF bindings has a
> separate registration for every single LED.
>
> Now that it comes to it, I worry that this driver takes the wrong
> approach. The number of resources dedicated per LED in this driver
> seems pretty loony to me (one of_platform device per LED). The fact
> that the binding specifies one node per LED makes of_platform not a very
> efficient solution.
>
> I think it would be better to have a module that scans the device tree
> for LED nodes and registers a single leds-gpio platform device for the
> whole lot.
>
> Thoughts?
I like the idea, thanks.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
[...]
> > I think it would be better to have a module that scans the device tree
> > for LED nodes and registers a single leds-gpio platform device for the
> > whole lot.
> >
> > Thoughts?
>
> I like the idea, thanks.
Ugh, no. The idea sounds good, but in practice it isn't, since we'll
have to handle suspend/resume ops ourselves. When we stick with the
device/driver model we're getting all this for free.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jul 17, 2008 at 06:13:35PM +0400, Anton Vorontsov wrote:
> On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
> [...]
> > > I think it would be better to have a module that scans the device tree
> > > for LED nodes and registers a single leds-gpio platform device for the
> > > whole lot.
> > >
> > > Thoughts?
> >
> > I like the idea, thanks.
>
> Ugh, no. The idea sounds good, but in practice it isn't, since we'll
> have to handle suspend/resume ops ourselves. When we stick with the
> device/driver model we're getting all this for free.
Won't the leds-gpio driver give you suspend/resume support?
g.
On Thu, 17 Jul 2008 15:07:30 +0400
"Anton Vorontsov" <[email protected]> wrote:
> This is tricky question. Personally I place them inside the gpio
> controller node that is responsible for the LED. But I think placing
> the led nodes at top level would be also fine (maybe with "leds { }"
> node as a parent for all board's LEDs. What would you suggest for a
> "best practice"?
I also put the leds under the gpio controller for the Warp. It is then
very clear which gpio controller the leds belong to.
Putting them at the top level does not associate the leds with the
correct gpio controller.
Warning: I am *not* using the of gpio led driver, but I hope to move to
it once the dust settles and drop the current Warp specific driver ;)
Cheers,
Sean
On Thu, Jul 17, 2008 at 03:07:30PM +0400, Anton Vorontsov wrote:
> On Thu, Jul 17, 2008 at 07:59:03AM +0200, Segher Boessenkool wrote:
> > What would be the parent node of this, btw?
>
> This is tricky question. Personally I place them inside the gpio
> controller node that is responsible for the LED. But I think placing the
> led nodes at top level would be also fine (maybe with "leds { }" node as
> a parent for all board's LEDs. What would you suggest for a "best
> practice"?
I like this idea (a 'leds' parent node). They aren't really children
of the GPIO node or any other device/bus in the system. Putting them
under a dedicated 'leds' node would make them easy to find and would
have the added advantage of making it easier to have a single driver
instance manage the whole lot.
g.
On Thu, Jul 17, 2008 at 09:04:22AM -0600, Grant Likely wrote:
> On Thu, Jul 17, 2008 at 06:13:35PM +0400, Anton Vorontsov wrote:
> > On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
> > [...]
> > > > I think it would be better to have a module that scans the device tree
> > > > for LED nodes and registers a single leds-gpio platform device for the
> > > > whole lot.
> > > >
> > > > Thoughts?
> > >
> > > I like the idea, thanks.
> >
> > Ugh, no. The idea sounds good, but in practice it isn't, since we'll
> > have to handle suspend/resume ops ourselves. When we stick with the
> > device/driver model we're getting all this for free.
>
> Won't the leds-gpio driver give you suspend/resume support?
Ah. I just wrongly read your message. You're purposing to use gpio
leds platform driver... I think this is doable, yes.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jul 17, 2008 at 9:20 AM, Anton Vorontsov
<[email protected]> wrote:
> On Thu, Jul 17, 2008 at 09:04:22AM -0600, Grant Likely wrote:
>> On Thu, Jul 17, 2008 at 06:13:35PM +0400, Anton Vorontsov wrote:
>> > On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
>> > [...]
>> > > > I think it would be better to have a module that scans the device tree
>> > > > for LED nodes and registers a single leds-gpio platform device for the
>> > > > whole lot.
>> > > >
>> > > > Thoughts?
>> > >
>> > > I like the idea, thanks.
>> >
>> > Ugh, no. The idea sounds good, but in practice it isn't, since we'll
>> > have to handle suspend/resume ops ourselves. When we stick with the
>> > device/driver model we're getting all this for free.
>>
>> Won't the leds-gpio driver give you suspend/resume support?
>
> Ah. I just wrongly read your message. You're purposing to use gpio
> leds platform driver... I think this is doable, yes.
Alternately, I would also be okay with a scheme where all LED nodes
have a common parent and an of_platform driver would bind against the
parent node; not the individual children. Then the leds-gpio driver
could be refactored to have both platform and of_platform bus
bindings.
Cheers,
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
On Thu, 17 Jul 2008, Anton Vorontsov wrote:
> On Wed, Jul 16, 2008 at 10:13:06PM -0700, Trent Piepho wrote:
>> Ok, how about adding code the existing leds-gpio driver so that it can creates
>> LEDs from of_platform devices too?
>
> Few comments below.
>
>> I've made a patch to do this and it works ok. The code added to leds-gpio is
>> about half what was involved in Anton's new driver.
>
> This isn't true.
Your new driver was 194 lines, not counting docs or Kconfig. My patch
added about 104 lines to the existing leds-gpio driver. So yes, about half
the code.
>> There is still one of_platform device per led because of how the bindings work
>> (but that could be changed with new bindings), but there are zero extra
>> platform devices created.
>
> You didn't count extra platform driver. You can't #ifdef it. The only way
> you can avoid this is creating leds-gpio-base.c or something, and place the
> helper functions there.
I guess, in terms of compiled size, the combined platform + of_platform
driver is bigger than the of_platform only driver. Though it's much
smaller than having both the platform only and of_platform only drivers.
In terms of source code, there's less with the combined driver.
I don't see why the combined leds-gpio driver can't have an ifdef for the
platform part. All the platform driver specific code is already in one
contiguous block. In fact, I just did it and it works fine. My LEDs from
the dts were created and the LED I have as a platform device wasn't, as
expected. Here's the patch, pretty simple:
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -108,2 +108,3 @@ static int create_gpio_led(struct gpio_led *cur_led,
+#ifdef CONFIG_LEDS_GPIO_PLATFORM
static int gpio_led_probe(struct platform_device *pdev)
@@ -222,2 +223,3 @@ module_init(gpio_led_init);
module_exit(gpio_led_exit);
+#endif /* CONFIG_LEDS_GPIO_PLATFORM */
>> +static int create_gpio_led(struct gpio_led *cur_led,
>
> The create_gpio_led() interface is also quite weird, since it implies that
> we have to pass two GPIO LED "handles": struct gpio_led_data (that we
> allocated) and temporary struct gpio_led. And this helper function will
> just assign things from one struct to another, and then will register the
> led.
It creates a "thing" from a template passed a pointer to a struct. This is
very common, there must be hundreds of functions in the kernel that work
this way. The difference is instead of allocating and returning the
created thing, you pass it a blank pre-allocated thing to fill in and
register. I know there is other code that works this way too. It's
usually used, like it is here, so you can allocate a bunch of things at
once and then register them one at a time.
> With OF driver I don't need "struct gpio_led". Only the platform driver
> need this so platforms could pass gpio led info through it, while with OF
> we're getting all information from the device tree.
The struct gpio_led is just used to pass the stats to the led creation
function. It doesn't stick around. You could use local variables for the
gpio and name and pass them to the create led function. Bundling them into
a struct is an tiny change that lets more code be shared.
>> +/* #ifdef CONFIG_LEDS_GPIO_OF */
>
> Heh.
Obviously the OF binding code should be conditional, selectable from
kconfig if the platform has OF support. It's all in one contiguous block
and that shows where the ifdef would go. I didn't think it was necessary
to include the obvious kconfig patch.
On Thu, 17 Jul 2008, Grant Likely wrote:
> Alternately, I would also be okay with a scheme where all LED nodes
> have a common parent and an of_platform driver would bind against the
> parent node; not the individual children. Then the leds-gpio driver
> could be refactored to have both platform and of_platform bus
> bindings.
Basically what I did then in my patch then, refactor leds-gpio so most of
it is shared and there is a block of code that does platform binding and
another block that does of_platform binding.
I didn't change the OF platform binding syntax so as not to complicate the
example, but that's easy to do. Something like:
leds {
compatible = "gpio-led";
gpios = <&mpc8572 6 0
&mpc8572 7 0>;
labels = "red", "green";
};
Or like this, which needs a little more code to parse:
leds {
compatible = "gpio-led";
led@6 {
gpios = <&mpc8572 6 0>;
label = "red";
};
led@7 {
gpios = <&mpc8572 7 0>;
label = "green";
};
};
I like the first better. It follows the example from the docs about how
devices with multiple gpios should work too.
On Thu, Jul 17, 2008 at 01:18:18PM -0700, Trent Piepho wrote:
> On Thu, 17 Jul 2008, Grant Likely wrote:
> > Alternately, I would also be okay with a scheme where all LED nodes
> > have a common parent and an of_platform driver would bind against the
> > parent node; not the individual children. Then the leds-gpio driver
> > could be refactored to have both platform and of_platform bus
> > bindings.
>
> Basically what I did then in my patch then, refactor leds-gpio so most of
> it is shared and there is a block of code that does platform binding and
> another block that does of_platform binding.
Yes
> I didn't change the OF platform binding syntax so as not to complicate the
> example, but that's easy to do. Something like:
>
> leds {
> compatible = "gpio-led";
> gpios = <&mpc8572 6 0
> &mpc8572 7 0>;
> labels = "red", "green";
> };
>
> Or like this, which needs a little more code to parse:
>
> leds {
> compatible = "gpio-led";
> led@6 {
> gpios = <&mpc8572 6 0>;
> label = "red";
> };
> led@7 {
> gpios = <&mpc8572 7 0>;
> label = "green";
> };
> };
I kind of like the second option better, because there is less chance
of doing bad stuff if the gpio specifier was buggered up; but I'm cool
with either.
However, if the second option is chosen then something like the following
might be better as it eliminates the meaningless @<number> specifier.
leds {
compatible = "gpio-led";
red {
gpios = <&mpc8572 6 0>;
};
green {
gpios = <&mpc8572 7 0>;
};
};
Cheers,
g.
On Tue, 2008-07-15 at 19:19 +0400, Anton Vorontsov wrote:
> + led->cdev.name = of_get_property(np, "label", NULL);
> + if (!led->cdev.name)
> + led->cdev.name = dev_name(&ofdev->dev);
How about also supporting a "trigger" property which would set
cdev.default_trigger in the same manner? It would be useful for boards
to be able to specify this in the device tree (e.g., if a certain LED is
meant to be used as a "heartbeat").
--
Nate Case <[email protected]>
On Thu, Jul 17, 2008 at 01:18:18PM -0700, Trent Piepho wrote:
> On Thu, 17 Jul 2008, Grant Likely wrote:
> > Alternately, I would also be okay with a scheme where all LED nodes
> > have a common parent and an of_platform driver would bind against the
> > parent node; not the individual children. Then the leds-gpio driver
> > could be refactored to have both platform and of_platform bus
> > bindings.
>
> Basically what I did then in my patch then, refactor leds-gpio so most of
> it is shared and there is a block of code that does platform binding and
> another block that does of_platform binding.
Ok. I must admit I'm quite burned out with OF gpio-leds. I was posting the
bindings since April, probably four or five times. Last time a week ago,
IIRC.
During the months I received just a few replies, one from Grant ("Looks
good to me."), few from Segher (with a lot of criticism, that I much
appreciated and tried to fix all spotted issues), and one from Laurent
(about active-low LEDs).
Of course, I know we're all busy etc and we don't always read or reply to
RFCs, so don't get me wrong, I'm not blaming or something. It just would be
great if all these ideas would be spoken up a bit earlier, i.e. not in the
middle of the merge window... But yes, we have what we have.
And the true thing is that I tend to agree with all your comments, and I
strongly believe that the issues you pointed out should be fixed prior to
merging. But I'm tired of these leds, they aren't _that_ interesting,
btw. ;-)
So..
Trent, I encourage you to collect your patch snippets into complete patch,
and post it so it could slip into 2.6.27 (the bad thing is that your
approach involves changes to the existing drivers, not just adding new
driver that could slip even into -rc9).
That is, I have a bunch of patches in my stg queue on which I can work
with more fun, so I'm somewhat glad that somebody will take care of the
notorious OF GPIO LEDs. ;-)
For what it's worth:
> I didn't change the OF platform binding syntax so as not to complicate the
> example, but that's easy to do. Something like:
>
> leds {
> compatible = "gpio-led";
> gpios = <&mpc8572 6 0
> &mpc8572 7 0>;
> labels = "red", "green";
> };
>
I don't like this.
> Or like this, which needs a little more code to parse:
>
> leds {
> compatible = "gpio-led";
> led@6 {
> gpios = <&mpc8572 6 0>;
> label = "red";
> };
> led@7 {
> gpios = <&mpc8572 7 0>;
> label = "green";
> };
> };
I like this. Or better what Grant suggested, i.e. move label to node
name.
> I like the first better. It follows the example from the docs about how
> devices with multiple gpios should work too.
IMO, each LED is a device. So, I would rather define compatible = <> for
each led.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Thu, Jul 17, 2008 at 09:07:15AM -0600, Grant Likely wrote:
> On Thu, Jul 17, 2008 at 03:07:30PM +0400, Anton Vorontsov wrote:
> > On Thu, Jul 17, 2008 at 07:59:03AM +0200, Segher Boessenkool wrote:
> > > What would be the parent node of this, btw?
> >
> > This is tricky question. Personally I place them inside the gpio
> > controller node that is responsible for the LED. But I think placing the
> > led nodes at top level would be also fine (maybe with "leds { }" node as
> > a parent for all board's LEDs. What would you suggest for a "best
> > practice"?
>
> I like this idea (a 'leds' parent node). They aren't really children
> of the GPIO node or any other device/bus in the system. Putting them
> under a dedicated 'leds' node would make them easy to find and would
> have the added advantage of making it easier to have a single driver
> instance manage the whole lot.
Hmm. Putting them under the gpio seems reasonable to me. The gpio
lines are the LEDs' "bus" to the limited extent that they have any bus
at all.
This brings us back to the issue we also have with DCR controlled
devices. Possibly we should have two ways of representing these
connections: for "pure" GPIO-only or DCR-only devices, they appear
under the relevant controller with the addresses encoded with 'reg'.
For devices on other busses which also have a few GPIO lines / DCR
registers, they would appear on the other bus with 'gpios' or
'dcr-reg' properties (or some new, generalized 'other-bus-reg'
property).
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
On Fri, Jul 18, 2008 at 01:35:12PM +1000, David Gibson wrote:
> This brings us back to the issue we also have with DCR controlled
> devices. Possibly we should have two ways of representing these
> connections: for "pure" GPIO-only or DCR-only devices, they appear
> under the relevant controller with the addresses encoded with 'reg'.
> For devices on other busses which also have a few GPIO lines / DCR
> registers, they would appear on the other bus with 'gpios' or
> 'dcr-reg' properties (or some new, generalized 'other-bus-reg'
> property).
On the other hand; it's just LEDs. I think a single scheme is enough.
:-)
g.
On Fri, Jul 18, 2008 at 03:42:01AM +0400, Anton Vorontsov wrote:
>
<lots of legitimate frustration snipped>
> So..
>
> Trent, I encourage you to collect your patch snippets into complete patch,
> and post it so it could slip into 2.6.27 (the bad thing is that your
> approach involves changes to the existing drivers, not just adding new
> driver that could slip even into -rc9).
>
> That is, I have a bunch of patches in my stg queue on which I can work
> with more fun, so I'm somewhat glad that somebody will take care of the
> notorious OF GPIO LEDs. ;-)
Okay. Thank you for all your work; it is *much* appreciated. I'm sorry
that these things didn't come up earlier and I feel your pain. :-)
Trent, I'd be happy to help and do what I can to expedite getting of
gpio-leds support merged in the .27 window.
> > I like the first better. It follows the example from the docs about how
> > devices with multiple gpios should work too.
>
> IMO, each LED is a device. So, I would rather define compatible = <> for
> each led.
If we choose that all gpio-leds will have a common parent, then I'd
still argue for the compatible property to be on the parent node because
all the children are homogeneous.
g.
On Fri, 18 Jul 2008, Anton Vorontsov wrote:
> On Thu, Jul 17, 2008 at 01:18:18PM -0700, Trent Piepho wrote:
>> Basically what I did then in my patch then, refactor leds-gpio so most of
>> it is shared and there is a block of code that does platform binding and
>> another block that does of_platform binding.
>
> Ok. I must admit I'm quite burned out with OF gpio-leds. I was posting the
> bindings since April, probably four or five times. Last time a week ago,
> IIRC.
>
> During the months I received just a few replies, one from Grant ("Looks
> good to me."), few from Segher (with a lot of criticism, that I much
> appreciated and tried to fix all spotted issues), and one from Laurent
> (about active-low LEDs).
I'm sorry, I never saw those emails. Were they all to linuxppc-dev? I hate
reading that list, gmane, marc, and lkml.org don't archive it and
mail-archive.com isn't nearly as nice.
Is this the last version?
http://www.mail-archive.com/[email protected]/msg18355.html
Why did you get rid of "linux,default-trigger" and the active-low property?
I couldn't find any discussion about this. When I first saw your current
patch I was going to add them, but I see you already had them and then
removed them.
I'd like to replace my "led-hack" stg patch with this, but I need
default-trigger to do that. I don't need active-low or default-brightness,
but they seem like a good idea. Actually, if you look closely at the patch
I posted, you'll see I had modified the existing leds-gpio driver to add a
default-brightness feature, though I don't need it anymore. The requirement
changed from having an led on during kernel boot to having it flash. I
have another hack for that, since there is no way to pass parameters to a
trigger.
>> leds {
>> compatible = "gpio-led";
>> led@6 {
>> gpios = <&mpc8572 6 0>;
>> label = "red";
>> };
>> led@7 {
>> gpios = <&mpc8572 7 0>;
>> label = "green";
>> };
>> };
>
> I like this. Or better what Grant suggested, i.e. move label to node
> name.
Ok, I used that. It's like the new way partitions are defined in NOR flash
OF bindings. My first example was more like the old way. The led name can
come from a label property or if there is none then the node name is used.
I don't think you can have colons or spaces in node names. I don't have a
default trigger property, but I'd really like to have that. I could add an
active low flag too. Maybe compatible should be "gpio-leds", since you can
define more than one?
The patch is done and works for me. One can select platform device and/or
of_platform device support via Kconfig. I'll port it to the git kernel and
post it soon.
On Fri, Jul 18, 2008 at 02:20:42AM -0700, Trent Piepho wrote:
> On Fri, 18 Jul 2008, Anton Vorontsov wrote:
> > On Thu, Jul 17, 2008 at 01:18:18PM -0700, Trent Piepho wrote:
> >> Basically what I did then in my patch then, refactor leds-gpio so most of
> >> it is shared and there is a block of code that does platform binding and
> >> another block that does of_platform binding.
> >
> > Ok. I must admit I'm quite burned out with OF gpio-leds. I was posting the
> > bindings since April, probably four or five times. Last time a week ago,
> > IIRC.
> >
> > During the months I received just a few replies, one from Grant ("Looks
> > good to me."), few from Segher (with a lot of criticism, that I much
> > appreciated and tried to fix all spotted issues), and one from Laurent
> > (about active-low LEDs).
>
> I'm sorry, I never saw those emails. Were they all to linuxppc-dev? I hate
> reading that list, gmane, marc, and lkml.org don't archive it and
> mail-archive.com isn't nearly as nice.
Usually I use this archive: http://ozlabs.org/pipermail/linuxppc-dev/
> Is this the last version?
> http://www.mail-archive.com/[email protected]/msg18355.html
No, this is v2 or something. The last version is the same as the current
one:
http://ozlabs.org/pipermail/linuxppc-dev/2008-July/059156.html
> Why did you get rid of "linux,default-trigger"
Nobody liked it, even I myself. ;-)
http://ozlabs.org/pipermail/linuxppc-dev/2008-May/056943.html
Later I tried to use aliases for default-trigger.
http://ozlabs.org/pipermail/linuxppc-dev/2008-June/057244.html
But after "i2c vs. cell-index vs. aliases" discussion I decided to
remove this stuff completely until I find something better (or
somebody will purpose).
As for linux,default-brightness... we don't need this since now we
have default-on trigger.
> and the active-low property?
Active-low can be coded inside the gpios = <>, via flags cell. And GPIO
controller can transparently support active-low GPIO states (i.e. just
like IRQ controllers automatically setting up IRQ trigger types based
on information from the device tree).
I implemented active-low flags for pixis gpio controller, but it is easy
to do this for any controller, when needed. Here is example:
diff --git a/arch/powerpc/platforms/86xx/pixis_gpio.c b/arch/powerpc/platforms/86xx/pixis_gpio.c
new file mode 100644
index 0000000..85254f9
--- /dev/null
+++ b/arch/powerpc/platforms/86xx/pixis_gpio.c
@@ -0,0 +1,141 @@
+/*
+ * PIXIS GPIOs
+ *
+ * Copyright (c) MontaVista Software, Inc. 2008.
+ *
+ * Author: Anton Vorontsov <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/spinlock.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
+
+#define PIXIS_GPIO_PINS 8
+
+struct px_gpio_chip {
+ struct of_mm_gpio_chip mm_gc;
+ spinlock_t lock;
+
+ /* mask for active-low pins */
+ u8 active_low;
+};
+
+static inline struct px_gpio_chip *
+to_px_gpio_chip(struct of_mm_gpio_chip *mm_gc)
+{
+ return container_of(mm_gc, struct px_gpio_chip, mm_gc);
+}
+
+static inline u8 pin2mask(unsigned int pin)
+{
+ return 1 << (PIXIS_GPIO_PINS - 1 - pin);
+}
+
+static int px_gpio_get(struct gpio_chip *gc, unsigned int gpio)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct px_gpio_chip *px_gc = to_px_gpio_chip(mm_gc);
+ u8 __iomem *regs = mm_gc->regs;
+
+ return (in_8(regs) ^ px_gc->active_low) & pin2mask(gpio);
+}
+
+static void px_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc);
+ struct px_gpio_chip *px_gc = to_px_gpio_chip(mm_gc);
+ u8 __iomem *regs = mm_gc->regs;
+ unsigned long flags;
+ u8 val_mask = val ? pin2mask(gpio) : 0;
+
+ spin_lock_irqsave(&px_gc->lock, flags);
+
+ if ((val_mask ^ px_gc->active_low) & pin2mask(gpio))
+ setbits8(regs, pin2mask(gpio));
+ else
+ clrbits8(regs, pin2mask(gpio));
+
+ spin_unlock_irqrestore(&px_gc->lock, flags);
+}
+
+static int px_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
+{
+ return 0;
+}
+
+static int px_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
+{
+ px_gpio_set(gc, gpio, val);
+ return 0;
+}
+
+#define PX_GPIO_FLAG_ACTIVE_LOW (1 << 0)
+
+int px_gpio_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
+ const void *gpio_spec)
+{
+ struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(&of_gc->gc);
+ struct px_gpio_chip *px_gc = to_px_gpio_chip(mm_gc);
+ const u32 *gpio = gpio_spec;
+
+ if (*gpio > of_gc->gc.ngpio)
+ return -EINVAL;
+
+ if (gpio[1] & PX_GPIO_FLAG_ACTIVE_LOW)
+ px_gc->active_low |= pin2mask(*gpio);
+
+ return *gpio;
+}
+
+static int __init pixis_gpio_init(void)
+{
+ struct device_node *np;
+
+ for_each_compatible_node(np, NULL, "fsl,fpga-pixis-gpio-bank") {
+ int ret;
+ struct px_gpio_chip *px_gc;
+ struct of_mm_gpio_chip *mm_gc;
+ struct of_gpio_chip *of_gc;
+ struct gpio_chip *gc;
+
+ px_gc = kzalloc(sizeof(*px_gc), GFP_KERNEL);
+ if (!px_gc) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ spin_lock_init(&px_gc->lock);
+
+ mm_gc = &px_gc->mm_gc;
+ of_gc = &mm_gc->of_gc;
+ gc = &of_gc->gc;
+
+ of_gc->gpio_cells = 2;
+ of_gc->xlate = px_gpio_xlate;
+ gc->ngpio = PIXIS_GPIO_PINS;
+ gc->direction_input = px_gpio_dir_in;
+ gc->direction_output = px_gpio_dir_out;
+ gc->get = px_gpio_get;
+ gc->set = px_gpio_set;
+
+ ret = of_mm_gpiochip_add(np, mm_gc);
+ if (ret)
+ goto err;
+ continue;
+err:
+ pr_err("%s: registration failed with status %d\n",
+ np->full_name, ret);
+ kfree(px_gc);
+ /* try others anyway */
+ }
+ return 0;
+}
+arch_initcall(pixis_gpio_init);
On Fri, 18 Jul 2008, Anton Vorontsov wrote:
> +int px_gpio_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> + const void *gpio_spec)
> +{
> + if (gpio[1] & PX_GPIO_FLAG_ACTIVE_LOW)
> + px_gc->active_low |= pin2mask(*gpio);
You have a race here. What if px_gpio_xlate() is called at the same time
for different gpios? This is an easy one to fix, since you can use the
atomic bitops.
It doesn't look like you have any way to unset the active low flag. What if
I unload the leds-gpio driver (or another gpio user) and then try to use the
gpio with something else? The active low flag is stuck on! It doesn't show
in sysfs or debugfs either. That could be very confusing.
I also wonder if it's ok to have the xlate function do flag setting?
of_get_property() just gets the property, it doesn't allocate it. Same with
of_get_address() and of_get_pci_address(), they don't actually allocate or
map an address, they just get the value. of_get_gpio() doesn't allocate the
gpio, that gets done later with gpio_request(). It seems like what it's
supposed to do is just get the translated value of the gpio property.
Except, your pixis gpio xlate function sets the gpio's flags. What if one
wants to just look up a gpio number, but not allocate it? The flags will
still get set.
Most gpio users, including leds-gpio, can handle gpios being busy. If
leds-gpio can't get one of the gpios, it rolls back all the leds it did
create, doesn't drive the device and returns EBUSY. Except with
of_get_gpio() setting the flags, it will change the flags out from under
whatever had the gpio already allocated!
On Sat, Jul 19, 2008 at 02:08:01PM -0700, Trent Piepho wrote:
> On Fri, 18 Jul 2008, Anton Vorontsov wrote:
> > +int px_gpio_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> > + const void *gpio_spec)
> > +{
> > + if (gpio[1] & PX_GPIO_FLAG_ACTIVE_LOW)
> > + px_gc->active_low |= pin2mask(*gpio);
>
> You have a race here. What if px_gpio_xlate() is called at the same time
> for different gpios? This is an easy one to fix, since you can use the
> atomic bitops.
Thanks, I'll fix this. But I think spinlock would be less cryptic here,
since we have one, and the xlate is called quite rarely anyway.
> It doesn't look like you have any way to unset the active low flag. What if
> I unload the leds-gpio driver (or another gpio user) and then try to use the
> gpio with something else? The active low flag is stuck on!
Why would you want to unset the flags? It is specified in the device
tree, and can't be changed. Specifying different flags for the same GPIO
would be an error (plus, Linux forbids shared gpios, so you simply can't
specify one gpio for several devices).
> It doesn't show
> in sysfs or debugfs either. That could be very confusing.
It is in the /proc/device-tree. But I agree, it would be great if
/sys/debug/gpio would show active-low gpios. But this would need gpiolib
changes, with which David will not agree, I suppose. But I didn't try.
> I also wonder if it's ok to have the xlate function do flag setting?
Why not? Of course, we can implement of_gpio_is_active_low(device_node,
gpion), but this is just less convenient than handling active-low gpios
transparently (i.e. for pure OF drivers you don't have to do all
this if (active_low) in the drivers themselves).
> of_get_property() just gets the property, it doesn't allocate it. Same with
> of_get_address() and of_get_pci_address(), they don't actually allocate or
> map an address, they just get the value. of_get_gpio() doesn't allocate the
> gpio, that gets done later with gpio_request(). It seems like what it's
> supposed to do is just get the translated value of the gpio property.
Yes, it translates gpio value, and its flags, it also "caches" the flag
so that further gpio_ calls could use it. But the flags for the
particular gpio is constant. So each xlate must end up with the same
active-low flag, otherwise you did something wrong.
> Except, your pixis gpio xlate function sets the gpio's flags. What if one
> wants to just look up a gpio number, but not allocate it? The flags will
> still get set.
Nothing is allocated.
> Most gpio users, including leds-gpio, can handle gpios being busy. If
> leds-gpio can't get one of the gpios, it rolls back all the leds it did
> create, doesn't drive the device and returns EBUSY. Except with
> of_get_gpio() setting the flags, it will change the flags out from under
> whatever had the gpio already allocated!
You're still assuming that something was allocated. It wasn't. The flag
was set, and it should not change. It is irrelevant if request() failed
or not.
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Mon, 21 Jul 2008, Anton Vorontsov wrote:
> On Sat, Jul 19, 2008 at 02:08:01PM -0700, Trent Piepho wrote:
>> It doesn't look like you have any way to unset the active low flag. What if
>> I unload the leds-gpio driver (or another gpio user) and then try to use the
>> gpio with something else? The active low flag is stuck on!
>
> Why would you want to unset the flags? It is specified in the device
> tree, and can't be changed. Specifying different flags for the same GPIO
> would be an error (plus, Linux forbids shared gpios, so you simply can't
> specify one gpio for several devices).
You can't use the same gpio for two different things at the same time, but you
can load a driver, unload it, and then load another.
>> Most gpio users, including leds-gpio, can handle gpios being busy. If
>> leds-gpio can't get one of the gpios, it rolls back all the leds it did
>> create, doesn't drive the device and returns EBUSY. Except with
>> of_get_gpio() setting the flags, it will change the flags out from under
>> whatever had the gpio already allocated!
>
> You're still assuming that something was allocated. It wasn't. The flag
> was set, and it should not change. It is irrelevant if request() failed
> or not.
Another driver has requested the gpio with active low NOT set. Then the led
driver looks up the property with active low set, which changes the flags.
When it tries to request the gpio it fails since the gpio is already in use.
The led driver handles this failure. Except the active low flag is now set
and the driver that was using the gpio before will now mysteriously stop
working. Perhaps by erasing flash instead of reading it or something nasty
like that.
Is this the proper way to handle a dts mistake that has two drivers trying to
use the same gpio? Without the gpio flags getting set when looking up the
gpio, this situation is handled without any difficulty.
And is having a gpio used twice in the device tree really a mistake? What if
there are two different devices that can be attached to the gpios? For
example, an LED bank that can be unplugged and a serial console attached in
its place, sharing the same connector and gpios. Other than gpio flags
getting set when looking up a gpio, it works fine to list both devices in the
device tree as long as userspace has the correct driver loaded first.
On Mon, Jul 21, 2008 at 02:12:20PM -0700, Trent Piepho wrote:
> On Mon, 21 Jul 2008, Anton Vorontsov wrote:
> > On Sat, Jul 19, 2008 at 02:08:01PM -0700, Trent Piepho wrote:
> >> It doesn't look like you have any way to unset the active low flag. What if
> >> I unload the leds-gpio driver (or another gpio user) and then try to use the
> >> gpio with something else? The active low flag is stuck on!
> >
> > Why would you want to unset the flags? It is specified in the device
> > tree, and can't be changed. Specifying different flags for the same GPIO
> > would be an error (plus, Linux forbids shared gpios, so you simply can't
> > specify one gpio for several devices).
>
> You can't use the same gpio for two different things at the same time, but you
> can load a driver, unload it, and then load another.
Hm.. yeah, this might happen. Now I tend to think that transparent
active-low handling wasn't that good idea after all. So, something like
of_gpio_is_active_low(device_node, gpioidx) should be implemented
instead. This function will parse the gpio's = <> flags. Please speak up
if you have any better ideas though.
Thanks for bringing this up,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Wed, 23 Jul 2008, Anton Vorontsov wrote:
> On Mon, Jul 21, 2008 at 02:12:20PM -0700, Trent Piepho wrote:
>> On Mon, 21 Jul 2008, Anton Vorontsov wrote:
>>> On Sat, Jul 19, 2008 at 02:08:01PM -0700, Trent Piepho wrote:
>>>> It doesn't look like you have any way to unset the active low flag. What if
>>>> I unload the leds-gpio driver (or another gpio user) and then try to use the
>>>> gpio with something else? The active low flag is stuck on!
>>>
>>> Why would you want to unset the flags? It is specified in the device
>>> tree, and can't be changed. Specifying different flags for the same GPIO
>>> would be an error (plus, Linux forbids shared gpios, so you simply can't
>>> specify one gpio for several devices).
>>
>> You can't use the same gpio for two different things at the same time, but you
>> can load a driver, unload it, and then load another.
>
> Hm.. yeah, this might happen. Now I tend to think that transparent
> active-low handling wasn't that good idea after all. So, something like
> of_gpio_is_active_low(device_node, gpioidx) should be implemented
> instead. This function will parse the gpio's = <> flags. Please speak up
> if you have any better ideas though.
The flags could be provided via of_get_gpio. E.g., something like
of_get_gpio(...., u32 *flags), and if flags is non-NULL the gpio flags are
left there. of_get_gpio already has the flags and some other of_get_*
functions return multiple things like this.
Or just have an active low property for the led:
led.active_low = !!of_get_property(np, "active-low", NULL);
Pretty simple, just one line of code. At least if one looks just at
leds-gpio, that's obviously the simplest way. Is active low a property of
the led or a property of the gpio? I guess one could argue either way.
It seems like putting one line of code leds-gpio driver is better than
putting much more complex code into the gpio controller driver. And each
gpio controller has to have that code too.
Now you could say that each gpio user needing to support inverting gpios is
a lot of code too, but I don't think it's necessary. Active low LEDs are
common since gpios can usually sink more current than they can source. But
other gpio users, like the I2C, SPI, MDIO drivers etc., haven't had a need
to support inverting each signal.
I'm also loathe to add software gpio inverting to my mpc8572 gpio driver.
In addition to some LEDs there is also a bit-banged JTAG bus on the CPU
GPIOs. I had to go to great lengths to get it fast enough and each
instruction added to a gpio operation is going to cost me MHz.
This function is alike to the of_get_gpio(), but accepts new argument:
flags. Now the of_get_gpio() call is a wrapper around of_get_gpio_flags().
This new function will be used by the drivers that need to retrive GPIO
information, such as active-low flag.
Signed-off-by: Anton Vorontsov <[email protected]>
---
On Wed, Jul 23, 2008 at 04:42:24PM -0700, Trent Piepho wrote:
> On Wed, 23 Jul 2008, Anton Vorontsov wrote:
> > On Mon, Jul 21, 2008 at 02:12:20PM -0700, Trent Piepho wrote:
> >> On Mon, 21 Jul 2008, Anton Vorontsov wrote:
> >>> On Sat, Jul 19, 2008 at 02:08:01PM -0700, Trent Piepho wrote:
> >>>> It doesn't look like you have any way to unset the active low flag. What if
> >>>> I unload the leds-gpio driver (or another gpio user) and then try to use the
> >>>> gpio with something else? The active low flag is stuck on!
> >>>
> >>> Why would you want to unset the flags? It is specified in the device
> >>> tree, and can't be changed. Specifying different flags for the same GPIO
> >>> would be an error (plus, Linux forbids shared gpios, so you simply can't
> >>> specify one gpio for several devices).
> >>
> >> You can't use the same gpio for two different things at the same time, but you
> >> can load a driver, unload it, and then load another.
> >
> > Hm.. yeah, this might happen. Now I tend to think that transparent
> > active-low handling wasn't that good idea after all. So, something like
> > of_gpio_is_active_low(device_node, gpioidx) should be implemented
> > instead. This function will parse the gpio's = <> flags. Please speak up
> > if you have any better ideas though.
>
> The flags could be provided via of_get_gpio. E.g., something like
> of_get_gpio(...., u32 *flags), and if flags is non-NULL the gpio flags are
> left there. of_get_gpio already has the flags and some other of_get_*
> functions return multiple things like this.
Good idea, thanks. This patch implements of_get_gpio_flags() in addition
to of_get_gpio() (since we don't want to break existing users).
The name seems a bit unfortunate though, because one could read the
function as it gets gpio flags only (though, we might implement
this instead, but this way average driver will end up with parsing
gpios = <> twice).
of_get_gpio_wflags() would be better. Or maybe leave it as is, since
it is documented anyway. ;-)
> Or just have an active low property for the led:
> led.active_low = !!of_get_property(np, "active-low", NULL);
>
> Pretty simple, just one line of code. At least if one looks just at
> leds-gpio, that's obviously the simplest way. Is active low a property of
> the led or a property of the gpio? I guess one could argue either way.
As I said previously, I'm not against this as a temporary solution.
Because we can always deprecate the active-low property later, in a
backward compatible way.
See http://ozlabs.org/pipermail/linuxppc-dev/2008-April/055202.html
So, assuming that the of_get_gpio_flags() will appear only in 2.6.28,
I'm fine with explicit active-low property for now.
> It seems like putting one line of code leds-gpio driver is better than
> putting much more complex code into the gpio controller driver. And each
> gpio controller has to have that code too.
>
> Now you could say that each gpio user needing to support inverting gpios is
> a lot of code too,
[...]
Quite the reverse, actually. ;-) I tend to agree. With this patch
every gpio controller will able to parse flags, w/o single line
added. So this approach is obviously better.
Plus, with transparent active-low handling, most drivers will end up
with checking active-low twice, because most drivers are doing if
(active_low) already.
Thanks.
drivers/of/gpio.c | 35 ++++++++++++++++++++++++++++-------
include/linux/of_gpio.h | 38 ++++++++++++++++++++++++++++++++++----
2 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 1c9cab8..5be67dd 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -19,14 +19,16 @@
#include <asm/prom.h>
/**
- * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
+ * of_get_gpio - Get a GPIO number and flags to use with GPIO API
* @np: device node to get GPIO from
* @index: index of the GPIO
+ * @flags: a flags pointer to fill in
*
* Returns GPIO number to use with Linux generic GPIO API, or one of the errno
- * value on the error condition.
+ * value on the error condition. This function also will fill in the flags.
*/
-int of_get_gpio(struct device_node *np, int index)
+int of_get_gpio_flags(struct device_node *np, int index,
+ enum of_gpio_flags *flags)
{
int ret = -EINVAL;
struct device_node *gc;
@@ -102,7 +104,11 @@ int of_get_gpio(struct device_node *np, int index)
goto err0;
}
- ret = of_gc->xlate(of_gc, np, gpio_spec);
+ /* .xlate might decide to not fill in the flags, so clear it here */
+ if (flags)
+ *flags = 0;
+
+ ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
if (ret < 0)
goto err1;
@@ -113,26 +119,41 @@ err0:
pr_debug("%s exited with status %d\n", __func__, ret);
return ret;
}
-EXPORT_SYMBOL(of_get_gpio);
+EXPORT_SYMBOL(of_get_gpio_flags);
/**
- * of_gpio_simple_xlate - translate gpio_spec to the GPIO number
+ * of_gpio_simple_xlate - translate gpio_spec to the GPIO number and flags
* @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: a flags pointer to fill in
*
* 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;
+ /*
+ * We're discouraging gpio_cells < 2, since this way you'll have to
+ * write your own xlate function, which will retrive the GPIO number
+ * and its flags from a single gpio cell. This is possible, but not
+ * recommended.
+ */
+ if (of_gc->gpio_cells < 2) {
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
if (*gpio > of_gc->gc.ngpio)
return -EINVAL;
+ if (flags)
+ *flags = gpio[1];
+
return *gpio;
}
EXPORT_SYMBOL(of_gpio_simple_xlate);
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 67db101..8dc9bcb 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -14,19 +14,32 @@
#ifndef __LINUX_OF_GPIO_H
#define __LINUX_OF_GPIO_H
+#include <linux/compiler.h>
+#include <linux/kernel.h>
#include <linux/errno.h>
#include <linux/gpio.h>
+struct device_node;
+
#ifdef CONFIG_OF_GPIO
/*
+ * This is Linux-specific flags. By default controllers' and Linux' mapping
+ * match, but GPIO controllers are free to translate their own flags to
+ * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended.
+ */
+enum of_gpio_flags {
+ OF_GPIO_ACTIVE_LOW = 0x1,
+};
+
+/*
* 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,20 +63,37 @@ 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_flags(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_flags(struct device_node *np, int index,
+ enum of_gpio_flags *flags)
{
return -ENOSYS;
}
#endif /* CONFIG_OF_GPIO */
+/**
+ * of_get_gpio - Get a GPIO number to use with GPIO API
+ * @np: device node to get GPIO from
+ * @index: index of the GPIO
+ *
+ * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
+ * value on the error condition.
+ */
+static inline int of_get_gpio(struct device_node *np, int index)
+{
+ return of_get_gpio_flags(np, index, NULL);
+}
+
#endif /* __LINUX_OF_GPIO_H */
--
1.5.5.4
Here are my patches for the OF bindings. The first is just a tiny change to
the leds code to silence a warning. The second is the real patch.
The leds-gpio driver gets two sub-options in Kconfig, one for platform
device support and one for openfirmware platform device support.
There is support for setting an LED trigger. I didn't include support for
active-low or initial brightness, but I think those would be good features
to add. See below for more on that.
Since each device node can describe multiple leds, I used "gpio-leds"
instead of "gpio-led" for the compatible property.
Examples of the dts syntax:
leds {
compatible = "gpio-leds";
hdd {
label = "IDE activity";
gpios = <&mcu_pio 0 0>;
function = "ide-disk";
};
};
run-control {
compatible = "gpio-leds";
red {
gpios = <&mpc8572 6 0>;
};
green {
gpios = <&mpc8572 7 0>;
};
}
On Fri, 18 Jul 2008, Anton Vorontsov wrote:
> Later I tried to use aliases for default-trigger.
>
> http://ozlabs.org/pipermail/linuxppc-dev/2008-June/057244.html
I doesn't seem to me that the alias method will work very well. If I want
an LED that starts on, I need to add code to the kernel to support an
"led-on-while-kernel-boots" alias? And if I want red & green leds to flash
while booting, I need to add aliases "led-flashing-while-kernel-boots-1" and
"led-flashing-while-kernel-boots-2", since you can't assign two leds to the
same alias?
> As for linux,default-brightness... we don't need this since now we
> have default-on trigger.
Except we can't use triggers....
There is an issue with the default-on trigger, besides requiring led
triggers be turned on and the extra code that needs. It causes the led to
have a glitch in it. Suppose the LED is already on from reset or the boot
loader. When the gpio is allocated, it's allocated with an initial value of
off. Then, later, it's turned back on when the trigger runs. But say the
trigger doesn't run?
Here's a real example. I have an LED that comes on the board powers on. It
was supposed to turn off when the kernel has finished booting. But suppose
the kernel panics between the gpio getting lowered when the led is added and
the trigger turning it back on? Now the LED is off and it appears the
problem happened after the kernel finished booting, but really it happened
during the kernel boot. In an embedded system with no console, that's quite
a bit of misinformation. It can be entirely avoided by changing just three
lines of code with the leds-gpio driver to add an option to start the led
on.
It could also be the case that the gpio the led is on also triggers some
other piece of hardware. An alarm that's on the same line as a fault led
for example. The glitch created by the default-on trigger could be
unacceptable in that case.
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 | 44 ++++-
drivers/leds/Kconfig | 21 ++-
drivers/leds/leds-gpio.c | 225 ++++++++++++++++++-----
3 files changed, 236 insertions(+), 54 deletions(-)
diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
index ff51f4c..ed01297 100644
--- a/Documentation/powerpc/dts-bindings/gpio/led.txt
+++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
@@ -1,15 +1,39 @@
-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
- taken from the node name (excluding the unit address).
-- gpios : should specify LED GPIO.
+- compatible : should be "gpio-leds".
-Example:
+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@0 {
- compatible = "gpio-led";
- label = "hdd";
- gpios = <&mcu_pio 0 1>;
+LED node properties:
+- gpios : Should specify the LED GPIO.
+- label : (optional) The label for this LED. If omitted, the label
+ is taken from the node name (excluding the unit address).
+- function : (optional) This parameter, if present, is a string
+ defining the function of the LED. It can be used to put the LED
+ under software control, e.g. Linux LED triggers like "heartbeat",
+ "ide-disk", and "timer". Or it could be used to attach a hardware
+ signal to the LED, e.g. a SoC that can configured to put a SATA
+ activity signal on a GPIO line.
+
+Examples:
+
+leds {
+ compatible = "gpio-leds";
+ hdd {
+ label = "IDE activity";
+ gpios = <&mcu_pio 0 0>;
+ function = "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 86a369b..8344256 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -109,7 +109,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_CM_X270
tristate "LED Support for the CM-X270 LEDs"
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index b13bd29..f10f123 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -71,11 +71,52 @@ 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)
@@ -87,34 +128,10 @@ static int gpio_led_probe(struct platform_device *pdev)
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 +139,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 +155,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 +220,137 @@ 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 *led_pdata;
+ int count = 0, ret;
+
+ /* count LEDs defined by this device, so we now how much to allocate */
+ for_each_child_of_node(np, child)
+ count++;
+ if (!count)
+ return 0; /* or ENODEV? */
+
+ led_pdata = kzalloc(sizeof(*led_pdata) +
+ sizeof(struct gpio_led_data) * count, GFP_KERNEL);
+ if (!led_pdata)
+ return -ENOMEM;
+
+ memset(&led, 0, sizeof(led));
+ for_each_child_of_node(np, child) {
+ led.gpio = of_get_gpio(child, 0);
+ 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,
+ &led_pdata->led_data[led_pdata->num_leds++],
+ &ofdev->dev, NULL);
+ if (ret < 0)
+ goto err;
+ }
+
+ dev_set_drvdata(&ofdev->dev, led_pdata);
+
+ return 0;
+
+err:
+ for (count = led_pdata->num_leds - 1; count >= 0; count--)
+ delete_gpio_led(&led_pdata->led_data[count]);
+
+ kfree(led_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-led", },
+ {},
+};
+
+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
The default_trigger fields of struct gpio_led and thus struct led_classdev
are pretty much always assigned from a string literal, which means the
string can't be modified. Which is fine, since there is no reason to
modify the string and in fact it never is.
But they should be marked const to prevent such code from being added, to
prevent warnings if -Wwrite-strings is used and when assigned from a
constant string other than a string literal (which produces a warning under
current kernel compiler flags), and for general good coding practices.
Signed-off-by: Trent Piepho <[email protected]>
---
include/linux/leds.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 519df72..defe693 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -48,7 +48,7 @@ struct led_classdev {
struct device *dev;
struct list_head node; /* LED Device list */
- char *default_trigger; /* Trigger to use */
+ const char *default_trigger; /* Trigger to use */
#ifdef CONFIG_LEDS_TRIGGERS
/* Protects the trigger data below */
@@ -121,7 +121,7 @@ extern void ledtrig_ide_activity(void);
/* For the leds-gpio driver */
struct gpio_led {
const char *name;
- char *default_trigger;
+ const char *default_trigger;
unsigned gpio;
u8 active_low;
};
--
1.5.4.3
On Fri, 25 Jul 2008, Anton Vorontsov wrote:
>
> The name seems a bit unfortunate though, because one could read the
> function as it gets gpio flags only (though, we might implement
> this instead, but this way average driver will end up with parsing
> gpios = <> twice).
It is kind of a confusing name. I'd be tempted to just change of_get_gpio,
it's a new function and it's only called from four places so it's not that
hard to change.
On Fri, Jul 25, 2008 at 02:01:45PM -0700, Trent Piepho wrote:
> Add bindings to support LEDs defined as of_platform devices in addition to
> the existing bindings for platform devices.
(adding [email protected] to the cc list)
> 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.
I like this approach. I think it is a good pattern.
> 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 | 44 ++++-
> drivers/leds/Kconfig | 21 ++-
> drivers/leds/leds-gpio.c | 225 ++++++++++++++++++-----
> 3 files changed, 236 insertions(+), 54 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/gpio/led.txt b/Documentation/powerpc/dts-bindings/gpio/led.txt
> index ff51f4c..ed01297 100644
> --- a/Documentation/powerpc/dts-bindings/gpio/led.txt
> +++ b/Documentation/powerpc/dts-bindings/gpio/led.txt
> @@ -1,15 +1,39 @@
> -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
> - taken from the node name (excluding the unit address).
> -- gpios : should specify LED GPIO.
> +- compatible : should be "gpio-leds".
>
> -Example:
> +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@0 {
> - compatible = "gpio-led";
> - label = "hdd";
> - gpios = <&mcu_pio 0 1>;
> +LED node properties:
> +- gpios : Should specify the LED GPIO.
Question: it is possible/desirable for a single LED to be assigned
multiple GPIO pins? Say, for a tri-color LED? (I'm fishing for
opinions; I really don't know if it would be a good idea or not)
> +- label : (optional) The label for this LED. If omitted, the label
> + is taken from the node name (excluding the unit address).
> +- function : (optional) This parameter, if present, is a string
> + defining the function of the LED. It can be used to put the LED
> + under software control, e.g. Linux LED triggers like "heartbeat",
> + "ide-disk", and "timer". Or it could be used to attach a hardware
> + signal to the LED, e.g. a SoC that can configured to put a SATA
> + activity signal on a GPIO line.
This makes me nervous. It exposes Linux internal implementation details
into the device tree data. If you want to have a property that
describes the LED usage, then the possible values and meanings should be
documented here.
> + memset(&led, 0, sizeof(led));
> + for_each_child_of_node(np, child) {
> + led.gpio = of_get_gpio(child, 0);
> + led.name = of_get_property(child, "label", NULL) ? : child->name;
> + led.default_trigger =
> + of_get_property(child, "linux,default-trigger", NULL);
This isn't in the documented binding. I assume that you mean 'function'
here?
Otherwise, the code looks pretty good to me.
g.
On Fri, Jul 25, 2008 at 02:01:44PM -0700, Trent Piepho wrote:
> The default_trigger fields of struct gpio_led and thus struct led_classdev
> are pretty much always assigned from a string literal, which means the
> string can't be modified. Which is fine, since there is no reason to
> modify the string and in fact it never is.
>
> But they should be marked const to prevent such code from being added, to
> prevent warnings if -Wwrite-strings is used and when assigned from a
> constant string other than a string literal (which produces a warning under
> current kernel compiler flags), and for general good coding practices.
>
> Signed-off-by: Trent Piepho <[email protected]>
Acked-by: Grant Likely <[email protected]>
> ---
> include/linux/leds.h | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 519df72..defe693 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -48,7 +48,7 @@ struct led_classdev {
>
> struct device *dev;
> struct list_head node; /* LED Device list */
> - char *default_trigger; /* Trigger to use */
> + const char *default_trigger; /* Trigger to use */
>
> #ifdef CONFIG_LEDS_TRIGGERS
> /* Protects the trigger data below */
> @@ -121,7 +121,7 @@ extern void ledtrig_ide_activity(void);
> /* For the leds-gpio driver */
> struct gpio_led {
> const char *name;
> - char *default_trigger;
> + const char *default_trigger;
> unsigned gpio;
> u8 active_low;
> };
> --
> 1.5.4.3
>
Hi Trent,
On Sat, 26 Jul 2008 20:08:57 -0600 Grant Likely <[email protected]> wrote:
>
> On Fri, Jul 25, 2008 at 02:01:44PM -0700, Trent Piepho wrote:
> > The default_trigger fields of struct gpio_led and thus struct led_classdev
> > are pretty much always assigned from a string literal, which means the
> > string can't be modified. Which is fine, since there is no reason to
> > modify the string and in fact it never is.
> >
> > But they should be marked const to prevent such code from being added, to
> > prevent warnings if -Wwrite-strings is used and when assigned from a
> > constant string other than a string literal (which produces a warning under
> > current kernel compiler flags), and for general good coding practices.
> >
> > Signed-off-by: Trent Piepho <[email protected]>
> Acked-by: Grant Likely <[email protected]>
I would ack this as well, except I am not sure what tree this patch is
against. In the current powerpc next tree,
> > +++ b/include/linux/leds.h
> > @@ -48,7 +48,7 @@ struct led_classdev {
> >
> > struct device *dev;
> > struct list_head node; /* LED Device list */
> > - char *default_trigger; /* Trigger to use */
> > + const char *default_trigger; /* Trigger to use */
this is already const, but there is another structure slightly further
down (struct led_info) that has a non-const default_tigger.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
On Sun, 27 Jul 2008, Stephen Rothwell wrote:
> On Sat, 26 Jul 2008 20:08:57 -0600 Grant Likely <[email protected]> wrote:
>> On Fri, Jul 25, 2008 at 02:01:44PM -0700, Trent Piepho wrote:
>>> The default_trigger fields of struct gpio_led and thus struct led_classdev
>>> are pretty much always assigned from a string literal, which means the
>>> string can't be modified. Which is fine, since there is no reason to
>>> modify the string and in fact it never is.
>>>
>>> But they should be marked const to prevent such code from being added, to
>>> prevent warnings if -Wwrite-strings is used and when assigned from a
>>> constant string other than a string literal (which produces a warning under
>>> current kernel compiler flags), and for general good coding practices.
>>>
>>> Signed-off-by: Trent Piepho <[email protected]>
>> Acked-by: Grant Likely <[email protected]>
>
> I would ack this as well, except I am not sure what tree this patch is
> against. In the current powerpc next tree,
It was against powerpc next from Jul 22nd, current at the time I made the
patch. It looks like that file has changed in the last few days. There is a
patch from Anton Vorontsov, "leds: mark led_classdev.default_trigger as
const", which adds const to one of the structs I modified, but doesn't get the
other one (struct gpio_led).
Then another patch from Nate Case added a new LED chip driver, and the
platform data for this driver was added as "generic led platform data", which
I don't entirely agree with. And this new struct didn't make default_trigger
const, probably because it was just copied from the gpio led platform data
with some fields removed (so it's not really that generic then, is it?).
I'll send an updated patch for current powerpc next.
The default_trigger fields of struct gpio_led and thus struct led_classdev
are pretty much always assigned from a string literal, which means the
string can't be modified. Which is fine, since there is no reason to
modify the string and in fact it never is.
But they should be marked const to prevent such code from being added, to
prevent warnings if -Wwrite-strings is used and when assigned from a
constant string other than a string literal (which produces a warning under
current kernel compiler flags), and for general good coding practices.
Signed-off-by: Trent Piepho <[email protected]>
---
include/linux/leds.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 519df72..defe693 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -48,7 +48,7 @@ struct led_classdev {
struct device *dev;
struct list_head node; /* LED Device list */
- char *default_trigger; /* Trigger to use */
+ const char *default_trigger; /* Trigger to use */
#ifdef CONFIG_LEDS_TRIGGERS
/* Protects the trigger data below */
@@ -121,7 +121,7 @@ extern void ledtrig_ide_activity(void);
/* For the leds-gpio driver */
struct gpio_led {
const char *name;
- char *default_trigger;
+ const char *default_trigger;
unsigned gpio;
u8 active_low;
};
--
1.5.4.3
On Sat, 26 Jul 2008, Grant Likely wrote:
> On Fri, Jul 25, 2008 at 02:01:45PM -0700, Trent Piepho wrote:
>> Add bindings to support LEDs defined as of_platform devices in addition to
>> the existing bindings for platform devices.
>
>> +- gpios : Should specify the LED GPIO.
>
> Question: it is possible/desirable for a single LED to be assigned
> multiple GPIO pins? Say, for a tri-color LED? (I'm fishing for
> opinions; I really don't know if it would be a good idea or not)
Good question. The Linux LED layer has no concept of multi-color LEDs, so
it's more difficult that just adding support to the gpio led driver. I have
a device with a tri-color red/green/orange LED and this posed some
difficulty. It's defined as independent red and greed LEDs, which is mostly
fine, except I wanted it to flash orange. I can make both the red LED and
green LED flash, but there is nothing to insure their flashing remains in
sync.
Other OF bindings allow multiple GPIOs to be listed in a gpios property, so
that's not a problem if someone wants to do that. There would need to be a
way to define what the gpios mean. I don't think it's worthwhile to come up
with a binding for that until there is a real user.
>> +- function : (optional) This parameter, if present, is a string
>> + defining the function of the LED. It can be used to put the LED
>> + under software control, e.g. Linux LED triggers like "heartbeat",
>> + "ide-disk", and "timer". Or it could be used to attach a hardware
>> + signal to the LED, e.g. a SoC that can configured to put a SATA
>> + activity signal on a GPIO line.
>
> This makes me nervous. It exposes Linux internal implementation details
> into the device tree data. If you want to have a property that
> describes the LED usage, then the possible values and meanings should be
> documented here.
Should it be a linux specific property then? I could list all the current
linux triggers, but enumerating every possible function someone might want
to assign to an LED seems hopeless.
>> + led.default_trigger =
>> + of_get_property(child, "linux,default-trigger", NULL);
>
> This isn't in the documented binding. I assume that you mean 'function'
> here?
Looks like I emailed the wrong patch file. That should be changed to
"function" and there are a few cosmetic changes that are missing.
On Sun, Jul 27, 2008 at 06:56:49PM -0700, Trent Piepho wrote:
> On Sun, 27 Jul 2008, Stephen Rothwell wrote:
> > On Sat, 26 Jul 2008 20:08:57 -0600 Grant Likely <[email protected]> wrote:
> >> On Fri, Jul 25, 2008 at 02:01:44PM -0700, Trent Piepho wrote:
> >>> The default_trigger fields of struct gpio_led and thus struct led_classdev
> >>> are pretty much always assigned from a string literal, which means the
> >>> string can't be modified. Which is fine, since there is no reason to
> >>> modify the string and in fact it never is.
> >>>
> >>> But they should be marked const to prevent such code from being added, to
> >>> prevent warnings if -Wwrite-strings is used and when assigned from a
> >>> constant string other than a string literal (which produces a warning under
> >>> current kernel compiler flags), and for general good coding practices.
> >>>
> >>> Signed-off-by: Trent Piepho <[email protected]>
> >> Acked-by: Grant Likely <[email protected]>
> >
> > I would ack this as well, except I am not sure what tree this patch is
> > against. In the current powerpc next tree,
>
> It was against powerpc next from Jul 22nd, current at the time I made the
> patch. It looks like that file has changed in the last few days. There is a
> patch from Anton Vorontsov, "leds: mark led_classdev.default_trigger as
> const", which adds const to one of the structs I modified, but doesn't get the
> other one (struct gpio_led).
Yes, I posted the patch for my version of OF GPIO LEDs, which didn't use
struct gpio_led's default_trigger.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Mon, Jul 28, 2008 at 01:31:47AM -0700, Trent Piepho wrote:
> On Sat, 26 Jul 2008, Grant Likely wrote:
> > On Fri, Jul 25, 2008 at 02:01:45PM -0700, Trent Piepho wrote:
> >> Add bindings to support LEDs defined as of_platform devices in addition to
> >> the existing bindings for platform devices.
> >
> >> +- gpios : Should specify the LED GPIO.
> >
> > Question: it is possible/desirable for a single LED to be assigned
> > multiple GPIO pins? Say, for a tri-color LED? (I'm fishing for
> > opinions; I really don't know if it would be a good idea or not)
>
> Good question. The Linux LED layer has no concept of multi-color LEDs, so
> it's more difficult that just adding support to the gpio led driver. I have
> a device with a tri-color red/green/orange LED and this posed some
> difficulty. It's defined as independent red and greed LEDs, which is mostly
> fine, except I wanted it to flash orange. I can make both the red LED and
> green LED flash, but there is nothing to insure their flashing remains in
> sync.
>
> Other OF bindings allow multiple GPIOs to be listed in a gpios property, so
> that's not a problem if someone wants to do that. There would need to be a
> way to define what the gpios mean. I don't think it's worthwhile to come up
> with a binding for that until there is a real user.
True. The binding can be extended at a later date anyway. It might be
as simple as adding an array of strings that define what each gpio value
means. ie. if 2 gpio lines are used for a led, then that maps to
numbers in the range [0, 1, 2, 3]. It could be encoded thus:
led-states = "off", "green", "red", "orange";
The driver would then need to interpret/adapt these strings to something
useful in the LED driver. Just a thought.
> >> +- function : (optional) This parameter, if present, is a string
> >> + defining the function of the LED. It can be used to put the LED
> >> + under software control, e.g. Linux LED triggers like "heartbeat",
> >> + "ide-disk", and "timer". Or it could be used to attach a hardware
> >> + signal to the LED, e.g. a SoC that can configured to put a SATA
> >> + activity signal on a GPIO line.
> >
> > This makes me nervous. It exposes Linux internal implementation details
> > into the device tree data. If you want to have a property that
> > describes the LED usage, then the possible values and meanings should be
> > documented here.
>
> Should it be a linux specific property then? I could list all the current
> linux triggers, but enumerating every possible function someone might want
> to assign to an LED seems hopeless.
I don't like adding Linux specific properties to the device tree if at
all possible, and I really don't like encoding Linux internal details
(like trigger names). They can change between kernel versions and
breaking compatibility with older device trees is strongly avoided.
That's why so much effort goes into getting bindings correct the first
time.
I'd rather see the device tree provide 'hints' toward the expected usage
and if a platform needs something specific, then the platform specific
code should setup the trigger.
Regardless, any hints provided by the binding must be documented. In
most cases the gpio-leds driver should be able to figure out which trigger
to bind without platform code intervention.
g.
On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]
> > >> +- function : (optional) This parameter, if present, is a string
> > >> + defining the function of the LED. It can be used to put the LED
> > >> + under software control, e.g. Linux LED triggers like "heartbeat",
> > >> + "ide-disk", and "timer". Or it could be used to attach a hardware
> > >> + signal to the LED, e.g. a SoC that can configured to put a SATA
> > >> + activity signal on a GPIO line.
> > >
> > > This makes me nervous. It exposes Linux internal implementation details
> > > into the device tree data. If you want to have a property that
> > > describes the LED usage, then the possible values and meanings should be
> > > documented here.
> >
> > Should it be a linux specific property then? I could list all the current
> > linux triggers, but enumerating every possible function someone might want
> > to assign to an LED seems hopeless.
>
> I don't like adding Linux specific properties to the device tree if at
> all possible, and I really don't like encoding Linux internal details
> (like trigger names). They can change between kernel versions and
> breaking compatibility with older device trees is strongly avoided.
> That's why so much effort goes into getting bindings correct the first
> time.
>
> I'd rather see the device tree provide 'hints' toward the expected usage
> and if a platform needs something specific, then the platform specific
> code should setup the trigger.
>
> Regardless, any hints provided by the binding must be documented. In
> most cases the gpio-leds driver should be able to figure out which trigger
> to bind without platform code intervention.
Maybe we can encode leds into devices themselves, via phandles?
E.g.
sata@101 {
compatible = "fsl,sata";
leds = <&red_led>;
};
And then the OF GPIO LEDs driver could do something like:
char *ide_disk_trigger_compatibles[] = {
"fsl,sata",
"ide-generic",
...
};
for_each_node_with_leds_property(node, led_phandle) {
if (if_ide_disk_compatible(node)) {
struct gpio_led *led = phandle_to_led(led_phandle);
led->default_trigger = "ide-disk";
}
}
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Mon, Jul 28, 2008 at 10:02:04PM +0400, Anton Vorontsov wrote:
> On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
> > I'd rather see the device tree provide 'hints' toward the expected usage
> > and if a platform needs something specific, then the platform specific
> > code should setup the trigger.
> >
> > Regardless, any hints provided by the binding must be documented. In
> > most cases the gpio-leds driver should be able to figure out which trigger
> > to bind without platform code intervention.
>
> Maybe we can encode leds into devices themselves, via phandles?
>
> E.g.
>
> sata@101 {
> compatible = "fsl,sata";
> leds = <&red_led>;
> };
I like that idea! That neatly solves the problem for many use cases.
> And then the OF GPIO LEDs driver could do something like:
>
> char *ide_disk_trigger_compatibles[] = {
> "fsl,sata",
> "ide-generic",
> ...
> };
>
> for_each_node_with_leds_property(node, led_phandle) {
> if (if_ide_disk_compatible(node)) {
> struct gpio_led *led = phandle_to_led(led_phandle);
>
> led->default_trigger = "ide-disk";
> }
> }
I'm not sure what would be best for implementation details, but
implementation details can easily be changed.
>
> --
> Anton Vorontsov
> email: [email protected]
> irc://irc.freenode.net/bd2
On Mon, 28 Jul 2008, Anton Vorontsov wrote:
> On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
> [...]
>>>>> +- function : (optional) This parameter, if present, is a string
>>>>> + defining the function of the LED. It can be used to put the LED
>>>>> + under software control, e.g. Linux LED triggers like "heartbeat",
>>>>> + "ide-disk", and "timer". Or it could be used to attach a hardware
>>>>> + signal to the LED, e.g. a SoC that can configured to put a SATA
>>>>> + activity signal on a GPIO line.
>>>>
>>>> This makes me nervous. It exposes Linux internal implementation details
>>>> into the device tree data. If you want to have a property that
>>>> describes the LED usage, then the possible values and meanings should be
>>>> documented here.
>>>
>>> Should it be a linux specific property then? I could list all the current
>>> linux triggers, but enumerating every possible function someone might want
>>> to assign to an LED seems hopeless.
>>
>> I'd rather see the device tree provide 'hints' toward the expected usage
>> and if a platform needs something specific, then the platform specific
>> code should setup the trigger.
>>
> Maybe we can encode leds into devices themselves, via phandles?
How will this work for anything besides ide activity? For example, flashing,
heartbeat, default on, overheat, fan failed, kernel panic, etc.
> And then the OF GPIO LEDs driver could do something like:
>
> char *ide_disk_trigger_compatibles[] = {
> "fsl,sata",
> "ide-generic",
> ...
> };
Everytime someone added a new ide driver, this table would have to be updated.
On Mon, Jul 28, 2008 at 11:26:28AM -0700, Trent Piepho wrote:
> On Mon, 28 Jul 2008, Anton Vorontsov wrote:
> > On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
> > [...]
> >>>>> +- function : (optional) This parameter, if present, is a string
> >>>>> + defining the function of the LED. It can be used to put the LED
> >>>>> + under software control, e.g. Linux LED triggers like "heartbeat",
> >>>>> + "ide-disk", and "timer". Or it could be used to attach a hardware
> >>>>> + signal to the LED, e.g. a SoC that can configured to put a SATA
> >>>>> + activity signal on a GPIO line.
> >>>>
> >>>> This makes me nervous. It exposes Linux internal implementation details
> >>>> into the device tree data. If you want to have a property that
> >>>> describes the LED usage, then the possible values and meanings should be
> >>>> documented here.
> >>>
> >>> Should it be a linux specific property then? I could list all the current
> >>> linux triggers, but enumerating every possible function someone might want
> >>> to assign to an LED seems hopeless.
> >>
> >> I'd rather see the device tree provide 'hints' toward the expected usage
> >> and if a platform needs something specific, then the platform specific
> >> code should setup the trigger.
> >>
> > Maybe we can encode leds into devices themselves, via phandles?
>
> How will this work for anything besides ide activity? For example, flashing,
> heartbeat, default on, overheat, fan failed, kernel panic, etc.
It certainly doesn't work for everything; but where it does work it
makes a lot of sense because the property position provides a lot of
context.
>
> > And then the OF GPIO LEDs driver could do something like:
> >
> > char *ide_disk_trigger_compatibles[] = {
> > "fsl,sata",
> > "ide-generic",
> > ...
> > };
>
> Everytime someone added a new ide driver, this table would have to be updated.
Yes, the implementation needs thought.
g.
On Mon, Jul 28, 2008 at 11:26:28AM -0700, Trent Piepho wrote:
> On Mon, 28 Jul 2008, Anton Vorontsov wrote:
> > On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
> > [...]
> >>>>> +- function : (optional) This parameter, if present, is a string
> >>>>> + defining the function of the LED. It can be used to put the LED
> >>>>> + under software control, e.g. Linux LED triggers like "heartbeat",
> >>>>> + "ide-disk", and "timer". Or it could be used to attach a hardware
> >>>>> + signal to the LED, e.g. a SoC that can configured to put a SATA
> >>>>> + activity signal on a GPIO line.
> >>>>
> >>>> This makes me nervous. It exposes Linux internal implementation details
> >>>> into the device tree data. If you want to have a property that
> >>>> describes the LED usage, then the possible values and meanings should be
> >>>> documented here.
> >>>
> >>> Should it be a linux specific property then? I could list all the current
> >>> linux triggers, but enumerating every possible function someone might want
> >>> to assign to an LED seems hopeless.
> >>
> >> I'd rather see the device tree provide 'hints' toward the expected usage
> >> and if a platform needs something specific, then the platform specific
> >> code should setup the trigger.
> >>
> > Maybe we can encode leds into devices themselves, via phandles?
>
> How will this work for anything besides ide activity? For example, flashing,
> heartbeat, default on, overheat, fan failed, kernel panic, etc.
Everything is possible, but will look weird. For example,
Default on (power led) could be encoded in the root node.
fan and overheat in a PM controller's node.
Kernel panic in the chosen node.
> > And then the OF GPIO LEDs driver could do something like:
> >
> > char *ide_disk_trigger_compatibles[] = {
> > "fsl,sata",
> > "ide-generic",
> > ...
> > };
>
> Everytime someone added a new ide driver, this table would have to be updated.
Yes. device_type would be helpful here. :-)
Well, otherwise, we could provide a trigger map in the chosen node:
chosen {
/* leds map: default-on, ide-disk, nand-disk, panic */
linux,leds = <&green_led &red_led 0 0>;
};
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Mon, 28 Jul 2008, Anton Vorontsov wrote:
> On Mon, Jul 28, 2008 at 11:26:28AM -0700, Trent Piepho wrote:
>> On Mon, 28 Jul 2008, Anton Vorontsov wrote:
>>> On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
>>> [...]
>>>>>>> +- function : (optional) This parameter, if present, is a string
>>>>>>> + defining the function of the LED. It can be used to put the LED
>>>>>>> + under software control, e.g. Linux LED triggers like "heartbeat",
>>>>>>> + "ide-disk", and "timer". Or it could be used to attach a hardware
>>>>>>> + signal to the LED, e.g. a SoC that can configured to put a SATA
>>>>>>> + activity signal on a GPIO line.
>>>>>>
>>>>>> This makes me nervous. It exposes Linux internal implementation details
>>>>>> into the device tree data. If you want to have a property that
>>>>>> describes the LED usage, then the possible values and meanings should be
>>>>>> documented here.
>>>>>
>>>>> Should it be a linux specific property then? I could list all the current
>>>>> linux triggers, but enumerating every possible function someone might want
>>>>> to assign to an LED seems hopeless.
>>>>
>>>> I'd rather see the device tree provide 'hints' toward the expected usage
>>>> and if a platform needs something specific, then the platform specific
>>>> code should setup the trigger.
>>>>
>>> Maybe we can encode leds into devices themselves, via phandles?
>>
>> How will this work for anything besides ide activity? For example, flashing,
>> heartbeat, default on, overheat, fan failed, kernel panic, etc.
>
> Everything is possible, but will look weird. For example,
>
> Default on (power led) could be encoded in the root node.
> fan and overheat in a PM controller's node.
> Kernel panic in the chosen node.
What about flashing? What if the sensor chip isn't an OF device?
>>> And then the OF GPIO LEDs driver could do something like:
>>>
>>> char *ide_disk_trigger_compatibles[] = {
>>> "fsl,sata",
>>> "ide-generic",
>>> ...
>>> };
>>
>> Everytime someone added a new ide driver, this table would have to be updated.
>
> Yes. device_type would be helpful here. :-)
>
>
> Well, otherwise, we could provide a trigger map in the chosen node:
>
> chosen {
> /* leds map: default-on, ide-disk, nand-disk, panic */
> linux,leds = <&green_led &red_led 0 0>;
> };
What if you have multiple leds that you want to be default on? What happens
when new functions are added?
The default_trigger fields of struct gpio_led and thus struct led_classdev
are pretty much always assigned from a string literal, which means the
string can't be modified. Which is fine, since there is no reason to
modify the string and in fact it never is.
But they should be marked const to prevent such code from being added, to
prevent warnings if -Wwrite-strings is used and when assigned from a
constant string other than a string literal (which produces a warning under
current kernel compiler flags), and for general good coding practices.
Signed-off-by: Trent Piepho <[email protected]>
---
Reposting this again. It still applies to powerpc-next as of Aug 28 and I
don't think anyone had any problems with it.
include/linux/leds.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d41ccb5..d3a73f5 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -123,7 +123,7 @@ extern void ledtrig_ide_activity(void);
*/
struct led_info {
const char *name;
- char *default_trigger;
+ const char *default_trigger;
int flags;
};
@@ -135,7 +135,7 @@ struct led_platform_data {
/* For the leds-gpio driver */
struct gpio_led {
const char *name;
- char *default_trigger;
+ const char *default_trigger;
unsigned gpio;
u8 active_low;
};
--
1.5.4.3