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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx> > + * > + * 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@xxxxxxxxxx>"); > +MODULE_DESCRIPTION("ADJD-S311 color sensor"); > +MODULE_LICENSE("GPL"); > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
