Driver for BlinkM i2c LED module

Driver for BlinkM i2c LED module

[Jan-Simon Möller]

Hi all!

*drum roll*

This is the first version of the blinkM i2c led driver.

blinkM is an RGB led module which hooks up to an i2c bus.
See http://thingm.com/products/blinkm .

The protocol uses sequences of i2c commands to communicate with the tiny 
embedded controller.

This driver implements the needed bits to make the blinkM work as
LED device (accepting the triggers in sysfs) and also has a sysfs group for 
the more "advanced settings" exposed by the controller.
Of course not all advanced options are implemented yet ;).

Comments ?

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

Have Phun!

Best,
Jan-Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: blinkm.c
Type: text/x-csrc
Size: 21442 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20120601/6c990545/attachment-0001.bin 

Driver for BlinkM i2c LED module

[Jonathan Neuschäfer]

On Fri, Jun 01, 2012 at 03:45:59PM +0200, Jan-Simon M?ller wrote:
> Hi all!
> 
> *drum roll*
> 
> This is the first version of the blinkM i2c led driver.
> 
> blinkM is an RGB led module which hooks up to an i2c bus.
> See http://thingm.com/products/blinkm .
> 
> The protocol uses sequences of i2c commands to communicate with the tiny 
> embedded controller.
> 
> This driver implements the needed bits to make the blinkM work as
> LED device (accepting the triggers in sysfs) and also has a sysfs group for 
> the more "advanced settings" exposed by the controller.
> Of course not all advanced options are implemented yet ;).
> 
> Comments ?

Just some nitpicking. I don't have a device for testing.

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

I had fun reviewing the code. :-)

> 
> Best,
> Jan-Simon


> 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?

> 	u8 green;		/* c_g  -  color green */
> 	u8 blue;		/* c_b  -  color blue */
> /* internal use */
> 	u8 args[7];		/* set of args for transmission */
> 	u8 i2c_addr;		/* i2c addr */
> 	u8 fw_ver;		/* firmware version */
> /* used, but not from userspace */
> 	u8 hue;			/* c_h  -  HSB  hue */
> 	u8 saturation;		/* c_s  -  HSB  saturation */
> 	u8 brightness;		/* c_br -  HSB  brightness */
> /* currently unused / todo */
> 	u8 fade_speed;		/* fade speed     1 - 255 */
> 	s8 time_adjust;		/* time adjust -128 - 127 */
> 	u8 fade:1;		/* fade on = 1, off = 0 */
> 	u8 rand:1;		/* rand fade mode on = 1 */
> 	u8 script_id;		/* script ID */
> 	u8 script_repeats;	/* repeats of script */
> 	u8 script_startline;	/* line to start */
> };
> 

> #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?
What's the difference between write-read and read-write?

> 
> /* mapping command names to cmd chars - see datasheet */
> #define BLM_GO_RGB            0
> #define BLM_FADE_RGB          1
> #define BLM_FADE_HSB          2
> #define BLM_FADE_RAND_RGB     3
> #define BLM_FADE_RAND_HSB     4
> #define BLM_PLAY_SCRIPT       5
> #define BLM_STOP_SCRIPT       6
> #define BLM_SET_FADE_SPEED    7
> #define BLM_SET_TIME_ADJ      8
> #define BLM_GET_CUR_RGB       9
> #define BLM_WRITE_SCRIPT_LINE 10
> #define BLM_READ_SCRIPT_LINE  11
> #define BLM_SET_SCRIPT_LR     12	/* Length & Repeats */
> #define BLM_SET_ADDR          13
> #define BLM_GET_ADDR          14
> #define BLM_GET_FW_VER        15
> #define BLM_SET_STARTUP_PARAM 16
> 

> /* BlinkM Commands*/
> /* cmdchar = command (ascii)
>    cmdbyte = command in hex
>    nr_args = number of arguments to send
>    nr_ret  = number of return values
>    dir = direction (0 = read, 1 = write)

I think this is where you would use the BLM_DIR_* macros.

>  */
> 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.

> 	char cmdchar;
> 	u8 cmdbyte;

