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 sheet 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: [EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems [EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
