Date:	Tue, 10 Mar 2009 17:49:55 +0000
From:	Jonathan Cameron <jic23@....ac.uk>
To:	Daniel Mack <daniel@...aq.de>
CC:	Jean Delvare <khali@...ux-fr.org>, linux-i2c@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Added driver for ISL29003 ambient light sensor

Daniel Mack wrote:
> Hi Jonathan,
> 
> many thanks for your review!
You are welcome.
> 
> On Tue, Mar 10, 2009 at 01:11:21PM +0000, Jonathan Cameron wrote:
>> I notice that this device is extremely similar to the ISL29004 where only
>> differences being with i2c address selection (that one has some pins to
>> allow more than one option).  Worth rolling support for that device in
>> here?
> 
> Well, the I2C address isn't coded in the driver but defined in the
> i2c_board_info array of the board support file, so there is no reason
> why it shouldn't work. Or are you talking about the names of files and
> functions that you would like to see reflecting this?
function names and filenames are fine (typically you just pick a random device
that the driver supports and name everything after that).
I think the only addition would be documentation of the patch and and id in
the id table.
> 
>> This device has some interesting interrupt / timing options which will fit
>> nicely in the IIO framework.  For now the driver sensibly ignores them
>> entirely. (if you don't need them, why bother?)
> 
> Right, I don't need them personally, and things are not connected to
> the CPU, so I can't test that. I would expect anyone who needs this
> functions to add support to the code :)
> 
>>> +The ISL29003 does not have an ID register which could be used to identify
>>> +it, so the detection routine will just try to read from the configured I2C
>>> +addess and consider the device to be present as soon as it ACKs the
>>> +transfer.
>> This is a little nasty given the chances of something else sitting on that
>> address, but not much else you can do.
> 
> True.
> 
>> Some of the following are acting only as documentation. It's
>> a matter of personal preference whether you specify them.
> 
> I defined them to make future feature additions easy ...
> 
>>> +#define ISL29003_REG_COMMAND		0x00
>>> +#define ISL29003_ADC_ENABLED		(1 << 7)
>>> +#define ISL29003_ADC_PD			(1 << 6)
>>> +#define ISL29003_TIMING_INT		(1 << 5)
>>> +#define ISL29003_MODE_SHIFT		(2)
>>> +#define ISL29003_MODE_MASK		(0x3 << ISL29003_MODE_SHIFT)
>>> +#define ISL29003_RES_SHIFT		(0)
>>> +#define ISL29003_RES_MASK		(0x3 << ISL29003_RES_SHIFT)
> 
> [...]
> 
>>> +static int __isl29003_write_reg(struct i2c_client *client,
>>> +				u32 reg, u8 mask, u8 shift, u8 val)
>>> +{
>>> +	struct isl29003_data *data = i2c_get_clientdata(client);
>>> +	int ret = 0;
>>> +	u8 tmp;
>>> +
>>> +	mutex_lock(&data->lock);
>>> +
>>> +	tmp = data->reg_cache[reg];
>>> +	tmp &= ~mask;
>>> +	tmp |= val << shift;
>>> +
>>> +	ret = i2c_smbus_write_byte_data(client, reg, tmp);
>> As i2c_smbus_write_byte_data is defined as returning zero on success
>> this could simply be, if (!ret).
> 
> Fixed.
> 
>>> +}
>>> +
>>> +/* power_state */
>>> +static int isl29003_set_power_state(struct i2c_client *client, int state)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = __isl29003_write_reg(client, ISL29003_REG_COMMAND,
>>> +		ISL29003_ADC_ENABLED, 0, state);
>> As this is either 0 or less than zero, again if (ret) will suffice.
> 
> Fixed.
> 
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	ret = __isl29003_write_reg(client, ISL29003_REG_COMMAND,
>>> +		ISL29003_ADC_PD, 0, ~state);
>> Just return ret hence losing these 2 lines.
> 
> Fixed.
> 
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int isl29003_get_power_state(struct i2c_client *client)
>>> +{
>>> +	struct isl29003_data *data = i2c_get_clientdata(client);
>>> +	u8 cmdreg = data->reg_cache[ISL29003_REG_COMMAND];
>>> +	return (cmdreg & ISL29003_ADC_ENABLED) && (~cmdreg & ISL29003_ADC_PD);
>>> +}
>> Unfortunately this chip has gain controlled by combination of the i2c
>> accessed registers and an external resitor.  I guess this falls
>> into the category of things to be fixed in userspace.  Perhaps
>> some documentation to indicate this issue would help?
>> (table 9 of data sheet)
> 
> The resistor does not affect the value read from the register - it's
> about integration time only. Or did I get it wrong?
My reading of table 9 on the data sheet is that the full scale
range is affected by the resistor. This is also described in the
bit about calculating lux.
> 
>>> +	if ((strict_strtoul(buf, 10, &val) < 0) || (val > 3))
>>> +		return -EINVAL;
>> See as there are rather a lot of calls like this, why don't
>> you consider moving this test into the register write command.
>> If the device is powered up then it will get copied to the
>> device. If not, just store it in the cache and it will be
>> written on resume anyway (assuming my understanding of your
>> suspend resume code is right!)
> 
> It's not even necessary to do that - the driver can access all registers
> while the PD bit is set. So the only check is to not read sensor data
> when this condition is matched.
> 
>>> +static ssize_t isl29003_store_power_state(struct device *dev,
>>> +					  struct device_attribute *attr,
>>> +					  const char *buf, size_t count)
>>> +{
>>> +	struct i2c_client *client = to_i2c_client(dev);
>>> +	unsigned long val;
>>> +	int ret;
>>> +	if ((strict_strtoul(buf, 10, &val) < 0) || (val > 1))
>>> +		return -EINVAL;
>>> +
>>> +	ret = isl29003_set_power_state(client, val);
>>> +	if (ret < 0)
>>> +		return ret;
>> I'd go with more concise "return ret ? ret : count;" but thats down
>> to personal preference.
> 
> Fixed.
> 
>>> +	/* read all the registers once to fill the cache.
>>> +	 * if one of the reads fails, we consider the init failed */
>> Why are you caching registers 4-7?  They are read only data registers
>> and those you use are read on demand anyway.  It's not a problem here,
>> but worth noting that even the first 2 are not simply read / write
>> control registers and hence any caching method has to be very careful
>>   (there is a interrupt flag in control according to the data sheet.)
> 
> You're right. I changed the cache to only store the first 4 registers
> for now. Interrupt handling will need some extra work anyway, so I'll
> leave that for now.
> 
>> If we are restoring registers from cache, why are we reading them?
>>
>>> +	for (i = 0; i < ARRAY_SIZE(data->reg_cache); i++)
>>> +		if (i2c_smbus_read_byte_data(client, i, data->reg_cache[i]) < 0)
>>> +			return -ENODEV;
> 
> Typo - couldn't test the suspend/resume code yet. Fixed now.
> 
> Please see the patch below.
> 
> Thanks again,
> Daniel
> 
> 
>>>From 8ee5021834900f312ff26cd2586c18e99af31a5d Mon Sep 17 00:00:00 2001
> From: Daniel Mack <daniel@...aq.de>
> Date: Tue, 10 Mar 2009 16:13:07 +0100
> Subject: [PATCH] Added driver for ISL29003 ambient light sensor
> 
> This patch adds a driver for Intersil's ISL29003 ambient light sensor
> device plus some documentation. Inspired by tsl2550.c, a driver for a
> similar device.
> 
> It is put to drivers/misc for now until the industrial I/O framework
> gets merged.
> 
> Signed-off-by: Daniel Mack <daniel@...aq.de>
> ---
>  New version with Jonathan Cameron's review notes addressed.
> 
>  Documentation/misc-devices/isl29003 |   62 +++++
>  drivers/misc/Kconfig                |   10 +
>  drivers/misc/Makefile               |    1 +
>  drivers/misc/isl29003.c             |  470 +++++++++++++++++++++++++++++++++++
>  4 files changed, 543 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/misc-devices/isl29003
>  create mode 100644 drivers/misc/isl29003.c
> 
> diff --git a/Documentation/misc-devices/isl29003 b/Documentation/misc-devices/isl29003
> new file mode 100644
> index 0000000..c4ff5f3
> --- /dev/null
> +++ b/Documentation/misc-devices/isl29003
> @@ -0,0 +1,62 @@
> +Kernel driver isl29003
> +=====================
> +
> +Supported chips:
> +* Intersil ISL29003
> +Prefix: 'isl29003'
> +Addresses scanned: none
> +Datasheet:
> +http://www.intersil.com/data/fn/fn7464.pdf
> +
> +Author: Daniel Mack <daniel@...aq.de>
> +
> +
> +Description
> +-----------
> +The ISL29003 is an integrated light sensor with a 16-bit integrating type
> +ADC, I2C user programmable lux range select for optimized counts/lux, and
> +I2C multi-function control and monitoring capabilities. The internal ADC
> +provides 16-bit resolution while rejecting 50Hz and 60Hz flicker caused by
> +artificial light sources.
> +
> +The driver allows to set the lux range, the bit resolution, the operational
> +mode (see below) and the power state of device and can read the current lux
> +value, of course.
> +
> +
> +Detection
> +---------
> +
> +The ISL29003 does not have an ID register which could be used to identify
> +it, so the detection routine will just try to read from the configured I2C
> +addess and consider the device to be present as soon as it ACKs the
> +transfer.
> +
> +
> +Sysfs entries
> +-------------
> +
> +range:
> +	0: 0 lux to 1000 lux (default)
> +	1: 0 lux to 4000 lux
> +	2: 0 lux to 16,000 lux
> +	3: 0 lux to 64,000 lux
> +
> +resolution:
> +	0: 2^16 cycles (default)
> +	1: 2^12 cycles
> +	2: 2^8 cycles
> +	3: 2^4 cycles
> +
> +mode:
> +	0: diode1's current (unsigned 16bit) (default)
> +	1: diode1's current (unsigned 16bit)
> +	2: difference between diodes (l1 - l2, signed 15bit)
> +
> +power_state:
> +	0: device is disabled (default)
> +	1: device is enabled
> +
> +lux (read only):
> +	returns the value from the last sensor reading
> +
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index c64e679..b883b19 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -223,6 +223,16 @@ config DELL_LAPTOP
>  	This driver adds support for rfkill and backlight control to Dell
>  	laptops.
>  
> +config ISL29003
> +	tristate "Intersil ISL29003 ambient light sensor"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the Intersil ISL29003
> +	  ambient light sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called isl29003.
> +
>  source "drivers/misc/c2port/Kconfig"
>  source "drivers/misc/eeprom/Kconfig"
>  
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index bc11998..7871f05 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -18,5 +18,6 @@ obj-$(CONFIG_KGDB_TESTS)	+= kgdbts.o
>  obj-$(CONFIG_SGI_XP)		+= sgi-xp/
>  obj-$(CONFIG_SGI_GRU)		+= sgi-gru/
>  obj-$(CONFIG_HP_ILO)		+= hpilo.o
> +obj-$(CONFIG_ISL29003)		+= isl29003.o
>  obj-$(CONFIG_C2PORT)		+= c2port/
>  obj-y				+= eeprom/
> diff --git a/drivers/misc/isl29003.c b/drivers/misc/isl29003.c
> new file mode 100644
> index 0000000..2e2a592
> --- /dev/null
> +++ b/drivers/misc/isl29003.c
> @@ -0,0 +1,470 @@
> +/*
> + *  isl29003.c - Linux kernel module for
> + * 	Intersil ISL29003 ambient light sensor
> + *
> + *  See file:Documentation/misc-devices/isl29003
> + *
> + *  Copyright (c) 2009 Daniel Mack <daniel@...aq.de>
> + *
> + *  Based on code written by
> + *  	Rodolfo Giometti <giometti@...ux.it>
> + *  	Eurotech S.p.A. <info@...otech.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>
> +
> +#define ISL29003_DRV_NAME	"isl29003"
> +#define DRIVER_VERSION		"1.0"
> +
> +#define ISL29003_REG_COMMAND		0x00
> +#define ISL29003_ADC_ENABLED		(1 << 7)
> +#define ISL29003_ADC_PD			(1 << 6)
> +#define ISL29003_TIMING_INT		(1 << 5)
> +#define ISL29003_MODE_SHIFT		(2)
> +#define ISL29003_MODE_MASK		(0x3 << ISL29003_MODE_SHIFT)
> +#define ISL29003_RES_SHIFT		(0)
> +#define ISL29003_RES_MASK		(0x3 << ISL29003_RES_SHIFT)
> +
> +#define ISL29003_REG_CONTROL		0x01
> +#define ISL29003_INT_FLG		(1 << 5)
> +#define ISL29003_RANGE_SHIFT		(2)
> +#define ISL29003_RANGE_MASK		(0x3 << ISL29003_RANGE_SHIFT)
> +#define ISL29003_INT_PERSISTS_SHIFT	(0)
> +#define ISL29003_INT_PERSISTS_MASK	(0xf << ISL29003_INT_PERSISTS_SHIFT)
> +
> +#define ISL29003_REG_IRQ_THRESH_HI	0x02
> +#define ISL29003_REG_IRQ_THRESH_LO	0x03
> +#define ISL29003_REG_LSB_SENSOR		0x04
> +#define ISL29003_REG_MSB_SENSOR		0x05
> +#define ISL29003_REG_LSB_TIMER		0x06
> +#define ISL29003_REG_MSB_TIMER		0x07
> +
> +#define ISL29003_NUM_CACHABLE_REGS	4
> +
> +struct isl29003_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	u8 reg_cache[ISL29003_NUM_CACHABLE_REGS];
> +};
> +
> +static int gain_range[] = {
> +	1000, 4000, 16000, 64000
> +};
> +
> +/*
> + * register access helpers
> + */
> +
> +static int __isl29003_read_reg(struct i2c_client *client,
> +			       u32 reg, u8 mask, u8 shift)
> +{
> +	struct isl29003_data *data = i2c_get_clientdata(client);
> +	return (data->reg_cache[reg] & mask) >> shift;
> +}
> +
> +static int __isl29003_write_reg(struct i2c_client *client,
> +				u32 reg, u8 mask, u8 shift, u8 val)
> +{
> +	struct isl29003_data *data = i2c_get_clientdata(client);
> +	int ret = 0;
> +	u8 tmp;
> +
> +	if (reg >= ISL29003_NUM_CACHABLE_REGS)
> +		return -EINVAL;
> +
> +	mutex_lock(&data->lock);
> +
> +	tmp = data->reg_cache[reg];
> +	tmp &= ~mask;
> +	tmp |= val << shift;
> +
> +	ret = i2c_smbus_write_byte_data(client, reg, tmp);
> +	if (!ret)
> +		data->reg_cache[reg] = tmp;
> +
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +/*
> + * internally used functions
> + */
> +
> +/* range */
> +static int isl29003_get_range(struct i2c_client *client)
> +{
> +	return __isl29003_read_reg(client, ISL29003_REG_CONTROL,
> +		ISL29003_RANGE_MASK, ISL29003_RANGE_SHIFT);
> +}
> +
> +static int isl29003_set_range(struct i2c_client *client, int range)
> +{
> +	return __isl29003_write_reg(client, ISL29003_REG_CONTROL,
> +		ISL29003_RANGE_MASK, ISL29003_RANGE_SHIFT, range);
> +}
> +
> +/* resolution */
> +static int isl29003_get_resolution(struct i2c_client *client)
> +{
> +	return __isl29003_read_reg(client, ISL29003_REG_COMMAND,
> +		ISL29003_RES_MASK, ISL29003_RES_SHIFT);
> +}
> +
> +static int isl29003_set_resolution(struct i2c_client *client, int res)
> +{
> +	return __isl29003_write_reg(client, ISL29003_REG_COMMAND,
> +		ISL29003_RES_MASK, ISL29003_RES_SHIFT, res);
> +}
> +
> +/* mode */
> +static int isl29003_get_mode(struct i2c_client *client)
> +{
> +	return __isl29003_read_reg(client, ISL29003_REG_COMMAND,
> +		ISL29003_RES_MASK, ISL29003_RES_SHIFT);
> +}
> +
> +static int isl29003_set_mode(struct i2c_client *client, int mode)
> +{
> +	return __isl29003_write_reg(client, ISL29003_REG_COMMAND,
> +		ISL29003_RES_MASK, ISL29003_RES_SHIFT, mode);
> +}
> +
> +/* power_state */
> +static int isl29003_set_power_state(struct i2c_client *client, int state)
> +{
> +	return __isl29003_write_reg(client, ISL29003_REG_COMMAND,
> +				ISL29003_ADC_ENABLED | ISL29003_ADC_PD, 0,
> +				state ? ISL29003_ADC_ENABLED : ISL29003_ADC_PD);
> +}
> +
> +static int isl29003_get_power_state(struct i2c_client *client)
> +{
> +	struct isl29003_data *data = i2c_get_clientdata(client);
> +	u8 cmdreg = data->reg_cache[ISL29003_REG_COMMAND];
> +	return ~cmdreg & ISL29003_ADC_PD;
> +}
> +
> +static int isl29003_get_adc_value(struct i2c_client *client)
> +{
> +	struct isl29003_data *data = i2c_get_clientdata(client);
> +	int lsb, msb, range, bitdepth;
> +
> +	mutex_lock(&data->lock);
> +	lsb = i2c_smbus_read_byte_data(client, ISL29003_REG_LSB_SENSOR);
> +
> +	if (lsb < 0) {
> +		mutex_unlock(&data->lock);
> +		return lsb;
> +	}
> +
> +	msb = i2c_smbus_read_byte_data(client, ISL29003_REG_MSB_SENSOR);
> +	mutex_unlock(&data->lock);
> +
> +	if (msb < 0)
> +		return msb;
> +
> +	range = isl29003_get_range(client);
> +	bitdepth = (4 - isl29003_get_resolution(client)) * 4;
> +	return (((msb << 8) | lsb) * gain_range[range]) >> bitdepth;
> +}
> +
> +/*
> + * sysfs layer
> + */
> +
> +/* range */
> +static ssize_t isl29003_show_range(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%i\n", isl29003_get_range(client));
> +}
> +
> +static ssize_t isl29003_store_range(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if ((strict_strtoul(buf, 10, &val) < 0) || (val > 3))
> +		return -EINVAL;
> +
> +	ret = isl29003_set_range(client, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(range, S_IWUSR | S_IRUGO,
> +		   isl29003_show_range, isl29003_store_range);
> +
> +
> +/* resolution */
> +static ssize_t isl29003_show_resolution(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%d\n", isl29003_get_resolution(client));
> +}
> +
> +static ssize_t isl29003_store_resolution(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if ((strict_strtoul(buf, 10, &val) < 0) || (val > 3))
> +		return -EINVAL;
> +
> +	ret = isl29003_set_resolution(client, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(resolution, S_IWUSR | S_IRUGO,
> +		   isl29003_show_resolution, isl29003_store_resolution);
> +
> +/* mode */
> +static ssize_t isl29003_show_mode(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%d\n", isl29003_get_mode(client));
> +}
> +
> +static ssize_t isl29003_store_mode(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if ((strict_strtoul(buf, 10, &val) < 0) || (val > 2))
> +		return -EINVAL;
> +
> +	ret = isl29003_set_mode(client, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(mode, S_IWUSR | S_IRUGO,
> +		   isl29003_show_mode, isl29003_store_mode);
> +
> +
> +/* power state */
> +static ssize_t isl29003_show_power_state(struct device *dev,
> +					 struct device_attribute *attr,
> +					 char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	return sprintf(buf, "%d\n", isl29003_get_power_state(client));
> +}
> +
> +static ssize_t isl29003_store_power_state(struct device *dev,
> +					  struct device_attribute *attr,
> +					  const char *buf, size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	if ((strict_strtoul(buf, 10, &val) < 0) || (val > 1))
> +		return -EINVAL;
> +
> +	ret = isl29003_set_power_state(client, val);
> +	return ret ? ret : count;
> +}
> +
> +static DEVICE_ATTR(power_state, S_IWUSR | S_IRUGO,
> +		   isl29003_show_power_state, isl29003_store_power_state);
> +
> +
> +/* lux */
> +static ssize_t isl29003_show_lux(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +
> +	/* No LUX data if not operational */
> +	if (!isl29003_get_power_state(client))
> +		return -EBUSY;
> +
> +	return sprintf(buf, "%d\n", isl29003_get_adc_value(client));
> +}
> +
> +static DEVICE_ATTR(lux, S_IRUGO, isl29003_show_lux, NULL);
> +
> +static struct attribute *isl29003_attributes[] = {
> +	&dev_attr_range.attr,
> +	&dev_attr_resolution.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_power_state.attr,
> +	&dev_attr_lux.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group isl29003_attr_group = {
> +	.attrs = isl29003_attributes,
> +};
> +
> +static int isl29003_init_client(struct i2c_client *client)
> +{
> +	struct isl29003_data *data = i2c_get_clientdata(client);
> +	int i;
> +
> +	/* read all the registers once to fill the cache.
> +	 * if one of the reads fails, we consider the init failed */
> +	for (i = 0; i < ARRAY_SIZE(data->reg_cache); i++) {
> +		int v = i2c_smbus_read_byte_data(client, i);
> +		if (v < 0)
> +			return -ENODEV;
> +
> +		data->reg_cache[i] = v;
> +	}
> +
> +	/* set defaults */
> +	isl29003_set_range(client, 0);
> +	isl29003_set_resolution(client, 0);
> +	isl29003_set_mode(client, 0);
> +	isl29003_set_power_state(client, 0);
> +
> +	return 0;
> +}
> +
> +/*
> + * I2C layer
> + */
> +
> +static int __devinit isl29003_probe(struct i2c_client *client,
> +				    const struct i2c_device_id *id)
> +{
> +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> +	struct isl29003_data *data;
> +	int err = 0;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE))
> +		return -EIO;
> +
> +	data = kzalloc(sizeof(struct isl29003_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	i2c_set_clientdata(client, data);
> +	mutex_init(&data->lock);
> +
> +	/* initialize the ISL29003 chip */
> +	err = isl29003_init_client(client);
> +	if (err)
> +		goto exit_kfree;
> +
> +	/* register sysfs hooks */
> +	err = sysfs_create_group(&client->dev.kobj, &isl29003_attr_group);
> +	if (err)
> +		goto exit_kfree;
> +
> +	dev_info(&client->dev, "driver version %s enabled\n", DRIVER_VERSION);
> +	return 0;
> +
> +exit_kfree:
> +	kfree(data);
> +	return err;
> +}
> +
> +static int __devexit isl29003_remove(struct i2c_client *client)
> +{
> +	sysfs_remove_group(&client->dev.kobj, &isl29003_attr_group);
> +	isl29003_set_power_state(client, 0);
> +	kfree(i2c_get_clientdata(client));
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int isl29003_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	return isl29003_set_power_state(client, 0);
> +}
> +
> +static int isl29003_resume(struct i2c_client *client)
> +{
> +	int i;
> +	struct isl29003_data *data = i2c_get_clientdata(client);
> +
> +	/* restore registers from cache */
> +	for (i = 0; i < ARRAY_SIZE(data->reg_cache); i++)
> +		if (!i2c_smbus_write_byte_data(client, i, data->reg_cache[i]))
> +			return -EIO;
> +
> +	return 0;
> +}
> +
> +#else
> +#define isl29003_suspend	NULL
> +#define isl29003_resume		NULL
> +#endif /* CONFIG_PM */
> +
> +static const struct i2c_device_id isl29003_id[] = {
> +	{ "isl29003", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, isl29003_id);
> +
> +static struct i2c_driver isl29003_driver = {
> +	.driver = {
> +		.name	= ISL29003_DRV_NAME,
> +		.owner	= THIS_MODULE,
> +	},
> +	.suspend = isl29003_suspend,
> +	.resume	= isl29003_resume,
> +	.probe	= isl29003_probe,
> +	.remove	= __devexit_p(isl29003_remove),
> +	.id_table = isl29003_id,
> +};
> +
> +static int __init isl29003_init(void)
> +{
> +	return i2c_add_driver(&isl29003_driver);
> +}
> +
> +static void __exit isl29003_exit(void)
> +{
> +	i2c_del_driver(&isl29003_driver);
> +}
> +
> +MODULE_AUTHOR("Daniel Mack <daniel@...aq.de>");
> +MODULE_DESCRIPTION("ISL29003 ambient light sensor driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_VERSION(DRIVER_VERSION);
> +
> +module_init(isl29003_init);
> +module_exit(isl29003_exit);
> +

All looks fine to me.

Acked-by: Jonathan Cameron <jic23@....ac.uk>

Cheers,

Jonathan
--
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