On 06/22/2015 04:40 PM, Peter Meerwald wrote: > On Mon, 22 Jun 2015, Andreas Dannenberg wrote: > >> TI's opt3001 light sensor is a simple and yet powerful >> little device. The device provides 99% IR rejection, >> automatic full-scale, very low power consumption and >> measurements from 0.01 to 83k lux. >> >> This patch adds support for that device using the IIO >> framework. > > comments below > >> See http://www.ti.com/product/opt3001 for more information. >> >> Signed-off-by: Felipe Balbi <balbi@xxxxxx> >> Signed-off-by: Andreas Dannenberg <dannenberg@xxxxxx> >> >> --- >> Since an RFC of this patch was last submitted by Felipe Balbi a few >> changes have been made as listed below. Also I've since taken over >> ownership of this driver. >> >> * Remove hysteresis functionality from event interface >> * Remove unnecessary delay >> * Add missing mutex functionality >> * Make sensor reading interrupt-driven >> >> The individual commits can be reviewed at: >> http://git.ti.com/cgit/cgit.cgi/ti-analog-linux-kernel/adannenb-analog.git/log/?h=opt3001 >> >> drivers/iio/light/Kconfig | 10 + >> drivers/iio/light/Makefile | 1 + >> drivers/iio/light/opt3001.c | 801 ++++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 812 insertions(+) >> create mode 100644 drivers/iio/light/opt3001.c >> >> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig >> index 01a1a16..665b182 100644 >> --- a/drivers/iio/light/Kconfig >> +++ b/drivers/iio/light/Kconfig >> @@ -186,6 +186,16 @@ config TCS3414 >> This driver can also be built as a module. If so, the module >> will be called tcs3414. >> >> +config OPT3001 >> + tristate "Texas Instruments OPT3001 Light Sensor" >> + depends on I2C >> + help >> + If you say Y or M here, you get support for Texas Instruments >> + OPT3001 Ambient Light Sensor. >> + >> + If built as a dynamically linked module, it will be called >> + opt3001. >> + >> config TCS3472 >> tristate "TAOS TCS3472 color light-to-digital converter" >> depends on I2C >> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile >> index ad7c30f..a498631 100644 >> --- a/drivers/iio/light/Makefile >> +++ b/drivers/iio/light/Makefile >> @@ -17,6 +17,7 @@ obj-$(CONFIG_ISL29125) += isl29125.o >> obj-$(CONFIG_JSA1212) += jsa1212.o >> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o >> obj-$(CONFIG_LTR501) += ltr501.o >> +obj-$(CONFIG_OPT3001) += opt3001.o >> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o >> obj-$(CONFIG_TCS3414) += tcs3414.o >> obj-$(CONFIG_TCS3472) += tcs3472.o >> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c >> new file mode 100644 >> index 0000000..70ee33f >> --- /dev/null >> +++ b/drivers/iio/light/opt3001.c >> @@ -0,0 +1,801 @@ >> +/** >> + * opt3001.c - Texas Instruments OPT3001 Light Sensor >> + * >> + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com >> + * >> + * Author: Andreas Dannenberg <dannenberg@xxxxxx> >> + * Based on previous work from: Felipe Balbi <balbi@xxxxxx> >> + * >> + * This program is free software: you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 of the License >> + * as published by the Free Software Foundation. >> + * >> + * 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. >> + */ >> + >> +#include <linux/bitops.h> >> +#include <linux/delay.h> >> +#include <linux/device.h> >> +#include <linux/i2c.h> >> +#include <linux/interrupt.h> >> +#include <linux/irq.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/slab.h> >> +#include <linux/types.h> >> + >> +#include <linux/iio/events.h> >> +#include <linux/iio/iio.h> >> +#include <linux/iio/sysfs.h> >> + >> +#define OPT3001_RESULT 0x00 >> +#define OPT3001_CONFIGURATION 0x01 >> +#define OPT3001_LOW_LIMIT 0x02 >> +#define OPT3001_HIGH_LIMIT 0x03 >> +#define OPT3001_MANUFACTURER_ID 0x7e >> +#define OPT3001_DEVICE_ID 0x7f >> + >> +#define OPT3001_CONFIGURATION_RN_MASK (0xf << 12) >> +#define OPT3001_CONFIGURATION_RN_AUTO (0xc << 12) >> + >> +#define OPT3001_CONFIGURATION_CT BIT(11) >> + >> +#define OPT3001_CONFIGURATION_M_MASK (3 << 9) >> +#define OPT3001_CONFIGURATION_M_SHUTDOWN (0 << 9) >> +#define OPT3001_CONFIGURATION_M_SINGLE (1 << 9) >> +#define OPT3001_CONFIGURATION_M_CONTINUOUS (2 << 9) /* also 3 << 9 */ >> + >> +#define OPT3001_CONFIGURATION_OVF BIT(8) >> +#define OPT3001_CONFIGURATION_CRF BIT(7) >> +#define OPT3001_CONFIGURATION_FH BIT(6) >> +#define OPT3001_CONFIGURATION_FL BIT(5) >> +#define OPT3001_CONFIGURATION_L BIT(4) >> +#define OPT3001_CONFIGURATION_POL BIT(3) >> +#define OPT3001_CONFIGURATION_ME BIT(2) >> + >> +#define OPT3001_CONFIGURATION_FC_MASK (3 << 0) >> + >> +/* The end-of-conversion enable is located in the low-limit register */ >> +#define OPT3001_LOW_LIMIT_EOC_ENABLE 0xc000 >> + >> +#define OPT3001_REG_EXPONENT(n) ((n) >> 12) >> +#define OPT3001_REG_MANTISSA(n) ((n) & 0xfff) >> + >> +/* >> + * Time to wait for conversion result to be ready. The device datasheet >> + * worst-case max value is 880ms. Add some slack to be on the safe side. >> + */ >> +#define OPT3001_RESULT_READY_TIMEOUT msecs_to_jiffies(1000) >> + >> +struct opt3001 { >> + struct i2c_client *client; >> + struct device *dev; >> + >> + struct mutex lock; >> + u16 ok_to_ignore_lock:1; >> + u16 result_ready:1; >> + wait_queue_head_t result_ready_queue; >> + u16 result; >> + >> + u32 int_time; >> + u32 mode; >> + >> + u16 high_thresh_mantissa; >> + u16 low_thresh_mantissa; >> + >> + u8 high_thresh_exp; >> + u8 low_thresh_exp; >> +}; >> + >> +struct opt3001_scale { >> + int val; >> + int val2; >> +}; >> + >> +static const struct opt3001_scale opt3001_scales[] = { >> + { >> + .val = 40, >> + .val2 = 950000, >> + }, >> + { >> + .val = 81, >> + .val2 = 900000, >> + }, >> + { >> + .val = 81, >> + .val2 = 900000, >> + }, >> + { >> + .val = 163, >> + .val2 = 800000, >> + }, >> + { >> + .val = 327, >> + .val2 = 600000, >> + }, >> + { >> + .val = 655, >> + .val2 = 200000, >> + }, >> + { >> + .val = 1310, >> + .val2 = 400000, >> + }, >> + { >> + .val = 2620, >> + .val2 = 800000, >> + }, >> + { >> + .val = 5241, >> + .val2 = 600000, >> + }, >> + { >> + .val = 10483, >> + .val2 = 200000, >> + }, >> + { >> + .val = 20966, >> + .val2 = 400000, >> + }, >> + { >> + .val = 83865, >> + .val2 = 600000, >> + }, >> +}; >> + >> +static int opt3001_find_scale(const struct opt3001 *opt, int val, >> + int val2, u8 *exponent) >> +{ >> + int i; >> + >> + for (i = 0; i < ARRAY_SIZE(opt3001_scales); i++) { >> + const struct opt3001_scale *scale = &opt3001_scales[i]; >> + >> + if (val <= scale->val && val2 <= scale->val2) { > > this looks suspicious; if val:=1 and val2:=999999 I think the exponent > should be 0, but the condition won't match Peter - thanks for your time reviewing this and youre valuable feedback. Good catch! You are right there is a mistake in the logic. I have since changed the algorithm to form a combined integer and fractional (micro) value for comparison purposes and performed testing to make sure it works. While doing that I found and fixed an issue with a duplicated entry in the associated lookup table. >> + *exponent = i; >> + return 0; >> + } >> + } >> + >> + return -EINVAL; >> +} >> + >> +static void opt3001_to_iio_ret(struct opt3001 *opt, u8 exponent, >> + u16 mantissa, int *val, int *val2) >> +{ >> + int lux; >> + >> + lux = 10 * (mantissa << exponent); >> + *val = lux / 1000; >> + *val2 = (lux - (*val * 1000)) * 1000; >> +} >> + >> +static void opt3001_set_mode(struct opt3001 *opt, u16 *reg, u16 mode) >> +{ >> + *reg &= ~OPT3001_CONFIGURATION_M_MASK; >> + *reg |= mode; >> + opt->mode = mode; >> +} >> + >> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.8"); >> + >> +static struct attribute *opt3001_attributes[] = { >> + &iio_const_attr_integration_time_available.dev_attr.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group opt3001_attribute_group = { >> + .attrs = opt3001_attributes, >> +}; >> + >> +static const struct iio_event_spec opt3001_event_spec[] = { >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_RISING, >> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >> + BIT(IIO_EV_INFO_ENABLE), >> + }, >> + { >> + .type = IIO_EV_TYPE_THRESH, >> + .dir = IIO_EV_DIR_FALLING, >> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | >> + BIT(IIO_EV_INFO_ENABLE), >> + }, >> +}; >> + >> +static const struct iio_chan_spec opt3001_channels[] = { >> + { >> + .type = IIO_LIGHT, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) | >> + BIT(IIO_CHAN_INFO_INT_TIME), >> + .event_spec = opt3001_event_spec, >> + .num_event_specs = ARRAY_SIZE(opt3001_event_spec), >> + }, >> + IIO_CHAN_SOFT_TIMESTAMP(1), >> +}; >> + >> +static int opt3001_get_lux(struct opt3001 *opt, int *val, int *val2) >> +{ >> + int ret; >> + u16 mantissa; >> + u16 reg; >> + u8 exponent; >> + u16 value; >> + >> + /* >> + * Enable the end-of-conversion interrupt mechanism. Note that doing >> + * so will overwrite the low-level limit value however we will restore >> + * this value later on. >> + */ >> + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT, >> + OPT3001_LOW_LIMIT_EOC_ENABLE); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", >> + OPT3001_LOW_LIMIT); >> + return ret; >> + } >> + >> + /* Reset data-ready indicator flag (will be set in the IRQ routine) */ >> + opt->result_ready = false; >> + >> + /* Allow IRQ to access the device despite lock being set */ >> + opt->ok_to_ignore_lock = true; >> + >> + /* Configure for single-conversion mode and start a new conversion */ >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + reg = ret; >> + opt3001_set_mode(opt, ®, OPT3001_CONFIGURATION_M_SINGLE); >> + >> + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION, >> + reg); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + /* Wait for the IRQ to indicate the conversion is complete */ >> + ret = wait_event_timeout(opt->result_ready_queue, opt->result_ready, >> + OPT3001_RESULT_READY_TIMEOUT); >> + >> + /* Disallow IRQ to access the device while lock is active */ >> + opt->ok_to_ignore_lock = false; >> + >> + if (ret == 0) >> + return -ETIMEDOUT; >> + else if (ret < 0) >> + return ret; >> + >> + /* >> + * Disable the end-of-conversion interrupt mechanism by restoring the >> + * low-level limit value (clearing OPT3001_LOW_LIMIT_EOC_ENABLE). Note >> + * that selectively clearing those enable bits would affect the actual >> + * limit value due to bit-overlap and therefore can't be done. >> + */ >> + value = opt->low_thresh_exp << 12 | opt->low_thresh_mantissa; >> + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_LOW_LIMIT, >> + value); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", >> + OPT3001_LOW_LIMIT); >> + return ret; >> + } >> + >> + exponent = OPT3001_REG_EXPONENT(opt->result); >> + mantissa = OPT3001_REG_MANTISSA(opt->result); >> + >> + opt3001_to_iio_ret(opt, exponent, mantissa, val, val2); >> + >> + return IIO_VAL_INT_PLUS_MICRO; >> +} >> + >> +static int opt3001_get_int_time(struct opt3001 *opt, int *val, int *val2) >> +{ >> + *val = 0; >> + *val2 = opt->int_time; >> + >> + return IIO_VAL_INT_PLUS_MICRO; >> +} >> + >> +static int opt3001_set_int_time(struct opt3001 *opt, int time) >> +{ >> + int ret; >> + u16 reg; >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + reg = ret; >> + >> + switch (time) { >> + case 100000: >> + reg &= ~OPT3001_CONFIGURATION_CT; >> + opt->int_time = 100000; >> + break; >> + case 800000: >> + reg |= OPT3001_CONFIGURATION_CT; >> + opt->int_time = 800000; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + return i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION, >> + reg); >> +} >> + >> +static int opt3001_read_raw(struct iio_dev *iio, >> + struct iio_chan_spec const *chan, int *val, int *val2, >> + long mask) >> +{ >> + struct opt3001 *opt = iio_priv(iio); >> + int ret = 0; > > no need to initialize ret to 0 Ok will remove. >> + >> + if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) >> + return -EBUSY; >> + >> + if (chan->type != IIO_LIGHT) >> + return -EINVAL; >> + >> + mutex_lock(&opt->lock); >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_PROCESSED: >> + ret = opt3001_get_lux(opt, val, val2); >> + break; >> + case IIO_CHAN_INFO_INT_TIME: >> + ret = opt3001_get_int_time(opt, val, val2); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + mutex_unlock(&opt->lock); >> + >> + return ret; >> +} >> + >> +static int opt3001_write_raw(struct iio_dev *iio, >> + struct iio_chan_spec const *chan, int val, int val2, >> + long mask) >> +{ >> + struct opt3001 *opt = iio_priv(iio); >> + int ret = 0; > > no need to initialize ret to 0 Ok will remove. >> + >> + if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) >> + return -EBUSY; >> + >> + if (chan->type != IIO_LIGHT) >> + return -EINVAL; >> + >> + if (mask != IIO_CHAN_INFO_INT_TIME) >> + return -EINVAL; >> + >> + mutex_lock(&opt->lock); > > check that val == 0 Good point. This will make the interface more robust. >> + ret = opt3001_set_int_time(opt, val2); >> + mutex_unlock(&opt->lock); >> + >> + return ret; >> +} >> + >> +static int opt3001_read_event_value(struct iio_dev *iio, >> + const struct iio_chan_spec *chan, enum iio_event_type type, >> + enum iio_event_direction dir, enum iio_event_info info, >> + int *val, int *val2) >> +{ >> + struct opt3001 *opt = iio_priv(iio); >> + int ret = IIO_VAL_INT_PLUS_MICRO; >> + >> + mutex_lock(&opt->lock); >> + >> + switch (dir) { >> + case IIO_EV_DIR_RISING: >> + opt3001_to_iio_ret(opt, opt->high_thresh_exp, >> + opt->high_thresh_mantissa, val, val2); >> + break; >> + case IIO_EV_DIR_FALLING: >> + opt3001_to_iio_ret(opt, opt->low_thresh_exp, >> + opt->low_thresh_mantissa, val, val2); >> + break; >> + default: >> + ret = -EINVAL; >> + } >> + >> + mutex_unlock(&opt->lock); >> + >> + return ret; >> +} >> + >> +static int opt3001_write_event_value(struct iio_dev *iio, >> + const struct iio_chan_spec *chan, enum iio_event_type type, >> + enum iio_event_direction dir, enum iio_event_info info, >> + int val, int val2) >> +{ >> + struct opt3001 *opt = iio_priv(iio); >> + int ret = 0; > > no need to initialize ret to 0 Will remove. >> + >> + u16 mantissa; >> + u16 value; >> + u16 reg; >> + >> + u8 exponent; >> + >> + mutex_lock(&opt->lock); >> + >> + ret = opt3001_find_scale(opt, val, val2, &exponent); >> + if (ret < 0) { >> + dev_err(opt->dev, "can't find scale for %d.%d\n", val, val2); > > should be %d.%06u? Good point. Definitely need the leading zeroes here. Will change. I've also added a check to make sure val is not negative (the OPT3001 doesn't support negative lux values which if you think about it kind of makes sense). >> + goto err; >> + } >> + >> + mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; >> + value = exponent << 12 | mantissa; > > parenthesis would make it clearer; or a MACRO() to compute the value? > >> + >> + switch (dir) { >> + case IIO_EV_DIR_RISING: >> + reg = OPT3001_HIGH_LIMIT; >> + opt->high_thresh_mantissa = mantissa; >> + opt->high_thresh_exp = exponent; >> + break; >> + case IIO_EV_DIR_FALLING: >> + reg = OPT3001_LOW_LIMIT; >> + opt->low_thresh_mantissa = mantissa; >> + opt->low_thresh_exp = exponent; >> + break; >> + default: >> + ret = -EINVAL; >> + goto err; >> + } >> + >> + ret = i2c_smbus_write_word_swapped(opt->client, reg, value); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", reg); >> + goto err; >> + } >> + >> +err: >> + mutex_unlock(&opt->lock); >> + >> + return ret; >> +} >> + >> +static int opt3001_read_event_config(struct iio_dev *iio, >> + const struct iio_chan_spec *chan, enum iio_event_type type, >> + enum iio_event_direction dir) >> +{ >> + struct opt3001 *opt = iio_priv(iio); >> + >> + return opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS; >> +} >> + >> +static int opt3001_write_event_config(struct iio_dev *iio, >> + const struct iio_chan_spec *chan, enum iio_event_type type, >> + enum iio_event_direction dir, int state) >> +{ >> + struct opt3001 *opt = iio_priv(iio); >> + int ret; >> + u16 mode; >> + u16 reg; >> + >> + if (state && opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) >> + return 0; >> + >> + if (!state && opt->mode == OPT3001_CONFIGURATION_M_SHUTDOWN) >> + return 0; >> + >> + mutex_lock(&opt->lock); >> + >> + mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS >> + : OPT3001_CONFIGURATION_M_SHUTDOWN; >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_CONFIGURATION); >> + goto err; >> + } >> + >> + reg = ret; >> + opt3001_set_mode(opt, ®, mode); >> + >> + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION, >> + reg); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", >> + OPT3001_CONFIGURATION); >> + goto err; >> + } >> + >> +err: >> + mutex_unlock(&opt->lock); >> + >> + return ret; >> +} >> + >> +static const struct iio_info opt3001_info = { >> + .driver_module = THIS_MODULE, >> + .attrs = &opt3001_attribute_group, >> + .read_raw = opt3001_read_raw, >> + .write_raw = opt3001_write_raw, >> + .read_event_value = opt3001_read_event_value, >> + .write_event_value = opt3001_write_event_value, >> + .read_event_config = opt3001_read_event_config, >> + .write_event_config = opt3001_write_event_config, >> +}; >> + >> +static int opt3001_read_id(struct opt3001 *opt) >> +{ >> + char manufacturer[2]; >> + u16 device_id; >> + int ret; >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_MANUFACTURER_ID); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_MANUFACTURER_ID); >> + return ret; >> + } >> + >> + manufacturer[0] = ret >> 8; >> + manufacturer[1] = ret & 0xff; >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_DEVICE_ID); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_DEVICE_ID); >> + return ret; >> + } >> + >> + device_id = ret; >> + >> + dev_info(opt->dev, "Found %c%c OPT%04x\n", manufacturer[0], >> + manufacturer[1], device_id); >> + >> + return 0; >> +} >> + >> +static int opt3001_configure(struct opt3001 *opt) >> +{ >> + int ret; >> + u16 reg; >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + reg = ret; >> + >> + /* Enable automatic full-scale setting mode */ >> + reg &= ~OPT3001_CONFIGURATION_RN_MASK; >> + reg |= OPT3001_CONFIGURATION_RN_AUTO; >> + >> + /* Reflect status of the device's integration time setting */ >> + if (reg & OPT3001_CONFIGURATION_CT) >> + opt->int_time = 800000; >> + else >> + opt->int_time = 100000; >> + >> + /* Ensure device is in shutdown initially */ >> + opt3001_set_mode(opt, ®, OPT3001_CONFIGURATION_M_SHUTDOWN); >> + >> + /* Configure for latched window-style comparison operation */ >> + reg |= OPT3001_CONFIGURATION_L; >> + reg &= ~OPT3001_CONFIGURATION_POL; >> + reg &= ~OPT3001_CONFIGURATION_ME; >> + reg &= ~OPT3001_CONFIGURATION_FC_MASK; >> + >> + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION, >> + reg); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_LOW_LIMIT); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_LOW_LIMIT); >> + return ret; >> + } >> + >> + opt->low_thresh_mantissa = OPT3001_REG_MANTISSA(ret); >> + opt->low_thresh_exp = OPT3001_REG_EXPONENT(ret); >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_HIGH_LIMIT); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_HIGH_LIMIT); >> + return ret; >> + } >> + >> + opt->high_thresh_mantissa = OPT3001_REG_MANTISSA(ret); >> + opt->high_thresh_exp = OPT3001_REG_EXPONENT(ret); >> + >> + return 0; >> +} >> + >> +static irqreturn_t opt3001_irq(int irq, void *_iio) >> +{ >> + struct iio_dev *iio = _iio; >> + struct opt3001 *opt = iio_priv(iio); >> + int ret; >> + >> + if (!opt->ok_to_ignore_lock) >> + mutex_lock(&opt->lock); >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_CONFIGURATION); >> + goto out; >> + } >> + >> + if ((ret & OPT3001_CONFIGURATION_M_MASK) == >> + OPT3001_CONFIGURATION_M_CONTINUOUS) { >> + if (ret & OPT3001_CONFIGURATION_FH) >> + iio_push_event(iio, >> + IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_RISING), >> + iio_get_time_ns()); >> + if (ret & OPT3001_CONFIGURATION_FL) >> + iio_push_event(iio, >> + IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0, >> + IIO_EV_TYPE_THRESH, >> + IIO_EV_DIR_FALLING), >> + iio_get_time_ns()); >> + } else if (ret & OPT3001_CONFIGURATION_CRF) { >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_RESULT); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_RESULT); >> + goto out; >> + } >> + opt->result = ret; >> + opt->result_ready = true; >> + wake_up(&opt->result_ready_queue); >> + } >> + >> +out: >> + if (!opt->ok_to_ignore_lock) >> + mutex_unlock(&opt->lock); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static int opt3001_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct device *dev = &client->dev; >> + >> + struct iio_dev *iio; >> + struct opt3001 *opt; >> + int irq = client->irq; >> + int ret = -ENOMEM; >> + >> + iio = iio_device_alloc(sizeof(*opt)); > > could use devm_ Ok will change. I noticed that devm_ was present in another version of this driver that I saw, will go double-check to make sure all previous feedback from you and Jonathan Cameron really made it. >> + if (!iio) >> + goto err_iio_alloc; >> + >> + opt = iio_priv(iio); >> + opt->client = client; >> + opt->dev = dev; >> + >> + mutex_init(&opt->lock); >> + init_waitqueue_head(&opt->result_ready_queue); >> + i2c_set_clientdata(client, iio); >> + >> + ret = opt3001_read_id(opt); >> + if (ret) >> + goto err_read; >> + >> + ret = opt3001_configure(opt); >> + if (ret) >> + goto err_read; >> + >> + iio->name = client->name; >> + iio->channels = opt3001_channels; >> + iio->num_channels = ARRAY_SIZE(opt3001_channels); >> + iio->dev.parent = dev; >> + iio->modes = INDIO_DIRECT_MODE; >> + iio->info = &opt3001_info; >> + >> + ret = iio_device_register(iio); >> + if (ret) { >> + dev_err(dev, "failed to register IIO device\n"); >> + goto err_read; >> + } >> + >> + ret = request_threaded_irq(irq, NULL, opt3001_irq, >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> + "opt3001", iio); > > shouldn't this be requested before registering the IIO device? There was a discussion regarding this previously between Felipe (original author) and Jonathan Cameron, see here (towards the end): http://article.gmane.org/gmane.linux.kernel.iio/13447 There was a point made that when changing it you could end up triggering the IRQ handler when not everything is in place yet (data structures accessed from the handler, etc.) I'm thinking it would be nice if the iio_device_register() and request_threaded_irq() could be done atomicly but I guess we don't have this option. I can change the order but I share the concerns voiced by Felipe. >> + if (ret) { >> + dev_err(dev, "failed to request IRQ #%d\n", irq); >> + goto err_request_irq; >> + } >> + >> + return 0; >> + >> +err_request_irq: >> + iio_device_unregister(iio); >> + >> +err_read: >> + iio_device_free(iio); >> + >> +err_iio_alloc: >> + return ret; >> +} >> + >> +static int opt3001_remove(struct i2c_client *client) >> +{ >> + struct iio_dev *iio = i2c_get_clientdata(client); >> + struct opt3001 *opt = iio_priv(iio); >> + int ret; >> + u16 reg; >> + >> + free_irq(client->irq, iio); >> + iio_device_unregister(iio); >> + >> + ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to read register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + reg = ret; >> + opt3001_set_mode(opt, ®, OPT3001_CONFIGURATION_M_SHUTDOWN); >> + >> + ret = i2c_smbus_write_word_swapped(opt->client, OPT3001_CONFIGURATION, >> + reg); >> + if (ret < 0) { >> + dev_err(opt->dev, "failed to write register %02x\n", >> + OPT3001_CONFIGURATION); >> + return ret; >> + } >> + >> + iio_device_free(iio); >> + >> + return 0; >> +} >> + >> +static const struct i2c_device_id opt3001_id[] = { >> + { "opt3001", 0 }, >> + { } /* Terminating Entry */ >> +}; >> +MODULE_DEVICE_TABLE(i2c, opt3001_id); >> + >> +static struct i2c_driver opt3001_driver = { >> + .probe = opt3001_probe, >> + .remove = opt3001_remove, >> + .id_table = opt3001_id, >> + >> + .driver = { >> + .name = "opt3001", >> + .owner = THIS_MODULE, >> + }, >> +}; >> + >> +module_i2c_driver(opt3001_driver); >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_AUTHOR("Andreas Dannenberg <dannenberg@xxxxxx>"); >> +MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver"); >> > I'll prepare an updated version of the patch internally but I'm waiting another day or so before re-posting. Thanks again, -- Andreas Dannenberg Texas Instruments Inc. -- 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
