leds: implement OpenFirmare GPIO LED driver

+err_get_gpio:

drivers/leds/leds-of-gpio.c line 75

Quoted 1 line

drivers/leds/leds-of-gpio.c new file +1 -0

+	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);

drivers/leds/leds-of-gpio.c line 87

Quoted 3 lines

drivers/leds/leds-of-gpio.c new file +3 -0

+	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);

drivers/leds/leds-of-gpio.c line 90

Quoted 3 lines

drivers/leds/leds-of-gpio.c new file +3 -0

+	led->cdev.brightness_set = gpio_led_set;
+
+	ret = gpio_request(led->gpio, ofdev->dev.bus_id);

dev_name() again.

Quoted 1 line

+err_get_gpio:

	of_node_put(led->np);

drivers/leds/leds-of-gpio.c line 133

Quoted 12 lines

drivers/leds/leds-of-gpio.c new file +12 -0

+	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.

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).

Quoted 3 lines · Anton Vorontsov

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.

Quoted 4 lines · Richard Purdie

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?

Quoted 1 line

"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.

Quoted 4 lines

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? ;-)

Quoted 3 lines

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?

Quoted 5 lines · Anton Vorontsov

Quoted 4 lines · Richard Purdie

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?

[...]

Quoted 5 lines

Quoted 1 line

"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?

Quoted 7 lines

Quoted 4 lines

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...

Quoted 2 lines

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.

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).

Quoted 3 lines · Anton Vorontsov

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(...)
{

(quoted diff) +0 -0

 	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));

}

Quoted 5 lines · Trent Piepho

Quoted 3 lines · Anton Vorontsov

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).

Quoted 3 lines

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.

Quoted 10 lines · Grant Likely

Quoted 8 lines · Trent Piepho

Quoted 3 lines · Anton Vorontsov

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:

Quoted 4 lines

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.

drivers/leds/leds-gpio.c +35 -30

+
+{
+	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);

drivers/leds/leds-gpio.c +99 -0

+/* 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);

Quoted 2 lines · Trent Piepho

Ok, how about adding code the existing leds-gpio driver so that it can creates LEDs from of_platform devices too?

Few comments below.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 1 line

+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.

Quoted 1 line

+/* #ifdef CONFIG_LEDS_GPIO_OF */

Heh.

Quoted 20 lines · Trent Piepho

Quoted 3 lines · Anton Vorontsov

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(...)
{

(quoted diff) +0 -0

 	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));

Quoted 1 line

}

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?

Quoted 5 lines

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.

On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
[...]

Quoted 4 lines

Quoted 4 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 8 lines · Anton Vorontsov

On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
[...]

Quoted 6 lines

Quoted 5 lines

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?

Quoted 5 lines

Quoted 1 line

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?

Quoted 11 lines · Grant Likely

Quoted 10 lines · Anton Vorontsov

On Thu, Jul 17, 2008 at 06:05:19PM +0400, Anton Vorontsov wrote:
[...]

Quoted 7 lines

Quoted 5 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 5 lines · Grant Likely

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 {

(quoted diff) +0 -0

 	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 {

(quoted diff) +0 -0

 	compatible = "gpio-led";
 	led@6 {
 		gpios = <&mpc8572 6 0>;
 		label = "red";
 	};
 	led@7 {
 		gpios = <&mpc8572 7 0>;
 		label = "green";
 	};

};

Quoted 5 lines · Grant Likely

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 {

(quoted diff) +0 -0

 	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 {

(quoted diff) +0 -0

 	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.

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. ;-)

Quoted 2 lines

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.

Quoted 3 lines · Trent Piepho

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).

Quoted 11 lines

leds {

(quoted diff) +0 -0

 	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.

Quoted 9 lines · Anton Vorontsov

Quoted 3 lines · Trent Piepho

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.

Quoted 4 lines

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?

arch/powerpc/platforms/86xx/pixis_gpio.c line 93

Quoted 5 lines · Anton Vorontsov

arch/powerpc/platforms/86xx/pixis_gpio.c new file +5 -0

+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!

Quoted 3 lines · Trent Piepho

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).

Quoted 5 lines

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.

Quoted 5 lines · Anton Vorontsov

Quoted 3 lines · Trent Piepho

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!

Quoted 4 lines

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.

Quoted 11 lines · Trent Piepho

Quoted 9 lines · Anton Vorontsov

Quoted 3 lines · Trent Piepho

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).

Quoted 2 lines

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.

Quoted 12 lines · Anton Vorontsov

Quoted 11 lines · Trent Piepho

Quoted 9 lines · Anton Vorontsov

