Re: [PATCH RESEND] add led driver for Bachmann's ot200

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

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

>
> ...
>