[i2c] [patch 1/1] I2C: TSL2550 support

Jean Delvare khali at linux-fr.org
Sat Feb 17 21:35:33 CET 2007


Hi Rodolfo,

Sorry for the delay in reviewing your patch, I've been quite busy
lately. I see that Andrew Morton has been reviewing it already, so
hopefully it's near perfect now :)

On Fri, 16 Feb 2007 01:39:08 -0800, akpm at linux-foundation.org wrote:
> From: Rodolfo Giometti <giometti at enneenne.com>
> 
> Add support for Taos TSL2550 ambient light sensors.
> (http://www.taosinc.com/product_detail.asp?cateid=4&proid=18).
> 
> Signed-off-by: Rodolfo Giometti <giometti at enneenne.com>
> Signed-off-by: Andrew Morton <akpm at osdl.org>
> ---
> 
>  drivers/i2c/chips/Kconfig   |   10 
>  drivers/i2c/chips/Makefile  |    1 
>  drivers/i2c/chips/tsl2550.c |  457 ++++++++++++++++++++++++++++++++++
>  include/linux/i2c-id.h      |    1 
>  4 files changed, 469 insertions(+)
> 
> diff -puN drivers/i2c/chips/Kconfig~i2c-tsl2550-support drivers/i2c/chips/Kconfig
> --- a/drivers/i2c/chips/Kconfig~i2c-tsl2550-support
> +++ a/drivers/i2c/chips/Kconfig
> @@ -125,4 +125,14 @@ config SENSORS_MAX6875
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called max6875.
>  
> +config SENSORS_TSL2550
> +	tristate "Taos TSL2550 ambient light sensor"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Taos TSL2550
> +	  ambient light sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tsl2550.
> +
>  endmenu
> diff -puN drivers/i2c/chips/Makefile~i2c-tsl2550-support drivers/i2c/chips/Makefile
> --- a/drivers/i2c/chips/Makefile~i2c-tsl2550-support
> +++ a/drivers/i2c/chips/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_SENSORS_PCF8574)	+= pcf8574
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
>  obj-$(CONFIG_ISP1301_OMAP)	+= isp1301_omap.o
>  obj-$(CONFIG_TPS65010)		+= tps65010.o
> +obj-$(CONFIG_SENSORS_TSL2550)	+= tsl2550.o
>  
>  ifeq ($(CONFIG_I2C_DEBUG_CHIP),y)
>  EXTRA_CFLAGS += -DDEBUG
> diff -puN /dev/null drivers/i2c/chips/tsl2550.c
> --- /dev/null
> +++ a/drivers/i2c/chips/tsl2550.c
> @@ -0,0 +1,457 @@
> +/*
> + *  tsl2550.c - Linux kernel modules for ambient light sensor
> + *
> + *  Copyright (C) 2007 Rodolfo Giometti <giometti at linux.it>
> + *  Copyright (C) 2007 Eurotech S.p.A. <info at eurotech.it>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#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;
> +module_param(operating_mode, int, 0);
> +MODULE_PARM_DESC(operating_mode,
> +	"Operating mode: 0 = standard (default), 1 = extended");
> +
> +#define DRIVER_NAME     (tsl2550_driver.driver.name)
> +#define DRIVER_VERSION  "1.0.0"
> +
> +/* --- Defines ------------------------------------------------------------- */
> +
> +#define TSL2550_POWER_DOWN		0x00
> +#define TSL2550_POWER_UP		0x03
> +#define TSL2550_STANDARD_RANGE		0x18
> +#define TSL2550_EXTENDED_RANGE		0x1d
> +#define TSL2550_READ_ADC0		
> +#define TSL2550_READ_ADC1		0x83
> +
> +/* --- Structs ------------------------------------------------------------- */
> +
> +struct tsl2550_data {
> +	struct i2c_client client;
> +	struct mutex update_lock;
> +
> +	unsigned int power_state : 1;
> +	unsigned int operating_mode : 1;
> +};
> +
> +/* --- Management functions ------------------------------------------------ */
> +
> +static int tsl2550_set_operating_mode(struct i2c_client *client, int mode)
> +{
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +	int ret;
> +
> +	mode = !!mode;

This isn't the right place for this operation. You call this function
from 3 different places, and only once the data is user-provided and
needs to be checked. So you should move this operation to
tsl2550_store_operating_mode.

I would also suggest that you plain reject values which are not either
0 or 1, instead or considering anything non-zero to be 1. This makes it
possible to allow more values later if e.g. your driver supports more
chips with more features later.

> +	if (mode == data->operating_mode)
> +		return 0;

I don't think this is a good idea to do this. If the user is writing
something to the sysfs file, then you must write that value to the
chip. You shouldn't assume that your cached value is in sync with the
chip. In theory it is. It practice it might not be. In particular, at
the time you load the driver, you have no idea what the operating mode
of the chip is.

> +
> +	if (mode == 0)
> +		ret = i2c_smbus_write_byte(client, TSL2550_STANDARD_RANGE);
> +	else
> +		ret = i2c_smbus_write_byte(client, TSL2550_EXTENDED_RANGE);

Might be convenient to define a static array:
static const u8 TSL2550_RANGE[2] = { 0x18, 0x1d };

> +
> +	data->operating_mode = mode;
> +
> +	return ret;
> +}
> +
> +static int tsl2550_set_power_state(struct i2c_client *client, int state)
> +{
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +	int ret;
> +
> +	state = !!state;

Same as above, I'd at least move this operation to
tsl2550_store_power_state, and ideally change it to a rejection of
invalid values.

> +	if (state == data->power_state)
> +		return 0;

Same as above, I don't think this test is a good idea.

> +
> +	if (state == 0)
> +		ret = i2c_smbus_write_byte(client, TSL2550_POWER_DOWN);
> +	else {
> +		ret = i2c_smbus_write_byte(client, TSL2550_POWER_UP);

Here too a static array might be convenient.

> +
> +		/* On power up we should reset operating mode also... */
> +		tsl2550_set_operating_mode(client, data->operating_mode);
> +	}
> +
> +	data->power_state = state;
> +
> +	return ret;
> +}
> +
> +static int tsl2550_get_adc_value(struct i2c_client *client, int channel)
> +{
> +	u8 cmd = channel == 0 ? TSL2550_READ_ADC0 : TSL2550_READ_ADC1;

What about:
static const u8 TSL2550_READ_ADC[2] = { 0x43, 0x83 };

Another approach is to pass the command, rather than the channel, as
the second parameter of this function. That would be even more
efficient.

> +	unsigned long end;
> +	int loop = 0, ret = 0;
> +
> +	/* Read ADC channel waiting at most 400ms (see data sheet for further
> +         * info).

Broken indentation, you must use a tab instead of spaces.

> +	 * To avoid long busy wait we spin for few milliseconds then
> +	 * start sleeping. */
> +	end = jiffies + msecs_to_jiffies(400);
> +	while (time_before(jiffies, end)) {
> +		i2c_smbus_write_byte(client, cmd);
> +
> +		if (loop++ < 5)
> +			mdelay(1);
> +		else
> +			msleep(1);
> +
> +		ret = i2c_smbus_read_byte(client);
> +		if (ret < 0)
> +			return ret;
> +		else if (ret & 0x0080)
> +			break;
> +	}

I'm a bit surprised by this construct. My reading of the datasheet
differs significantly from your implementation.

If I read the datasheet properly, the only case where the reading might
not be right (bit 7 set) is right after power up, before the device has
the time to do the first integration. I think it's OK to immediately
return -EAGAIN or similar in this case, and let user-space try again
later. In all other cases the reading will be OK, as the device is
updating the registers continuously.

So I believe that the loop and delays/sleeps above are no needed and
could be dropped.

> +	if (!(ret & 0x0080))
> +		return -EIO;

It's a bit odd to use a (visually) 16-bit mask on an 8-bit value.

> +	return ((u8) ret) & 0x7f;	/* remove the "valid" bit */

The cast to u8 isn't needed.

> +}
> +
> +/* --- LUX calculation ----------------------------------------------------- */
> +
> +#define	TSL2550_MAX_LUX		1846

