rev2 - Driver for BlinkM i2c LED module

[Jan-Simon Möller]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: leds-blinkm.c
Type: text/x-csrc
Size: 19125 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20120602/19cb7da5/attachment.bin