* [PATCH RESEND] add led driver for Bachmann's ot200
@ 2011-12-13 9:57 Christian Gmeiner
2011-12-13 22:20 ` Andrew Morton
2011-12-14 9:04 ` Lars-Peter Clausen
0 siblings, 2 replies; 5+ messages in thread
From: Christian Gmeiner @ 2011-12-13 9:57 UTC (permalink / raw)
To: linux-kernel, akpm; +Cc: Sebastian Andrzej Siewior, rpurdie
>From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Date: Fri, 18 Nov 2011 10:21:48 +0100
Subject: [PATCH] add led driver for Bachmann's ot200
This patch adds support for leds on Bachmann's ot200 visualisation device.
The device has three leds on the back panel (led_err, led_init and led_run)
and can handle up to seven leds on the front panel.
The driver was written by Linutronix on behalf of
Bachmann electronic GmbH.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
---
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-ot200.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 213 insertions(+), 0 deletions(-)
create mode 100644 drivers/leds/leds-ot200.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ff203a4..6cf0dd6 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -387,6 +387,14 @@ config LEDS_RENESAS_TPU
pin function. The latter to support brightness control.
Brightness control is supported but hardware blinking is not.
+config LEDS_OT200
+ tristate "LED support for Bachmann OT200"
+ depends on LEDS_CLASS
+ help
+ This option enables support for the LEDs on the Bachmann OT200. The
+ LEDs are controlled through LPC bus.
+ Say Y to enable LEDs on the Bachmann OT200.
+
config LEDS_TRIGGERS
bool "LED Trigger support"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e4f6bf5..0814d42 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -43,6 +43,7 @@ obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
obj-$(CONFIG_LEDS_RENESAS_TPU) += leds-renesas-tpu.o
+obj-$(CONFIG_LEDS_OT200) += leds-ot200.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
new file mode 100644
index 0000000..60f118b
--- /dev/null
+++ b/drivers/leds/leds-ot200.c
@@ -0,0 +1,204 @@
+/*
+ * Bachmann ot200 leds driver.
+ *
+ * Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
+ * License: GPL as published by the FSF.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/leds.h>
+#include <linux/io.h>
+#include <linux/module.h>
+
+struct ot200_led {
+ struct led_classdev cdev;
+ char name[8];
+ unsigned int num;
+};
+
+static u8 old_val;
+static u8 old_val_back;
+DEFINE_SPINLOCK(value_lock);
+
+static void ot200_led_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
+ u8 val;
+ u8 mask;
+ unsigned long flags;
+
+ mask = 1 << led->num;
+
+ spin_lock_irqsave(&value_lock, flags);
+ val = old_val;
+
+ if (value == LED_OFF)
+ val &= ~mask;
+ else
+ val |= mask;
+
+ old_val = val;
+ spin_unlock_irqrestore(&value_lock, flags);
+ outb(val, 0x49);
+}
+
+static void ot200_led_back_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+ struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
+ u8 val;
+ u8 mask;
+ unsigned long flags;
+
+ mask = 1 << led->num;
+
+ spin_lock_irqsave(&value_lock, flags);
+ val = old_val_back;
+
+ if (value == LED_OFF)
+ val &= ~mask;
+ else
+ val |= mask;
+
+ old_val_back = val;
+ spin_unlock_irqrestore(&value_lock, flags);
+ outb(val, 0x5a);
+}
+
+static int __devinit create_ot200_led(struct platform_device *pdev, int num,
+ struct ot200_led *led)
+{
+ int ret;
+
+ num += 1;
+ snprintf(led->name, sizeof(led->name), "led%d", num);
+ led->num = 7 - num;
+ led->cdev.name = led->name;
+ led->cdev.default_trigger = NULL;
+ led->cdev.blink_set = NULL;
+ led->cdev.brightness_set = ot200_led_set;
+
+ ret = led_classdev_register(&pdev->dev, &led->cdev);
+ if (ret < 0)
+ goto out;
+
+ dev_set_drvdata(led->cdev.dev, led);
+ return 0;
+
+ led_classdev_unregister(&led->cdev);
+out:
+ return ret;
+}
+
+static int __devinit create_ot200_led_back(struct platform_device *pdev,
+ int num, struct ot200_led *led)
+{
+ char *led_name;
+ int ret;
+
+ switch (num) {
+ case 0:
+ led_name = "run";
+ break;
+ case 1:
+ led_name = "init";
+ break;
+ case 2:
+ led_name = "err";
+ break;
+ default:
+ BUG();
+ };
+
+ snprintf(led->name, sizeof(led->name), "led_%s", led_name);
+ led->num = num;
+ led->cdev.name = led->name;
+ led->cdev.blink_set = NULL;
+ led->cdev.brightness_set = ot200_led_back_set;
+
+ ret = led_classdev_register(&pdev->dev, &led->cdev);
+ if (ret < 0)
+ goto out;
+
+ dev_set_drvdata(led->cdev.dev, led);
+ return 0;
+
+ led_classdev_unregister(&led->cdev);
+out:
+ return ret;
+
+}
+
+#define NUM_LEDS 7
+#define NUM_LEDS_BACK 3
+#define NUM_LEDS_TOTAL (NUM_LEDS + NUM_LEDS_BACK)
+
+static int __devinit ot200_led_probe(struct platform_device *pdev)
+{
+ struct ot200_led *leds;
+ int i;
+ int ret;
+
+ leds = kzalloc(sizeof(struct ot200_led) * NUM_LEDS_TOTAL, GFP_KERNEL);
+ if (!leds)
+ return -ENOMEM;
+
+ for (i = 0; i < NUM_LEDS; i++) {
+ ret = create_ot200_led(pdev, i, &leds[i]);
+ if (ret < 0)
+ goto err;
+ }
+
+ for (i = NUM_LEDS; i < NUM_LEDS + NUM_LEDS_BACK; i++) {
+ ret = create_ot200_led_back(pdev, i - NUM_LEDS, &leds[i]);
+ if (ret < 0)
+ goto err;
+ }
+
+ platform_set_drvdata(pdev, leds);
+ outb(0x0, 0x49); /* turn of all front leds */
+ outb(0x2, 0x5a); /* turn on init led */
+ return 0;
+
+err:
+ for (i = i - 1; i >= 0; i--)
+ led_classdev_unregister(&leds[i].cdev);
+
+ kfree(leds);
+
+ return ret;
+}
+
+static int __devexit ot200_led_remove(struct platform_device *pdev)
+{
+ int i;
+ struct ot200_led *leds;
+
+ leds = platform_get_drvdata(pdev);
+
+ for (i = 0; i < NUM_LEDS_TOTAL; i++)
+ led_classdev_unregister(&leds[i].cdev);
+
+ kfree(leds);
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+static struct platform_driver ot200_led_driver = {
+ .probe = ot200_led_probe,
+ .remove = __devexit_p(ot200_led_remove),
+ .driver = {
+ .name = "leds-ot200",
+ .owner = THIS_MODULE,
+ },
+};
+
+module_platform_driver(ot200_led_driver);
+
+MODULE_AUTHOR("Sebastian A. Siewior <bigeasy@linutronix.de>");
+MODULE_DESCRIPTION("ot200 LED driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:leds-ot200");
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] add led driver for Bachmann's ot200
2011-12-13 9:57 [PATCH RESEND] add led driver for Bachmann's ot200 Christian Gmeiner
@ 2011-12-13 22:20 ` Andrew Morton
2011-12-14 8:10 ` Christian Gmeiner
2011-12-14 9:04 ` Lars-Peter Clausen
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2011-12-13 22:20 UTC (permalink / raw)
To: Christian Gmeiner; +Cc: linux-kernel, Sebastian Andrzej Siewior, rpurdie
On Tue, 13 Dec 2011 10:57:30 +0100
Christian Gmeiner <christian.gmeiner@gmail.com> wrote:
> >From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date: Fri, 18 Nov 2011 10:21:48 +0100
> Subject: [PATCH] add led driver for Bachmann's ot200
>
> This patch adds support for leds on Bachmann's ot200 visualisation device.
> The device has three leds on the back panel (led_err, led_init and led_run)
> and can handle up to seven leds on the front panel.
>
> The driver was written by Linutronix on behalf of
> Bachmann electronic GmbH.
The code uses no tabs and indents with seven-spaces. Please convert it
to use hard tabs in the conventional fashion? In the Makefile and
Kconfig, too.
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ff203a4..6cf0dd6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -387,6 +387,14 @@ config LEDS_RENESAS_TPU
> pin function. The latter to support brightness control.
> Brightness control is supported but hardware blinking is not.
>
> +config LEDS_OT200
> + tristate "LED support for Bachmann OT200"
> + depends on LEDS_CLASS
> + help
> + This option enables support for the LEDs on the Bachmann OT200. The
> + LEDs are controlled through LPC bus.
> + Say Y to enable LEDs on the Bachmann OT200.
> +
> config LEDS_TRIGGERS
> bool "LED Trigger support"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e4f6bf5..0814d42 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
> obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
> obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
> obj-$(CONFIG_LEDS_RENESAS_TPU) += leds-renesas-tpu.o
> +obj-$(CONFIG_LEDS_OT200) += leds-ot200.o
>
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
> new file mode 100644
> index 0000000..60f118b
> --- /dev/null
> +++ b/drivers/leds/leds-ot200.c
> @@ -0,0 +1,204 @@
> +/*
> + * Bachmann ot200 leds driver.
> + *
> + * Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> + * License: GPL as published by the FSF.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/leds.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +
> +struct ot200_led {
> + struct led_classdev cdev;
> + char name[8];
> + unsigned int num;
> +};
> +
> +static u8 old_val;
> +static u8 old_val_back;
What's going on here? The driver is using eight bits to store the
current control setting, but the changelog says the driver can handle
ten LEDs.
<looks>
OK, so we're using seven bits in old_val and three in old_val_back.
> +DEFINE_SPINLOCK(value_lock);
Should be static.
> +static void ot200_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
> + u8 val;
> + u8 mask;
> + unsigned long flags;
> +
> + mask = 1 << led->num;
> +
> + spin_lock_irqsave(&value_lock, flags);
> + val = old_val;
> +
> + if (value == LED_OFF)
> + val &= ~mask;
> + else
> + val |= mask;
> +
> + old_val = val;
> + spin_unlock_irqrestore(&value_lock, flags);
> + outb(val, 0x49);
> +}
It's a bit worrisome doing the outb() outside the locked section, but
all the races I can think of result from userspace doing things which
were unavoidably indeterminate anyway.
> +static void ot200_led_back_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
> + u8 val;
> + u8 mask;
> + unsigned long flags;
> +
> + mask = 1 << led->num;
> +
> + spin_lock_irqsave(&value_lock, flags);
> + val = old_val_back;
> +
> + if (value == LED_OFF)
> + val &= ~mask;
> + else
> + val |= mask;
> +
> + old_val_back = val;
> + spin_unlock_irqrestore(&value_lock, flags);
> + outb(val, 0x5a);
> +}
> +
> +static int __devinit create_ot200_led(struct platform_device *pdev, int num,
> + struct ot200_led *led)
> +{
> + int ret;
> +
> + num += 1;
> + snprintf(led->name, sizeof(led->name), "led%d", num);
> + led->num = 7 - num;
> + led->cdev.name = led->name;
> + led->cdev.default_trigger = NULL;
> + led->cdev.blink_set = NULL;
> + led->cdev.brightness_set = ot200_led_set;
> +
> + ret = led_classdev_register(&pdev->dev, &led->cdev);
> + if (ret < 0)
> + goto out;
> +
> + dev_set_drvdata(led->cdev.dev, led);
> + return 0;
> +
> + led_classdev_unregister(&led->cdev);
> +out:
> + return ret;
> +}
> +
> +static int __devinit create_ot200_led_back(struct platform_device *pdev,
> + int num, struct ot200_led *led)
> +{
> + char *led_name;
> + int ret;
> +
> + switch (num) {
> + case 0:
> + led_name = "run";
> + break;
> + case 1:
> + led_name = "init";
> + break;
> + case 2:
> + led_name = "err";
> + break;
> + default:
> + BUG();
> + };
> +
> + snprintf(led->name, sizeof(led->name), "led_%s", led_name);
> + led->num = num;
> + led->cdev.name = led->name;
> + led->cdev.blink_set = NULL;
> + led->cdev.brightness_set = ot200_led_back_set;
> +
> + ret = led_classdev_register(&pdev->dev, &led->cdev);
> + if (ret < 0)
> + goto out;
> +
> + dev_set_drvdata(led->cdev.dev, led);
> + return 0;
> +
> + led_classdev_unregister(&led->cdev);
> +out:
> + return ret;
> +
> +}
This is implementing a userspace API (led_run, led_init, led_err) but
that API isn't documented anywhere. Can we add a few words of
Documentation so our users know how to use this driver?
>
> ...
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] add led driver for Bachmann's ot200
2011-12-13 22:20 ` Andrew Morton
@ 2011-12-14 8:10 ` Christian Gmeiner
0 siblings, 0 replies; 5+ messages in thread
From: Christian Gmeiner @ 2011-12-14 8:10 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-kernel, Sebastian Andrzej Siewior, rpurdie
Hi Andrew
2011/12/13 Andrew Morton <akpm@linux-foundation.org>:
> On Tue, 13 Dec 2011 10:57:30 +0100
> Christian Gmeiner <christian.gmeiner@gmail.com> wrote:
>
>> >From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Date: Fri, 18 Nov 2011 10:21:48 +0100
>> Subject: [PATCH] add led driver for Bachmann's ot200
>>
>> This patch adds support for leds on Bachmann's ot200 visualisation device.
>> The device has three leds on the back panel (led_err, led_init and led_run)
>> and can handle up to seven leds on the front panel.
>>
>> The driver was written by Linutronix on behalf of
>> Bachmann electronic GmbH.
>
> The code uses no tabs and indents with seven-spaces. Please convert it
> to use hard tabs in the conventional fashion? In the Makefile and
> Kconfig, too.
okay
>
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ff203a4..6cf0dd6 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -387,6 +387,14 @@ config LEDS_RENESAS_TPU
>> pin function. The latter to support brightness control.
>> Brightness control is supported but hardware blinking is not.
>>
>> +config LEDS_OT200
>> + tristate "LED support for Bachmann OT200"
>> + depends on LEDS_CLASS
>> + help
>> + This option enables support for the LEDs on the Bachmann OT200. The
>> + LEDs are controlled through LPC bus.
>> + Say Y to enable LEDs on the Bachmann OT200.
>> +
>> config LEDS_TRIGGERS
>> bool "LED Trigger support"
>> depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index e4f6bf5..0814d42 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
>> obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
>> obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
>> obj-$(CONFIG_LEDS_RENESAS_TPU) += leds-renesas-tpu.o
>> +obj-$(CONFIG_LEDS_OT200) += leds-ot200.o
>>
>> # LED SPI Drivers
>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
>> new file mode 100644
>> index 0000000..60f118b
>> --- /dev/null
>> +++ b/drivers/leds/leds-ot200.c
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Bachmann ot200 leds driver.
>> + *
>> + * Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> + * License: GPL as published by the FSF.
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/leds.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +
>> +struct ot200_led {
>> + struct led_classdev cdev;
>> + char name[8];
>> + unsigned int num;
>> +};
>> +
>> +static u8 old_val;
>> +static u8 old_val_back;
>
> What's going on here? The driver is using eight bits to store the
> current control setting, but the changelog says the driver can handle
> ten LEDs.
>
> <looks>
>
> OK, so we're using seven bits in old_val and three in old_val_back.
>
Exactly.. I have not found a better way to store the led state. Any
recommendations?
>> +DEFINE_SPINLOCK(value_lock);
>
> Should be static.
>
okay
>> +static void ot200_led_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
>> + u8 val;
>> + u8 mask;
>> + unsigned long flags;
>> +
>> + mask = 1 << led->num;
>> +
>> + spin_lock_irqsave(&value_lock, flags);
>> + val = old_val;
>> +
>> + if (value == LED_OFF)
>> + val &= ~mask;
>> + else
>> + val |= mask;
>> +
>> + old_val = val;
>> + spin_unlock_irqrestore(&value_lock, flags);
>> + outb(val, 0x49);
>> +}
>
> It's a bit worrisome doing the outb() outside the locked section, but
> all the races I can think of result from userspace doing things which
> were unavoidably indeterminate anyway.
>
I think that it is really no problem to do outb() in the looked area.
Will change
this for v2 of this patch.
>> +static void ot200_led_back_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
>> + u8 val;
>> + u8 mask;
>> + unsigned long flags;
>> +
>> + mask = 1 << led->num;
>> +
>> + spin_lock_irqsave(&value_lock, flags);
>> + val = old_val_back;
>> +
>> + if (value == LED_OFF)
>> + val &= ~mask;
>> + else
>> + val |= mask;
>> +
>> + old_val_back = val;
>> + spin_unlock_irqrestore(&value_lock, flags);
>> + outb(val, 0x5a);
>> +}
>> +
>> +static int __devinit create_ot200_led(struct platform_device *pdev, int num,
>> + struct ot200_led *led)
>> +{
>> + int ret;
>> +
>> + num += 1;
>> + snprintf(led->name, sizeof(led->name), "led%d", num);
>> + led->num = 7 - num;
>> + led->cdev.name = led->name;
>> + led->cdev.default_trigger = NULL;
>> + led->cdev.blink_set = NULL;
>> + led->cdev.brightness_set = ot200_led_set;
>> +
>> + ret = led_classdev_register(&pdev->dev, &led->cdev);
>> + if (ret < 0)
>> + goto out;
>> +
>> + dev_set_drvdata(led->cdev.dev, led);
>> + return 0;
>> +
>> + led_classdev_unregister(&led->cdev);
>> +out:
>> + return ret;
>> +}
>> +
>> +static int __devinit create_ot200_led_back(struct platform_device *pdev,
>> + int num, struct ot200_led *led)
>> +{
>> + char *led_name;
>> + int ret;
>> +
>> + switch (num) {
>> + case 0:
>> + led_name = "run";
>> + break;
>> + case 1:
>> + led_name = "init";
>> + break;
>> + case 2:
>> + led_name = "err";
>> + break;
>> + default:
>> + BUG();
>> + };
>> +
>> + snprintf(led->name, sizeof(led->name), "led_%s", led_name);
>> + led->num = num;
>> + led->cdev.name = led->name;
>> + led->cdev.blink_set = NULL;
>> + led->cdev.brightness_set = ot200_led_back_set;
>> +
>> + ret = led_classdev_register(&pdev->dev, &led->cdev);
>> + if (ret < 0)
>> + goto out;
>> +
>> + dev_set_drvdata(led->cdev.dev, led);
>> + return 0;
>> +
>> + led_classdev_unregister(&led->cdev);
>> +out:
>> + return ret;
>> +
>> +}
>
> This is implementing a userspace API (led_run, led_init, led_err) but
> that API isn't documented anywhere. Can we add a few words of
> Documentation so our users know how to use this driver?
To be exact the driver uses the led subsystem - led_classdev_register() - and
creates these led devices. So we are not creating any new API, which should
be documented - or?
# ls /sys/class/leds/ -l
total 0
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led1 ->
../../devices/platform/leds-ot200.0/leds/led1
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led2 ->
../../devices/platform/leds-ot200.0/leds/led2
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led3 ->
../../devices/platform/leds-ot200.0/leds/led3
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led4 ->
../../devices/platform/leds-ot200.0/leds/led4
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led5 ->
../../devices/platform/leds-ot200.0/leds/led5
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led6 ->
../../devices/platform/leds-ot200.0/leds/led6
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led7 ->
../../devices/platform/leds-ot200.0/leds/led7
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led_err ->
../../devices/platform/leds-ot200.0/leds/led_err
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led_ini ->
../../devices/platform/leds-ot200.0/leds/led_ini
lrwxrwxrwx 1 root root 0 Dec 14 07:57 led_run ->
../../devices/platform/leds-ot200.0/leds/led_run
Thanks for your review.
--
Christian Gmeiner, MSc
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] add led driver for Bachmann's ot200
2011-12-13 9:57 [PATCH RESEND] add led driver for Bachmann's ot200 Christian Gmeiner
2011-12-13 22:20 ` Andrew Morton
@ 2011-12-14 9:04 ` Lars-Peter Clausen
2011-12-15 12:10 ` Christian Gmeiner
1 sibling, 1 reply; 5+ messages in thread
From: Lars-Peter Clausen @ 2011-12-14 9:04 UTC (permalink / raw)
To: Christian Gmeiner; +Cc: linux-kernel, akpm, Sebastian Andrzej Siewior, rpurdie
On 12/13/2011 10:57 AM, Christian Gmeiner wrote:
> From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Date: Fri, 18 Nov 2011 10:21:48 +0100
> Subject: [PATCH] add led driver for Bachmann's ot200
>
> This patch adds support for leds on Bachmann's ot200 visualisation device.
> The device has three leds on the back panel (led_err, led_init and led_run)
> and can handle up to seven leds on the front panel.
>
> The driver was written by Linutronix on behalf of
> Bachmann electronic GmbH.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
> drivers/leds/Kconfig | 8 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-ot200.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 213 insertions(+), 0 deletions(-)
> create mode 100644 drivers/leds/leds-ot200.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ff203a4..6cf0dd6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -387,6 +387,14 @@ config LEDS_RENESAS_TPU
> pin function. The latter to support brightness control.
> Brightness control is supported but hardware blinking is not.
>
> +config LEDS_OT200
> + tristate "LED support for Bachmann OT200"
> + depends on LEDS_CLASS
> + help
> + This option enables support for the LEDs on the Bachmann OT200. The
> + LEDs are controlled through LPC bus.
> + Say Y to enable LEDs on the Bachmann OT200.
> +
> config LEDS_TRIGGERS
> bool "LED Trigger support"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index e4f6bf5..0814d42 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
> obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
> obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
> obj-$(CONFIG_LEDS_RENESAS_TPU) += leds-renesas-tpu.o
> +obj-$(CONFIG_LEDS_OT200) += leds-ot200.o
>
> # LED SPI Drivers
> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
> diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
> new file mode 100644
> index 0000000..60f118b
> --- /dev/null
> +++ b/drivers/leds/leds-ot200.c
> @@ -0,0 +1,204 @@
> +/*
> + * Bachmann ot200 leds driver.
> + *
> + * Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> + * License: GPL as published by the FSF.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/leds.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +
> +struct ot200_led {
> + struct led_classdev cdev;
> + char name[8];
> + unsigned int num;
> +};
> +
> +static u8 old_val;
> +static u8 old_val_back;
> +DEFINE_SPINLOCK(value_lock);
> +
> +static void ot200_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
> + u8 val;
> + u8 mask;
> + unsigned long flags;
> +
> + mask = 1 << led->num;
> +
> + spin_lock_irqsave(&value_lock, flags);
> + val = old_val;
> +
> + if (value == LED_OFF)
> + val &= ~mask;
> + else
> + val |= mask;
> +
> + old_val = val;
> + spin_unlock_irqrestore(&value_lock, flags);
> + outb(val, 0x49);
> +}
> +
These two functions (above and beneath) look rather similar, they could
probably share most of their code. Maybe use the leds num field to
distinguish between the two types and choose the address where to write the
value accordingly.
> +static void ot200_led_back_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
> + u8 val;
> + u8 mask;
> + unsigned long flags;
> +
> + mask = 1 << led->num;
> +
> + spin_lock_irqsave(&value_lock, flags);
> + val = old_val_back;
> +
> + if (value == LED_OFF)
> + val &= ~mask;
> + else
> + val |= mask;
> +
> + old_val_back = val;
> + spin_unlock_irqrestore(&value_lock, flags);
> + outb(val, 0x5a);
> +}
> +
> +static int __devinit create_ot200_led(struct platform_device *pdev, int num,
> + struct ot200_led *led)
> +{
> + int ret;
> +
> + num += 1;
> + snprintf(led->name, sizeof(led->name), "led%d", num);
> + led->num = 7 - num;
> + led->cdev.name = led->name;
> + led->cdev.default_trigger = NULL;
> + led->cdev.blink_set = NULL;
> + led->cdev.brightness_set = ot200_led_set;
> +
> + ret = led_classdev_register(&pdev->dev, &led->cdev);
> + if (ret < 0)
> + goto out;
> +
> + dev_set_drvdata(led->cdev.dev, led);
> + return 0;
> +
> + led_classdev_unregister(&led->cdev);
> +out:
> + return ret;
> +}
These two look also rather similar.
> +
> +static int __devinit create_ot200_led_back(struct platform_device *pdev,
> + int num, struct ot200_led *led)
> +{
> + char *led_name;
> + int ret;
> +
> + switch (num) {
> + case 0:
> + led_name = "run";
> + break;
> + case 1:
> + led_name = "init";
> + break;
> + case 2:
> + led_name = "err";
> + break;
> + default:
> + BUG();
> + };
Since your led names are const and do not depend on platform data a global
static array containing all the names (including the led prefix) should
probably work as well.
> +
> + snprintf(led->name, sizeof(led->name), "led_%s", led_name);
> + led->num = num;
> + led->cdev.name = led->name;
> + led->cdev.blink_set = NULL;
> + led->cdev.brightness_set = ot200_led_back_set;
> +
> + ret = led_classdev_register(&pdev->dev, &led->cdev);
> + if (ret < 0)
> + goto out;
> +
> + dev_set_drvdata(led->cdev.dev, led);
> + return 0;
> +
> + led_classdev_unregister(&led->cdev);
> +out:
> + return ret;
> +
> +}
> +
> +#define NUM_LEDS 7
> +#define NUM_LEDS_BACK 3
> +#define NUM_LEDS_TOTAL (NUM_LEDS + NUM_LEDS_BACK)
> +
> +static int __devinit ot200_led_probe(struct platform_device *pdev)
> +{
> + struct ot200_led *leds;
> + int i;
> + int ret;
> +
> + leds = kzalloc(sizeof(struct ot200_led) * NUM_LEDS_TOTAL, GFP_KERNEL);
kcalloc
> + if (!leds)
> + return -ENOMEM;
> +
> + for (i = 0; i < NUM_LEDS; i++) {
> + ret = create_ot200_led(pdev, i, &leds[i]);
> + if (ret < 0)
> + goto err;
> + }
> +
> + for (i = NUM_LEDS; i < NUM_LEDS + NUM_LEDS_BACK; i++) {
> + ret = create_ot200_led_back(pdev, i - NUM_LEDS, &leds[i]);
> + if (ret < 0)
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, leds);
> + outb(0x0, 0x49); /* turn of all front leds */
off
> + outb(0x2, 0x5a); /* turn on init led */
> + return 0;
> +
> +err:
> + for (i = i - 1; i >= 0; i--)
> + led_classdev_unregister(&leds[i].cdev);
> +
> + kfree(leds);
> +
> + return ret;
> +}
> +
> +static int __devexit ot200_led_remove(struct platform_device *pdev)
> +{
> + int i;
> + struct ot200_led *leds;
> +
> + leds = platform_get_drvdata(pdev);
> +
> + for (i = 0; i < NUM_LEDS_TOTAL; i++)
> + led_classdev_unregister(&leds[i].cdev);
> +
> + kfree(leds);
> + platform_set_drvdata(pdev, NULL);
platform_set_drvdata(pdev, NULL) is not necessary.
> +
> + return 0;
> +}
> +
> +static struct platform_driver ot200_led_driver = {
> + .probe = ot200_led_probe,
> + .remove = __devexit_p(ot200_led_remove),
> + .driver = {
> + .name = "leds-ot200",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +module_platform_driver(ot200_led_driver);
> +
> +MODULE_AUTHOR("Sebastian A. Siewior <bigeasy@linutronix.de>");
> +MODULE_DESCRIPTION("ot200 LED driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:leds-ot200");
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RESEND] add led driver for Bachmann's ot200
2011-12-14 9:04 ` Lars-Peter Clausen
@ 2011-12-15 12:10 ` Christian Gmeiner
0 siblings, 0 replies; 5+ messages in thread
From: Christian Gmeiner @ 2011-12-15 12:10 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: linux-kernel, akpm, Sebastian Andrzej Siewior, rpurdie
Hi Lars-Peter,
2011/12/14 Lars-Peter Clausen <lars@metafoo.de>:
> On 12/13/2011 10:57 AM, Christian Gmeiner wrote:
>> From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
>> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Date: Fri, 18 Nov 2011 10:21:48 +0100
>> Subject: [PATCH] add led driver for Bachmann's ot200
>>
>> This patch adds support for leds on Bachmann's ot200 visualisation device.
>> The device has three leds on the back panel (led_err, led_init and led_run)
>> and can handle up to seven leds on the front panel.
>>
>> The driver was written by Linutronix on behalf of
>> Bachmann electronic GmbH.
>>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
>> ---
>> drivers/leds/Kconfig | 8 ++
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-ot200.c | 204 +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 213 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/leds/leds-ot200.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ff203a4..6cf0dd6 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -387,6 +387,14 @@ config LEDS_RENESAS_TPU
>> pin function. The latter to support brightness control.
>> Brightness control is supported but hardware blinking is not.
>>
>> +config LEDS_OT200
>> + tristate "LED support for Bachmann OT200"
>> + depends on LEDS_CLASS
>> + help
>> + This option enables support for the LEDs on the Bachmann OT200. The
>> + LEDs are controlled through LPC bus.
>> + Say Y to enable LEDs on the Bachmann OT200.
>> +
>> config LEDS_TRIGGERS
>> bool "LED Trigger support"
>> depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index e4f6bf5..0814d42 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
>> obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
>> obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
>> obj-$(CONFIG_LEDS_RENESAS_TPU) += leds-renesas-tpu.o
>> +obj-$(CONFIG_LEDS_OT200) += leds-ot200.o
>>
>> # LED SPI Drivers
>> obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
>> diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
>> new file mode 100644
>> index 0000000..60f118b
>> --- /dev/null
>> +++ b/drivers/leds/leds-ot200.c
>> @@ -0,0 +1,204 @@
>> +/*
>> + * Bachmann ot200 leds driver.
>> + *
>> + * Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> + * License: GPL as published by the FSF.
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/leds.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +
>> +struct ot200_led {
>> + struct led_classdev cdev;
>> + char name[8];
>> + unsigned int num;
>> +};
>> +
>> +static u8 old_val;
>> +static u8 old_val_back;
>> +DEFINE_SPINLOCK(value_lock);
>> +
>> +static void ot200_led_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
>> + u8 val;
>> + u8 mask;
>> + unsigned long flags;
>> +
>> + mask = 1 << led->num;
>> +
>> + spin_lock_irqsave(&value_lock, flags);
>> + val = old_val;
>> +
>> + if (value == LED_OFF)
>> + val &= ~mask;
>> + else
>> + val |= mask;
>> +
>> + old_val = val;
>> + spin_unlock_irqrestore(&value_lock, flags);
>> + outb(val, 0x49);
>> +}
>> +
>
> These two functions (above and beneath) look rather similar, they could
> probably share most of their code. Maybe use the leds num field to
> distinguish between the two types and choose the address where to write the
> value accordingly.
Sounds like a good idea.
>
>> +static void ot200_led_back_set(struct led_classdev *led_cdev,
>> + enum led_brightness value)
>> +{
>> + struct ot200_led *led = container_of(led_cdev, struct ot200_led, cdev);
>> + u8 val;
>> + u8 mask;
>> + unsigned long flags;
>> +
>> + mask = 1 << led->num;
>> +
>> + spin_lock_irqsave(&value_lock, flags);
>> + val = old_val_back;
>> +
>> + if (value == LED_OFF)
>> + val &= ~mask;
>> + else
>> + val |= mask;
>> +
>> + old_val_back = val;
>> + spin_unlock_irqrestore(&value_lock, flags);
>> + outb(val, 0x5a);
>> +}
>> +
>> +static int __devinit create_ot200_led(struct platform_device *pdev, int num,
>> + struct ot200_led *led)
>> +{
>> + int ret;
>> +
>> + num += 1;
>> + snprintf(led->name, sizeof(led->name), "led%d", num);
>> + led->num = 7 - num;
>> + led->cdev.name = led->name;
>> + led->cdev.default_trigger = NULL;
>> + led->cdev.blink_set = NULL;
>> + led->cdev.brightness_set = ot200_led_set;
>> +
>> + ret = led_classdev_register(&pdev->dev, &led->cdev);
>> + if (ret < 0)
>> + goto out;
>> +
>> + dev_set_drvdata(led->cdev.dev, led);
>> + return 0;
>> +
>> + led_classdev_unregister(&led->cdev);
>> +out:
>> + return ret;
>> +}
>
> These two look also rather similar.
Correct... will improve it in v2.
>
>> +
>> +static int __devinit create_ot200_led_back(struct platform_device *pdev,
>> + int num, struct ot200_led *led)
>> +{
>> + char *led_name;
>> + int ret;
>> +
>> + switch (num) {
>> + case 0:
>> + led_name = "run";
>> + break;
>> + case 1:
>> + led_name = "init";
>> + break;
>> + case 2:
>> + led_name = "err";
>> + break;
>> + default:
>> + BUG();
>> + };
>
> Since your led names are const and do not depend on platform data a global
> static array containing all the names (including the led prefix) should
> probably work as well.
>
I am fine with this.
>> +
>> + snprintf(led->name, sizeof(led->name), "led_%s", led_name);
>> + led->num = num;
>> + led->cdev.name = led->name;
>> + led->cdev.blink_set = NULL;
>> + led->cdev.brightness_set = ot200_led_back_set;
>> +
>> + ret = led_classdev_register(&pdev->dev, &led->cdev);
>> + if (ret < 0)
>> + goto out;
>> +
>> + dev_set_drvdata(led->cdev.dev, led);
>> + return 0;
>> +
>> + led_classdev_unregister(&led->cdev);
>> +out:
>> + return ret;
>> +
>> +}
>> +
>> +#define NUM_LEDS 7
>> +#define NUM_LEDS_BACK 3
>> +#define NUM_LEDS_TOTAL (NUM_LEDS + NUM_LEDS_BACK)
>> +
>> +static int __devinit ot200_led_probe(struct platform_device *pdev)
>> +{
>> + struct ot200_led *leds;
>> + int i;
>> + int ret;
>> +
>> + leds = kzalloc(sizeof(struct ot200_led) * NUM_LEDS_TOTAL, GFP_KERNEL);
>
> kcalloc
>
okay
>> + if (!leds)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < NUM_LEDS; i++) {
>> + ret = create_ot200_led(pdev, i, &leds[i]);
>> + if (ret < 0)
>> + goto err;
>> + }
>> +
>> + for (i = NUM_LEDS; i < NUM_LEDS + NUM_LEDS_BACK; i++) {
>> + ret = create_ot200_led_back(pdev, i - NUM_LEDS, &leds[i]);
>> + if (ret < 0)
>> + goto err;
>> + }
>> +
>> + platform_set_drvdata(pdev, leds);
>> + outb(0x0, 0x49); /* turn of all front leds */
>
> off
>
ups
>> + outb(0x2, 0x5a); /* turn on init led */
>> + return 0;
>> +
>> +err:
>> + for (i = i - 1; i >= 0; i--)
>> + led_classdev_unregister(&leds[i].cdev);
>> +
>> + kfree(leds);
>> +
>> + return ret;
>> +}
>> +
>> +static int __devexit ot200_led_remove(struct platform_device *pdev)
>> +{
>> + int i;
>> + struct ot200_led *leds;
>> +
>> + leds = platform_get_drvdata(pdev);
>> +
>> + for (i = 0; i < NUM_LEDS_TOTAL; i++)
>> + led_classdev_unregister(&leds[i].cdev);
>> +
>> + kfree(leds);
>> + platform_set_drvdata(pdev, NULL);
>
> platform_set_drvdata(pdev, NULL) is not necessary.
okay
>
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver ot200_led_driver = {
>> + .probe = ot200_led_probe,
>> + .remove = __devexit_p(ot200_led_remove),
>> + .driver = {
>> + .name = "leds-ot200",
>> + .owner = THIS_MODULE,
>> + },
>> +};
>> +
>> +module_platform_driver(ot200_led_driver);
>> +
>> +MODULE_AUTHOR("Sebastian A. Siewior <bigeasy@linutronix.de>");
>> +MODULE_DESCRIPTION("ot200 LED driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:leds-ot200");
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-12-15 12:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-13 9:57 [PATCH RESEND] add led driver for Bachmann's ot200 Christian Gmeiner
2011-12-13 22:20 ` Andrew Morton
2011-12-14 8:10 ` Christian Gmeiner
2011-12-14 9:04 ` Lars-Peter Clausen
2011-12-15 12:10 ` Christian Gmeiner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).