What about extended mode? My reading of the documentation is that this
max value applies to standard mode only. I expect the max to be 5 times greater for extended mode.

> +
> +static const 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 const 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
> +};

The TSL2550 datasheet explains how the values above are computed. The
count_lut table above can actually be shrunk to 8 values:

static const u16 count_lut[8] = {
	0, 16, 49, 115, 247, 511, 1039, 2095
};

Then the count can be computed from the channel value using the following formula:

chord = reg >> 4;
count = count_lut[chord] + (1 << chord) * (reg & 0x0f);

This would be much prefered over a 128-value look-up table. Actually
there's even a formula go get rid of the look-up table entirely, if you
prefer. I'm OK with an 8-value table though.

> +
> +/* This function is described into Taos TSL2550 Designer's Notebook
> + * pages 2, 3 */
> +static int tsl2550_calculate_lux(u8 ch0, u8 ch1)
> +{
> +	unsigned int lux;
> +
> +	/* Look up count from channel values */
> +	u16 c0 = count_lut[ch0];
> +	u16 c1 = count_lut[ch1];
> +
> +	/* Calculate ratio.
> +	 * Note: the "128" is a scaling factor */

I don't think this comment is particularly relevant.

> +	u8 r = 128;

You are not using this value, so why initialize it?

> +
> +	/* Avoid division by 0 and count 1 cannot be greater than count 0 */
> +	if (c0 && (c1 <= c0))
> +		r = c1 * 128 / c0;
> +	else
> +		return -1;

I don't think you want to return an error here. Returning 0 would make
more sense IMHO.

> +
> +	/* Calculare LUX */

Typo, I guess you meant "Calculate".

> +	lux = ((c0 - c1) * ratio_lut[r]) / 256;
> +
> +	/* LUX range check */
> +	return lux > TSL2550_MAX_LUX ? TSL2550_MAX_LUX : lux;

Please use min_t().

> +}
> +
> +/* --- SysFS support ------------------------------------------------------- */
> +
> +static ssize_t tsl2550_show_power_state(struct device *dev, struct device_attribute *attr, char *buf)

