Re: [PATCH] TSL2550 support (I2C device driver)

On Mon, Jan 29, 2007 at 06:39:50PM -0800, Andrew Morton wrote:

+/* Insmod parameters */
+I2C_CLIENT_INSMOD_1(tsl2550);
+
+static int operating_mode = 0;

The `= 0' is unneeded and undesirable.

Fixed.

...

+static int tsl2550_get_adc_value(struct i2c_client *client, int channel)
+{
+ u8 cmd = channel == 0 ? TSL2550_READ_ADC0 : TSL2550_READ_ADC1;
+ int timeout, ret;
+
+ /* Read ADC channel waiting at most 400ms (see data *** for further
+ * info) */
+ for (timeout = 400; timeout > 0; timeout--) {
+ i2c_smbus_write_byte(client, cmd);
+ mdelay(1);
+ ret = i2c_smbus_read_byte(client);
+ if (ret < 0)
+ return ret;
+ else if (ret & 0x0080)
+ break;
+ }
+ if (timeout == 0)
+ return -EIO;
+ return ((u8) ret) & 0x7f; /* remove the "valid" bit */
+}

eek. Is there no way to avoid the busy-wait? We cannot sleep here?

That's why I have to retry reading data for at most 400ms otherwise
the chip will start a new ADC conversion.

I can replace mdelay(1) with schedule_timeout(HZ/1000) but doing this
I'm not sure that just 1ms has elapesed. Also the chip has no irq
lines to use for that.

+/* --- LUX calculation ----------------------------------------------------- */
+
+#define TSL2550_MAX_LUX 1846
+
+static u8 ratio_lut[] = {
+ 100, 100, 100, 100, 100, 100, 100, 100,
[snip]
+
+static u16 count_lut[] = {
+ 0, 1, 2, 3, 4, 5, 6, 7,
[snip]

These tables could perhaps be const?

Fixed.

+static ssize_t tsl2550_show_power_state(struct device *dev, struct device_attribute *attr, char *buf)

It's preferred to try to fit the code into an 80-col window. (But some
people disagree with this specifically for function definitions such as
this).

I agree with them. :)

Preferred coding style is

err = i2c_detach_client(client);
if (err)
return err;

(multiple places)

Fixed.

I'm going to repost the patch after some tests.

Thanks,

Rodolfo

--

GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxxx
Embedded Systems giometti@xxxxxxxx
UNIX programming phone: +39 349 2432127
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/