Hello,
attached a patch to add support for Taos TSL2550 ambient light sensors
(http://www.taosinc.com/product_detail.asp?cateid=4&proid=18).
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Tue, 30 Jan 2007 00:56:19 +0100
Rodolfo Giometti <[email protected]> wrote:
> Hello,
>
> attached a patch to add support for Taos TSL2550 ambient light sensors
> (http://www.taosinc.com/product_detail.asp?cateid=4&proid=18).
>
Some minor things:
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +
> +/* Address to scan */
> +static unsigned short normal_i2c[] = { 0x39, I2C_CLIENT_END };
> +
> +/* Insmod parameters */
> +I2C_CLIENT_INSMOD_1(tsl2550);
> +
> +static int operating_mode = 0;
The `= 0' is unneeded and undesirable.
> ...
>
> +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?
> +/* --- LUX calculation ----------------------------------------------------- */
> +
> +#define TSL2550_MAX_LUX 1846
> +
> +static u8 ratio_lut[] = {
> + 100, 100, 100, 100, 100, 100, 100, 100,
> + 100, 100, 100, 100, 100, 100, 99, 99,
> + 99, 99, 99, 99, 99, 99, 99, 99,
> + 99, 99, 99, 98, 98, 98, 98, 98,
> + 98, 98, 97, 97, 97, 97, 97, 96,
> + 96, 96, 96, 95, 95, 95, 94, 94,
> + 93, 93, 93, 92, 92, 91, 91, 90,
> + 89, 89, 88, 87, 87, 86, 85, 84,
> + 83, 82, 81, 80, 79, 78, 77, 75,
> + 74, 73, 71, 69, 68, 66, 64, 62,
> + 60, 58, 56, 54, 52, 49, 47, 44,
> + 42, 41, 40, 40, 39, 39, 38, 38,
> + 37, 37, 37, 36, 36, 36, 35, 35,
> + 35, 35, 34, 34, 34, 34, 33, 33,
> + 33, 33, 32, 32, 32, 32, 32, 31,
> + 31, 31, 31, 31, 30, 30, 30, 30,
> + 30,
> +};
> +
> +static u16 count_lut[] = {
> + 0, 1, 2, 3, 4, 5, 6, 7,
> + 8, 9, 10, 11, 12, 13, 14, 15,
> + 16, 18, 20, 22, 24, 26, 28, 30,
> + 32, 34, 36, 38, 40, 42, 44, 46,
> + 49, 53, 57, 61, 65, 69, 73, 77,
> + 81, 85, 89, 93, 97, 101, 105, 109,
> + 115, 123, 131, 139, 147, 155, 163, 171,
> + 179, 187, 195, 203, 211, 219, 227, 235,
> + 247, 263, 279, 295, 311, 327, 343, 359,
> + 375, 391, 407, 423, 439, 455, 471, 487,
> + 511, 543, 575, 607, 639, 671, 703, 735,
> + 767, 799, 831, 863, 895, 927, 959, 991,
> + 1039, 1103, 1167, 1231, 1295, 1359, 1423, 1487,
> + 1551, 1615, 1679, 1743, 1807, 1871, 1935, 1999,
> + 2095, 2223, 2351, 2479, 2607, 2735, 2863, 2991,
> + 3119, 3247, 3375, 3503, 3631, 3759, 3887, 4015
> +};
These tables could perhaps be const?
> +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).
> +static ssize_t tsl2550_store_power_state(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
Ditto (and other places).
> +static int tsl2550_detach_client(struct i2c_client *client)
> +{
> + int err;
> +
> + sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
> +
> + if ((err = i2c_detach_client(client)))
> + return err;
Preferred coding style is
err = i2c_detach_client(client);
if (err)
return err;
(multiple places)
On Mon, 29 Jan 2007 18:39:50 -0800
Andrew Morton <[email protected]> wrote:
> > attached a patch to add support for Taos TSL2550 ambient light sensors
> > (http://www.taosinc.com/product_detail.asp?cateid=4&proid=18).
> >
>
> Some minor things:
Oh, and please send a signoff for this work as per section 11 of
Documentation/SubmittingPatches, thanks.
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
On Tue, 30 Jan 2007 11:26:42 +0100
Rodolfo Giometti <[email protected]> wrote:
> > > +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.
I expect an msleep(1) will be OK, if you can indeed sleep on this
codepath. Yes, that can delay for longer: ten milliseconds for sure
if HZ=100.
If that's a problem I guess you could spin for a millisecond or two, then
start sleeping:
end = jiffies + msecs_to_jiffies(400);
while (time_before(jiffies, end)) {
loop++;
...
if (loop < 5)
mdelay(1);
else
msleep(1);
}
or something like that.
From: Rodolfo Giometti <[email protected]>
Patch to add support for Taos TSL2550 ambient light sensors.
Signed-off-by: Rodolfo Giometti <[email protected]>
---