Re: Driver for BlinkM i2c LED module

June 1, 2012


      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