Cmdchar and cmdbyte seem to be the same (numerically) in the table.
Is that intended?

> 	u8 nr_args;
> 	u8 nr_ret;
> 	u8 dir:2;
> } blinkm_cmds[17] = {
> 	/* cmdchar, cmdbyte, nr_args, nr_ret,  dir */
> 	{
> 	0, 'n', 0x6e, 3, 0, 1}, {
> 	1, 'c', 0x63, 3, 0, 1}, {
> 	2, 'h', 0x68, 3, 0, 1}, {
> 	3, 'C', 0x43, 3, 0, 1}, {
> 	4, 'H', 0x48, 3, 0, 1}, {
> 	5, 'p', 0x70, 3, 0, 1}, {
> 	6, 'o', 0x6f, 0, 0, 1}, {
> 	7, 'f', 0x66, 1, 0, 1}, {
> 	8, 't', 0x74, 1, 0, 1}, {
> 	9, 'g', 0x67, 0, 3, 0}, {
> 	10, 'W', 0x57, 7, 0, 1}, {
> 	11, 'R', 0x52, 2, 5, 2}, {
> 	12, 'L', 0x4c, 3, 0, 1}, {
> 	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},
	/* ... */
};

> static ssize_t show_blue(struct device *dev, struct device_attribute *attr,
> 			 char *buf)
> {
> 	struct i2c_client *client;
> 	struct blinkm_data *data;
> 	int ret;
> 
> 	client = to_i2c_client(dev);
> 	data = i2c_get_clientdata(client);
> 
> 	ret = blinkm_transfer_hw(client, BLM_GET_CUR_RGB);
> 	if (ret < 0)
> 		return -1;
> 	return scnprintf(buf, PAGE_SIZE, "%02X\n", data->blue);
> }
> 
> static ssize_t store_blue(struct device *dev, struct device_attribute *attr,
> 			  const char *buf, size_t count)
> {
> 	struct i2c_client *client;
> 	struct blinkm_data *data;
> 	int ret;
> 	u8 value;
> 
> 	client = to_i2c_client(dev);
> 	data = i2c_get_clientdata(client);
> 
> 	ret = kstrtou8(buf, 10, &value);
> 	if (ret < 0) {
> 		dev_err(dev, "BlinkM: value too large!\n");
> 		return ret;
> 	}
> 	data->blue = value;
> 
> 	/* if mode ... (todo:fading ?) */
> 	ret = blinkm_transfer_hw(client, BLM_GO_RGB);
> 	if (ret < 0) {
> 		dev_err(dev, "BlinkM: can't set RGB\n");
> 		return ret;
> 	}
> 
> 	return count;
> }
> 
> 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_*.

> 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". :-)

> 	 * or writes after a command. It will make the blinkM misbehave.
> 	 * Sequence is key here.
> 	 */
> 
> 	/* args / return are in private data struct */
> 	struct blinkm_data *data = i2c_get_clientdata(client);
> 
> 	/* We start hardware transfers which are not to be
> 	 * mixed with other commands. Aquire a lock now. */
> 	if (mutex_lock_interruptible(&data->update_lock) < 0)
> 		return -EAGAIN;
> 
> 	/* switch cmd - usually write before reads */
> 	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;

You need to unlock the mutex.

> 	}			/* end switch(cmd) */
> 
> 	/* transfers done, unlock */
> 	mutex_unlock(&data->update_lock);
> 	return 0;
> }
> 
> static void led_work(struct work_struct *work)
> {
> 	int ret;
> 	struct blinkm_led *led;
> 	struct blinkm_work *blm_work = work_to_blmwork(work);
> 
> 	led = blm_work->blinkm_led;
> 	ret = blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
> 	atomic_dec(&led->active);
> 	kfree(blm_work);
> }
> 

