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 thei2c_board_info array of the board support file, so there is no reasonwhy it shouldn't work. Or are you talking about the names of files andfunctions 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 tothe CPU, so I can't test that. I would expect anyone who needs thisfunctions 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.

The resistor does not affect the value read from the register - it'sabout 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 registerswhile the PD bit is set. So the only check is to not read sensor datawhen this condition is matched.

> > + /* 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 registersfor now. Interrupt handling will need some extra work anyway, so I'llleave that for now.

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