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
