Re: rev2 - Driver for BlinkM i2c LED module

June 3, 2012


      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