rev2 - Driver for BlinkM i2c LED module

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:
[...]

quoted

I'm also looking for the best place to fit this in.
Staging ? drivers/led ?

Have Phun!

I had fun reviewing the code. :-)

:D
 

quoted

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.

[...]

quoted

#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! ;).

 
[...]

quoted

 */

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.

quoted

	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.

[...]

quoted

	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.

 

quoted

static ssize_t show_blue(struct device *dev, struct device_attribute

[...]

quoted

static ssize_t store_blue(struct device *dev, struct device_attribute

[...]

quoted

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.

quoted

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.

[...]

quoted

	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;

quoted

	case BLM_SET_STARTUP_PARAM:
		blinkm_write(client, cmd, data->args);
		break;
	
	default:
		return -1;

Done.

[...]

quoted

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.

[...]

quoted

	
	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.

 

quoted

	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