> static void blinkm_led_red_set(struct led_classdev *led_cdev,
> 			       enum led_brightness value)
> {
> 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
> 	struct blinkm_led *led = cdev_to_blmled(led_cdev);
> 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
> 	struct blinkm_work *bl_work_r = kzalloc(sizeof(struct blinkm_work),
> 						GFP_ATOMIC);
> 
> 	switch (value) {
> 	case 0:
> 		data->red = 0;
> 		break;
> 	case 127:
> 		data->red = 0x88;
> 		break;
> 	case 255:
> 		data->red = 0xFF;
> 		break;
> 	default:
> 		data->red = 0;
> 	}
> /*      data->red=(u8)value;        we know it fits ... 0..255 */
> 	atomic_inc(&led->active);
> 
> 	bl_work_r->blinkm_led = led;
> 	INIT_WORK(&bl_work_r->work, led_work);
> 	schedule_work(&bl_work_r->work);
> }
> 
> 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)

> static int blinkm_probe(struct i2c_client *client,
> 			const struct i2c_device_id *id)
> {
> 	struct blinkm_data *data;
> 	struct blinkm_led *ledr;
> 	struct blinkm_led *ledg;
> 	struct blinkm_led *ledb;
> 	int err;
> 
> 	data = kzalloc(sizeof(struct blinkm_data), GFP_KERNEL);
> 	if (!data) {
> 		err = -ENOMEM;
> 		goto exit;
> 	}
> 
> 	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)

> 	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?

> static int blinkm_remove(struct i2c_client *client)
> {
> 	struct blinkm_data *data = i2c_get_clientdata(client);
> 	int ret = 0;
> 	int maxcount;
> 	int i;
> 
> 	for (i = 0; i < 3; i++) {
> 		maxcount=99;
> 		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
> 		while (atomic_read(&data->blinkm_leds[i].active) > 0){
> 			if (maxcount == 0)
> 			    break;
> 			msleep(100);
> 			maxcount--;
> 		}
> 	}
> 
> 	/* reset rgb */
> 	data->red = 0x05;
> 	data->green = 0x05;
> 	data->blue = 0x05;

Why is it 0x05?

> 	ret = blinkm_transfer_hw(client, BLM_FADE_RGB);
> 	if (ret < 0)
> 		printk(KERN_INFO
> 		       "Failure in blinkm_remove ignored. Continuing.\n");
> 
> 	/* reset hsb */
> 	data->hue = 0x00;
> 	data->saturation = 0x00;
> 	data->brightness = 0x00;
> 	ret = blinkm_transfer_hw(client, BLM_FADE_HSB);
> 	if (ret < 0)
> 		printk(KERN_INFO
> 		       "Failure in blinkm_remove ignored. Continuing.\n");
> 
> 	/* red fade to off */
> 	data->red = 0xff;
> 	ret = blinkm_transfer_hw(client, BLM_GO_RGB);
> 	if (ret < 0)
> 		printk(KERN_INFO
> 		       "Failure in blinkm_remove ignored. Continuing.\n");
> 
> 	/* off */
> 	data->red = 0x00;
> 	data->green = 0x00;
> 	data->blue = 0x00;
> 	ret = blinkm_transfer_hw(client, BLM_FADE_RGB);
> 	if (ret < 0)
> 		printk(KERN_INFO
> 		       "Failure in blinkm_remove ignored. Continuing.\n");
> 
> 	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> 	kfree(data);
> 	return 0;
> }
> 
> static const struct i2c_device_id blinkm_id[] = {
> 	{"blinkm", 0},
> 	{}
> };
> 
> MODULE_DEVICE_TABLE(i2c, blinkm_id);
> 
> /* This is the driver that will be inserted */
> static struct i2c_driver blinkm_driver = {
> 	.class = I2C_CLASS_HWMON,
> 	.driver = {
> 		   .name = "blinkm",
> 		   },
> 	.probe = blinkm_probe,
> 	.remove = blinkm_remove,
> 	.id_table = blinkm_id,
> 	.detect = blinkm_detect,
> 	.address_list = normal_i2c,
> };
> 
> static int __init blinkm_init(void)
> {
> 	return i2c_add_driver(&blinkm_driver);
> }
> 
> static void __exit blinkm_exit(void)
> {
> 	i2c_del_driver(&blinkm_driver);
> }
> 
> MODULE_AUTHOR("Jan-Simon Moeller <dl9pf@gmx.de>");
> MODULE_DESCRIPTION("BlinkM");