Quoted 3 lines · Trent Piepho

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).

Quoted 8 lines

Quoted 2 lines

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,

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).

Later I tried to use aliases for default-trigger.

http://ozlabs.org/pipermail/linuxppc-dev/2008-June/057244.html

As for linux,default-brightness... we don't need this since now we have default-on trigger.

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(-)

include/linux/leds.h +2 -2

index 519df72..defe693 100644
@@ -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;
 };

Signature · 1 line

1.5.4.3

Quoted 5 lines · Trent Piepho

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.

Quoted 6 lines

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 struct led_classdev { · line 51

Quoted 7 lines

+++ b/include/linux/leds.h

include/linux/leds.h +1 -1

@@ -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 */

Quoted 11 lines · On Sat, 26 Jul 2008 20:08:57 -0600 Grant Likely <[email protected]>…

Quoted 10 lines · Trent Piepho

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.

Quoted 2 lines

Quoted 1 line

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,

Quoted 15 lines · Stephen Rothwell

Quoted 13 lines · On Sat, 26 Jul 2008 20:08:57 -0600 Grant Likely <[email protected]>…

Quoted 11 lines · Trent Piepho

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]>

Quoted 2 lines

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).

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(-)

Documentation/powerpc/dts-bindings/gpio/led.txt +2 -5

index ff51f4c..ed01297 100644
@@ -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".
 

Quoted 4 lines · Trent Piepho

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)

Documentation/powerpc/dts-bindings/gpio/led.txt line 18

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

Documentation/powerpc/dts-bindings/gpio/led.txt +5 -0

+  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.

drivers/leds/leds-gpio.c static void __exit gpio_led_exit(void) · line 260

Quoted 2 lines

drivers/leds/leds-gpio.c +2 -0

+		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?

Quoted 9 lines · Grant Likely

Quoted 4 lines · Trent Piepho

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.

Quoted 7 lines

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

Documentation/powerpc/dts-bindings/gpio/led.txt +5 -0

+  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.

Quoted 4 lines

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.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

Documentation/powerpc/dts-bindings/gpio/led.txt +5 -0

+  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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines · Grant Likely

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";
	}
}

Signature · 4 lines

-- 
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2

On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

Documentation/powerpc/dts-bindings/gpio/led.txt line 18

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

Documentation/powerpc/dts-bindings/gpio/led.txt +5 -0

+  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.

Quoted 4 lines

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?

And then the OF GPIO LEDs driver could do something like:

char *ide_disk_trigger_compatibles[] = {
	"fsl,sata",
	"ide-generic",
	...
};

Quoted 18 lines · Anton Vorontsov

On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

Documentation/powerpc/dts-bindings/gpio/led.txt line 18

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

Documentation/powerpc/dts-bindings/gpio/led.txt +5 -0

+  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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 8 lines

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.

Quoted 18 lines · Anton Vorontsov

On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

Documentation/powerpc/dts-bindings/gpio/led.txt line 18

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

Documentation/powerpc/dts-bindings/gpio/led.txt +5 -0

+  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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 25 lines · Trent Piepho

Quoted 23 lines · Anton Vorontsov

On Mon, Jul 28, 2008 at 11:09:14AM -0600, Grant Likely wrote:
[...]

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Review

Quoted 6 lines

+- function : (optional) This parameter, if present, is a string

(quoted diff) +5 -0

+  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.

Quoted 8 lines

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 9 lines

Quoted 7 lines

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>;
};

Quoted 18 lines · Anton Vorontsov

Quoted 2 lines

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
...

Quoted 2 lines

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 :).

Documentation/powerpc/dts-bindings/gpio/led.txt line 4

Quoted 11 lines

Documentation/powerpc/dts-bindings/gpio/led.txt new file +4 -0

new file mode 100644
index 0000000..7e9ce81
@@ -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 :-(

Documentation/powerpc/dts-bindings/gpio/led.txt line 6

Quoted 2 lines

Documentation/powerpc/dts-bindings/gpio/led.txt new file +2 -0

+- 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).

Quoted 4 lines

Documentation/powerpc/dts-bindings/gpio/led.txt new file +4 -0

+- gpios : should specify LED GPIO.
+
+Example:
+

Documentation/powerpc/dts-bindings/gpio/led.txt line 15

Quoted 5 lines

Documentation/powerpc/dts-bindings/gpio/led.txt new file +5 -0

+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?

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"?

Quoted 1 line · Segher Boessenkool

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"?

Quoted 8 lines · Anton Vorontsov

Quoted 1 line · Segher Boessenkool

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.

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).