Messages in this thread | | | Date | Wed, 10 Sep 2008 09:42:53 -0600 | From | Jonathan Corbet <> | Subject | Re: [PATCH 05/18] lirc driver for i2c-based IR receivers |
| |
> +config LIRC_I2C > + tristate "I2C Based IR Receivers" > + default n > + depends on LIRC_DEV > + help > + Driver for I2C-based IR receivers, such as those commonly > + found onboard Hauppauge PVR-150/250/350 video capture cards
This driver offers relatively few things to gripe about. But I'm not one to shrink from a challenge...
> +#include <linux/semaphore.h>
It looks like this driver is semaphore-free? Actually, it looks kind of lock-free in general. That's probably my biggest concern; what is protecting against concurrent operations on the device?
> +static int add_to_buf_pcf8574(void *data, struct lirc_buffer *buf) > +{ > + struct IR *ir = data; > + int rc; > + unsigned char all, mask; > + unsigned char key; > + > + /* compute all valid bits (key code + pressed/release flag) */ > + all = ir->bits | ir->flag; > + > + /* save IR writable mask bits */ > + mask = i2c_smbus_read_byte(&ir->c) & ~all; > + > + /* send bit mask */ > + rc = i2c_smbus_write_byte(&ir->c, (0xff & all) | mask); > + > + /* receive scan code */ > + rc = i2c_smbus_read_byte(&ir->c); > + > + if (rc == -1) { > + dprintk("%s read error\n", ir->c.name); > + return -EIO; > + }
This ignores the return value from the first i2c_smbus_read_byte() and i2c_smbus_write_byte() calls. Beyond that, i2c_smbus_read_byte() can return -EANYTHING, so I think the check should be "rc < 0", no? Tests against -1 are common in this driver, I'll not point them all out.
> +static int ir_attach(struct i2c_adapter *adap, int addr, > + unsigned short flags, int kind) > +{
...
> + ir->l.minor = lirc_register_plugin(&ir->l); > + > + return 0; > +}
The lirc_register_plugin() call can fail, the result should be checked.
> +#ifdef MODULE > + > +int init_module(void) > +{ > + request_module("bttv"); > + request_module("rivatv"); > + request_module("ivtv"); > + request_module("cx8800"); > + i2c_add_driver(&driver); > + return 0; > +}
Why are all these modules being loaded here? That would seem like a job for udev.
jon
| |