* [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver
@ 2012-07-02 21:43 Peter Meerwald
2012-07-03 7:30 ` Lars-Peter Clausen
2012-07-08 9:22 ` Jonathan Cameron
0 siblings, 2 replies; 5+ messages in thread
From: Peter Meerwald @ 2012-07-02 21:43 UTC (permalink / raw)
To: linux-iio; +Cc: lars, Peter Meerwald
sensor has 4 channels (10-bit each, R/G/B and clear), sensitivity
and gain is controlled in the driver by ext_info integration_time
and CHAN_INFO_HARDWAREGAIN
driver supports triggered buffer and IIO_CHAN_INFO_RAW to get the
sensor data
patch depends on IIO_MOD_LIGHT_RED etc, the patch providing that
(http://permalink.gmane.org/gmane.linux.kernel.iio/4354) has not been
merged yet, a new proposal is following
v4: address comments by Lars-Peter Clausen
* make sure trigger handler is exited with iio_trigger_notify_done()
and IRQ_HANDLED
* kfree()/kalloc() -> krealloc()
v3:
* fix warnings
v2: address comments by Lars-Peter Clausen
* buffer allocation now in update_scan_mode instead of in trigger
handler
* simplify trigger code (assume active_scan_mask is not empty, use
for_each_set_bit, use iio_push_to_buffer)
* reorder entry in Makefile and Kconfig
* fix remove
Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
---
drivers/iio/light/Kconfig | 12 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/adjd_s311.c | 396 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 409 insertions(+)
create mode 100644 drivers/iio/light/adjd_s311.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index f3ea90d..91d15d2 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -3,6 +3,18 @@
#
menu "Light sensors"
+config ADJD_S311
+ tristate "ADJD-S311-CR999 digital color sensor"
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ depends on I2C
+ help
+ If you say yes here you get support for the Avago ADJD-S311-CR999
+ digital color light sensor.
+
+ This driver can also be built as a module. If so, the module
+ will be called adjd_s311.
+
config SENSORS_LM3533
tristate "LM3533 ambient light sensor"
depends on MFD_LM3533
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 06fa4d3..13f8a78 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -2,5 +2,6 @@
# Makefile for IIO Light sensors
#
+obj-$(CONFIG_ADJD_S311) += adjd_s311.o
obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
obj-$(CONFIG_VCNL4000) += vcnl4000.o
diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c
new file mode 100644
index 0000000..029d6e6
--- /dev/null
+++ b/drivers/iio/light/adjd_s311.c
@@ -0,0 +1,396 @@
+/*
+ * adjd_s311.c - Support for ADJD-S311-CR999 digital color sensor
+ *
+ * Copyright (C) 2012 Peter Meerwald <pmeerw@pmeerw.net>
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * driver for ADJD-S311-CR999 digital color sensor (10-bit channels for
+ * red, green, blue, clear); 7-bit I2C slave address 0x74
+ *
+ * limitations: no calibration, no offset mode, no sleep mode
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/i2c.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/bitmap.h>
+#include <linux/err.h>
+#include <linux/irq.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define ADJD_S311_DRV_NAME "adjd_s311"
+
+#define ADJD_S311_CTRL 0x00
+#define ADJD_S311_CONFIG 0x01
+#define ADJD_S311_CAP_RED 0x06
+#define ADJD_S311_CAP_GREEN 0x07
+#define ADJD_S311_CAP_BLUE 0x08
+#define ADJD_S311_CAP_CLEAR 0x09
+#define ADJD_S311_INT_RED_LO 0x0a
+#define ADJD_S311_INT_RED_HI 0x0b
+#define ADJD_S311_INT_GREEN_LO 0x0c
+#define ADJD_S311_INT_GREEN_HI 0x0d
+#define ADJD_S311_INT_BLUE_LO 0x0e
+#define ADJD_S311_INT_BLUE_HI 0x0f
+#define ADJD_S311_INT_CLEAR_LO 0x10
+#define ADJD_S311_INT_CLEAR_HI 0x11
+#define ADJD_S311_DATA_RED_LO 0x40
+#define ADJD_S311_DATA_RED_HI 0x41
+#define ADJD_S311_DATA_GREEN_LO 0x42
+#define ADJD_S311_DATA_GREEN_HI 0x43
+#define ADJD_S311_DATA_BLUE_LO 0x44
+#define ADJD_S311_DATA_BLUE_HI 0x45
+#define ADJD_S311_DATA_CLEAR_LO 0x46
+#define ADJD_S311_DATA_CLEAR_HI 0x47
+#define ADJD_S311_OFFSET_RED 0x48
+#define ADJD_S311_OFFSET_GREEN 0x49
+#define ADJD_S311_OFFSET_BLUE 0x4a
+#define ADJD_S311_OFFSET_CLEAR 0x4b
+
+#define ADJD_S311_CTRL_GOFS 0x02
+#define ADJD_S311_CTRL_GSSR 0x01
+#define ADJD_S311_CAP_MASK 0x0f
+#define ADJD_S311_INT_MASK 0x0fff
+#define ADJD_S311_DATA_MASK 0x03ff
+
+struct adjd_s311_data {
+ struct i2c_client *client;
+ u16 *buffer;
+};
+
+enum adjd_s311_channel_idx {
+ IDX_RED, IDX_GREEN, IDX_BLUE, IDX_CLEAR
+};
+
+#define ADJD_S311_DATA_REG(chan) \
+ (ADJD_S311_DATA_RED_LO + (chan) * 2)
+#define ADJD_S311_INT_REG(chan) \
+ (ADJD_S311_INT_RED_LO + (chan) * 2)
+#define ADJD_S311_CAP_REG(chan) \
+ (ADJD_S311_CAP_RED + (chan))
+
+static int adjd_s311_req_data(struct iio_dev *indio_dev)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ int tries = 10;
+
+ int ret = i2c_smbus_write_byte_data(data->client, ADJD_S311_CTRL,
+ ADJD_S311_CTRL_GSSR);
+ if (ret < 0)
+ return ret;
+
+ while (tries--) {
+ ret = i2c_smbus_read_byte_data(data->client, ADJD_S311_CTRL);
+ if (ret < 0)
+ return ret;
+ if (!(ret & ADJD_S311_CTRL_GSSR))
+ break;
+ msleep(20);
+ }
+
+ if (tries < 0) {
+ dev_err(&data->client->dev,
+ "adjd_s311_req_data() failed, data not ready\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int adjd_s311_read_data(struct iio_dev *indio_dev, u8 reg, int *val)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+
+ int ret = adjd_s311_req_data(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ ret = i2c_smbus_read_word_data(data->client, reg);
+ if (ret < 0)
+ return ret;
+
+ *val = ret & ADJD_S311_DATA_MASK;
+
+ return 0;
+}
+
+static ssize_t adjd_s311_read_int_time(struct iio_dev *indio_dev,
+ uintptr_t private, const struct iio_chan_spec *chan, char *buf)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ s32 ret;
+
+ ret = i2c_smbus_read_word_data(data->client,
+ ADJD_S311_INT_REG(chan->address));
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%d\n", ret & ADJD_S311_INT_MASK);
+}
+
+static ssize_t adjd_s311_write_int_time(struct iio_dev *indio_dev,
+ uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
+ size_t len)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ unsigned long int_time;
+ int ret;
+
+ ret = kstrtoul(buf, 10, &int_time);
+ if (ret)
+ return ret;
+
+ if (int_time > ADJD_S311_INT_MASK)
+ return -EINVAL;
+
+ ret = i2c_smbus_write_word_data(data->client,
+ ADJD_S311_INT_REG(chan->address), int_time);
+ if (ret < 0)
+ return ret;
+
+ return len;
+}
+
+static irqreturn_t adjd_s311_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ struct iio_buffer *buffer = indio_dev->buffer;
+ int len = 0;
+ int i, j = 0;
+
+ int ret = adjd_s311_req_data(indio_dev);
+ if (ret < 0)
+ goto done;
+
+ for_each_set_bit(i, indio_dev->active_scan_mask,
+ indio_dev->masklength) {
+
+ ret = i2c_smbus_read_word_data(data->client,
+ ADJD_S311_DATA_REG(i));
+ if (ret < 0)
+ goto done;
+
+ data->buffer[j++] = ret & ADJD_S311_DATA_MASK;
+ len += 2;
+ }
+
+ if (indio_dev->scan_timestamp)
+ *(s64 *)((phys_addr_t)data->buffer + ALIGN(len, sizeof(s64)))
+ = pf->timestamp;
+ iio_push_to_buffer(buffer, (u8 *)data->buffer, pf->timestamp);
+
+done:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static const struct iio_chan_spec_ext_info adjd_s311_ext_info[] = {
+ {
+ .name = "integration_time",
+ .read = adjd_s311_read_int_time,
+ .write = adjd_s311_write_int_time,
+ },
+ { }
+};
+
+static const struct iio_chan_spec adjd_s311_channels[] = {
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .address = IDX_RED,
+ .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+ IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
+ .channel2 = IIO_MOD_LIGHT_RED,
+ .scan_index = 0,
+ .scan_type = IIO_ST('u', 10, 16, 0),
+ .ext_info = adjd_s311_ext_info,
+ }, {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .address = IDX_GREEN,
+ .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+ IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
+ .channel2 = IIO_MOD_LIGHT_GREEN,
+ .scan_index = 1,
+ .scan_type = IIO_ST('u', 10, 16, 0),
+ .ext_info = adjd_s311_ext_info,
+ }, {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .address = IDX_GREEN,
+ .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+ IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
+ .channel2 = IIO_MOD_LIGHT_BLUE,
+ .scan_index = 2,
+ .scan_type = IIO_ST('u', 10, 16, 0),
+ .ext_info = adjd_s311_ext_info,
+ }, {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .address = IDX_CLEAR,
+ .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
+ IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .scan_index = 3,
+ .scan_type = IIO_ST('u', 10, 16, 0),
+ .ext_info = adjd_s311_ext_info,
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(4),
+};
+
+static int adjd_s311_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = adjd_s311_read_data(indio_dev, chan->address, val);
+ if (ret < 0)
+ return ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ ret = i2c_smbus_read_byte_data(data->client,
+ ADJD_S311_CAP_REG(chan->address));
+ if (ret < 0)
+ return ret;
+ *val = ret & ADJD_S311_CAP_MASK;
+ return IIO_VAL_INT;
+ }
+ return -EINVAL;
+}
+
+static int adjd_s311_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ if (val < 0 || val > ADJD_S311_CAP_MASK)
+ return -EINVAL;
+
+ ret = i2c_smbus_write_byte_data(data->client,
+ ADJD_S311_CAP_REG(chan->address), val);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev,
+ const unsigned long *scan_mask)
+{
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+ data->buffer = krealloc(data->buffer, indio_dev->scan_bytes,
+ GFP_KERNEL);
+ if (!data->buffer)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static const struct iio_info adjd_s311_info = {
+ .read_raw = adjd_s311_read_raw,
+ .write_raw = adjd_s311_write_raw,
+ .update_scan_mode = adjd_s311_update_scan_mode,
+ .driver_module = THIS_MODULE,
+};
+
+static int __devinit adjd_s311_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct adjd_s311_data *data;
+ struct iio_dev *indio_dev;
+ int err;
+
+ indio_dev = iio_device_alloc(sizeof(*data));
+ if (indio_dev == NULL) {
+ err = -ENOMEM;
+ goto exit;
+ }
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->info = &adjd_s311_info;
+ indio_dev->name = ADJD_S311_DRV_NAME;
+ indio_dev->channels = adjd_s311_channels;
+ indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ err = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
+ adjd_s311_trigger_handler, NULL);
+ if (err < 0)
+ goto exit_free_device;
+
+ err = iio_device_register(indio_dev);
+ if (err)
+ goto exit_unreg_buffer;
+
+ dev_info(&client->dev, "ADJD-S311 color sensor registered\n");
+
+ return 0;
+
+exit_unreg_buffer:
+ iio_triggered_buffer_cleanup(indio_dev);
+exit_free_device:
+ iio_device_free(indio_dev);
+exit:
+ return err;
+}
+
+static int __devexit adjd_s311_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct adjd_s311_data *data = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+ iio_triggered_buffer_cleanup(indio_dev);
+ kfree(data->buffer);
+ iio_device_free(indio_dev);
+
+ return 0;
+}
+
+static const struct i2c_device_id adjd_s311_id[] = {
+ { "adjd_s311", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, adjd_s311_id);
+
+static struct i2c_driver adjd_s311_driver = {
+ .driver = {
+ .name = ADJD_S311_DRV_NAME,
+ },
+ .probe = adjd_s311_probe,
+ .remove = __devexit_p(adjd_s311_remove),
+ .id_table = adjd_s311_id,
+};
+module_i2c_driver(adjd_s311_driver);
+
+MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
+MODULE_DESCRIPTION("ADJD-S311 color sensor");
+MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver
2012-07-02 21:43 [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver Peter Meerwald
@ 2012-07-03 7:30 ` Lars-Peter Clausen
2012-07-12 19:26 ` Jonathan Cameron
2012-07-08 9:22 ` Jonathan Cameron
1 sibling, 1 reply; 5+ messages in thread
From: Lars-Peter Clausen @ 2012-07-03 7:30 UTC (permalink / raw)
To: Peter Meerwald; +Cc: linux-iio
On 07/02/2012 11:43 PM, Peter Meerwald wrote:
> sensor has 4 channels (10-bit each, R/G/B and clear), sensitivity
> and gain is controlled in the driver by ext_info integration_time
> and CHAN_INFO_HARDWAREGAIN
>
> driver supports triggered buffer and IIO_CHAN_INFO_RAW to get the
> sensor data
>
> patch depends on IIO_MOD_LIGHT_RED etc, the patch providing that
> (http://permalink.gmane.org/gmane.linux.kernel.iio/4354) has not been
> merged yet, a new proposal is following
>
> v4: address comments by Lars-Peter Clausen
> * make sure trigger handler is exited with iio_trigger_notify_done()
> and IRQ_HANDLED
> * kfree()/kalloc() -> krealloc()
>
> v3:
> * fix warnings
>
> v2: address comments by Lars-Peter Clausen
> * buffer allocation now in update_scan_mode instead of in trigger
> handler
> * simplify trigger code (assume active_scan_mask is not empty, use
> for_each_set_bit, use iio_push_to_buffer)
> * reorder entry in Makefile and Kconfig
> * fix remove
>
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
Looks good,
Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
> ---
> drivers/iio/light/Kconfig | 12 ++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/adjd_s311.c | 396 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 409 insertions(+)
> create mode 100644 drivers/iio/light/adjd_s311.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index f3ea90d..91d15d2 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -3,6 +3,18 @@
> #
> menu "Light sensors"
>
> +config ADJD_S311
> + tristate "ADJD-S311-CR999 digital color sensor"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on I2C
> + help
> + If you say yes here you get support for the Avago ADJD-S311-CR999
> + digital color light sensor.
> +
> + This driver can also be built as a module. If so, the module
There's an extra space before the "If".
> + will be called adjd_s311.
> +
> config SENSORS_LM3533
> tristate "LM3533 ambient light sensor"
> depends on MFD_LM3533
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 06fa4d3..13f8a78 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -2,5 +2,6 @@
> # Makefile for IIO Light sensors
> #
>
> +obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
> obj-$(CONFIG_VCNL4000) += vcnl4000.o
> diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c
> new file mode 100644
> index 0000000..029d6e6
> --- /dev/null
> +++ b/drivers/iio/light/adjd_s311.c
> @@ -0,0 +1,396 @@
> +/*
> + * adjd_s311.c - Support for ADJD-S311-CR999 digital color sensor
> + *
> + * Copyright (C) 2012 Peter Meerwald <pmeerw@pmeerw.net>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * driver for ADJD-S311-CR999 digital color sensor (10-bit channels for
> + * red, green, blue, clear); 7-bit I2C slave address 0x74
> + *
> + * limitations: no calibration, no offset mode, no sleep mode
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/bitmap.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define ADJD_S311_DRV_NAME "adjd_s311"
> +
> +#define ADJD_S311_CTRL 0x00
> +#define ADJD_S311_CONFIG 0x01
> +#define ADJD_S311_CAP_RED 0x06
> +#define ADJD_S311_CAP_GREEN 0x07
> +#define ADJD_S311_CAP_BLUE 0x08
> +#define ADJD_S311_CAP_CLEAR 0x09
> +#define ADJD_S311_INT_RED_LO 0x0a
> +#define ADJD_S311_INT_RED_HI 0x0b
> +#define ADJD_S311_INT_GREEN_LO 0x0c
> +#define ADJD_S311_INT_GREEN_HI 0x0d
> +#define ADJD_S311_INT_BLUE_LO 0x0e
> +#define ADJD_S311_INT_BLUE_HI 0x0f
> +#define ADJD_S311_INT_CLEAR_LO 0x10
> +#define ADJD_S311_INT_CLEAR_HI 0x11
> +#define ADJD_S311_DATA_RED_LO 0x40
> +#define ADJD_S311_DATA_RED_HI 0x41
> +#define ADJD_S311_DATA_GREEN_LO 0x42
> +#define ADJD_S311_DATA_GREEN_HI 0x43
> +#define ADJD_S311_DATA_BLUE_LO 0x44
> +#define ADJD_S311_DATA_BLUE_HI 0x45
> +#define ADJD_S311_DATA_CLEAR_LO 0x46
> +#define ADJD_S311_DATA_CLEAR_HI 0x47
> +#define ADJD_S311_OFFSET_RED 0x48
> +#define ADJD_S311_OFFSET_GREEN 0x49
> +#define ADJD_S311_OFFSET_BLUE 0x4a
> +#define ADJD_S311_OFFSET_CLEAR 0x4b
> +
> +#define ADJD_S311_CTRL_GOFS 0x02
> +#define ADJD_S311_CTRL_GSSR 0x01
> +#define ADJD_S311_CAP_MASK 0x0f
> +#define ADJD_S311_INT_MASK 0x0fff
> +#define ADJD_S311_DATA_MASK 0x03ff
> +
> +struct adjd_s311_data {
> + struct i2c_client *client;
> + u16 *buffer;
> +};
> +
> +enum adjd_s311_channel_idx {
> + IDX_RED, IDX_GREEN, IDX_BLUE, IDX_CLEAR
> +};
> +
> +#define ADJD_S311_DATA_REG(chan) \
> + (ADJD_S311_DATA_RED_LO + (chan) * 2)
> +#define ADJD_S311_INT_REG(chan) \
> + (ADJD_S311_INT_RED_LO + (chan) * 2)
> +#define ADJD_S311_CAP_REG(chan) \
> + (ADJD_S311_CAP_RED + (chan))
> +
> +static int adjd_s311_req_data(struct iio_dev *indio_dev)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + int tries = 10;
> +
> + int ret = i2c_smbus_write_byte_data(data->client, ADJD_S311_CTRL,
> + ADJD_S311_CTRL_GSSR);
> + if (ret < 0)
> + return ret;
> +
> + while (tries--) {
> + ret = i2c_smbus_read_byte_data(data->client, ADJD_S311_CTRL);
> + if (ret < 0)
> + return ret;
> + if (!(ret & ADJD_S311_CTRL_GSSR))
> + break;
> + msleep(20);
> + }
> +
> + if (tries < 0) {
> + dev_err(&data->client->dev,
> + "adjd_s311_req_data() failed, data not ready\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int adjd_s311_read_data(struct iio_dev *indio_dev, u8 reg, int *val)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> +
> + int ret = adjd_s311_req_data(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_word_data(data->client, reg);
> + if (ret < 0)
> + return ret;
> +
> + *val = ret & ADJD_S311_DATA_MASK;
> +
> + return 0;
> +}
> +
> +static ssize_t adjd_s311_read_int_time(struct iio_dev *indio_dev,
> + uintptr_t private, const struct iio_chan_spec *chan, char *buf)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + s32 ret;
> +
> + ret = i2c_smbus_read_word_data(data->client,
> + ADJD_S311_INT_REG(chan->address));
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", ret & ADJD_S311_INT_MASK);
> +}
> +
> +static ssize_t adjd_s311_write_int_time(struct iio_dev *indio_dev,
> + uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
> + size_t len)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + unsigned long int_time;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &int_time);
> + if (ret)
> + return ret;
> +
> + if (int_time > ADJD_S311_INT_MASK)
> + return -EINVAL;
> +
> + ret = i2c_smbus_write_word_data(data->client,
> + ADJD_S311_INT_REG(chan->address), int_time);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +static irqreturn_t adjd_s311_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + struct iio_buffer *buffer = indio_dev->buffer;
> + int len = 0;
> + int i, j = 0;
> +
> + int ret = adjd_s311_req_data(indio_dev);
> + if (ret < 0)
> + goto done;
> +
> + for_each_set_bit(i, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> +
> + ret = i2c_smbus_read_word_data(data->client,
> + ADJD_S311_DATA_REG(i));
> + if (ret < 0)
> + goto done;
> +
> + data->buffer[j++] = ret & ADJD_S311_DATA_MASK;
> + len += 2;
> + }
> +
> + if (indio_dev->scan_timestamp)
> + *(s64 *)((phys_addr_t)data->buffer + ALIGN(len, sizeof(s64)))
> + = pf->timestamp;
> + iio_push_to_buffer(buffer, (u8 *)data->buffer, pf->timestamp);
> +
> +done:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static const struct iio_chan_spec_ext_info adjd_s311_ext_info[] = {
> + {
> + .name = "integration_time",
> + .read = adjd_s311_read_int_time,
> + .write = adjd_s311_write_int_time,
> + },
> + { }
> +};
> +
> +static const struct iio_chan_spec adjd_s311_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,There seems to be a extra space before the If.
> + .address = IDX_RED,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_RED,
> + .scan_index = 0,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + }, {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_GREEN,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_GREEN,
> + .scan_index = 1,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + }, {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_GREEN,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_BLUE,
> + .scan_index = 2,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + }, {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_CLEAR,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .scan_index = 3,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static int adjd_s311_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = adjd_s311_read_data(indio_dev, chan->address, val);
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + ret = i2c_smbus_read_byte_data(data->client,
> + ADJD_S311_CAP_REG(chan->address));
> + if (ret < 0)
> + return ret;
> + *val = ret & ADJD_S311_CAP_MASK;
> + return IIO_VAL_INT;
> + }
> + return -EINVAL;
> +}
> +
> +static int adjd_s311_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + if (val < 0 || val > ADJD_S311_CAP_MASK)
> + return -EINVAL;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + ADJD_S311_CAP_REG(chan->address), val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + data->buffer = krealloc(data->buffer, indio_dev->scan_bytes,
> + GFP_KERNEL);
> + if (!data->buffer)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static const struct iio_info adjd_s311_info = {
> + .read_raw = adjd_s311_read_raw,
> + .write_raw = adjd_s311_write_raw,
> + .update_scan_mode = adjd_s311_update_scan_mode,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int __devinit adjd_s311_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct adjd_s311_data *data;
> + struct iio_dev *indio_dev;
> + int err;
> +
> + indio_dev = iio_device_alloc(sizeof(*data));
> + if (indio_dev == NULL) {
> + err = -ENOMEM;
> + goto exit;
> + }
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &adjd_s311_info;
> + indio_dev->name = ADJD_S311_DRV_NAME;
> + indio_dev->channels = adjd_s311_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + err = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> + adjd_s311_trigger_handler, NULL);
> + if (err < 0)
> + goto exit_free_device;
> +
> + err = iio_device_register(indio_dev);
> + if (err)
> + goto exit_unreg_buffer;
> +
> + dev_info(&client->dev, "ADJD-S311 color sensor registered\n");
> +
> + return 0;
> +
> +exit_unreg_buffer:
> + iio_triggered_buffer_cleanup(indio_dev);
> +exit_free_device:
> + iio_device_free(indio_dev);
> +exit:
> + return err;
> +}
> +
> +static int __devexit adjd_s311_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + iio_triggered_buffer_cleanup(indio_dev);
> + kfree(data->buffer);
> + iio_device_free(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id adjd_s311_id[] = {
> + { "adjd_s311", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, adjd_s311_id);
> +
> +static struct i2c_driver adjd_s311_driver = {
> + .driver = {
> + .name = ADJD_S311_DRV_NAME,
> + },
> + .probe = adjd_s311_probe,
> + .remove = __devexit_p(adjd_s311_remove),
> + .id_table = adjd_s311_id,
> +};
> +module_i2c_driver(adjd_s311_driver);
> +
> +MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
> +MODULE_DESCRIPTION("ADJD-S311 color sensor");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver
2012-07-02 21:43 [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver Peter Meerwald
2012-07-03 7:30 ` Lars-Peter Clausen
@ 2012-07-08 9:22 ` Jonathan Cameron
2012-07-10 21:29 ` Peter Meerwald
1 sibling, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2012-07-08 9:22 UTC (permalink / raw)
To: Peter Meerwald; +Cc: linux-iio, lars
On 07/02/2012 10:43 PM, Peter Meerwald wrote:
> sensor has 4 channels (10-bit each, R/G/B and clear), sensitivity
> and gain is controlled in the driver by ext_info integration_time
> and CHAN_INFO_HARDWAREGAIN
>
> driver supports triggered buffer and IIO_CHAN_INFO_RAW to get the
> sensor data
>
> patch depends on IIO_MOD_LIGHT_RED etc, the patch providing that
> (http://permalink.gmane.org/gmane.linux.kernel.iio/4354) has not been
> merged yet, a new proposal is following
>
> v4: address comments by Lars-Peter Clausen
> * make sure trigger handler is exited with iio_trigger_notify_done()
> and IRQ_HANDLED
> * kfree()/kalloc() -> krealloc()
>
> v3:
> * fix warnings
>
> v2: address comments by Lars-Peter Clausen
> * buffer allocation now in update_scan_mode instead of in trigger
> handler
> * simplify trigger code (assume active_scan_mask is not empty, use
> for_each_set_bit, use iio_push_to_buffer)
> * reorder entry in Makefile and Kconfig
> * fix remove
>
Couple of minor comments inline.
Mostly they are suggested little tweaks to shorten / simplify code.
The question of where to take the timestamp is more interesting as
at least in theory any change to that in the future would be an abi
change (be it a very subtle one!)
Nice driver by the way!
Jonathan
> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> ---
> drivers/iio/light/Kconfig | 12 ++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/adjd_s311.c | 396 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 409 insertions(+)
> create mode 100644 drivers/iio/light/adjd_s311.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index f3ea90d..91d15d2 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -3,6 +3,18 @@
> #
> menu "Light sensors"
>
> +config ADJD_S311
> + tristate "ADJD-S311-CR999 digital color sensor"
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + depends on I2C
> + help
> + If you say yes here you get support for the Avago ADJD-S311-CR999
> + digital color light sensor.
> +
> + This driver can also be built as a module. If so, the module
> + will be called adjd_s311.
> +
> config SENSORS_LM3533
> tristate "LM3533 ambient light sensor"
> depends on MFD_LM3533
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 06fa4d3..13f8a78 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -2,5 +2,6 @@
> # Makefile for IIO Light sensors
> #
>
> +obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
> obj-$(CONFIG_VCNL4000) += vcnl4000.o
> diff --git a/drivers/iio/light/adjd_s311.c b/drivers/iio/light/adjd_s311.c
> new file mode 100644
> index 0000000..029d6e6
> --- /dev/null
> +++ b/drivers/iio/light/adjd_s311.c
> @@ -0,0 +1,396 @@
> +/*
> + * adjd_s311.c - Support for ADJD-S311-CR999 digital color sensor
> + *
> + * Copyright (C) 2012 Peter Meerwald <pmeerw@pmeerw.net>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * driver for ADJD-S311-CR999 digital color sensor (10-bit channels for
> + * red, green, blue, clear); 7-bit I2C slave address 0x74
> + *
> + * limitations: no calibration, no offset mode, no sleep mode
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/bitmap.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define ADJD_S311_DRV_NAME "adjd_s311"
> +
> +#define ADJD_S311_CTRL 0x00
> +#define ADJD_S311_CONFIG 0x01
> +#define ADJD_S311_CAP_RED 0x06
> +#define ADJD_S311_CAP_GREEN 0x07
> +#define ADJD_S311_CAP_BLUE 0x08
> +#define ADJD_S311_CAP_CLEAR 0x09
Hmm.. some of these clearly aren't used, but I guess they
provide useful documentation so lets leave them for now..
> +#define ADJD_S311_INT_RED_LO 0x0a
> +#define ADJD_S311_INT_RED_HI 0x0b
> +#define ADJD_S311_INT_GREEN_LO 0x0c
> +#define ADJD_S311_INT_GREEN_HI 0x0d
> +#define ADJD_S311_INT_BLUE_LO 0x0e
> +#define ADJD_S311_INT_BLUE_HI 0x0f
> +#define ADJD_S311_INT_CLEAR_LO 0x10
> +#define ADJD_S311_INT_CLEAR_HI 0x11
> +#define ADJD_S311_DATA_RED_LO 0x40
> +#define ADJD_S311_DATA_RED_HI 0x41
> +#define ADJD_S311_DATA_GREEN_LO 0x42
> +#define ADJD_S311_DATA_GREEN_HI 0x43
> +#define ADJD_S311_DATA_BLUE_LO 0x44
> +#define ADJD_S311_DATA_BLUE_HI 0x45
> +#define ADJD_S311_DATA_CLEAR_LO 0x46
> +#define ADJD_S311_DATA_CLEAR_HI 0x47
> +#define ADJD_S311_OFFSET_RED 0x48
> +#define ADJD_S311_OFFSET_GREEN 0x49
> +#define ADJD_S311_OFFSET_BLUE 0x4a
> +#define ADJD_S311_OFFSET_CLEAR 0x4b
> +
> +#define ADJD_S311_CTRL_GOFS 0x02
> +#define ADJD_S311_CTRL_GSSR 0x01
> +#define ADJD_S311_CAP_MASK 0x0f
> +#define ADJD_S311_INT_MASK 0x0fff
> +#define ADJD_S311_DATA_MASK 0x03ff
> +
> +struct adjd_s311_data {
> + struct i2c_client *client;
> + u16 *buffer;
Given this an i2c device and hence I don't think we
have any cacheline issues, you could just embed the
buffer at it's maximum size here and drop the dynamic
allocation stuff.
> +};
> +
> +enum adjd_s311_channel_idx {
> + IDX_RED, IDX_GREEN, IDX_BLUE, IDX_CLEAR
> +};
> +
These look short enough to be on single lines to me..
> +#define ADJD_S311_DATA_REG(chan) \
> + (ADJD_S311_DATA_RED_LO + (chan) * 2)
(just checking given my editor is set to 80 chars wide ;)
#define ADJD_S311_DATA_REG(chan) (ADJD_S311_DATA_RED_LO + (chan) * 2)
> +#define ADJD_S311_INT_REG(chan) \
> + (ADJD_S311_INT_RED_LO + (chan) * 2)
> +#define ADJD_S311_CAP_REG(chan) \
> + (ADJD_S311_CAP_RED + (chan))
> +
> +static int adjd_s311_req_data(struct iio_dev *indio_dev)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + int tries = 10;
> +
> + int ret = i2c_smbus_write_byte_data(data->client, ADJD_S311_CTRL,
> + ADJD_S311_CTRL_GSSR);
> + if (ret < 0)
> + return ret;
> +
> + while (tries--) {
> + ret = i2c_smbus_read_byte_data(data->client, ADJD_S311_CTRL);
> + if (ret < 0)
> + return ret;
> + if (!(ret & ADJD_S311_CTRL_GSSR))
> + break;
> + msleep(20);
> + }
> +
> + if (tries < 0) {
> + dev_err(&data->client->dev,
> + "adjd_s311_req_data() failed, data not ready\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int adjd_s311_read_data(struct iio_dev *indio_dev, u8 reg, int *val)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> +
> + int ret = adjd_s311_req_data(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_smbus_read_word_data(data->client, reg);
> + if (ret < 0)
> + return ret;
> +
> + *val = ret & ADJD_S311_DATA_MASK;
> +
> + return 0;
> +}
> +
> +static ssize_t adjd_s311_read_int_time(struct iio_dev *indio_dev,
> + uintptr_t private, const struct iio_chan_spec *chan, char *buf)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + s32 ret;
> +
> + ret = i2c_smbus_read_word_data(data->client,
> + ADJD_S311_INT_REG(chan->address));
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", ret & ADJD_S311_INT_MASK);
> +}
> +
> +static ssize_t adjd_s311_write_int_time(struct iio_dev *indio_dev,
> + uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
> + size_t len)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + unsigned long int_time;
> + int ret;
> +
> + ret = kstrtoul(buf, 10, &int_time);
> + if (ret)
> + return ret;
> +
> + if (int_time > ADJD_S311_INT_MASK)
> + return -EINVAL;
> +
> + ret = i2c_smbus_write_word_data(data->client,
> + ADJD_S311_INT_REG(chan->address), int_time);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +static irqreturn_t adjd_s311_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + struct iio_buffer *buffer = indio_dev->buffer;
> + int len = 0;
> + int i, j = 0;
> +
> + int ret = adjd_s311_req_data(indio_dev);
> + if (ret < 0)
> + goto done;
> +
> + for_each_set_bit(i, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
nitpick of the day: Blank line here is a bit unusual...
> +
> + ret = i2c_smbus_read_word_data(data->client,
> + ADJD_S311_DATA_REG(i));
> + if (ret < 0)
> + goto done;
> +
> + data->buffer[j++] = ret & ADJD_S311_DATA_MASK;
> + len += 2;
> + }
> +
> + if (indio_dev->scan_timestamp)
> + *(s64 *)((phys_addr_t)data->buffer + ALIGN(len, sizeof(s64)))
> + = pf->timestamp;
> + iio_push_to_buffer(buffer, (u8 *)data->buffer, pf->timestamp);
> +
> +done:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
hmm.. one to add to our info_mask perhaps? Its a simple numeric value
so easy enough to do and it's clearly common enough. That can happen
as a later patch though.
> +static const struct iio_chan_spec_ext_info adjd_s311_ext_info[] = {
> + {
> + .name = "integration_time",
> + .read = adjd_s311_read_int_time,
> + .write = adjd_s311_write_int_time,
> + },
> + { }
> +};
> +
I'd personally have been tempted by a macro for these channel
setups e.g. ADJD_S311_CHANNEL(scan_index, modifier).
That's mainly because I'm forever making typos in these and
having just one to check makes this less likely ;)
> +static const struct iio_chan_spec adjd_s311_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_RED,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_RED,
> + .scan_index = 0,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + }, {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_GREEN,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_GREEN,
> + .scan_index = 1,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + }, {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_GREEN,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_BLUE,
> + .scan_index = 2,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + }, {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = IDX_CLEAR,
> + .info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |
> + IIO_CHAN_INFO_HARDWAREGAIN_SEPARATE_BIT,
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .scan_index = 3,
> + .scan_type = IIO_ST('u', 10, 16, 0),
> + .ext_info = adjd_s311_ext_info,
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static int adjd_s311_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = adjd_s311_read_data(indio_dev, chan->address, val);
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + ret = i2c_smbus_read_byte_data(data->client,
> + ADJD_S311_CAP_REG(chan->address));
> + if (ret < 0)
> + return ret;
> + *val = ret & ADJD_S311_CAP_MASK;
> + return IIO_VAL_INT;
> + }
> + return -EINVAL;
> +}
> +
for consistency with the previous function, perhaps return
directly within the switch statement.
> +static int adjd_s311_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + if (val < 0 || val > ADJD_S311_CAP_MASK)
> + return -EINVAL;
> +
> + ret = i2c_smbus_write_byte_data(data->client,
> + ADJD_S311_CAP_REG(chan->address), val);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
As stated above, I can't immediately see any real advantage to
doing this allocation dynamically.
> +static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> + data->buffer = krealloc(data->buffer, indio_dev->scan_bytes,
> + GFP_KERNEL);
> + if (!data->buffer)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static const struct iio_info adjd_s311_info = {
> + .read_raw = adjd_s311_read_raw,
> + .write_raw = adjd_s311_write_raw,
> + .update_scan_mode = adjd_s311_update_scan_mode,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int __devinit adjd_s311_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct adjd_s311_data *data;
> + struct iio_dev *indio_dev;
> + int err;
> +
> + indio_dev = iio_device_alloc(sizeof(*data));
> + if (indio_dev == NULL) {
> + err = -ENOMEM;
> + goto exit;
> + }
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &adjd_s311_info;
> + indio_dev->name = ADJD_S311_DRV_NAME;
> + indio_dev->channels = adjd_s311_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adjd_s311_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
Given you are using triggers form elsewhere and capturing on demand,
does it not make sense to grab the timestamp in adjd_s311_trigger_handler
just grabbing the data as that will be nearer the true time of the
reading?
> + err = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> + adjd_s311_trigger_handler, NULL);
> + if (err < 0)
> + goto exit_free_device;
> +
> + err = iio_device_register(indio_dev);
> + if (err)
> + goto exit_unreg_buffer;
> +
> + dev_info(&client->dev, "ADJD-S311 color sensor registered\n");
> +
> + return 0;
> +
> +exit_unreg_buffer:
> + iio_triggered_buffer_cleanup(indio_dev);
> +exit_free_device:
> + iio_device_free(indio_dev);
> +exit:
> + return err;
> +}
> +
> +static int __devexit adjd_s311_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct adjd_s311_data *data = iio_priv(indio_dev);
> +
> + iio_device_unregister(indio_dev);
> + iio_triggered_buffer_cleanup(indio_dev);
> + kfree(data->buffer);
> + iio_device_free(indio_dev);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id adjd_s311_id[] = {
> + { "adjd_s311", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, adjd_s311_id);
> +
> +static struct i2c_driver adjd_s311_driver = {
> + .driver = {
> + .name = ADJD_S311_DRV_NAME,
> + },
> + .probe = adjd_s311_probe,
> + .remove = __devexit_p(adjd_s311_remove),
> + .id_table = adjd_s311_id,
> +};
> +module_i2c_driver(adjd_s311_driver);
> +
> +MODULE_AUTHOR("Peter Meerwald <pmeerw@pmeerw.net>");
> +MODULE_DESCRIPTION("ADJD-S311 color sensor");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver
2012-07-08 9:22 ` Jonathan Cameron
@ 2012-07-10 21:29 ` Peter Meerwald
0 siblings, 0 replies; 5+ messages in thread
From: Peter Meerwald @ 2012-07-10 21:29 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, lars
> As stated above, I can't immediately see any real advantage to
> doing this allocation dynamically.
> > +static int adjd_s311_update_scan_mode(struct iio_dev *indio_dev,
> > + const unsigned long *scan_mask)
> > +{
> > + struct adjd_s311_data *data = iio_priv(indio_dev);
> > + data->buffer = krealloc(data->buffer, indio_dev->scan_bytes,
> > + GFP_KERNEL);
> > + if (!data->buffer)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
one problem is to compute the maximum buffer size; for this driver it is
easy: 4*2 + padding + timestamp -- padding turns out to be 0 assuming the
padding rule is set in stone; I think IIO core should do that computation
iio_compute_scan_bytes() is not directly callable; I'd rather keep this as
is
regards, p.
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver
2012-07-03 7:30 ` Lars-Peter Clausen
@ 2012-07-12 19:26 ` Jonathan Cameron
0 siblings, 0 replies; 5+ messages in thread
From: Jonathan Cameron @ 2012-07-12 19:26 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: Peter Meerwald, linux-iio
On 07/03/2012 08:30 AM, Lars-Peter Clausen wrote:
> On 07/02/2012 11:43 PM, Peter Meerwald wrote:
>> sensor has 4 channels (10-bit each, R/G/B and clear), sensitivity
>> and gain is controlled in the driver by ext_info integration_time
>> and CHAN_INFO_HARDWAREGAIN
>>
>> driver supports triggered buffer and IIO_CHAN_INFO_RAW to get the
>> sensor data
>>
>> patch depends on IIO_MOD_LIGHT_RED etc, the patch providing that
>> (http://permalink.gmane.org/gmane.linux.kernel.iio/4354) has not been
>> merged yet, a new proposal is following
>>
>> v4: address comments by Lars-Peter Clausen
>> * make sure trigger handler is exited with iio_trigger_notify_done()
>> and IRQ_HANDLED
>> * kfree()/kalloc() -> krealloc()
>>
>> v3:
>> * fix warnings
>>
>> v2: address comments by Lars-Peter Clausen
>> * buffer allocation now in update_scan_mode instead of in trigger
>> handler
>> * simplify trigger code (assume active_scan_mask is not empty, use
>> for_each_set_bit, use iio_push_to_buffer)
>> * reorder entry in Makefile and Kconfig
>> * fix remove
>>
>> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
>
> Looks good,
>
> Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>
Sorry Lars-Peter, I'd forgotten you'd done that for this earlier
version so didn't add it to v5 when merging. Peter, if you make minor
changes people 'normally' don't mind if you carry there acked-by
etc on with it.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-07-12 19:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-02 21:43 [PATCH v4] iio: add adjd_s311 I2C digital color sensor driver Peter Meerwald
2012-07-03 7:30 ` Lars-Peter Clausen
2012-07-12 19:26 ` Jonathan Cameron
2012-07-08 9:22 ` Jonathan Cameron
2012-07-10 21:29 ` Peter Meerwald
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.