[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20080910094253.73a03ab3@bike.lwn.net>
Date: Wed, 10 Sep 2008 09:42:53 -0600
From: Jonathan Corbet <corbet@....net>
To: Jarod Wilson <jwilson@...hat.com>
Cc: linux-kernel@...r.kernel.org, Janne Grunau <j@...nua.net>,
Christoph Bartelmus <lirc@...telmus.de>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists