| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Dec 2011 13:10:17 +0100
From: Christian Gmeiner <christian.gmeiner@...il.com>
To: Lars-Peter Clausen <lars@...afoo.de>
Cc: linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
rpurdie@...ys.net
Subject: Re: [PATCH RESEND] add led driver for Bachmann's ot200
Hi Lars-Peter,
2011/12/14 Lars-Peter Clausen <lars@...afoo.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@...utronix.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@...utronix.de>
>> Signed-off-by: Christian Gmeiner <christian.gmeiner@...il.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@...utronix.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@...utronix.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@...r.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists