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 <[email protected]>
---
(The discussion about where to put the files for this has some
history on linux-i2c list and the verdict there was to re-post
the driver to the LKML for further review as both drivers/hwmon and
drivers/i2c/chips are unsuitable places for it)
Documentation/misc-devices/isl29003 | 62 +++++
drivers/misc/Kconfig | 10 +
drivers/misc/Makefile | 1 +
drivers/misc/isl29003.c | 486 +++++++++++++++++++++++++++++++++++
4 files changed, 559 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 <[email protected]>
+
+
+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..9c6fb0e
--- /dev/null
+++ b/drivers/misc/isl29003.c
@@ -0,0 +1,486 @@
+/*
+ * isl29003.c - Linux kernel module for
+ * Intersil ISL29003 ambient light sensor
+ *
+ * See file:Documentation/misc-devices/isl29003
+ *
+ * Copyright (c) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on code written by
+ * Rodolfo Giometti <[email protected]>
+ * Eurotech S.p.A. <[email protected]>
+ *
+ * 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
+
+struct isl29003_data {
+ struct i2c_client *client;
+ struct mutex lock;
+ u8 reg_cache[8];
+};
+
+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;
+
+ 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 >= 0)
+ 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)
+{
+ int ret;
+
+ ret = __isl29003_write_reg(client, ISL29003_REG_COMMAND,
+ ISL29003_ADC_ENABLED, 0, state);
+ if (ret < 0)
+ return ret;
+
+ ret = __isl29003_write_reg(client, ISL29003_REG_COMMAND,
+ ISL29003_ADC_PD, 0, ~state);
+ 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);
+}
+
+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;
+
+ if (!isl29003_get_power_state(client))
+ return -EBUSY;
+
+ 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;
+
+ if (!isl29003_get_power_state(client))
+ return -EBUSY;
+
+ 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;
+
+ if (!isl29003_get_power_state(client))
+ return -EBUSY;
+
+ 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);
+ if (ret < 0)
+ return ret;
+
+ return 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;
+
+ /* restore registers from cache */
+ 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;
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("ISL29003 ambient light sensor driver");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRIVER_VERSION);
+
+module_init(isl29003_init);
+module_exit(isl29003_exit);
+
--
1.6.2
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
>From 8ee5021834900f312ff26cd2586c18e99af31a5d Mon Sep 17 00:00:00 2001
From: Daniel Mack <[email protected]>
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 <[email protected]>
---
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 <[email protected]>
+
+
+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 <[email protected]>
+ *
+ * Based on code written by
+ * Rodolfo Giometti <[email protected]>
+ * Eurotech S.p.A. <[email protected]>
+ *
+ * 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 <[email protected]>");
+MODULE_DESCRIPTION("ISL29003 ambient light sensor driver");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRIVER_VERSION);
+
+module_init(isl29003_init);
+module_exit(isl29003_exit);
+
--
1.6.2
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 <[email protected]>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> +
> +
> +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 <[email protected]>
> + *
> + * Based on code written by
> + * Rodolfo Giometti <[email protected]>
> + * Eurotech S.p.A. <[email protected]>
> + *
> + * 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 <[email protected]>");
> +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 <[email protected]>
Cheers,
Jonathan
On Tue, Mar 10, 2009 at 05:49:55PM +0000, Jonathan Cameron wrote:
> >>From 8ee5021834900f312ff26cd2586c18e99af31a5d Mon Sep 17 00:00:00 2001
> > From: Daniel Mack <[email protected]>
> > 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 <[email protected]>
[...]
> All looks fine to me.
>
> Acked-by: Jonathan Cameron <[email protected]>
Ok, wonderful :)
What's the merge path for that, who'll queue it?
Thanks,
Daniel
On Wed, 11 Mar 2009 01:31:55 +0100 Daniel Mack <[email protected]> wrote:
> On Tue, Mar 10, 2009 at 05:49:55PM +0000, Jonathan Cameron wrote:
> > >>From 8ee5021834900f312ff26cd2586c18e99af31a5d Mon Sep 17 00:00:00 2001
> > > From: Daniel Mack <[email protected]>
> > > 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 <[email protected]>
>
> [...]
>
> > All looks fine to me.
> >
> > Acked-by: Jonathan Cameron <[email protected]>
I cannot find this review which Jonathan performed. Fumbled
reply-to-all, or did vger break?
> Ok, wonderful :)
>
> What's the merge path for that, who'll queue it?
I can take care of it.
Was any thought given to the CONFIG_SYSFS=n situation?
On Tue, Mar 10, 2009 at 09:28:26PM -0700, Andrew Morton wrote:
> On Wed, 11 Mar 2009 01:31:55 +0100 Daniel Mack <[email protected]> wrote:
> > > All looks fine to me.
> > >
> > > Acked-by: Jonathan Cameron <[email protected]>
>
> I cannot find this review which Jonathan performed. Fumbled
> reply-to-all, or did vger break?
Jonathan replied to the posting on linux-i2c and linux-kernel wasn't in
the loop, sorry. Find his posting here:
http://marc.info/?l=linux-i2c&m=123669064909549&w=2
> I can take care of it.
>
> Was any thought given to the CONFIG_SYSFS=n situation?
You're right, it should depend on that. You want me so send a patch on
top of the other or one that replaces it?
Thanks,
Daniel
On Wed, 11 Mar 2009 09:32:52 +0100 Daniel Mack <[email protected]> wrote:
> On Tue, Mar 10, 2009 at 09:28:26PM -0700, Andrew Morton wrote:
> > On Wed, 11 Mar 2009 01:31:55 +0100 Daniel Mack <[email protected]> wrote:
> > > > All looks fine to me.
> > > >
> > > > Acked-by: Jonathan Cameron <[email protected]>
> >
> > I cannot find this review which Jonathan performed. Fumbled
> > reply-to-all, or did vger break?
>
> Jonathan replied to the posting on linux-i2c and linux-kernel wasn't in
> the loop, sorry. Find his posting here:
>
> http://marc.info/?l=linux-i2c&m=123669064909549&w=2
OK. Please always use reply-to-all, guys.
> > I can take care of it.
> >
> > Was any thought given to the CONFIG_SYSFS=n situation?
>
> You're right, it should depend on that. You want me so send a patch on
> top of the other or one that replaces it?
Either way is OK by me at this time.
On Wed, Mar 11, 2009 at 09:32:34AM -0700, Andrew Morton wrote:
> > Jonathan replied to the posting on linux-i2c and linux-kernel wasn't in
> > the loop, sorry. Find his posting here:
> >
> > http://marc.info/?l=linux-i2c&m=123669064909549&w=2
>
> OK. Please always use reply-to-all, guys.
The thread moved from linux-i2c to linux-kernel, hence the trouble.
> > You're right, it should depend on that. You want me so send a patch on
> > top of the other or one that replaces it?
>
> Either way is OK by me at this time.
Ok, I chose the shorter variant :) See the patch below.
Thanks,
Daniel
>From 9143c849682280ca3b0544e580bc90241c385927 Mon Sep 17 00:00:00 2001
From: Daniel Mack <[email protected]>
Date: Wed, 11 Mar 2009 17:39:13 +0100
Subject: [PATCH] make isl29003 depend on sysfs
Signed-off-by: Daniel Mack <[email protected]>
---
drivers/misc/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b883b19..16e1815 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -225,7 +225,7 @@ config DELL_LAPTOP
config ISL29003
tristate "Intersil ISL29003 ambient light sensor"
- depends on I2C
+ depends on I2C && SYSFS
help
If you say yes here you get support for the Intersil ISL29003
ambient light sensor.
--
1.6.2
>>>>> "Daniel" == Daniel Mack <[email protected]> writes:
Trivial comment on the docs:
Daniel> +mode:
Daniel> + 0: diode1's current (unsigned 16bit) (default)
Daniel> + 1: diode1's current (unsigned 16bit)
Daniel> + 2: difference between diodes (l1 - l2, signed 15bit)
That has diode1 twice. Should it, instead, be:
+mode:
+ 0: diode1's current (unsigned 16bit) (default)
+ 1: diode2's current (unsigned 16bit)
+ 2: difference between diodes (l1 - l2, signed 15bit)
?
-JimC
--
James Cloos <[email protected]> OpenPGP: 1024D/ED7DAEA6