I'd call it "BlinkM LED driver" or something, "BlinkM" alone isn't
really descriptive.

> MODULE_LICENSE("GPL");
> 
> module_init(blinkm_init);
> module_exit(blinkm_exit);


Thanks,
	Jonathan Neusch?fer

Driver for BlinkM i2c LED module

[Jan-Simon Möller]

Thanks for the review, Jonathan.
I'll do a rev2 asap.

Best,
JS

Am Freitag, Juni 01, 2012, 05:16:12 PM schrieb Jonathan Neusch?fer:
> On Fri, Jun 01, 2012 at 03:45:59PM +0200, Jan-Simon M?ller wrote:
> > Hi all!
> > 
> > *drum roll*
> > 
> > This is the first version of the blinkM i2c led driver.
> > 
> > blinkM is an RGB led module which hooks up to an i2c bus.
> > See http://thingm.com/products/blinkm .
> > 
> > The protocol uses sequences of i2c commands to communicate with the tiny
> > embedded controller.
> > 
> > This driver implements the needed bits to make the blinkM work as
> > LED device (accepting the triggers in sysfs) and also has a sysfs group
> > for the more "advanced settings" exposed by the controller.
> > Of course not all advanced options are implemented yet ;).
> > 
> > Comments ?
> 
> Just some nitpicking. I don't have a device for testing.
> 
> > I'm also looking for the best place to fit this in.
> > Staging ? drivers/led ?
> > 
> > Have Phun!
> 
> I had fun reviewing the code. :-)
> 
> > Best,
> > Jan-Simon
> > 
> > 
> > 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?
> 
> > 	u8 green;		/* c_g  -  color green */
> > 	u8 blue;		/* c_b  -  color blue */
> > 
> > /* internal use */
> > 
> > 	u8 args[7];		/* set of args for transmission */
> > 	u8 i2c_addr;		/* i2c addr */
> > 	u8 fw_ver;		/* firmware version */
> > 
> > /* used, but not from userspace */
> > 
> > 	u8 hue;			/* c_h  -  HSB  hue */
> > 	u8 saturation;		/* c_s  -  HSB  saturation */
> > 	u8 brightness;		/* c_br -  HSB  brightness */
> > 
> > /* currently unused / todo */
> > 
> > 	u8 fade_speed;		/* fade speed     1 - 255 */
> > 	s8 time_adjust;		/* time adjust -128 - 127 */
> > 	u8 fade:1;		/* fade on = 1, off = 0 */
> > 	u8 rand:1;		/* rand fade mode on = 1 */
> > 	u8 script_id;		/* script ID */
> > 	u8 script_repeats;	/* repeats of script */
> > 	u8 script_startline;	/* line to start */
> > 
> > };
> > 
> > 
> > #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?
> What's the difference between write-read and read-write?
> 
> > /* mapping command names to cmd chars - see datasheet */
> > #define BLM_GO_RGB            0
> > #define BLM_FADE_RGB          1
> > #define BLM_FADE_HSB          2
> > #define BLM_FADE_RAND_RGB     3
> > #define BLM_FADE_RAND_HSB     4
> > #define BLM_PLAY_SCRIPT       5
> > #define BLM_STOP_SCRIPT       6
> > #define BLM_SET_FADE_SPEED    7
> > #define BLM_SET_TIME_ADJ      8
> > #define BLM_GET_CUR_RGB       9
> > #define BLM_WRITE_SCRIPT_LINE 10
> > #define BLM_READ_SCRIPT_LINE  11
> > #define BLM_SET_SCRIPT_LR     12	/* Length & Repeats */
> > #define BLM_SET_ADDR          13
> > #define BLM_GET_ADDR          14
> > #define BLM_GET_FW_VER        15
> > #define BLM_SET_STARTUP_PARAM 16
> > 
> > 
> > /* BlinkM Commands*/
> > /* cmdchar = command (ascii)
> > 
> >    cmdbyte = command in hex
> >    nr_args = number of arguments to send
> >    nr_ret  = number of return values
> >    dir = direction (0 = read, 1 = write)
> 
> I think this is where you would use the BLM_DIR_* macros.
> 
> >  */
> > 
> > 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.
> 
> > 	char cmdchar;
> > 	u8 cmdbyte;
> 
> Cmdchar and cmdbyte seem to be the same (numerically) in the table.
> Is that intended?
> 
> > 	u8 nr_args;
> > 	u8 nr_ret;
> > 	u8 dir:2;
> > 
> > } blinkm_cmds[17] = {
> > 
> > 	/* cmdchar, cmdbyte, nr_args, nr_ret,  dir */
> > 	{
> > 	0, 'n', 0x6e, 3, 0, 1}, {
> > 	1, 'c', 0x63, 3, 0, 1}, {
> > 	2, 'h', 0x68, 3, 0, 1}, {
> > 	3, 'C', 0x43, 3, 0, 1}, {
> > 	4, 'H', 0x48, 3, 0, 1}, {
> > 	5, 'p', 0x70, 3, 0, 1}, {
> > 	6, 'o', 0x6f, 0, 0, 1}, {
> > 	7, 'f', 0x66, 1, 0, 1}, {
> > 	8, 't', 0x74, 1, 0, 1}, {
> > 	9, 'g', 0x67, 0, 3, 0}, {
> > 	10, 'W', 0x57, 7, 0, 1}, {
> > 	11, 'R', 0x52, 2, 5, 2}, {
> > 	12, 'L', 0x4c, 3, 0, 1}, {
> > 	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},
> 	/* ... */
> };
> 
> > static ssize_t show_blue(struct device *dev, struct device_attribute
> > *attr,
> > 
> > 			 char *buf)
> > 
> > {
> > 
> > 	struct i2c_client *client;
> > 	struct blinkm_data *data;
> > 	int ret;
> > 	
> > 	client = to_i2c_client(dev);
> > 	data = i2c_get_clientdata(client);
> > 	
> > 	ret = blinkm_transfer_hw(client, BLM_GET_CUR_RGB);
> > 	if (ret < 0)
> > 	
> > 		return -1;
> > 	
> > 	return scnprintf(buf, PAGE_SIZE, "%02X\n", data->blue);
> > 
> > }
> > 
> > static ssize_t store_blue(struct device *dev, struct device_attribute
> > *attr,
> > 
> > 			  const char *buf, size_t count)
> > 
> > {
> > 
> > 	struct i2c_client *client;
> > 	struct blinkm_data *data;
> > 	int ret;
> > 	u8 value;
> > 	
> > 	client = to_i2c_client(dev);
> > 	data = i2c_get_clientdata(client);
> > 	
> > 	ret = kstrtou8(buf, 10, &value);
> > 	if (ret < 0) {
> > 	
> > 		dev_err(dev, "BlinkM: value too large!\n");
> > 		return ret;
> > 	
> > 	}
> > 	data->blue = value;
> > 	
> > 	/* if mode ... (todo:fading ?) */
> > 	ret = blinkm_transfer_hw(client, BLM_GO_RGB);
> > 	if (ret < 0) {
> > 	
> > 		dev_err(dev, "BlinkM: can't set RGB\n");
> > 		return ret;
> > 	
> > 	}
> > 	
> > 	return count;
> > 
> > }
> > 
> > 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_*.
> 
> > 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". :-)
> 
> > 	 * or writes after a command. It will make the blinkM misbehave.
> > 	 * Sequence is key here.
> > 	 */
> > 	
> > 	/* args / return are in private data struct */
> > 	struct blinkm_data *data = i2c_get_clientdata(client);
> > 	
> > 	/* We start hardware transfers which are not to be
> > 	
> > 	 * mixed with other commands. Aquire a lock now. */
> > 	
> > 	if (mutex_lock_interruptible(&data->update_lock) < 0)
> > 	
> > 		return -EAGAIN;
> > 	
> > 	/* switch cmd - usually write before reads */
> > 	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;
> 
> You need to unlock the mutex.
> 
> > 	}			/* end switch(cmd) */
> > 	
> > 	/* transfers done, unlock */
> > 	mutex_unlock(&data->update_lock);
> > 	return 0;
> > 
> > }
> > 
> > static void led_work(struct work_struct *work)
> > {
> > 
> > 	int ret;
> > 	struct blinkm_led *led;
> > 	struct blinkm_work *blm_work = work_to_blmwork(work);
> > 	
> > 	led = blm_work->blinkm_led;
> > 	ret = blinkm_transfer_hw(led->i2c_client, BLM_GO_RGB);
> > 	atomic_dec(&led->active);
> > 	kfree(blm_work);
> > 
> > }
> > 
> > 
> > static void blinkm_led_red_set(struct led_classdev *led_cdev,
> > 
> > 			       enum led_brightness value)
> > 
> > {
> > 
> > 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
> > 	struct blinkm_led *led = cdev_to_blmled(led_cdev);
> > 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
> > 	struct blinkm_work *bl_work_r = kzalloc(sizeof(struct blinkm_work),
> > 	
> > 						GFP_ATOMIC);
> > 	
> > 	switch (value) {
> > 	
> > 	case 0:
> > 		data->red = 0;
> > 		break;
> > 	
> > 	case 127:
> > 		data->red = 0x88;
> > 		break;
> > 	
> > 	case 255:
> > 		data->red = 0xFF;
> > 		break;
> > 	
> > 	default:
> > 		data->red = 0;
> > 	
> > 	}
> > 
> > /*      data->red=(u8)value;        we know it fits ... 0..255 */
> > 
> > 	atomic_inc(&led->active);
> > 	
> > 	bl_work_r->blinkm_led = led;
> > 	INIT_WORK(&bl_work_r->work, led_work);
> > 	schedule_work(&bl_work_r->work);
> > 
> > }
> > 
> > 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)
> 
> > static int blinkm_probe(struct i2c_client *client,
> > 
> > 			const struct i2c_device_id *id)
> > 
> > {
> > 
> > 	struct blinkm_data *data;
> > 	struct blinkm_led *ledr;
> > 	struct blinkm_led *ledg;
> > 	struct blinkm_led *ledb;
> > 	int err;
> > 	
> > 	data = kzalloc(sizeof(struct blinkm_data), GFP_KERNEL);
> > 	if (!data) {
> > 	
> > 		err = -ENOMEM;
> > 		goto exit;
> > 	
> > 	}
> > 	
> > 	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)
> 
> > 	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?
> 
> > static int blinkm_remove(struct i2c_client *client)
> > {
> > 
> > 	struct blinkm_data *data = i2c_get_clientdata(client);
> > 	int ret = 0;
> > 	int maxcount;
> > 	int i;
> > 	
> > 	for (i = 0; i < 3; i++) {
> > 	
> > 		maxcount=99;
> > 		led_classdev_unregister(&data->blinkm_leds[i].led_cdev);
> > 		while (atomic_read(&data->blinkm_leds[i].active) > 0){
> > 		
> > 			if (maxcount == 0)
> > 			
> > 			    break;
> > 			
> > 			msleep(100);
> > 			maxcount--;
> > 		
> > 		}
> > 	
> > 	}
> > 	
> > 	/* reset rgb */
> > 	data->red = 0x05;
> > 	data->green = 0x05;
> > 	data->blue = 0x05;
> 
> Why is it 0x05?
> 
> > 	ret = blinkm_transfer_hw(client, BLM_FADE_RGB);
> > 	if (ret < 0)
> > 	
> > 		printk(KERN_INFO
> > 		
> > 		       "Failure in blinkm_remove ignored. Continuing.\n");
> > 	
> > 	/* reset hsb */
> > 	data->hue = 0x00;
> > 	data->saturation = 0x00;
> > 	data->brightness = 0x00;
> > 	ret = blinkm_transfer_hw(client, BLM_FADE_HSB);
> > 	if (ret < 0)
> > 	
> > 		printk(KERN_INFO
> > 		
> > 		       "Failure in blinkm_remove ignored. Continuing.\n");
> > 	
> > 	/* red fade to off */
> > 	data->red = 0xff;
> > 	ret = blinkm_transfer_hw(client, BLM_GO_RGB);
> > 	if (ret < 0)
> > 	
> > 		printk(KERN_INFO
> > 		
> > 		       "Failure in blinkm_remove ignored. Continuing.\n");
> > 	
> > 	/* off */
> > 	data->red = 0x00;
> > 	data->green = 0x00;
> > 	data->blue = 0x00;
> > 	ret = blinkm_transfer_hw(client, BLM_FADE_RGB);
> > 	if (ret < 0)
> > 	
> > 		printk(KERN_INFO
> > 		
> > 		       "Failure in blinkm_remove ignored. Continuing.\n");
> > 	
> > 	sysfs_remove_group(&client->dev.kobj, &blinkm_group);
> > 	kfree(data);
> > 	return 0;
> > 
> > }
> > 
> > static const struct i2c_device_id blinkm_id[] = {
> > 
> > 	{"blinkm", 0},
> > 	{}
> > 
> > };
> > 
> > MODULE_DEVICE_TABLE(i2c, blinkm_id);
> > 
> > /* This is the driver that will be inserted */
> > static struct i2c_driver blinkm_driver = {
> > 
> > 	.class = I2C_CLASS_HWMON,
> > 	.driver = {
> > 	
> > 		   .name = "blinkm",
> > 		   },
> > 	
> > 	.probe = blinkm_probe,
> > 	.remove = blinkm_remove,
> > 	.id_table = blinkm_id,
> > 	.detect = blinkm_detect,
> > 	.address_list = normal_i2c,
> > 
> > };
> > 
> > static int __init blinkm_init(void)
> > {
> > 
> > 	return i2c_add_driver(&blinkm_driver);
> > 
> > }
> > 
> > static void __exit blinkm_exit(void)
> > {
> > 
> > 	i2c_del_driver(&blinkm_driver);
> > 
> > }
> > 
> > MODULE_AUTHOR("Jan-Simon Moeller <dl9pf@gmx.de>");
> > MODULE_DESCRIPTION("BlinkM");
> 
> I'd call it "BlinkM LED driver" or something, "BlinkM" alone isn't
> really descriptive.
> 
> > MODULE_LICENSE("GPL");
> > 
> > module_init(blinkm_init);
> > module_exit(blinkm_exit);
> 
> Thanks,
> 	Jonathan Neusch?fer

