Driver for BlinkM i2c LED module
Jan-Simon Möller
dl9pf at gmx.de
Fri Jun 1 17:45:53 EDT 2012
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 at 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
More information about the Kernelnewbies
mailing list