Re: [PATCH] Added driver for ISL29003 ambient light sensor

[Daniel Mack]

Hi Jonathan,

many thanks for your review!

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?

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

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