Hi! Attached is revision 2 of the BlinkM i2c rgb led driver. A git tree with the patch applied on top of linux-next is available here: git://github.com/dl9pf/linux.git Find my comments inline ... Am Freitag, Juni 01, 2012, 05:16:12 PM schrieb Jonathan Neuschäfer: [...]
I'm also looking for the best place to fit this in. Staging ? drivers/led ?
Have Phun!
I had fun reviewing the code. :-)
:D
struct blinkm_data {
struct i2c_client *i2c_client; struct mutex update_lock;
/* used for led class interface */
struct blinkm_led blinkm_leds[3];
/* used for "blinkm" sysfs interface */
u8 red; /* c_r - color red */
Is c_r an old name? Yes, removed it.
[...]
#define BLM_DIR_READ 0 #define BLM_DIR_WRITE 1 #define BLM_DIR_WRITE_READ 2 #define BLM_DIR_READ_WRITE 3
Where are these values used? Well, nowhere actually. Just started with some defines. Removed.
What's the difference between write-read and read-write? Sequence! ;).
[...]
*/
static const struct {
int cmd;
I don't think you need the cmd field, as blinkm_cmds[N].cmd is always N as of now.
Yep, removed it.
char cmdchar; u8 cmdbyte;
Cmdchar and cmdbyte seem to be the same (numerically) in the table. Is that intended?
It is same - left it in by intention. [...]
13, 'A', 0x41, 4, 0, 1}, { 14, 'a', 0x61, 0, 1, 0}, { 15, 'Z', 0x5a, 0, 1, 0}, {
16, 'B', 0x42, 5, 0, 1},};
I would leave the array size out, but I guess that's a matter of preference. And I would place the curly brackets like this: static const struct { /* ... */ } blinkm_cmds[] = { {0, 'n', 0x6e, 3, 0, 1}, {1, 'c', 0x63, 3, 0, 1}, {2, 'h', 0x68, 3, 0, 1}, /* ... */ };
Yes, fixed that. I ran indent on it and forgot to check again.
static ssize_t show_blue(struct device *dev, struct device_attribute [...] static ssize_t store_blue(struct device *dev, struct device_attribute [...] static DEVICE_ATTR(blue, S_IRUGO | S_IWUGO, show_blue, store_blue);
Looks like store_red, store_green, and store_blue could be merged to de-duplicate some code. Same with show_*.
Did that.
static int blinkm_transfer_hw(struct i2c_client *client, int cmd) {
/* the protocol is simple but non-standard: * e.g. cmd 'g' (= 0x67) for "get device address" * - which defaults to 0x09 - would be the sequence: * a) write 0x67 to the device (byte write) * b) read the value (0x09) back right after (byte read) * * Watch out of "unfinished" sequences (i.e. not enough reads
It's "watch out for". :-)
Thanks for spotting this. [...]
switch (cmd) {
case BLM_GO_RGB: data->args[0] = data->red; data->args[1] = data->green; data->args[2] = data->blue; blinkm_write(client, cmd, data->args); break;
case BLM_FADE_RGB: data->args[0] = data->red; data->args[1] = data->green; data->args[2] = data->blue; blinkm_write(client, cmd, data->args); break;
case BLM_FADE_HSB: data->args[0] = data->hue; data->args[1] = data->saturation; data->args[2] = data->brightness; blinkm_write(client, cmd, data->args); break;
case BLM_FADE_RAND_RGB: data->args[0] = data->red; data->args[1] = data->green; data->args[2] = data->blue; blinkm_write(client, cmd, data->args); break;
case BLM_FADE_RAND_HSB: data->args[0] = data->hue; data->args[1] = data->saturation; data->args[2] = data->brightness; blinkm_write(client, cmd, data->args); break;
I would write the equivalent cases using fall-through to save space:
case BLM_GO_RGB: case BLM_FADE_RGB: case BLM_RAND_RGB: data->args[0] = data->red; data->args[1] = data->green; data->args[2] = data->blue; blinkm_write(client, cmd, data->args); break; case BLM_FADE_HSB: case BLM_FADE_RAND_HSB: data->args[0] = data->hue; data->args[1] = data->saturation; data->args[2] = data->brightness; blinkm_write(client, cmd, data->args); break;
case BLM_SET_STARTUP_PARAM: blinkm_write(client, cmd, data->args); break;
default: return -1;
Done. [...]
static void blinkm_led_green_set(struct led_classdev *led_cdev,...) [...] static void blinkm_led_blue_set(struct led_classdev *led_cdev,...) [...]
Code duplication again. (Or triplication :-D)
Fixed. [...]
data->i2c_addr = 0x09; data->red = 0x01; data->green = 0x01; data->blue = 0x01; data->hue = 0x01; data->saturation = 0x01; data->brightness = 0x01;
Why is it 1 instead of 0? (Just asking because it looks non-obvious)
Mainly testing purposes. 0x01 is the lowest brightness setting - so you don't get blind while checking if the driver really initialized all 3 leds.
data->fade = 0x01; data->rand = 0x00; data->fade_speed = 0x01; data->time_adjust = 0x01; data->i2c_addr = 0x08;
/* i2c addr - use fake addr of 0x08 initially (0x09)*/
What does the 0x09 in the parentheses mean?
That's the real and default address. Thanks for the review. Best, Jan-Simon