[PATCH 2/2] LEDS: generic driver

[PATCH 2/2] LEDS: generic driver

[Robin Farine]

From: Robin Farine <robin.farine@terminus.org>

This generic LED driver implements the platform independent part of a
LED driver letting platform specific code focus on the hardware
details. The driver binds to platform devices named "Generic-LED"
which provide the platform specific data and code needed to act on an
LED.

This is useful for platforms with exotic ways of controlling LEDs
which preclude the use of a common LED driver such as GPIO based
drivers.

Signed-off-by: Robin Farine <robin.farine@terminus.org>
---
 drivers/leds/Kconfig        |    7 ++
 drivers/leds/Makefile       |    1 +
 drivers/leds/leds-generic.c |  129 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h        |   17 ++++++
 4 files changed, 154 insertions(+), 0 deletions(-)
 create mode 100644 drivers/leds/leds-generic.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 87d2046..cbf19f8 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -95,6 +95,13 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server
 
+config LEDS_GENERIC
+	tristate "Generic LED driver"
+	depends on LEDS_CLASS
+	help
+	  This option enables the generic LED driver that binds to
+	  platform devices named "Generic-LED".
+
 comment "LED Triggers"
 
 config LEDS_TRIGGERS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index aa2c18e..238abb4 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net48xx.o
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_GENERIC)		+= leds-generic.o
 
 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
