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 <[email protected]>"); > > 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 [email protected] http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