Driver for BlinkM i2c LED module

[Jonathan Neuschäfer]

On Fri, Jun 01, 2012 at 11:45:53PM +0200, Jan-Simon M?ller wrote:
> Thanks for the review, Jonathan.
> I'll do a rev2 asap.

But please note that some of my "why" questions where simply questions
and no actual suggestions, so take my word with a grain of salt!   :-)

Jonathan

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 

rev2 - Driver for BlinkM i2c LED module

[Jonathan Neuschäfer]

On Sat, Jun 02, 2012 at 11:29:46AM +0200, Jan-Simon M?ller wrote:
> static const struct {
> 	char cmdchar;
> 	u8 cmdbyte;
> 	u8 nr_args;
> 	u8 nr_ret;
> 	u8 dir:2;
> } blinkm_cmds[17] = {
> 	/* cmdnr, cmdchar, cmdbyte, nr_args, nr_ret,  dir */

cmdnr isn't there anymore.

> 	/* We start hardware transfers which are not to be
> 	 * mixed with other commands. Aquire a lock now. */
> 	if (mutex_lock_interruptible(&data->update_lock) < 0)
> 		return -EAGAIN;
> 
> 	/* switch cmd - usually write before reads */
> 	switch (cmd) {
[...]
> 	default:
> 		printk(KERN_INFO "BlinkM: unknown command %d\n", cmd);
> 		return -1;

You need to unlock the mutex in the error case, too.

Also the -1 looks suspicious to me. When interpreted as -errno it would
be -EPERM on most (or even all) architectures.  (according to
include/asm-generic/errno-base.h)

> static int blinkm_led_common_set(struct led_classdev *led_cdev,
> 			       enum led_brightness value, int color)
> {
> 	/* led_brightness is 0, 127 or 255 - we just use it here as-is */
> 	struct blinkm_led *led = cdev_to_blmled(led_cdev);
> 	struct blinkm_data *data = i2c_get_clientdata(led->i2c_client);
> 	struct blinkm_work *bl_work = kzalloc(sizeof(struct blinkm_work),
> 						GFP_ATOMIC);
> 
> 	switch (color) {
> 	case RED:
> 		data->red = (u8)value;
> 		break;
> 	case GREEN:
> 		data->green = (u8)value;
> 		break;
> 	case BLUE:
> 		data->blue = (u8)value;
> 		break;
> 	default:
> 		printk(KERN_INFO "BlinkM: unknown color.\n");
> 		return -1;
> 	}
> /*      data->red=(u8)value;        we know it fits ... 0..255 */

This comment looks a little misplaced now.

> 	/* red fade to off */
> 	data->red = 0xff;
> 	ret = blinkm_transfer_hw(client, BLM_GO_RGB);
> 	if (ret < 0)
> 		printk(KERN_INFO
> 		       "Failure in blinkm_remove ignored. Continuing.\n");
> 
> 	/* off */
> 	data->red = 0x00;
> 	data->green = 0x00;
> 	data->blue = 0x00;
> 	ret = blinkm_transfer_hw(client, BLM_FADE_RGB);
> 	if (ret < 0)
> 		printk(KERN_INFO
> 		       "Failure in blinkm_remove ignored. Continuing.\n");

Did you leave this fading in for testing?


Thanks,
	Jonathan Neusch?fer

rev2 - Driver for BlinkM i2c LED module

[Jan-Simon Möller]

New version attched.

Am Sonntag, Juni 03, 2012, 07:32:08 PM schrieb Jonathan Neusch?fer:
> On Sat, Jun 02, 2012 at 11:29:46AM +0200, Jan-Simon M?ller wrote:
> > static const struct {
> > 
> > 	char cmdchar;
> > 	u8 cmdbyte;
> > 	u8 nr_args;
> > 	u8 nr_ret;
> > 	u8 dir:2;
> > 
> > } blinkm_cmds[17] = {
> > 
> > 	/* cmdnr, cmdchar, cmdbyte, nr_args, nr_ret,  dir */
> 
> cmdnr isn't there anymore.

Yep, tnx.

> > 	/* We start hardware transfers which are not to be
> > 	
> > 	 * mixed with other commands. Aquire a lock now. */
> > 	
> > 	if (mutex_lock_interruptible(&data->update_lock) < 0)
> > 	
> > 		return -EAGAIN;
> > 	
> > 	/* switch cmd - usually write before reads */
> > 	switch (cmd) {
> 
> [...]
> 
> > 	default:
> > 		printk(KERN_INFO "BlinkM: unknown command %d\n", cmd);
> > 		return -1;
> 
> You need to unlock the mutex in the error case, too.
> 
> Also the -1 looks suspicious to me. When interpreted as -errno it would
> be -EPERM on most (or even all) architectures.  (according to
> include/asm-generic/errno-base.h)

Thanks for spotting this. Fixed.

[...]

> > 	/* red fade to off */
> > 	data->red = 0xff;
> > 	ret = blinkm_transfer_hw(client, BLM_GO_RGB);
> > 	if (ret < 0)
> > 	
> > 		printk(KERN_INFO
> > 		
> > 		       "Failure in blinkm_remove ignored. Continuing.\n");
> > 	
> > 	/* off */
> > 	data->red = 0x00;
> > 	data->green = 0x00;
> > 	data->blue = 0x00;
> > 	ret = blinkm_transfer_hw(client, BLM_FADE_RGB);
> > 	if (ret < 0)
> > 	
> > 		printk(KERN_INFO
> > 		
> > 		       "Failure in blinkm_remove ignored. Continuing.\n");
> 
> Did you leave this fading in for testing?
That is in by intention.


Best,
Jan-Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-driver-for-BlinkM-device-to-drivers-leds.patch
Type: text/x-patch
Size: 22767 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20120603/1cc20080/attachment-0001.bin