lkml.org 
[lkml]   [2009]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] leds: Add BD2802GU LED driver
    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.



    \
     
     \ /
      Last update: 2009-02-19 23:53    [W:0.045 / U:152.732 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site