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@xxxxxx>"); > 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 _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
