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

[Christian Gmeiner]

Hi,


2012/1/9 Lars-Peter Clausen <lars@xxxxxxxxxx>:
> On 01/09/2012 11:24 AM, Christian Gmeiner wrote:
>> From a7fecf3426ef98fdd19e9d2610665b9d1ce358a0 Mon Sep 17 00:00:00 2001
>> From: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx
>> <mailto:bigeasy@xxxxxxxxxxxxx>>
>> Date: Mon, 9 Jan 2012 10:09:50 +0100
>> Subject: [PATCH v2] 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.
>>
>> Changes in v2:
>> - *incorporated* feedback from Andrew Morton and Lars-Peter Clausen
>>
>
> looks good to me except some minor style issues and the <mailto:...> tags.
>

I have switch to an other mail client now...

>
>> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx
>> <mailto:bigeasy@xxxxxxxxxxxxx>>
>> Signed-off-by: Christian Gmeiner <christian.gmeiner@xxxxxxxxx
>> <mailto:christian.gmeiner@xxxxxxxxx>>
>> ---
>> [...]
>> diff --git a/drivers/leds/leds-ot200.c b/drivers/leds/leds-ot200.c
>> new file mode 100644
>> index 0000000..4d000ac
>> --- /dev/null
>> +++ b/drivers/leds/leds-ot200.c
>> @@ -0,0 +1,177 @@
>> [...]
>> +
>> +static int __devinit ot200_led_probe(struct platform_device *pdev)
>> +{
>> + Â Âint i;
>> + Â Âint ret;
>> +
>> + Â Âfor (i = 0; i < ARRAY_SIZE(leds); i++) {
>> +
>> + Â Â Â Âleds[i].cdev.name = leds[i].name;
>> + Â Â Â Âleds[i].cdev.default_trigger = NULL;
>> + Â Â Â Âleds[i].cdev.blink_set = NULL;
>
> No need to initialize to NULL.

okay

>
>> + Â Â Â Âleds[i].cdev.brightness_set = ot200_led_brightness_set;
>> +
>> + Â Â Â Âret = led_classdev_register(&pdev->dev, &leds[i].cdev);
>> + Â Â Â Âif (ret < 0)
>> + Â Â Â Â Â Âgoto err;
>> +
>> + Â Â Â Âdev_set_drvdata(leds[i].cdev.dev, &leds[i]);
>
> Neither this ...
>
>> + Â Â}
>> +
>> + Â Âplatform_set_drvdata(pdev, leds);
>
> nor this is ever used.

you are right

>
>> +
>> + Â Âoutb(0x0, 0x49); Â Â/* turn off all front leds */
>> + Â Âoutb(0x2, 0x5a); Â Â/* turn on init led */
>> + Â Âleds_front = 0;
>> + Â Âleds_back = BIT(1);
>
> Maybe initialize leds_front and leds_back first and pass it to outb.
>
>> +
>> + Â Âreturn 0;
>> +
>> +err:
>> + Â Âfor (i = i - 1; i >= 0; i--)
>> + Â Â Â Âled_classdev_unregister(&leds[i].cdev);
>> +
>> + Â Âreturn ret;
>> +}
>> +

Hope v3 will make it :)

thanks
--
Christian Gmeiner, MSc
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/