Re: [PATCH] leds: Add BD2802GU LED driver

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 19 Feb 2009 12:23:18 +0900
Kim Kyuwon <chammoru@gmail.com> wrote:

> ROHM BD2802GU is a RGB LED controller attached to i2c bus and specifically
> engineered for decoration purposes. This RGB controller incorporates lighting
> patterns and illuminates.
> 
> This driver is designed to minimize power consumption, so when there is no
> emitting LED, it enters to reset state. And because the BD2802GU has lots of
> features that can't be covered by the current LED framework, it provides
> Advanced Configuration Function(ADF) mode, so that user applications can set
> registers of BD2802GU directly.
> 
> Here are basic usage examples :
> ; to turn on LED (not blink)
> $ echo 1 > /sys/class/leds/led1_R/brightness
> ; to blink LED
> $ echo timer > /sys/class/leds/led1_R/trigger
> $ echo 1 > /sys/class/leds/led1_R/delay_on
> $ echo 1 > /sys/class/leds/led1_R/delay_off
> ; to turn off LED
> $ echo 0 > /sys/class/leds/led1_R/brightness
> 

Your email client has wordwrapped the patch and has replaced all tabs
with eight spaces.  Please fix that up when resending. 
Documentation/email-clients.txt has some gmail tips.

I had a go at cleaning it up, and found lots of code whcih was indented
with seven-spaces.  Perhaps this file was origininaly edited with
space-space-space-space indenting.  If so, please don't do that - use
hard tab characters.

Anyway, I ended up with soemthing reasonable-looking which actually
compiles.  Please check the result.

>
> ...
>
> --- /dev/null
> +++ b/drivers/leds/leds-bd2802.c
> @@ -0,0 +1,765 @@
> +/*
> + * leds-bd2802.c - RGB LED Driver
> + *
> + * Copyright (C) 2009 Samsung Electronics
> + * Kim Kyuwon <q1.kim@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Datasheet: http://www.rohm.com/products/databook/driver/pdf/bd2802gu-e.pdf
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/gpio.h>
> +#include <linux/delay.h>
> +#include <linux/leds.h>
> +#include <linux/leds-bd2802.h>
> +
> +
> +#define LED_CTL(rgb2en, rgb1en) ((rgb2en) << 4 | (rgb1en << 0))

Argument `rgb1en' should have been parenthesised.

It would be better to implement this as a regular C function.

> +#define BD2802_LED_OFFSET              0xa
> +#define BD2802_COLOR_OFFSET            0x3
> +
> +#define BD2802_REG_CLKSETUP            0x00
> +#define BD2802_REG_CONTROL             0x01
> +#define BD2802_REG_HOURSETUP           0x02
> +#define BD2802_REG_CURRENT1SETUP       0x03
> +#define BD2802_REG_CURRENT2SETUP       0x04
> +#define BD2802_REG_WAVEPATTERN         0x05
> +
> +#define BD2802_CURRENT_032             0x10 /* 3.2mA */
> +#define BD2802_CURRENT_000             0x00 /* 0.0mA */
> +
> +#define BD2802_PATTERN_FULL            0x07
> +#define BD2802_PATTERN_HALF            0x03
> +
>
> ...
>
> +struct bd2802_led {
> +       struct bd2802_led_platform_data *pdata;
> +       struct i2c_client               *client;
> +       struct rw_semaphore             rwsem;
> +       struct work_struct              work;
> +
> +       struct led_state                led[2];
> +
> +       /*
> +        * Making led_classdev as array is not recommended, because array
> +        * members prevent using 'container_of' macro. So repetitive works
> +        * are needed.

hm.  Interesting.  I bet there's a way of using container_of(), but it
might end up being unpleasant.

> +        */
> +       struct led_classdev             cdev_led1r;
> +       struct led_classdev             cdev_led1g;
> +       struct led_classdev             cdev_led1b;
> +       struct led_classdev             cdev_led2r;
> +       struct led_classdev             cdev_led2g;
> +       struct led_classdev             cdev_led2b;
> +
> +       /*
> +        * Advanced Configuration Function(ADF) mode:
> +        * In ADF mode, user can set registers of BD2802GU directly,
> +        * therefore BD2802GU doesn't enter reset state.
> +        */
> +       int                             adf_on;
> +
> +       enum led_ids                    led_id;
> +       enum led_colors                 color;
> +       enum led_bits                   state;
> +};
>
> ...
>
> +
> +/*--------------------------------------------------------------*/
> +/*     BD2802GU core functions                                 */
> +/*--------------------------------------------------------------*/
> +
> +static int bd2802_write_byte(struct i2c_client *client, u8 reg, u8 val)
> +{
> +       int ret = i2c_smbus_write_byte_data(client, reg, val);
> +       if (ret >= 0)
> +               return 0;
> +
> +       dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
> +                                               __func__, reg, val, ret);
> +
> +       return ret;
> +}
> +
> +static void bd2802_update_state(struct bd2802_led *led, enum led_ids id,
> +                               enum led_colors color, enum led_bits led_bit)
> +{
> +       int i;
> +       u8 value;
> +
> +       for (i = 0 ; i < LED_NUM ; i++) {

We normally omit the spaces before the semicolons, and this driver omit
them in some places.

scripts/checkpatch.pl is supposed to detect this, but it seems that it
broke.

> +               if (i == id) {
> +                       switch (color) {
> +                       case RED:
> +                               led->led[i].r = led_bit;
> +                               break;
> +                       case GREEN:
> +                               led->led[i].g = led_bit;
> +                               break;
> +                       case BLUE:
> +                               led->led[i].b = led_bit;
> +                               break;
> +                       default:
> +                               dev_err(&led->client->dev,
> +                                       "%s: Invalid color\n", __func__);
> +                               return;
> +                       }
> +               }
> +       }
>
> ...
>
> +static inline void bd2802_turn_on(struct bd2802_led *led, enum led_ids id,
> +                               enum led_colors color, enum led_bits led_bit)
> +{
> +       if (led_bit == BD2802_OFF) {
> +               dev_err(&led->client->dev,
> +                                       "Only 'blink' and 'on' are allowed\n");
> +               return;
> +       }
> +
> +       if (led_bit == BD2802_BLINK)
> +               bd2802_set_blink(led, id, color);
> +       else
> +               bd2802_set_on(led, id, color);
> +}

The driver attemtps to inline a large number of large functions.  This
will (depending upon Kconfig options) produce a lot more code and is
probably slower than not inlining them.

>
> ...
>
> --- /dev/null
> +++ b/include/linux/leds-bd2802.h
> @@ -0,0 +1,26 @@
> +/*
> + * leds-bd2802.h - RGB LED Driver
> + *
> + * Copyright (C) 2009 Samsung Electronics
> + * Kim Kyuwon <q1.kim@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Datasheet: http://www.rohm.com/products/databook/driver/pdf/bd2802gu-e.pdf
> + *
> + */
> +#ifndef _LEDS_BD2802_H_
> +#define _LEDS_BD2802_H_
> +
> +struct bd2802_led_platform_data{
> +       int     reset_gpio;
> +       u8      rgb_time;
> +};
> +
> +#define RGB_TIME(slopedown, slopeup, waveform) \
> +       ((slopedown) << 6 | ((slopeup) << 4) | (waveform))

Again, it would be better to implement this as a plain old C function.

After all that - it's a nice-looking driver.