diff --git a/drivers/leds/leds-generic.c b/drivers/leds/leds-generic.c
new file mode 100644
index 0000000..f981ac6
--- /dev/null
+++ b/drivers/leds/leds-generic.c
@@ -0,0 +1,129 @@
+/*
+ * Generic LED driver
+ *
+ * This driver binds to platform devices named "Generic-LED". Platform specific
+ * code declares LEDs by registering one such platform device per LED with its
+ * dev.platform_data field pointing to an instance of struct led_generic_data or
+ * to an extension of it. The led_generic_data's name and brightness_set fields
+ * must be initialized to point to a valid string and function respectively.
+ *
+ * Copyright (C) 2007 Robin Farine <robin.farine@terminus.org>
+ *
+ * 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/init.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+
+struct led_generic_device {
+	struct led_classdev super;
+	struct led_generic_data *pdata;
+};
+
+static inline struct led_generic_device *pdev_to_led(
+	struct platform_device *pdev)
+{
+	return platform_get_drvdata(pdev);
+}
+
+static inline struct led_generic_device *cdev_to_led(struct led_classdev *cdev)
+{
+	return container_of(cdev, struct led_generic_device, super);
+}
+
+#ifdef CONFIG_PM
+static int leds_generic_suspend(struct platform_device *pdev,
+	pm_message_t state)
+{
+	struct led_generic_device *led = pdev_to_led(pdev);
+
+	(void)state;
+	led_classdev_suspend(&led->super);
+	return 0;
+}
+
+static int leds_generic_resume(struct platform_device *pdev)
+{
+	struct led_generic_device *led = pdev_to_led(pdev);
+
+	led_classdev_resume(&led->super);
+	return 0;
+}
+#else
+# define leds_generic_suspend NULL
+# define leds_generic_resume NULL
+#endif /* CONFIG_PM */
+
+static void leds_generic_set_brightness(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{
+	struct led_generic_data *pdata = cdev_to_led(cdev)->pdata;
+
+	pdata->brightness_set(pdata, brightness);
+}
+
+static int leds_generic_remove(struct platform_device *pdev)
+{
+	struct led_generic_device *led = pdev_to_led(pdev);
+
+	led_classdev_unregister(&led->super);
+	platform_set_drvdata(pdev, NULL);
+	kfree(led);
+	return 0;
+}
+
+static int leds_generic_probe(struct platform_device *pdev)
+{
+	struct led_generic_device *led;
+	struct led_generic_data *pdata = pdev->dev.platform_data;
+	int res = 0;
+
+	led = kzalloc(sizeof(struct led_generic_device), GFP_KERNEL);
+	if (led == NULL) {
+		dev_err(&pdev->dev, "no memory for LED device\n");
+		res = -ENOMEM;
+		goto out;
+	}
+	led->super.name = pdata->name;
+	led->super.brightness_set = leds_generic_set_brightness;
+	led->super.default_trigger = pdata->trigger;
+	led->pdata = pdata;
+	platform_set_drvdata(pdev, led);
+	res = led_classdev_register(&pdev->dev, &led->super);
+	if (res < 0) {
+		dev_err(&pdev->dev, "could not register LED device\n");
+		platform_set_drvdata(pdev, NULL);
+		kfree(led);
+	}
+ out:
+	return res;
+}
+
+static struct platform_driver leds_generic_driver = {
+	.driver.name	= "Generic-LED",
+	.probe		= leds_generic_probe,
+	.remove		= leds_generic_remove,
+	.suspend	= leds_generic_suspend,
+	.resume		= leds_generic_resume,
+};
+
+static int __init leds_generic_init(void)
+{
+	return platform_driver_register(&leds_generic_driver);
+}
+
+static void __exit leds_generic_exit(void)
+{
+	platform_driver_unregister(&leds_generic_driver);
+}
+
+module_init(leds_generic_init);
+module_exit(leds_generic_exit);
+
+MODULE_AUTHOR("Robin Farine <robin.farine@terminus.org>");
+MODULE_DESCRIPTION("Generic LED driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 958a14d..c688304 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -27,6 +27,23 @@ enum led_brightness {
 	LED_FULL	= 255,
 };
 
+/*
+ * Generic LED driver platform data.
+ *
+ * A generic LED is represented by a platform device named "Generic-LED" with
+ * its dev.platform_data field set to point to an initialized instance of struct
+ * led_generic_data or to an extension of it. The led_generic_data's name and
+ * brightness_set fields must be initialized to point to a valid string and
+ * function respectively. The trigger field, if non-NULL, is used to tie the LED
+ * to a default trigger.
+ */
+struct led_generic_data {
+	const char *name;
+	const char *trigger;
+	void (*brightness_set)(const struct led_generic_data *led_data,
+			       enum led_brightness brightness);
+};
+
 struct led_classdev {
 	const char		*name;
 	int			 brightness;
-- 
1.5.2.1

Re: [PATCH 2/2] LEDS: generic driver

[Richard Purdie]

On Wed, 2007-06-06 at 19:02 +0200, Robin Farine wrote: 
> From: Robin Farine <robin.farine@terminus.org>
> 
> This generic LED driver implements the platform independent part of a
> LED driver letting platform specific code focus on the hardware
> details. The driver binds to platform devices named "Generic-LED"
> which provide the platform specific data and code needed to act on an
> LED.
> 
> This is useful for platforms with exotic ways of controlling LEDs
> which preclude the use of a common LED driver such as GPIO based
> drivers.
> 
> Signed-off-by: Robin Farine <robin.farine@terminus.org>

I'm not sure about this to be honest. I can see a case for perhaps
having a couple of standard suspend/resume functions for platform device
based LED drivers as those functions are often identical. I'm not sure
whether there is going to be much need for more than that.

Having looked through a few of the LED drivers, even the suspend/resume
functions are often different...

>From a style point of view, why not s/pdev_to_led/platform_get_drvdata/?

What are your thoughts on multiple LEDs on a single device?

Given the LED class is going to get a conversion to struct device soon,
I'd prefer to put this on hold until after I've made that conversion at
which point I'll reconsider this.

Regards,

Richard

Re: [PATCH 2/2] LEDS: generic driver

[Robin Farine]

On Fri June 8 2007 01:27, Richard Purdie wrote:

> I'm not sure about this to be honest. I can see a case for
> perhaps having a couple of standard suspend/resume functions for
> platform device based LED drivers as those functions are often
> identical. I'm not sure whether there is going to be much need
> for more than that.

I am not persuaded either. To give a better idea of the context, my 
platform has to sets of LEDs, a first attached to a write-only 
register on the extension bus and the second on an optional 
daughter board and I2C controlled. The value of the register on the 
extension bus is shadowed and needs to be manipulated through an 
API that takes care of the locking.

Thus, instead of adding new board specific drivers to drivers/leds, 
my idea was that given a generic LED driver:

- the module that implements accesses to the write-only register 
defines one platform device for each LED attached to the register 
and implements the LED toggling internally;

- the I2C driver for the daughter board defines one platform device 
for each LED on the daughter board and implement the LED toggling 
internally as well.

> Having looked through a few of the LED drivers, even the
> suspend/resume functions are often different...

I looked at this too and it seemed to me that in all but one case 
the suspend function ends up by calling led_classdev_suspend for 
each LED the driver controls, and the resume function calls 
led_classdev_resume for each LED as well. In the generic LED 
driver, it is equivalent since there is a different platform device 
for each LED so only one call to led_classdev_xyz is needed.

> >From a style point of view, why not
> > s/pdev_to_led/platform_get_drvdata/?

Yes, of course :-).

> What are your thoughts on multiple LEDs on a single device?

Physically that is what I have on my platform, but the system sees 
each LED as a separate device. The platform device data and 
function for a set of LEDS attached to the same physical device 
take care of the multiplexing.

> Given the LED class is going to get a conversion to struct device
> soon, I'd prefer to put this on hold until after I've made that
> conversion at which point I'll reconsider this.

Yes, sure. I am well aware that it applies to very specific 
situations and that there may be a cleaner solution in which case I 
would be happy to drop this idea. I am not pushing for this to be 
included in mainline.

Thanks for looking into this and for the feed-back,

Robin