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