Driver for BlinkM i2c LED module

[j.neuschaefer@gmx.net (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