Please fold long lines.

> +{
> +	struct tsl2550_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return sprintf(buf, "%u\n", data->power_state);
> +}
> +
> +static ssize_t tsl2550_store_power_state(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +	unsigned long val = simple_strtoul(buf, NULL, 10);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = tsl2550_set_power_state(client, val);
> +	mutex_unlock(&data->update_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
> +		   tsl2550_show_power_state, tsl2550_store_power_state);
> +
> +static ssize_t tsl2550_show_operating_mode(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct tsl2550_data *data = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return sprintf(buf, "%u\n", data->operating_mode);
> +}
> +
> +static ssize_t tsl2550_store_operating_mode(struct device *dev, struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +	unsigned long val = simple_strtoul(buf, NULL, 10);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = tsl2550_set_operating_mode(client, val);
> +	mutex_unlock(&data->update_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(operating_mode, S_IWUSR | S_IRUGO,
> +		   tsl2550_show_operating_mode, tsl2550_store_operating_mode);
> +
> +static ssize_t __tsl2550_show_lux(struct i2c_client *client, struct device_attribute *attr, char *buf)
> +{
> +	u8 ch0, ch1;
> +	int ret;
> +
> +	ret = tsl2550_get_adc_value(client, 0);
> +	if (ret < 0)
> +		return ret;
> +	ch0 = (u8) ret;

Cast isn't needed.

> +
> +	mdelay(1);
> +
> +	ret = tsl2550_get_adc_value(client, 1);
> +	if (ret < 0)
> +		return ret;
> +	ch1 = (u8) ret;

Same here.

> +
> +	/* Do the job */
> +	ret = tsl2550_calculate_lux(ch0, ch1);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%u\n", ret);

%d

> +}
> +
> +static ssize_t tsl2550_show_lux(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +	int ret;
> +
> +	/* No LUX data if not operational */
> +	if (!data->power_state)
> +		return -EBUSY;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = __tsl2550_show_lux(client, attr, buf);
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR(lux, S_IRUGO,
> +		   tsl2550_show_lux, NULL);
> +
> +static struct attribute *tsl2550_attributes[] = {
> +	&dev_attr_power_state.attr,
> +	&dev_attr_operating_mode.attr,
> +	&dev_attr_lux.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group tsl2550_attr_group = {
> +	.attrs = tsl2550_attributes,
> +};
> +
> +/* --- Initialization function --------------------------------------------- */
> +
> +static void tsl2550_init_client(struct i2c_client *client)
> +{
> +	/* Power up the device and set the default operating mode */
> +	tsl2550_set_power_state(client, 1);
> +	tsl2550_set_operating_mode(client, operating_mode);
> +}
> +
> +/* --- I2C init/probing/exit functions ------------------------------------- */
> +
> +static struct i2c_driver tsl2550_driver;
> +static int tsl2550_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct i2c_client *new_client;
> +	struct tsl2550_data *data;
> +	int err = 0;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE
> +				     | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> +		goto exit;
> +
> +	/* OK. For now, we presume we have a valid client. We now create the
> +	   client structure, even though we cannot fill it completely yet. */
> +	data = kzalloc(sizeof(struct tsl2550_data), GFP_KERNEL);
> +	if (!data) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	new_client = &data->client;
> +	i2c_set_clientdata(new_client, data);
> +	new_client->addr = address;
> +	new_client->adapter = adapter;
> +	new_client->driver = &tsl2550_driver;
> +	new_client->flags = 0;
> +
> +	/* Probe the chip. To do so we try to power up the device and then to
> +	 * read back the 0x03 code */
> +	err = i2c_smbus_write_byte(new_client, TSL2550_POWER_UP);
> +	if (err < 0)
> +		goto exit_kfree;
> +	mdelay(1);
> +	err = i2c_smbus_read_byte(new_client);
> +	if (err != TSL2550_POWER_UP) {
> +		err = -ENODEV;
> +		goto exit_kfree;
> +	}
> +
> +	/* Determine the chip type - only one kind supported! */
> +	if (kind <= 0)
> +		kind = tsl2550;
> +
> +	/* Fill in the remaining client fields and put it into the global
> +	   list */
> +	strlcpy(new_client->name, "tsl2550", I2C_NAME_SIZE);
> +	mutex_init(&data->update_lock);
> +
> +	/* Tell the I2C layer a new client has arrived */
> +	err = i2c_attach_client(new_client);
> +	if (err)
> +		goto exit_kfree;
> +
> +	/* Initialize the TSL2550 chip */
> +	tsl2550_init_client(new_client);
> +
> +	/* Register sysfs hooks */
> +	err = sysfs_create_group(&new_client->dev.kobj, &tsl2550_attr_group);
> +	if (err)
> +		goto exit_detach;
> +
> +	pr_info("%s: TSL2550 I2C support enabled - ver. %s\n",
> +			DRIVER_NAME, DRIVER_VERSION);
> +	pr_info("%s: Copyright (C) 2007 Rodolfo Giometti <giometti at linux.it>\n",
> +			DRIVER_NAME);
> +
> +	return 0;
> +
> +exit_detach:
> +	i2c_detach_client(new_client);
> +exit_kfree:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int tsl2550_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	return i2c_probe(adapter, &addr_data, tsl2550_detect);
> +}
> +
> +static int tsl2550_detach_client(struct i2c_client *client)
> +{
> +	int err;
> +
> +	sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
> +
> +	err = i2c_detach_client(client);
> +	if (err)
> +		return err;
> +
> +	kfree(i2c_get_clientdata(client));
> +
> +	/* Power doen the device */
> +	tsl2550_set_power_state(client, 0);
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver tsl2550_driver = {
> +	.driver = {
> +		.name	= "tsl2550",
> +	},
> +	.id		= I2C_DRIVERID_TSL2550,
> +	.attach_adapter	= tsl2550_attach_adapter,
> +	.detach_client	= tsl2550_detach_client,
> +};
> +
> +static int __init tsl2550_init(void)
> +{
> +	if (operating_mode < 0 || operating_mode > 1) {
> +		printk(KERN_WARNING "tsl2550: invalid operating_mode (%d)\n",
> +		       operating_mode);
> +		return -EINVAL;
> +	}
> +
> +	return i2c_add_driver(&tsl2550_driver);
> +}
> +
> +static void __exit tsl2550_exit(void)
> +{
> +	i2c_del_driver(&tsl2550_driver);
> +}
> +
> +MODULE_AUTHOR("Rodolfo Giometti <giometti at linux.it>");
> +MODULE_DESCRIPTION("TSL2550 ambient light sensor driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(tsl2550_init);
> +module_exit(tsl2550_exit);
> diff -puN include/linux/i2c-id.h~i2c-tsl2550-support include/linux/i2c-id.h
> --- a/include/linux/i2c-id.h~i2c-tsl2550-support
> +++ a/include/linux/i2c-id.h
> @@ -159,6 +159,7 @@
>  #define I2C_DRIVERID_FSCHER 1046
>  #define I2C_DRIVERID_W83L785TS 1047
>  #define I2C_DRIVERID_OV7670 1048	/* Omnivision 7670 camera */
> +#define I2C_DRIVERID_TSL2550 1049
>  
>  /*
>   * ---- Adapter types ----------------------------------------------------
> _


-- 
Jean Delvare



More information about the i2c mailing list