lkml.org 
[lkml]   [2008]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-09-10 17:45    [W:0.254 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site