Messages in this thread | | | From | Christian Gmeiner <> | Date | Thu, 15 Dec 2011 13:10:17 +0100 | Subject | Re: [PATCH RESEND] add led driver for Bachmann's ot200 |
| |
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/ > -- 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/
| |