[RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

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.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---

Jonathan, I have a few doubts on how can I test buffered
mode properly. I can see my IRQs triggering now problem,
but where is the data pushed ? Can it be read from sysfs
or /dev somehwere ?

Also, there are a couple details about this device which
I need to know if it looks for you:

This device has a single enable bit which will enable both
rising and falling edge triggers, but the limits are separate.

The same limits are also used for hysteresis-style capture and
that's controlled by a single bit flip.

How do you want this to look on sysfs ? Currently, enable/disabling
any of rising/falling edges, will disable both.

Cheers

 drivers/iio/light/Kconfig   |  12 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/opt3001.c | 765 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 778 insertions(+)
 create mode 100644 drivers/iio/light/opt3001.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index bf05ca5..e4582d7 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -128,6 +128,18 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config OPT3001
+	tristate "Texas Instruments OPT3001 Light Sensor"
+	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	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 TCS3414
 	tristate "TAOS TCS3414 digital color sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 8b8c09f..898ef13 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
 obj-$(CONFIG_ISL29125)		+= isl29125.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..1d925fd1
--- /dev/null
+++ b/drivers/iio/light/opt3001.c
@@ -0,0 +1,765 @@
+/**
+ * opt3001.c - Texas Instruments OPT3001 Light Sensor
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Felipe Balbi <balbi@ti.com>
+ *
+ * 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	(1 << 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	(1 << 8)
+#define OPT3001_CONFIGURATION_CRF	(1 << 7)
+#define OPT3001_CONFIGURATION_FH	(1 << 6)
+#define OPT3001_CONFIGURATION_FL	(1 << 5)
+#define OPT3001_CONFIGURATION_L		(1 << 4)
+#define OPT3001_CONFIGURATION_POL	(1 << 3)
+#define OPT3001_CONFIGURATION_ME	(1 << 2)
+
+#define OPT3001_CONFIGURATION_FC_MASK	(3 << 0)
+
+#define OPT3001_REG_EXPONENT(n)	((n) >> 12)
+#define OPT3001_REG_MANTISSA(n)	((n) & 0xfff)
+
+struct opt3001 {
+	struct i2c_client	*client;
+	struct device		*dev;
+	struct iio_dev		*iio;
+
+	struct mutex		lock;
+
+	u32			int_time;
+	u32			mode;
+
+	u16			high_thresh_mantissa;
+	u16			low_thresh_mantissa;
+
+	u8			high_thresh_exp;
+	u8			low_thresh_exp;
+
+	unsigned int		hysteresis:1;
+};
+
+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) {
+			*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 int opt3001_read(const struct opt3001 *opt, u8 reg)
+{
+	return i2c_smbus_read_word_swapped(opt->client, reg);
+}
+
+static int opt3001_write(const struct opt3001 *opt, u8 reg, u16 value)
+{
+	return i2c_smbus_write_word_swapped(opt->client, reg, value);
+}
+
+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_HYSTERESIS) |
+			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_HYSTERESIS) |
+			BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_chan_spec opt3001_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_SCALE) |
+				BIT(IIO_CHAN_INFO_INT_TIME),
+		.channel = 0,
+		.scan_type = {
+			.sign = 'u',
+			.endianness = IIO_BE,
+		},
+		.scan_index = 0,
+		.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;
+
+	ret = opt3001_read(opt, 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, &reg, OPT3001_CONFIGURATION_M_SINGLE);
+
+	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	/* wait for conversion and give it an extra 5ms */
+	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
+
+	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	reg = ret;
+	if (!(reg & OPT3001_CONFIGURATION_CRF))
+		return -EPIPE;
+
+	ret = opt3001_read(opt, OPT3001_RESULT);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_RESULT);
+		return ret;
+	}
+
+	exponent = OPT3001_REG_EXPONENT(ret);
+	mantissa = OPT3001_REG_MANTISSA(ret);
+
+	opt3001_to_iio_ret(opt, exponent, mantissa, val, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int opt3001_get_scale(struct opt3001 *opt, int *val, int *val2)
+{
+	int ret;
+	u8 exponent;
+
+	ret = opt3001_read(opt, OPT3001_RESULT);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_RESULT);
+		return ret;
+	}
+
+	exponent = OPT3001_REG_EXPONENT(ret);
+	*val = opt3001_scales[exponent].val;
+	*val2 = opt3001_scales[exponent].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 = opt3001_read(opt, 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 opt3001_write(opt, 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;
+
+	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_RAW:
+		ret = opt3001_get_lux(opt, val, val2);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = opt3001_get_scale(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;
+
+	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_INT_TIME:
+		ret = opt3001_set_int_time(opt, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	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;
+
+	u16 *thresh_mantissa;
+	u16 mantissa;
+	u16 value;
+	u16 reg;
+
+	u8 *thresh_exp;
+
+	u8 exponent;
+
+	mutex_lock(&opt->lock);
+
+	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		goto err;
+	}
+
+	reg = ret;
+	if (info == IIO_EV_INFO_HYSTERESIS)
+		opt->hysteresis = true;
+	else
+		opt->hysteresis = false;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		reg = OPT3001_HIGH_LIMIT;
+		thresh_mantissa = &opt->high_thresh_mantissa;
+		thresh_exp = &opt->high_thresh_exp;
+		break;
+	case IIO_EV_DIR_FALLING:
+		reg = OPT3001_LOW_LIMIT;
+		thresh_mantissa = &opt->low_thresh_mantissa;
+		thresh_exp = &opt->low_thresh_exp;
+		break;
+	default:
+		ret = -EINVAL;
+		goto err;
+	}
+
+	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);
+		goto err;
+	}
+
+	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
+	value = exponent << 12 | mantissa;
+	ret = opt3001_write(opt, reg, value);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n", reg);
+		goto err;
+	}
+
+	*thresh_mantissa = mantissa;
+	*thresh_exp = exponent;
+
+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;
+
+	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
+		: OPT3001_CONFIGURATION_M_SHUTDOWN;
+
+	ret = opt3001_read(opt, 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, &reg, mode);
+
+	if (opt->hysteresis)
+		reg |= OPT3001_CONFIGURATION_L;
+	else
+		reg &= ~OPT3001_CONFIGURATION_L;
+
+	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	/* wait for mode change to go through */
+	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
+
+	return 0;
+}
+
+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 = opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	reg = ret;
+
+	if (reg & OPT3001_CONFIGURATION_CT)
+		opt->int_time = 800000;
+	else
+		opt->int_time = 100000;
+
+	reg &= ~OPT3001_CONFIGURATION_L;
+	reg &= ~OPT3001_CONFIGURATION_RN_MASK;
+	reg |= OPT3001_CONFIGURATION_RN_AUTO;
+
+	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to write register %02x\n",
+				OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt)
+{
+	struct opt3001 *opt = _opt;
+	int ret;
+
+	mutex_lock(&opt->lock);
+
+	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		goto out;
+	}
+
+	if (!(ret & OPT3001_CONFIGURATION_CT))
+		goto out;
+
+	if (ret & OPT3001_CONFIGURATION_FH)
+		iio_push_event(opt->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(opt->iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+					0, IIO_EV_TYPE_THRESH,
+					IIO_EV_DIR_FALLING), iio_get_time_ns());
+
+out:
+	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;
+
+	iio = devm_iio_device_alloc(dev, sizeof(*opt));
+	if (!iio)
+		return -ENOMEM;
+
+	opt = iio_priv(iio);
+	opt->client = client;
+	opt->dev = dev;
+	opt->iio = iio;
+
+	mutex_init(&opt->lock);
+	i2c_set_clientdata(client, opt);
+
+	ret = opt3001_read_id(opt);
+	if (ret)
+		return ret;
+
+	ret = opt3001_configure(opt);
+	if (ret)
+		return ret;
+
+	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 = devm_iio_device_register(dev, iio);
+	if (ret) {
+		dev_err(dev, "failed to register IIO device\n");
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
+			| IRQF_ONESHOT, "opt3001", opt);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ #%d\n", irq);
+		return ret;
+	}
+
+	return 0;
+
+}
+
+static int opt3001_remove(struct i2c_client *client)
+{
+	/* nothing to do here */
+
+	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("Felipe Balbi <balbi@ti.com>");
+MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver");
-- 
2.0.1.563.g66f467c

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Peter Meerwald]

Hello Felipe,

in subject: light sensor

please see my minor comments below

p.

> 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.
> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> Jonathan, I have a few doubts on how can I test buffered
> mode properly. I can see my IRQs triggering now problem,
> but where is the data pushed ? Can it be read from sysfs
> or /dev somehwere ?

there should be /dev/iio:deviceX 

> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index bf05ca5..e4582d7 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -128,6 +128,18 @@ config LTR501
>  	 This driver can also be built as a module.  If so, the module
>           will be called ltr501.
>  
> +config OPT3001
> +	tristate "Texas Instruments OPT3001 Light Sensor"
> +	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	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 TCS3414
>  	tristate "TAOS TCS3414 digital color sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 8b8c09f..898ef13 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
>  obj-$(CONFIG_ISL29125)		+= isl29125.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..1d925fd1
> --- /dev/null
> +++ b/drivers/iio/light/opt3001.c
> @@ -0,0 +1,765 @@
> +/**
> + * opt3001.c - Texas Instruments OPT3001 Light Sensor
> + *
> + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Author: Felipe Balbi <balbi@ti.com>
> + *
> + * 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	(1 << 11)

could use BIT() from bitops.h

> +
> +#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	(1 << 8)
> +#define OPT3001_CONFIGURATION_CRF	(1 << 7)
> +#define OPT3001_CONFIGURATION_FH	(1 << 6)
> +#define OPT3001_CONFIGURATION_FL	(1 << 5)
> +#define OPT3001_CONFIGURATION_L		(1 << 4)
> +#define OPT3001_CONFIGURATION_POL	(1 << 3)
> +#define OPT3001_CONFIGURATION_ME	(1 << 2)
> +
> +#define OPT3001_CONFIGURATION_FC_MASK	(3 << 0)
> +
> +#define OPT3001_REG_EXPONENT(n)	((n) >> 12)
> +#define OPT3001_REG_MANTISSA(n)	((n) & 0xfff)
> +
> +struct opt3001 {
> +	struct i2c_client	*client;
> +	struct device		*dev;
> +	struct iio_dev		*iio;
> +
> +	struct mutex		lock;
> +
> +	u32			int_time;
> +	u32			mode;
> +
> +	u16			high_thresh_mantissa;
> +	u16			low_thresh_mantissa;
> +
> +	u8			high_thresh_exp;
> +	u8			low_thresh_exp;
> +
> +	unsigned int		hysteresis:1;
> +};
> +
> +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) {
> +			*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 int opt3001_read(const struct opt3001 *opt, u8 reg)
> +{
> +	return i2c_smbus_read_word_swapped(opt->client, reg);
> +}
> +
> +static int opt3001_write(const struct opt3001 *opt, u8 reg, u16 value)
> +{
> +	return i2c_smbus_write_word_swapped(opt->client, reg, value);
> +}
> +

many small helper function; worth it to wrap a one-liner?

> +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_HYSTERESIS) |
> +			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_HYSTERESIS) |
> +			BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec opt3001_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE) |
> +				BIT(IIO_CHAN_INFO_INT_TIME),
> +		.channel = 0,

.channel not needed

> +		.scan_type = {
> +			.sign = 'u',

.realbits / .storagebits missing?

> +			.endianness = IIO_BE,
> +		},
> +		.scan_index = 0,
> +		.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;
> +
> +	ret = opt3001_read(opt, 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, &reg, OPT3001_CONFIGURATION_M_SINGLE);
> +
> +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",

"failed to write register ..."

> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	/* wait for conversion and give it an extra 5ms */
> +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> +
> +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	reg = ret;
> +	if (!(reg & OPT3001_CONFIGURATION_CRF))
> +		return -EPIPE;
> +
> +	ret = opt3001_read(opt, OPT3001_RESULT);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_RESULT);
> +		return ret;
> +	}
> +
> +	exponent = OPT3001_REG_EXPONENT(ret);
> +	mantissa = OPT3001_REG_MANTISSA(ret);
> +
> +	opt3001_to_iio_ret(opt, exponent, mantissa, val, val2);
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int opt3001_get_scale(struct opt3001 *opt, int *val, int *val2)
> +{
> +	int ret;
> +	u8 exponent;
> +

I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux()

> +	ret = opt3001_read(opt, OPT3001_RESULT);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_RESULT);
> +		return ret;
> +	}
> +
> +	exponent = OPT3001_REG_EXPONENT(ret);
> +	*val = opt3001_scales[exponent].val;
> +	*val2 = opt3001_scales[exponent].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 = opt3001_read(opt, 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 opt3001_write(opt, 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;

initialization not needed

> +
> +	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_RAW:
> +		ret = opt3001_get_lux(opt, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = opt3001_get_scale(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;

initialization not needed

> +
> +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
> +		return -EBUSY;
> +
> +	if (chan->type != IIO_LIGHT)
> +		return -EINVAL;
> +
> +	mutex_lock(&opt->lock);
> +
> +	switch (mask) {

is there no way to control the scale?

> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = opt3001_set_int_time(opt, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	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;
> +
> +	u16 *thresh_mantissa;
> +	u16 mantissa;
> +	u16 value;
> +	u16 reg;
> +
> +	u8 *thresh_exp;

why extra newlines?

> +
> +	u8 exponent;
> +
> +	mutex_lock(&opt->lock);
> +
> +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		goto err;
> +	}
> +
> +	reg = ret;
> +	if (info == IIO_EV_INFO_HYSTERESIS)
> +		opt->hysteresis = true;
> +	else
> +		opt->hysteresis = false;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		reg = OPT3001_HIGH_LIMIT;
> +		thresh_mantissa = &opt->high_thresh_mantissa;
> +		thresh_exp = &opt->high_thresh_exp;
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		reg = OPT3001_LOW_LIMIT;
> +		thresh_mantissa = &opt->low_thresh_mantissa;
> +		thresh_exp = &opt->low_thresh_exp;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
> +	value = exponent << 12 | mantissa;
> +	ret = opt3001_write(opt, reg, value);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n", reg);

"failed to write ..."

> +		goto err;
> +	}
> +
> +	*thresh_mantissa = mantissa;
> +	*thresh_exp = exponent;
> +
> +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;
> +
> +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
> +		: OPT3001_CONFIGURATION_M_SHUTDOWN;
> +
> +	ret = opt3001_read(opt, 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, &reg, mode);
> +
> +	if (opt->hysteresis)
> +		reg |= OPT3001_CONFIGURATION_L;
> +	else
> +		reg &= ~OPT3001_CONFIGURATION_L;
> +
> +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",

"failed to write"

> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	/* wait for mode change to go through */
> +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> +
> +	return 0;
> +}
> +
> +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 = opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	reg = ret;
> +
> +	if (reg & OPT3001_CONFIGURATION_CT)
> +		opt->int_time = 800000;
> +	else
> +		opt->int_time = 100000;
> +
> +	reg &= ~OPT3001_CONFIGURATION_L;
> +	reg &= ~OPT3001_CONFIGURATION_RN_MASK;
> +	reg |= OPT3001_CONFIGURATION_RN_AUTO;
> +
> +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to write register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt)
> +{
> +	struct opt3001 *opt = _opt;
> +	int ret;
> +
> +	mutex_lock(&opt->lock);
> +
> +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		goto out;
> +	}
> +
> +	if (!(ret & OPT3001_CONFIGURATION_CT))
> +		goto out;
> +
> +	if (ret & OPT3001_CONFIGURATION_FH)
> +		iio_push_event(opt->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(opt->iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> +					0, IIO_EV_TYPE_THRESH,
> +					IIO_EV_DIR_FALLING), iio_get_time_ns());
> +
> +out:
> +	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;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*opt));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	opt = iio_priv(iio);
> +	opt->client = client;
> +	opt->dev = dev;
> +	opt->iio = iio;
> +
> +	mutex_init(&opt->lock);
> +	i2c_set_clientdata(client, opt);
> +
> +	ret = opt3001_read_id(opt);
> +	if (ret)
> +		return ret;
> +
> +	ret = opt3001_configure(opt);
> +	if (ret)
> +		return ret;
> +
> +	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 = devm_iio_device_register(dev, iio);
> +	if (ret) {
> +		dev_err(dev, "failed to register IIO device\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> +			| IRQF_ONESHOT, "opt3001", opt);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ #%d\n", irq);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +}
> +
> +static int opt3001_remove(struct i2c_client *client)
> +{
> +	/* nothing to do here */

shutdown the device probably?

> +
> +	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("Felipe Balbi <balbi@ti.com>");
> +MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 8735 bytes --]

On Wed, Aug 06, 2014 at 11:11:52PM +0200, Peter Meerwald wrote:
> Hello Felipe,
> 
> in subject: light sensor
> 
> please see my minor comments below
> 
> p.
> 
> > 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.
> > 
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > Jonathan, I have a few doubts on how can I test buffered
> > mode properly. I can see my IRQs triggering now problem,
> > but where is the data pushed ? Can it be read from sysfs
> > or /dev somehwere ?
> 
> there should be /dev/iio:deviceX 
> 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index bf05ca5..e4582d7 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -128,6 +128,18 @@ config LTR501
> >  	 This driver can also be built as a module.  If so, the module
> >           will be called ltr501.
> >  
> > +config OPT3001
> > +	tristate "Texas Instruments OPT3001 Light Sensor"
> > +	depends on I2C
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> > +	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 TCS3414
> >  	tristate "TAOS TCS3414 digital color sensor"
> >  	depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 8b8c09f..898ef13 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
> >  obj-$(CONFIG_ISL29125)		+= isl29125.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..1d925fd1
> > --- /dev/null
> > +++ b/drivers/iio/light/opt3001.c
> > @@ -0,0 +1,765 @@
> > +/**
> > + * opt3001.c - Texas Instruments OPT3001 Light Sensor
> > + *
> > + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
> > + *
> > + * Author: Felipe Balbi <balbi@ti.com>
> > + *
> > + * 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	(1 << 11)
> 
> could use BIT() from bitops.h

sure, easy enough.

> > +static int opt3001_read(const struct opt3001 *opt, u8 reg)
> > +{
> > +	return i2c_smbus_read_word_swapped(opt->client, reg);
> > +}
> > +
> > +static int opt3001_write(const struct opt3001 *opt, u8 reg, u16 value)
> > +{
> > +	return i2c_smbus_write_word_swapped(opt->client, reg, value);
> > +}
> > +
> 
> many small helper function; worth it to wrap a one-liner?

why not ? I think it looks better and it'll get inlined anyway. It also
helps a lot when using kernel function tracer.

> > +		.scan_type = {
> > +			.sign = 'u',
> 
> .realbits / .storagebits missing?

yeah, couldn't figure this out. The HW gives me a 16-bit value where
12-bits are the mantissa and 4-bits are for the exponent. Should I call
realbits 16 and storagebits 32 ?

> 
> > +			.endianness = IIO_BE,
> > +		},
> > +		.scan_index = 0,
> > +		.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;
> > +
> > +	ret = opt3001_read(opt, 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, &reg, OPT3001_CONFIGURATION_M_SINGLE);
> > +
> > +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> 
> "failed to write register ..."

yeah thanks. copy&paste error.

> > +				OPT3001_CONFIGURATION);
> > +		return ret;
> > +	}
> > +
> > +	/* wait for conversion and give it an extra 5ms */
> > +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> > +
> > +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> > +				OPT3001_CONFIGURATION);
> > +		return ret;
> > +	}
> > +
> > +	reg = ret;
> > +	if (!(reg & OPT3001_CONFIGURATION_CRF))
> > +		return -EPIPE;
> > +
> > +	ret = opt3001_read(opt, OPT3001_RESULT);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> > +				OPT3001_RESULT);
> > +		return ret;
> > +	}
> > +
> > +	exponent = OPT3001_REG_EXPONENT(ret);
> > +	mantissa = OPT3001_REG_MANTISSA(ret);
> > +
> > +	opt3001_to_iio_ret(opt, exponent, mantissa, val, val2);
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int opt3001_get_scale(struct opt3001 *opt, int *val, int *val2)
> > +{
> > +	int ret;
> > +	u8 exponent;
> > +
> 
> I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux()

right. I'm only interested in the exponent part. It'll latch the
exponent from last conversion, 0 being default.

> > +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;
> 
> initialization not needed

I'll drop

> > +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;
> 
> initialization not needed

I'll drop

> > +
> > +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
> > +		return -EBUSY;
> > +
> > +	if (chan->type != IIO_LIGHT)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&opt->lock);
> > +
> > +	switch (mask) {
> 
> is there no way to control the scale?

yeah, but the HW does auto-scaling which works pretty well. Why would
you want to control the scale manually ?

> > +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;
> > +
> > +	u16 *thresh_mantissa;
> > +	u16 mantissa;
> > +	u16 value;
> > +	u16 reg;
> > +
> > +	u8 *thresh_exp;
> 
> why extra newlines?

why not ? I generally separate the types as it aids readability.

> > +static int opt3001_remove(struct i2c_client *client)
> > +{
> > +	/* nothing to do here */
> 
> shutdown the device probably?

it's always in shutdown unless it's doing a transfer. The only exception
being if we enable continuous mode and remove the module. Might be
better to check that and shut it down, sure.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Peter Meerwald]

> On Wed, Aug 06, 2014 at 11:11:52PM +0200, Peter Meerwald wrote:
> > Hello Felipe,
> > 
> > in subject: light sensor
> > 
> > please see my minor comments below
> > 
> > p.
> > 
> > > 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.
> > > 
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > > 
> > > Jonathan, I have a few doubts on how can I test buffered
> > > mode properly. I can see my IRQs triggering now problem,
> > > but where is the data pushed ? Can it be read from sysfs
> > > or /dev somehwere ?
> > 
> > there should be /dev/iio:deviceX 
> > 
> > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > index bf05ca5..e4582d7 100644
> > > --- a/drivers/iio/light/Kconfig
> > > +++ b/drivers/iio/light/Kconfig
> > > @@ -128,6 +128,18 @@ config LTR501
> > >  	 This driver can also be built as a module.  If so, the module
> > >           will be called ltr501.
> > >  
> > > +config OPT3001
> > > +	tristate "Texas Instruments OPT3001 Light Sensor"
> > > +	depends on I2C
> > > +	select IIO_BUFFER
> > > +	select IIO_TRIGGERED_BUFFER
> > > +	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 TCS3414
> > >  	tristate "TAOS TCS3414 digital color sensor"
> > >  	depends on I2C
> > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > > index 8b8c09f..898ef13 100644
> > > --- a/drivers/iio/light/Makefile
> > > +++ b/drivers/iio/light/Makefile
> > > @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
> > >  obj-$(CONFIG_ISL29125)		+= isl29125.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..1d925fd1
> > > --- /dev/null
> > > +++ b/drivers/iio/light/opt3001.c
> > > @@ -0,0 +1,765 @@
> > > +/**
> > > + * opt3001.c - Texas Instruments OPT3001 Light Sensor
> > > + *
> > > + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
> > > + *
> > > + * Author: Felipe Balbi <balbi@ti.com>
> > > + *
> > > + * 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	(1 << 11)
> > 
> > could use BIT() from bitops.h
> 
> sure, easy enough.
> 
> > > +static int opt3001_read(const struct opt3001 *opt, u8 reg)
> > > +{
> > > +	return i2c_smbus_read_word_swapped(opt->client, reg);
> > > +}
> > > +
> > > +static int opt3001_write(const struct opt3001 *opt, u8 reg, u16 value)
> > > +{
> > > +	return i2c_smbus_write_word_swapped(opt->client, reg, value);
> > > +}
> > > +
> > 
> > many small helper function; worth it to wrap a one-liner?
> 
> why not ? I think it looks better and it'll get inlined anyway. It also
> helps a lot when using kernel function tracer.
> 
> > > +		.scan_type = {
> > > +			.sign = 'u',
> > 
> > .realbits / .storagebits missing?
> 
> yeah, couldn't figure this out. The HW gives me a 16-bit value where
> 12-bits are the mantissa and 4-bits are for the exponent. Should I call
> realbits 16 and storagebits 32 ?

so .realbits = 16 and .storagebits = 16, but there is no way to describe 
the mantissa/exponent thing to userspace

you are not using iio_triggered_buffer_setup() yet, so no need for 
.scan_type (yet)

> > > +			.endianness = IIO_BE,
> > > +		},
> > > +		.scan_index = 0,
> > > +		.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;
> > > +
> > > +	ret = opt3001_read(opt, 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, &reg, OPT3001_CONFIGURATION_M_SINGLE);
> > > +
> > > +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> > > +	if (ret < 0) {
> > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > 
> > "failed to write register ..."
> 
> yeah thanks. copy&paste error.
> 
> > > +				OPT3001_CONFIGURATION);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* wait for conversion and give it an extra 5ms */
> > > +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> > > +
> > > +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> > > +	if (ret < 0) {
> > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > +				OPT3001_CONFIGURATION);
> > > +		return ret;
> > > +	}
> > > +
> > > +	reg = ret;
> > > +	if (!(reg & OPT3001_CONFIGURATION_CRF))
> > > +		return -EPIPE;
> > > +
> > > +	ret = opt3001_read(opt, OPT3001_RESULT);
> > > +	if (ret < 0) {
> > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > +				OPT3001_RESULT);
> > > +		return ret;
> > > +	}
> > > +
> > > +	exponent = OPT3001_REG_EXPONENT(ret);
> > > +	mantissa = OPT3001_REG_MANTISSA(ret);
> > > +
> > > +	opt3001_to_iio_ret(opt, exponent, mantissa, val, val2);
> > > +
> > > +	return IIO_VAL_INT_PLUS_MICRO;
> > > +}
> > > +
> > > +static int opt3001_get_scale(struct opt3001 *opt, int *val, int *val2)
> > > +{
> > > +	int ret;
> > > +	u8 exponent;
> > > +
> > 
> > I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux()
> 
> right. I'm only interested in the exponent part. It'll latch the
> exponent from last conversion, 0 being default.
> 
> > > +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;
> > 
> > initialization not needed
> 
> I'll drop
> 
> > > +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;
> > 
> > initialization not needed
> 
> I'll drop
> 
> > > +
> > > +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
> > > +		return -EBUSY;
> > > +
> > > +	if (chan->type != IIO_LIGHT)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&opt->lock);
> > > +
> > > +	switch (mask) {
> > 
> > is there no way to control the scale?
> 
> yeah, but the HW does auto-scaling which works pretty well. Why would
> you want to control the scale manually ?
> 
> > > +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;
> > > +
> > > +	u16 *thresh_mantissa;
> > > +	u16 mantissa;
> > > +	u16 value;
> > > +	u16 reg;
> > > +
> > > +	u8 *thresh_exp;
> > 
> > why extra newlines?
> 
> why not ? I generally separate the types as it aids readability.
> 
> > > +static int opt3001_remove(struct i2c_client *client)
> > > +{
> > > +	/* nothing to do here */
> > 
> > shutdown the device probably?
> 
> it's always in shutdown unless it's doing a transfer. The only exception
> being if we enable continuous mode and remove the module. Might be
> better to check that and shut it down, sure.
> 
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

Hi,

On Thu, Aug 07, 2014 at 12:09:36AM +0200, Peter Meerwald wrote:
> > > > +		.scan_type = {
> > > > +			.sign = 'u',
> > > 
> > > .realbits / .storagebits missing?
> > 
> > yeah, couldn't figure this out. The HW gives me a 16-bit value where
> > 12-bits are the mantissa and 4-bits are for the exponent. Should I call
> > realbits 16 and storagebits 32 ?
> 
> so .realbits = 16 and .storagebits = 16, but there is no way to describe 
> the mantissa/exponent thing to userspace
> 
> you are not using iio_triggered_buffer_setup() yet, so no need for 
> .scan_type (yet)

ok... but then that brings up a doubt. What would I use triggered buffer
for ? I already setup High/Low limits (thresholds) through
.write_event_value(). Isn't that, pretty much, a triggered buffer ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Peter Meerwald]

> > > > > +		.scan_type = {
> > > > > +			.sign = 'u',
> > > > 
> > > > .realbits / .storagebits missing?
> > > 
> > > yeah, couldn't figure this out. The HW gives me a 16-bit value where
> > > 12-bits are the mantissa and 4-bits are for the exponent. Should I call
> > > realbits 16 and storagebits 32 ?
> > 
> > so .realbits = 16 and .storagebits = 16, but there is no way to describe 
> > the mantissa/exponent thing to userspace
> > 
> > you are not using iio_triggered_buffer_setup() yet, so no need for 
> > .scan_type (yet)
> 
> ok... but then that brings up a doubt. What would I use triggered buffer
> for ? I already setup High/Low limits (thresholds) through
> .write_event_value(). Isn't that, pretty much, a triggered buffer ?

a triggered buffer reads data from the device and stores it into a buffer 
whenever the trigger goes off (you are interested to capture all samples)

events are created on certain conditions (you are not interested in all 
samples)

p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

Hi,

On Thu, Aug 07, 2014 at 12:25:59AM +0200, Peter Meerwald wrote:
> > > > > > +		.scan_type = {
> > > > > > +			.sign = 'u',
> > > > > 
> > > > > .realbits / .storagebits missing?
> > > > 
> > > > yeah, couldn't figure this out. The HW gives me a 16-bit value where
> > > > 12-bits are the mantissa and 4-bits are for the exponent. Should I call
> > > > realbits 16 and storagebits 32 ?
> > > 
> > > so .realbits = 16 and .storagebits = 16, but there is no way to describe 
> > > the mantissa/exponent thing to userspace
> > > 
> > > you are not using iio_triggered_buffer_setup() yet, so no need for 
> > > .scan_type (yet)
> > 
> > ok... but then that brings up a doubt. What would I use triggered buffer
> > for ? I already setup High/Low limits (thresholds) through
> > .write_event_value(). Isn't that, pretty much, a triggered buffer ?
> 
> a triggered buffer reads data from the device and stores it into a buffer 
> whenever the trigger goes off (you are interested to capture all samples)

Alright, so something like a contious capture starting after a GPIO goes
off, or something ?

> events are created on certain conditions (you are not interested in all 
> samples)

And this would be more like e.g. "tell me once I have 100 lux or more" ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Peter Meerwald]

> > > ok... but then that brings up a doubt. What would I use triggered buffer
> > > for ? I already setup High/Low limits (thresholds) through
> > > .write_event_value(). Isn't that, pretty much, a triggered buffer ?
> > 
> > a triggered buffer reads data from the device and stores it into a buffer 
> > whenever the trigger goes off (you are interested to capture all samples)
> 
> Alright, so something like a contious capture starting after a GPIO goes
> off, or something ?

right, one capture whenever the GPIO becomes active typically
 
> > events are created on certain conditions (you are not interested in all 
> > samples)
> 
> And this would be more like e.g. "tell me once I have 100 lux or more" ?

yes

p.

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On Thu, Aug 07, 2014 at 12:35:11AM +0200, Peter Meerwald wrote:
> 
> > > > ok... but then that brings up a doubt. What would I use triggered buffer
> > > > for ? I already setup High/Low limits (thresholds) through
> > > > .write_event_value(). Isn't that, pretty much, a triggered buffer ?
> > > 
> > > a triggered buffer reads data from the device and stores it into a buffer 
> > > whenever the trigger goes off (you are interested to capture all samples)
> > 
> > Alright, so something like a contious capture starting after a GPIO goes
> > off, or something ?
> 
> right, one capture whenever the GPIO becomes active typically
>  
> > > events are created on certain conditions (you are not interested in all 
> > > samples)
> > 
> > And this would be more like e.g. "tell me once I have 100 lux or more" ?
> 
> yes

cool thanks. I'll wait for some more review comments before sending a
new version. Latest is kept at [1]

[1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=opt3001

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

On 06/08/14 17:10, Felipe Balbi 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.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
I don't suppose there is a datasheet available?

When in M_CONTINUOUS is it just providing event interrupts
(threshold etc)?

> ---
>
> Jonathan, I have a few doubts on how can I test buffered
> mode properly. I can see my IRQs triggering now problem,
> but where is the data pushed ? Can it be read from sysfs
> or /dev somehwere ?
As  I guess has become clear from your thread with Peter,
what you have here in IIO terms are called events (separate
form the buffer which is for the main data flow).

Anyhow, see drivers/staging/iio/Documentation/iio_event_monitor.c.
Some trickery with an anonymous file descriptor is used to avoid
using lots of chardevs for the same device.

>
> Also, there are a couple details about this device which
> I need to know if it looks for you:
>
> This device has a single enable bit which will enable both
> rising and falling edge triggers, but the limits are separate.
>
> The same limits are also used for hysteresis-style capture and
> that's controlled by a single bit flip.
With this on, it generates an event whenever the value changes by
more than a certain amount?  If so this is better handled by
providing a trigger and a buffer.  Note that the events path
carries no data other than an event code.

>
> How do you want this to look on sysfs ? Currently, enable/disabling
> any of rising/falling edges, will disable both.
That's pretty common.  The ABI basically allows for any setting to
change any other (as there are much weirder connections than this
in some devices).
>
> Cheers
>
>  drivers/iio/light/Kconfig   |  12 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/opt3001.c | 765 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 778 insertions(+)
>  create mode 100644 drivers/iio/light/opt3001.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index bf05ca5..e4582d7 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -128,6 +128,18 @@ config LTR501
>  	 This driver can also be built as a module.  If so, the module
>           will be called ltr501.
>
> +config OPT3001
> +	tristate "Texas Instruments OPT3001 Light Sensor"
> +	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	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 TCS3414
>  	tristate "TAOS TCS3414 digital color sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 8b8c09f..898ef13 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
>  obj-$(CONFIG_ISL29125)		+= isl29125.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..1d925fd1
> --- /dev/null
> +++ b/drivers/iio/light/opt3001.c
> @@ -0,0 +1,765 @@
> +/**
> + * opt3001.c - Texas Instruments OPT3001 Light Sensor
> + *
> + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Author: Felipe Balbi <balbi@ti.com>
> + *
> + * 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	(1 << 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	(1 << 8)
> +#define OPT3001_CONFIGURATION_CRF	(1 << 7)
> +#define OPT3001_CONFIGURATION_FH	(1 << 6)
> +#define OPT3001_CONFIGURATION_FL	(1 << 5)
> +#define OPT3001_CONFIGURATION_L		(1 << 4)
> +#define OPT3001_CONFIGURATION_POL	(1 << 3)
> +#define OPT3001_CONFIGURATION_ME	(1 << 2)
> +
> +#define OPT3001_CONFIGURATION_FC_MASK	(3 << 0)
> +
> +#define OPT3001_REG_EXPONENT(n)	((n) >> 12)
> +#define OPT3001_REG_MANTISSA(n)	((n) & 0xfff)
> +
> +struct opt3001 {
> +	struct i2c_client	*client;
> +	struct device		*dev;
> +	struct iio_dev		*iio;
> +
> +	struct mutex		lock;
> +
> +	u32			int_time;
> +	u32			mode;
> +
> +	u16			high_thresh_mantissa;
> +	u16			low_thresh_mantissa;
> +
> +	u8			high_thresh_exp;
> +	u8			low_thresh_exp;
> +
> +	unsigned int		hysteresis:1;
> +};
> +
> +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) {
> +			*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 int opt3001_read(const struct opt3001 *opt, u8 reg)
> +{
> +	return i2c_smbus_read_word_swapped(opt->client, reg);
> +}
I'm very much against wrapper functions unless they add something. These
really do not.
> +
> +static int opt3001_write(const struct opt3001 *opt, u8 reg, u16 value)
> +{
> +	return i2c_smbus_write_word_swapped(opt->client, reg, value);
> +}
> +
> +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_HYSTERESIS) |
> +			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_HYSTERESIS) |
> +			BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec opt3001_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_SCALE) |
> +				BIT(IIO_CHAN_INFO_INT_TIME),
> +		.channel = 0,

No buffering at the moment so can drop scan_type and scan_index.
> +		.scan_type = {
> +			.sign = 'u',
> +			.endianness = IIO_BE,
> +		},
> +		.scan_index = 0,
> +		.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;
> +
> +	ret = opt3001_read(opt, 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, &reg, OPT3001_CONFIGURATION_M_SINGLE);
> +
> +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	/* wait for conversion and give it an extra 5ms */
> +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> +
> +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	reg = ret;
> +	if (!(reg & OPT3001_CONFIGURATION_CRF))
> +		return -EPIPE;
> +
> +	ret = opt3001_read(opt, OPT3001_RESULT);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_RESULT);
> +		return ret;
> +	}
> +
> +	exponent = OPT3001_REG_EXPONENT(ret);
> +	mantissa = OPT3001_REG_MANTISSA(ret);
> +
> +	opt3001_to_iio_ret(opt, exponent, mantissa, val, val2);
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int opt3001_get_scale(struct opt3001 *opt, int *val, int *val2)
> +{
> +	int ret;
> +	u8 exponent;
> +
> +	ret = opt3001_read(opt, OPT3001_RESULT);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_RESULT);
> +		return ret;
> +	}
> +
> +	exponent = OPT3001_REG_EXPONENT(ret);
> +	*val = opt3001_scales[exponent].val;
> +	*val2 = opt3001_scales[exponent].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 = opt3001_read(opt, 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 opt3001_write(opt, 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;
> +
> +	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_RAW:
> +		ret = opt3001_get_lux(opt, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = opt3001_get_scale(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;
> +
> +	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_INT_TIME:
> +		ret = opt3001_set_int_time(opt, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	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);
> +
So these will return the same value whether the value attribute is read
or the hysteresis one?
> +	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;
> +
> +	u16 *thresh_mantissa;
> +	u16 mantissa;
> +	u16 value;
> +	u16 reg;
> +
> +	u8 *thresh_exp;
> +
> +	u8 exponent;
> +
> +	mutex_lock(&opt->lock);
> +
> +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		goto err;
> +	}
> +
> +	reg = ret;
> +	if (info == IIO_EV_INFO_HYSTERESIS)
Without a datasheet I'm not entirely sure how the hysteresis is
applied here and what it's connection to the thresholds is...

> +		opt->hysteresis = true;
> +	else
> +		opt->hysteresis = false;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		reg = OPT3001_HIGH_LIMIT;
Why bother with the indirection via a pointer. Just have a second
switch statement at the end of the function that sets the right ones.
Easier to follow than this.
> +		thresh_mantissa = &opt->high_thresh_mantissa;
> +		thresh_exp = &opt->high_thresh_exp;
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		reg = OPT3001_LOW_LIMIT;
> +		thresh_mantissa = &opt->low_thresh_mantissa;
> +		thresh_exp = &opt->low_thresh_exp;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	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);
> +		goto err;
> +	}
> +
> +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
> +	value = exponent << 12 | mantissa;
> +	ret = opt3001_write(opt, reg, value);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n", reg);
> +		goto err;
> +	}
> +
As stated above an additional switch statement here with direct assignment
of the relevant threshold values would be clearer.
> +	*thresh_mantissa = mantissa;
> +	*thresh_exp = exponent;
> +
> +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;
> +
> +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
> +		: OPT3001_CONFIGURATION_M_SHUTDOWN;
> +
> +	ret = opt3001_read(opt, 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, &reg, mode);
> +
> +	if (opt->hysteresis)
> +		reg |= OPT3001_CONFIGURATION_L;
> +	else
> +		reg &= ~OPT3001_CONFIGURATION_L;
> +
> +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	/* wait for mode change to go through */
> +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> +
> +	return 0;
> +}
> +
> +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 = opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	reg = ret;
> +
> +	if (reg & OPT3001_CONFIGURATION_CT)
> +		opt->int_time = 800000;
> +	else
> +		opt->int_time = 100000;
> +
> +	reg &= ~OPT3001_CONFIGURATION_L;
> +	reg &= ~OPT3001_CONFIGURATION_RN_MASK;
> +	reg |= OPT3001_CONFIGURATION_RN_AUTO;
> +
> +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to write register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt)
> +{

Pass the iio_dev in to the irq setup instead of opt and
you will then not need the opt reference to the struct iio_dev.

> +	struct opt3001 *opt = _opt;
> +	int ret;
> +
> +	mutex_lock(&opt->lock);
> +
> +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		goto out;
> +	}
> +
> +	if (!(ret & OPT3001_CONFIGURATION_CT))
> +		goto out;
> +
> +	if (ret & OPT3001_CONFIGURATION_FH)
> +		iio_push_event(opt->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(opt->iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> +					0, IIO_EV_TYPE_THRESH,
> +					IIO_EV_DIR_FALLING), iio_get_time_ns());
> +
> +out:
> +	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;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*opt));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	opt = iio_priv(iio);
> +	opt->client = client;
> +	opt->dev = dev;

This is very circular and it rarely makes sense to have
the device private structure have a reference to the
iio_dev.  Here it is only used in the interrupt handler.
Avoid that by making the private data passed to the interrupt
handler the struct iio_dev instead of the struct opt3001.

> +	opt->iio = iio;
> +
> +	mutex_init(&opt->lock);
> +	i2c_set_clientdata(client, opt);
> +
> +	ret = opt3001_read_id(opt);
> +	if (ret)
> +		return ret;
> +
> +	ret = opt3001_configure(opt);
> +	if (ret)
> +		return ret;
> +
> +	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 = devm_iio_device_register(dev, iio);
> +	if (ret) {
> +		dev_err(dev, "failed to register IIO device\n");
> +		return ret;
> +	}
> +
Normally we'd expect the devm_iio_device_register (that provides the
userspace interfaces) to be after the irq request.
> +	ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> +			| IRQF_ONESHOT, "opt3001", opt);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ #%d\n", irq);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +}
> +
> +static int opt3001_remove(struct i2c_client *client)
> +{
> +	/* nothing to do here */
If there really is nothing to do here, then don't have a remove function.
There is no requirement to have one.
> +
> +	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("Felipe Balbi <balbi@ti.com>");
> +MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver");
>

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/08/14 22:42, Felipe Balbi wrote:
> On Wed, Aug 06, 2014 at 11:11:52PM +0200, Peter Meerwald wrote:
>> Hello Felipe,
>> 
>> in subject: light sensor
>> 
>> please see my minor comments below
>> 
>> p.
>> 
>>> 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.
>>> 
>>> Signed-off-by: Felipe Balbi <balbi@ti.com> ---
>>> 
>>> Jonathan, I have a few doubts on how can I test buffered mode properly. I can see my IRQs triggering now
>>> problem, but where is the data pushed ? Can it be read from sysfs or /dev somehwere ?
>> 
>> there should be /dev/iio:deviceX
>> 
>>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index bf05ca5..e4582d7 100644 ---
>>> a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -128,6 +128,18 @@ config LTR501 This driver can
>>> also be built as a module.  If so, the module will be called ltr501.
>>> 
>>> +config OPT3001 +	tristate "Texas Instruments OPT3001 Light Sensor" +	depends on I2C +	select IIO_BUFFER +
>>> select IIO_TRIGGERED_BUFFER +	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 TCS3414 tristate "TAOS TCS3414 digital color sensor" depends on I2C diff --git
>>> a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 8b8c09f..898ef13 100644 ---
>>> a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)
>>> += hid-sensor-prox.o obj-$(CONFIG_ISL29125)		+= isl29125.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..1d925fd1 ---
>>> /dev/null +++ b/drivers/iio/light/opt3001.c @@ -0,0 +1,765 @@ +/** + * opt3001.c - Texas Instruments OPT3001
>>> Light Sensor + * + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com + * + * Author:
>>> Felipe Balbi <balbi@ti.com> + * + * 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	(1 << 11)
>> 
>> could use BIT() from bitops.h
> 
> sure, easy enough.
> 
>>> +static int opt3001_read(const struct opt3001 *opt, u8 reg) +{ +	return
>>> i2c_smbus_read_word_swapped(opt->client, reg); +} + +static int opt3001_write(const struct opt3001 *opt, u8
>>> reg, u16 value) +{ +	return i2c_smbus_write_word_swapped(opt->client, reg, value); +} +
>> 
>> many small helper function; worth it to wrap a one-liner?
> 
> why not ? I think it looks better and it'll get inlined anyway. It also helps a lot when using kernel function
> tracer.
> 
>>> +		.scan_type = { +			.sign = 'u',
>> 
>> .realbits / .storagebits missing?
> 
> yeah, couldn't figure this out. The HW gives me a 16-bit value where 12-bits are the mantissa and 4-bits are for
> the exponent. Should I call realbits 16 and storagebits 32 ?
Only relevant if you are doing buffered mode. Unless we extend the bufferred
data description futher you'll need to unwind the format into a more
conventional form in kernel anyway.  With a 4 bit mantissa and 16 bit value
you'll need to have a

> 
>> 
>>> +			.endianness = IIO_BE, +		}, +		.scan_index = 0, +		.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; + +	ret =
>>> opt3001_read(opt, 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, &reg,
>>> OPT3001_CONFIGURATION_M_SINGLE); + +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +
>>> dev_err(opt->dev, "failed to read register %02x\n",
>> 
>> "failed to write register ..."
> 
> yeah thanks. copy&paste error.
> 
>>> +				OPT3001_CONFIGURATION); +		return ret; +	} + +	/* wait for conversion and give it an extra 5ms */ +
>>> usleep_range(opt->int_time + 5000, opt->int_time + 10000); + +	ret = opt3001_read(opt, OPT3001_CONFIGURATION); 
>>> +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_CONFIGURATION); +		return
>>> ret; +	} + +	reg = ret; +	if (!(reg & OPT3001_CONFIGURATION_CRF)) +		return -EPIPE; + +	ret = opt3001_read(opt,
>>> OPT3001_RESULT); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_RESULT); 
>>> +		return ret; +	} + +	exponent = OPT3001_REG_EXPONENT(ret); +	mantissa = OPT3001_REG_MANTISSA(ret); + +
>>> opt3001_to_iio_ret(opt, exponent, mantissa, val, val2); + +	return IIO_VAL_INT_PLUS_MICRO; +} + +static int
>>> opt3001_get_scale(struct opt3001 *opt, int *val, int *val2) +{ +	int ret; +	u8 exponent; +
>> 
>> I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux()
> 
> right. I'm only interested in the exponent part. It'll latch the exponent from last conversion, 0 being default.
Given you unwind the scale anyway in your read raw, I'd not bother
having a scale interface at all (assuming I'm right in thinking your
read raw is returning the value in lux?)  Also as you are in lux, it
should be the processed form not the raw one to indicate the scale
does not need to be applied to the value to convert to lux.

For a light sensor, the conversion here is actually fairly straight
forward. Almost all of them have to use the processed interface
as they are horribly non linear.
> 
>>> +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;
>> 
>> initialization not needed
> 
> I'll drop
> 
>>> +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;
>> 
>> initialization not needed
> 
> I'll drop
> 
>>> + +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) +		return -EBUSY; + +	if (chan->type != IIO_LIGHT) +
>>> return -EINVAL; + +	mutex_lock(&opt->lock); + +	switch (mask) {
>> 
>> is there no way to control the scale?
> 
> yeah, but the HW does auto-scaling which works pretty well. Why would you want to control the scale manually ?
On a slow device like this you probably wouldn't but for quicker devices
the overhead of converting into a standard form in kernel might be
excessive.
> 
>>> +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; + +	u16 *thresh_mantissa; +	u16 mantissa; +	u16 value; +
>>> u16 reg; + +	u8 *thresh_exp;
>> 
>> why extra newlines?
> 
> why not ? I generally separate the types as it aids readability.
> 
>>> +static int opt3001_remove(struct i2c_client *client) +{ +	/* nothing to do here */
>> 
>> shutdown the device probably?
> 
> it's always in shutdown unless it's doing a transfer. The only exception being if we enable continuous mode and
> remove the module. Might be better to check that and shut it down, sure.
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJT41AYAAoJEFSFNJnE9BaIge0QAJr/w1JYg8hdIO+sWbzqK6Ps
ifGSlbXbE0u14CCKovkjUytPxGVjddq/7EOJZlTq+P28j9x9YuAD8MXsKXF0G8Cb
rk1fwdty82WBdf1/IO5F752irmlJyO80oQlrqQjeN8xvbRYG9dIN7wiLYyyKSMBg
Nv9Ipl3m7q6Sk22R0rcwTmGaESKHUsGLh2OvujOZd81gTpeOp456mTaWUC6Dbkvs
BkxGZrC/xJsOYYBcjPfEvP5BT0tSLizRybL5UvFTgeagjP8E8LoBvIjoj9fr0Z+e
5nuJbfWnhAtWisHh/jzyyEyqdKL3RaZmM1sY7+9k6bhN7rRwheAe7mqk4dYTRMMq
DrwL8bIU6RyTEJlW1lwTL6fzQFGYqurR34+JqTwxnpcESCfle4SSnbRUaOnLpRff
VQIcN/SVXBKsp6y+h80wEmihjdxEHp42h0X8srke91rGm3cmqC+rWzizCM9/vsEh
Dwih+UeLlh5npk4AZyoVkPc+sFRYCFMd+HKvqB35QjLVhcIKg5eYCSSsRNuVaQc5
WJd5G/Gn7l2K8UyAiQg7m5+IqYlSgCbJurtuFznlK07SIX7lkGcmZC4VCWimv1H6
rzROP6b61VAeUCJPjp5uu5ItWIhmPcaMHJlZAFsGWoI637OgPEUYYyQHRU9m+4Po
lKg90ctYuwP4ja4d8rhR
=3RvV
-----END PGP SIGNATURE-----

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/08/14 23:38, Felipe Balbi wrote:
> On Thu, Aug 07, 2014 at 12:35:11AM +0200, Peter Meerwald wrote:
>> 
>>>>> ok... but then that brings up a doubt. What would I use triggered buffer for ? I already setup High/Low
>>>>> limits (thresholds) through .write_event_value(). Isn't that, pretty much, a triggered buffer ?
>>>> 
>>>> a triggered buffer reads data from the device and stores it into a buffer whenever the trigger goes off (you
>>>> are interested to capture all samples)
>>> 
>>> Alright, so something like a contious capture starting after a GPIO goes off, or something ?
>> 
>> right, one capture whenever the GPIO becomes active typically
Or if I am conjecturing correctly on what the hystersis mode here does, it
would be applicable for that.  The interrupts indicating a change would provide
the iio_trigger and that would result in the buffer being filled.

This allows you to poll/select on the data arriving.
The BMC100 driver currently under review does something very similar with
a motion detector interrupt so that data is only captured if significant
motion is going on.

>> 
>>>> events are created on certain conditions (you are not interested in all samples)
>>> 
>>> And this would be more like e.g. "tell me once I have 100 lux or more" ?
>> 
>> yes
> 
> cool thanks. I'll wait for some more review comments before sending a new version. Latest is kept at [1]
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=opt3001
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJT41E4AAoJEFSFNJnE9BaIqogQAJdlZ3uq6Nf6BDYazpn8YfPZ
E4t7FocO6DtZHYgUp+MLOadB4cQv5VKT+K/D3RSsc3C10JhC7la3b3Rxh0+ia1wq
iu8HPTNccYkkcexamw+JSPR5TjirC4uVnajFzmtaiZ7vGD/cexP9phBHrGZsts1b
VcK44VVxi59smFi+P+6LPYqm9cc5NwjUjCErfEf73x+NzOOA6myNwoBLrH6uGRHj
kIVp4icBA3ajN5FntBEbVCKHyqeLvFUQKCznKLb+wIn7nf5yjGiHCMntlAYF9O5+
m5seoOdp0znUMkp1hAhfceyVQvyPKEV1nNZYXnH7fnYrshtcy1P/hZuhen+wIooM
+WWVUQBRVRH6Jan1lwPhz8TkXvwFwPGd1+5BTq/wk4J0s6YEUpn+Vee1Q7m2e4u1
Rm4JSA2Kx/WppBuF9kTG+tjBBm9D0MVhN0qoNT/Q4qXTop24pY4Vxmf8ZyvkB8CH
fSrj+p/wkinNJksKjuu8VfCr3WH/Fni+I8comLfIo1AHl63ADR+3z1U/tMndTMdS
TBoAgYOnhWQHVoL1IL5qbDv//wBGqJdwZL9DoctYnwNaY2mtbmUdsciY1T4htmJe
M/1PGl1h5yjCMxiT54jp35XfWgtUav2MvVkdKguh6KlDt0RqSjvP4ZKkyOHJxvsG
3QlQTvxr+tupJzfYH+4v
=cUWM
-----END PGP SIGNATURE-----

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 13300 bytes --]

Hi,

On Thu, Aug 07, 2014 at 11:01:04AM +0100, Jonathan Cameron wrote:
> On 06/08/14 17:10, Felipe Balbi 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.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> I don't suppose there is a datasheet available?

There is a summary datasheet available. The device isn't released yet:

http://www.ti.com/product/OPT3001?keyMatch=opt3001&tisearch=Search-EN

> When in M_CONTINUOUS is it just providing event interrupts
> (threshold etc)?

It depends on the mode. If Latch bit is set, then it'll report event
interrupts. Datasheet calls this a "window-style comparison operation).

If Latch bit is cleared, then it'll continuously report the comparison
result. Datasheet calls this a "hysteresis-style comoparison operation).

> > Jonathan, I have a few doubts on how can I test buffered
> > mode properly. I can see my IRQs triggering now problem,
> > but where is the data pushed ? Can it be read from sysfs
> > or /dev somehwere ?
> As  I guess has become clear from your thread with Peter,
> what you have here in IIO terms are called events (separate
> form the buffer which is for the main data flow).
> 
> Anyhow, see drivers/staging/iio/Documentation/iio_event_monitor.c.
> Some trickery with an anonymous file descriptor is used to avoid
> using lots of chardevs for the same device.

ok, will look at it.

> > Also, there are a couple details about this device which
> > I need to know if it looks for you:
> >
> > This device has a single enable bit which will enable both
> > rising and falling edge triggers, but the limits are separate.
> >
> > The same limits are also used for hysteresis-style capture and
> > that's controlled by a single bit flip.
> With this on, it generates an event whenever the value changes by
> more than a certain amount?  If so this is better handled by
> providing a trigger and a buffer.  Note that the events path
> carries no data other than an event code.

It's still a bit weird to me. So, without Latch bit, then yeah, it'll be
more of a trigger e.g:

I can set low limit to 200 lux and high limit to 50 lux, then I'll get a
rising edge IRQ when I have more than 50 lux and a falling edge when I
get less than 200 lux.

If Latch bit is set, however, then it's almost like it ignores low/high
limit altogether and I get an IRQ ever $int_time ms or so.

> > How do you want this to look on sysfs ? Currently, enable/disabling
> > any of rising/falling edges, will disable both.
> That's pretty common.  The ABI basically allows for any setting to
> change any other (as there are much weirder connections than this
> in some devices).

ok, so it's alright to have two "enable" files and have they refer to
the same bit ?

> > +static int opt3001_read(const struct opt3001 *opt, u8 reg)
> > +{
> > +	return i2c_smbus_read_word_swapped(opt->client, reg);
> > +}
> I'm very much against wrapper functions unless they add something. These
> really do not.

as I mentioned before, it helps a lot when using function tracer. I can
just "echo opt3001* > set_trace_function". But if you guys are so much
against it, I'll remove it.

> > +static const struct iio_chan_spec opt3001_channels[] = {
> > +	{
> > +		.type = IIO_LIGHT,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				BIT(IIO_CHAN_INFO_SCALE) |
> > +				BIT(IIO_CHAN_INFO_INT_TIME),
> > +		.channel = 0,
> 
> No buffering at the moment so can drop scan_type and scan_index.

will do

> > +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);
> > +
> So these will return the same value whether the value attribute is read
> or the hysteresis one?

yeah, the comparison done is the same.

> > +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;
> > +
> > +	u16 *thresh_mantissa;
> > +	u16 mantissa;
> > +	u16 value;
> > +	u16 reg;
> > +
> > +	u8 *thresh_exp;
> > +
> > +	u8 exponent;
> > +
> > +	mutex_lock(&opt->lock);
> > +
> > +	ret = opt3001_read(opt, OPT3001_CONFIGURATION);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> > +				OPT3001_CONFIGURATION);
> > +		goto err;
> > +	}
> > +
> > +	reg = ret;
> > +	if (info == IIO_EV_INFO_HYSTERESIS)
> Without a datasheet I'm not entirely sure how the hysteresis is
> applied here and what it's connection to the thresholds is...

yeah, that'll still take a while to be fully released.

> 
> > +		opt->hysteresis = true;
> > +	else
> > +		opt->hysteresis = false;
> > +
> > +	switch (dir) {
> > +	case IIO_EV_DIR_RISING:
> > +		reg = OPT3001_HIGH_LIMIT;
> Why bother with the indirection via a pointer. Just have a second
> switch statement at the end of the function that sets the right ones.
> Easier to follow than this.

sure

> > +		thresh_mantissa = &opt->high_thresh_mantissa;
> > +		thresh_exp = &opt->high_thresh_exp;
> > +		break;
> > +	case IIO_EV_DIR_FALLING:
> > +		reg = OPT3001_LOW_LIMIT;
> > +		thresh_mantissa = &opt->low_thresh_mantissa;
> > +		thresh_exp = &opt->low_thresh_exp;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		goto err;
> > +	}
> > +
> > +	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);
> > +		goto err;
> > +	}
> > +
> > +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
> > +	value = exponent << 12 | mantissa;
> > +	ret = opt3001_write(opt, reg, value);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n", reg);
> > +		goto err;
> > +	}
> > +
> As stated above an additional switch statement here with direct assignment
> of the relevant threshold values would be clearer.
> > +	*thresh_mantissa = mantissa;
> > +	*thresh_exp = exponent;
> > +
> > +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;
> > +
> > +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
> > +		: OPT3001_CONFIGURATION_M_SHUTDOWN;
> > +
> > +	ret = opt3001_read(opt, 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, &reg, mode);
> > +
> > +	if (opt->hysteresis)
> > +		reg |= OPT3001_CONFIGURATION_L;
> > +	else
> > +		reg &= ~OPT3001_CONFIGURATION_L;
> > +
> > +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> > +				OPT3001_CONFIGURATION);
> > +		return ret;
> > +	}
> > +
> > +	/* wait for mode change to go through */
> > +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> > +
> > +	return 0;
> > +}
> > +
> > +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 = opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt, OPT3001_CONFIGURATION);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> > +				OPT3001_CONFIGURATION);
> > +		return ret;
> > +	}
> > +
> > +	reg = ret;
> > +
> > +	if (reg & OPT3001_CONFIGURATION_CT)
> > +		opt->int_time = 800000;
> > +	else
> > +		opt->int_time = 100000;
> > +
> > +	reg &= ~OPT3001_CONFIGURATION_L;
> > +	reg &= ~OPT3001_CONFIGURATION_RN_MASK;
> > +	reg |= OPT3001_CONFIGURATION_RN_AUTO;
> > +
> > +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to write register %02x\n",
> > +				OPT3001_CONFIGURATION);
> > +		return ret;
> > +	}
> > +
> > +	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt)
> > +{
> 
> Pass the iio_dev in to the irq setup instead of opt and
> you will then not need the opt reference to the struct iio_dev.

What's the difference ? When used this way we have:

	struct opt3001 *opt = _opt;
	struct iio_dev *iio = opt->dev;

Done your way we will have:

	struct iio_dev *iio = _iio;
	struct opt3001 *opt = iio_priv(iio);

I don't see the benefit of changing this as I'll access to both opt and
iio.

> > +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;
> > +
> > +	iio = devm_iio_device_alloc(dev, sizeof(*opt));
> > +	if (!iio)
> > +		return -ENOMEM;
> > +
> > +	opt = iio_priv(iio);
> > +	opt->client = client;
> > +	opt->dev = dev;
> 
> This is very circular and it rarely makes sense to have
> the device private structure have a reference to the
> iio_dev.  Here it is only used in the interrupt handler.
> Avoid that by making the private data passed to the interrupt
> handler the struct iio_dev instead of the struct opt3001.
> 
> > +	opt->iio = iio;
> > +
> > +	mutex_init(&opt->lock);
> > +	i2c_set_clientdata(client, opt);
> > +
> > +	ret = opt3001_read_id(opt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = opt3001_configure(opt);
> > +	if (ret)
> > +		return ret;
> > +
> > +	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 = devm_iio_device_register(dev, iio);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register IIO device\n");
> > +		return ret;
> > +	}
> > +
> Normally we'd expect the devm_iio_device_register (that provides the
> userspace interfaces) to be after the irq request.

that's wrong. By the time the IRQ is requested, an IRQ can actually fire
and you better have a valid e.g. iio_dev by then.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2490 bytes --]

Hi,

On Thu, Aug 07, 2014 at 11:08:24AM +0100, Jonathan Cameron wrote:
> >>> +				OPT3001_CONFIGURATION); +		return ret; +	} + +	/* wait for conversion and give it an extra 5ms */ +
> >>> usleep_range(opt->int_time + 5000, opt->int_time + 10000); + +	ret = opt3001_read(opt, OPT3001_CONFIGURATION); 
> >>> +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_CONFIGURATION); +		return
> >>> ret; +	} + +	reg = ret; +	if (!(reg & OPT3001_CONFIGURATION_CRF)) +		return -EPIPE; + +	ret = opt3001_read(opt,
> >>> OPT3001_RESULT); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_RESULT); 
> >>> +		return ret; +	} + +	exponent = OPT3001_REG_EXPONENT(ret); +	mantissa = OPT3001_REG_MANTISSA(ret); + +
> >>> opt3001_to_iio_ret(opt, exponent, mantissa, val, val2); + +	return IIO_VAL_INT_PLUS_MICRO; +} + +static int
> >>> opt3001_get_scale(struct opt3001 *opt, int *val, int *val2) +{ +	int ret; +	u8 exponent; +
> >> 
> >> I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux()
> > 
> > right. I'm only interested in the exponent part. It'll latch the exponent from last conversion, 0 being default.
> Given you unwind the scale anyway in your read raw, I'd not bother
> having a scale interface at all (assuming I'm right in thinking your
> read raw is returning the value in lux?)  Also as you are in lux, it
> should be the processed form not the raw one to indicate the scale
> does not need to be applied to the value to convert to lux.

alright, I'll switch to processed and remove scale.

> For a light sensor, the conversion here is actually fairly straight
> forward. Almost all of them have to use the processed interface
> as they are horribly non linear.

:)

> > 
> >>> + +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) +		return -EBUSY; + +	if (chan->type != IIO_LIGHT) +
> >>> return -EINVAL; + +	mutex_lock(&opt->lock); + +	switch (mask) {
> >> 
> >> is there no way to control the scale?
> > 
> > yeah, but the HW does auto-scaling which works pretty well. Why would you want to control the scale manually ?
> On a slow device like this you probably wouldn't but for quicker devices
> the overhead of converting into a standard form in kernel might be
> excessive.

hehe, even when you _do_ set the scale, you still have the top 4 bits as
exponent, the difference is that you know what they are before the
conversion is done :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/08/14 15:28, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Aug 07, 2014 at 11:01:04AM +0100, Jonathan Cameron wrote:
>> On 06/08/14 17:10, Felipe Balbi 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.
>>> 
>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>> I don't suppose there is a datasheet available?
> 
> There is a summary datasheet available. The device isn't released yet:
> 
> http://www.ti.com/product/OPT3001?keyMatch=opt3001&tisearch=Search-EN
> 
Ah well...  Not much on that ;)
>> When in M_CONTINUOUS is it just providing event interrupts (threshold etc)?
> 
> It depends on the mode. If Latch bit is set, then it'll report event interrupts. Datasheet calls this a
> "window-style comparison operation).
> 
> If Latch bit is cleared, then it'll continuously report the comparison result. Datasheet calls this a
> "hysteresis-style comoparison operation).
> 
>>> Jonathan, I have a few doubts on how can I test buffered mode properly. I can see my IRQs triggering now
>>> problem, but where is the data pushed ? Can it be read from sysfs or /dev somehwere ?
>> As  I guess has become clear from your thread with Peter, what you have here in IIO terms are called events
>> (separate form the buffer which is for the main data flow).
>> 
>> Anyhow, see drivers/staging/iio/Documentation/iio_event_monitor.c. Some trickery with an anonymous file
>> descriptor is used to avoid using lots of chardevs for the same device.
> 
> ok, will look at it.
> 
>>> Also, there are a couple details about this device which I need to know if it looks for you:
>>> 
>>> This device has a single enable bit which will enable both rising and falling edge triggers, but the limits are
>>> separate.
>>> 
>>> The same limits are also used for hysteresis-style capture and that's controlled by a single bit flip.
>> With this on, it generates an event whenever the value changes by more than a certain amount?  If so this is
>> better handled by providing a trigger and a buffer.  Note that the events path carries no data other than an
>> event code.
> 
> It's still a bit weird to me. So, without Latch bit, then yeah, it'll be more of a trigger e.g:
IIO triggers are actually more similar to those you find in video
systems than the osciloscope type triggers you are perhaps thinking of.
They cause a 'scan' of data to be captured every time the fire.

A 'scope trigger would cause a set of data to be captured everytime it
first (e.g. multiple scans of the available channels).  We have talked
a few times about adding this option, but it hasn't happened yet...
> 
> I can set low limit to 200 lux and high limit to 50 lux, then I'll get a rising edge IRQ when I have more than 50
> lux and a falling edge when I get less than 200 lux.
> 
> If Latch bit is set, however, then it's almost like it ignores low/high limit altogether and I get an IRQ ever
> $int_time ms or so.
That would be similar to the motion triggers we have for accelerometers.
They capture every sample as long as the threshold is met (during motion).
No idea why you'd really want to do that on a light sensor though.  Perhaps
not worth capturing light info if the cover is closed on a phone?
> 
>>> How do you want this to look on sysfs ? Currently, enable/disabling any of rising/falling edges, will disable
>>> both.
>> That's pretty common.  The ABI basically allows for any setting to change any other (as there are much weirder
>> connections than this in some devices).
> 
> ok, so it's alright to have two "enable" files and have they refer to the same bit ?
Absolutely.  Depending on the combinations it is sometimes possible to
construct an 'EITHER' enable file - for example
driver/iio/adc/xilinix-xadc-core.c

> 
>>> +static int opt3001_read(const struct opt3001 *opt, u8 reg) +{ +	return
>>> i2c_smbus_read_word_swapped(opt->client, reg); +}
>> I'm very much against wrapper functions unless they add something. These really do not.
> 
> as I mentioned before, it helps a lot when using function tracer. I can just "echo opt3001* > set_trace_function".
> But if you guys are so much against it, I'll remove it.
> 
>>> +static const struct iio_chan_spec opt3001_channels[] = { +	{ +		.type = IIO_LIGHT, +		.info_mask_separate =
>>> BIT(IIO_CHAN_INFO_RAW) | +				BIT(IIO_CHAN_INFO_SCALE) | +				BIT(IIO_CHAN_INFO_INT_TIME), +		.channel = 0,
>> 
>> No buffering at the moment so can drop scan_type and scan_index.
> 
> will do
> 
>>> +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); +
>> So these will return the same value whether the value attribute is read or the hysteresis one?
> 
> yeah, the comparison done is the same.
> 
>>> +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; + +	u16 *thresh_mantissa; +	u16 mantissa; +	u16 value; +
>>> u16 reg; + +	u8 *thresh_exp; + +	u8 exponent; + +	mutex_lock(&opt->lock); + +	ret = opt3001_read(opt,
>>> OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +
>>> OPT3001_CONFIGURATION); +		goto err; +	} + +	reg = ret; +	if (info == IIO_EV_INFO_HYSTERESIS)
>> Without a datasheet I'm not entirely sure how the hysteresis is applied here and what it's connection to the
>> thresholds is...
> 
> yeah, that'll still take a while to be fully released.
> 
>> 
>>> +		opt->hysteresis = true; +	else +		opt->hysteresis = false; + +	switch (dir) { +	case IIO_EV_DIR_RISING: +
>>> reg = OPT3001_HIGH_LIMIT;
>> Why bother with the indirection via a pointer. Just have a second switch statement at the end of the function
>> that sets the right ones. Easier to follow than this.
> 
> sure
> 
>>> +		thresh_mantissa = &opt->high_thresh_mantissa; +		thresh_exp = &opt->high_thresh_exp; +		break; +	case
>>> IIO_EV_DIR_FALLING: +		reg = OPT3001_LOW_LIMIT; +		thresh_mantissa = &opt->low_thresh_mantissa; +		thresh_exp =
>>> &opt->low_thresh_exp; +		break; +	default: +		ret = -EINVAL; +		goto err; +	} + +	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); +
>>> goto err; +	} + +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; +	value = exponent << 12 |
>>> mantissa; +	ret = opt3001_write(opt, reg, value); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read
>>> register %02x\n", reg); +		goto err; +	} +
>> As stated above an additional switch statement here with direct assignment of the relevant threshold values would
>> be clearer.
>>> +	*thresh_mantissa = mantissa; +	*thresh_exp = exponent; + +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; + +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS +		:
>>> OPT3001_CONFIGURATION_M_SHUTDOWN; + +	ret = opt3001_read(opt, 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, &reg, mode); + +	if (opt->hysteresis) +		reg |= OPT3001_CONFIGURATION_L; +	else +
>>> reg &= ~OPT3001_CONFIGURATION_L; + +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +
>>> dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_CONFIGURATION); +		return ret; +	} + +	/* wait
>>> for mode change to go through */ +	usleep_range(opt->int_time + 5000, opt->int_time + 10000); + +	return 0; +} 
>>> + +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 =
>>> opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt,
>>> OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +
>>> OPT3001_CONFIGURATION); +		return ret; +	} + +	reg = ret; + +	if (reg & OPT3001_CONFIGURATION_CT) +
>>> opt->int_time = 800000; +	else +		opt->int_time = 100000; + +	reg &= ~OPT3001_CONFIGURATION_L; +	reg &=
>>> ~OPT3001_CONFIGURATION_RN_MASK; +	reg |= OPT3001_CONFIGURATION_RN_AUTO; + +	ret = opt3001_write(opt,
>>> OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +		dev_err(opt->dev, "failed to write register %02x\n", +
>>> OPT3001_CONFIGURATION); +		return ret; +	} + +	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt) +{
>> 
>> Pass the iio_dev in to the irq setup instead of opt and you will then not need the opt reference to the struct
>> iio_dev.
> 
> What's the difference ? When used this way we have:
> 
> struct opt3001 *opt = _opt; struct iio_dev *iio = opt->dev;
> 
> Done your way we will have:
> 
> struct iio_dev *iio = _iio; struct opt3001 *opt = iio_priv(iio);
> 
> I don't see the benefit of changing this as I'll access to both opt and iio.
Because one way you get it from the IIO core support.  The other way around
you have a local copy.  Basically it's better from a consistency point
of view to have all drivers do it one way around than end up with a mixture
of the two (which way around doesn't really matter - but rather late to
change now!)
> 
>>> +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; + +	iio =
>>> devm_iio_device_alloc(dev, sizeof(*opt)); +	if (!iio) +		return -ENOMEM; + +	opt = iio_priv(iio); +	opt->client
>>> = client; +	opt->dev = dev;
>> 
>> This is very circular and it rarely makes sense to have the device private structure have a reference to the 
>> iio_dev.  Here it is only used in the interrupt handler. Avoid that by making the private data passed to the
>> interrupt handler the struct iio_dev instead of the struct opt3001.
>> 
>>> +	opt->iio = iio; + +	mutex_init(&opt->lock); +	i2c_set_clientdata(client, opt); + +	ret =
>>> opt3001_read_id(opt); +	if (ret) +		return ret; + +	ret = opt3001_configure(opt); +	if (ret) +		return ret; + +
>>> 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 = devm_iio_device_register(dev, iio); +	if (ret) { +		dev_err(dev, "failed to register
>>> IIO device\n"); +		return ret; +	} +
>> Normally we'd expect the devm_iio_device_register (that provides the userspace interfaces) to be after the irq
>> request.
> 
> that's wrong. By the time the IRQ is requested, an IRQ can actually fire and you better have a valid e.g. iio_dev
> by then.
Its very unusual to bring up a device with the IRQ already in a position
to fire.  Normally that would require some enabling from userspace after the
driver is loaded?  How can it fire here before that point?

Unless there is something odd going on, that would normally imply a bug.
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJT46i0AAoJEFSFNJnE9BaIYr0P/3wOD4bRMtaG6EeegC/eqUdp
cRQpNtJRgknKv/pJ7WRq+4p7JNVeTdfDIyNP2ITm+7BsO/Iy7EzfKjjzcOVEeku/
mR0y0CXAos6wvbM2OXoX/XPBMn6LMzv33LS5sQRAig3PNDPDT4tZ/ibFitM/sGTO
YnuxwX35xDnM8AVOEKjDlJWxVDWejyNjv6IC56FPCCls/mCoBLHg+aUn8vovqI/P
2ymwtcHlSbMlgjAYE1Pn3qXpIdw7dmAlOKNAIsljMVC/bbqVcP/B8xWwlHTxOKr4
qpJFXSEYT+7QR2PlHZLe/kHDztEuC24S8hGP/3ER8aKGfl9ze20slILTyzm84V6Y
fEFIW8vopuiofaAyZPA8nrGgWmdgu7BpufYKKstjqHbbb6C6CQjrzBD+0ldl60Jl
hBYpQRxiz0KbUu7RceL0ZPuSCF7aar7GVixDtCGVQLtDrey5UXqqb83EPmIm7co1
4LoRpvK5D8J+2Vho4q3aKXOpTpyeAIcu/nwQm6O6SWYoqKbYIO7XjZtKwQB0VDGM
YCHOdye+bOHj7TPtD6JtyZ+PZ+lpu3VPFZrY5Qh7kn420tJ6thJo+eEEnBfSus0Z
2OOptXteAgbZ5V007oHKZiK4SRDtuvbaQV4AW2RsHF06PbrvbYkjT1ZJEmrasJfS
Ng/WQOVl5zWSP9jYX1Z6
=8Vup
-----END PGP SIGNATURE-----

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/08/14 15:39, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Aug 07, 2014 at 11:08:24AM +0100, Jonathan Cameron wrote:
>>>>> +				OPT3001_CONFIGURATION); +		return ret; +	} + +	/* wait for conversion and give it an extra 5ms */ + 
>>>>> usleep_range(opt->int_time + 5000, opt->int_time + 10000); + +	ret = opt3001_read(opt,
>>>>> OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +
>>>>> OPT3001_CONFIGURATION); +		return ret; +	} + +	reg = ret; +	if (!(reg & OPT3001_CONFIGURATION_CRF)) +
>>>>> return -EPIPE; + +	ret = opt3001_read(opt, OPT3001_RESULT); +	if (ret < 0) { +		dev_err(opt->dev, "failed
>>>>> to read register %02x\n", +				OPT3001_RESULT); +		return ret; +	} + +	exponent =
>>>>> OPT3001_REG_EXPONENT(ret); +	mantissa = OPT3001_REG_MANTISSA(ret); + + opt3001_to_iio_ret(opt, exponent,
>>>>> mantissa, val, val2); + +	return IIO_VAL_INT_PLUS_MICRO; +} + +static int opt3001_get_scale(struct opt3001
>>>>> *opt, int *val, int *val2) +{ +	int ret; +	u8 exponent; +
>>>> 
>>>> I guess OPT3001_RESULT is only valid AFTER performing opt3001_get_lux()
>>> 
>>> right. I'm only interested in the exponent part. It'll latch the exponent from last conversion, 0 being
>>> default.
>> Given you unwind the scale anyway in your read raw, I'd not bother having a scale interface at all (assuming I'm
>> right in thinking your read raw is returning the value in lux?)  Also as you are in lux, it should be the
>> processed form not the raw one to indicate the scale does not need to be applied to the value to convert to lux.
> 
> alright, I'll switch to processed and remove scale.
> 
>> For a light sensor, the conversion here is actually fairly straight forward. Almost all of them have to use the
>> processed interface as they are horribly non linear.
> 
> :)
> 
>>> 
>>>>> + +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) +		return -EBUSY; + +	if (chan->type != IIO_LIGHT)
>>>>> + return -EINVAL; + +	mutex_lock(&opt->lock); + +	switch (mask) {
>>>> 
>>>> is there no way to control the scale?
>>> 
>>> yeah, but the HW does auto-scaling which works pretty well. Why would you want to control the scale manually ?
>> On a slow device like this you probably wouldn't but for quicker devices the overhead of converting into a
>> standard form in kernel might be excessive.
> 
> hehe, even when you _do_ set the scale, you still have the top 4 bits as exponent, the difference is that you know
> what they are before the conversion is done :-)
Then you can junk them and let userspace use the known value to deal with the raw data.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJT46kMAAoJEFSFNJnE9BaICmQP/iY3mtojtzobsU1BTFiyd6ah
xp/5vsyCM4pdqMOm8xUDwp6aBlvBvAQfVDGcQetR/KawJt7SMtKJdDxhR/hkFodE
E/3thrUXjCYw0VW9Xkg5doBucqbaWOjNsjnZDFH8HBZupFTWHv3I/QQq3acbdAbJ
DC0DRtfrTBF2JEOSLRssY11pAbxkDdeuZvIqQLpwZ80+Ptpi9MkeWoHlwOq9v67P
s0KODDTep2n7Z2w9KaY4V5L5YgLjW5YsDW+qv3zpQyq54+LB+CnCFMK2DZ4RqQlt
zPDuz7Cw8ujiqoHJlrs+bc/AbTk6gV/+puOyzb9zp6/HbSF/Q72Drw/1yF+2dIz4
rZtNaxsg0I+mH0e8xiKRYIeYHEKnHvwhO2e+6CJdLWabYvS8tSiYrU7qHKwkZVja
P04RGZNdqWrdmEOmxidd9CO+K7lvMqv/pSzVmVpQWq7Zw9EDx77h1SxyIHWtnD+3
CsiZluC++2JqCdvU01Cen+x95DPZPfK3hp6u7Y8SGgKwGluxxRMlN3QyYQXC5g5s
o9G4Jw/0wV0hshuTmwR9SifwkS2XMp/FbCJC2/c6A8PUx9O9H5IVIqk+ChSaWXkP
3NPl2O1hQIdjLgoDF5Xn4SD+F/CC3WPczMV+zcW8GrmZKjb30X0x5BNIv2UxKN0H
zJyy8hv/NLTUBaoXd5BG
=QBgz
-----END PGP SIGNATURE-----

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

Sorry all. I've dropped back to an old Hard drive after some laptop troubles
and seems my email is interestingly configured hence the wrapping occuring
when I sign and email...

Will just stop signing them for now!


On 07/08/14 17:26, Jonathan Cameron wrote:
> On 07/08/14 15:28, Felipe Balbi wrote:
>> Hi,
> 
>> On Thu, Aug 07, 2014 at 11:01:04AM +0100, Jonathan Cameron wrote:
>>> On 06/08/14 17:10, Felipe Balbi 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.
>>>>
>>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>> I don't suppose there is a datasheet available?
> 
>> There is a summary datasheet available. The device isn't released yet:
> 
>> http://www.ti.com/product/OPT3001?keyMatch=opt3001&tisearch=Search-EN
> 
> Ah well...  Not much on that ;)
>>> When in M_CONTINUOUS is it just providing event interrupts (threshold etc)?
> 
>> It depends on the mode. If Latch bit is set, then it'll report event interrupts. Datasheet calls this a
>> "window-style comparison operation).
> 
>> If Latch bit is cleared, then it'll continuously report the comparison result. Datasheet calls this a
>> "hysteresis-style comoparison operation).
> 
>>>> Jonathan, I have a few doubts on how can I test buffered mode properly. I can see my IRQs triggering now
>>>> problem, but where is the data pushed ? Can it be read from sysfs or /dev somehwere ?
>>> As  I guess has become clear from your thread with Peter, what you have here in IIO terms are called events
>>> (separate form the buffer which is for the main data flow).
>>>
>>> Anyhow, see drivers/staging/iio/Documentation/iio_event_monitor.c. Some trickery with an anonymous file
>>> descriptor is used to avoid using lots of chardevs for the same device.
> 
>> ok, will look at it.
> 
>>>> Also, there are a couple details about this device which I need to know if it looks for you:
>>>>
>>>> This device has a single enable bit which will enable both rising and falling edge triggers, but the limits are
>>>> separate.
>>>>
>>>> The same limits are also used for hysteresis-style capture and that's controlled by a single bit flip.
>>> With this on, it generates an event whenever the value changes by more than a certain amount?  If so this is
>>> better handled by providing a trigger and a buffer.  Note that the events path carries no data other than an
>>> event code.
> 
>> It's still a bit weird to me. So, without Latch bit, then yeah, it'll be more of a trigger e.g:
> IIO triggers are actually more similar to those you find in video
> systems than the osciloscope type triggers you are perhaps thinking of.
> They cause a 'scan' of data to be captured every time the fire.
> 
> A 'scope trigger would cause a set of data to be captured everytime it
> first (e.g. multiple scans of the available channels).  We have talked
> a few times about adding this option, but it hasn't happened yet...
> 
>> I can set low limit to 200 lux and high limit to 50 lux, then I'll get a rising edge IRQ when I have more than 50
>> lux and a falling edge when I get less than 200 lux.
> 
>> If Latch bit is set, however, then it's almost like it ignores low/high limit altogether and I get an IRQ ever
>> $int_time ms or so.
> That would be similar to the motion triggers we have for accelerometers.
> They capture every sample as long as the threshold is met (during motion).
> No idea why you'd really want to do that on a light sensor though.  Perhaps
> not worth capturing light info if the cover is closed on a phone?
> 
>>>> How do you want this to look on sysfs ? Currently, enable/disabling any of rising/falling edges, will disable
>>>> both.
>>> That's pretty common.  The ABI basically allows for any setting to change any other (as there are much weirder
>>> connections than this in some devices).
> 
>> ok, so it's alright to have two "enable" files and have they refer to the same bit ?
> Absolutely.  Depending on the combinations it is sometimes possible to
> construct an 'EITHER' enable file - for example
> driver/iio/adc/xilinix-xadc-core.c
> 
> 
>>>> +static int opt3001_read(const struct opt3001 *opt, u8 reg) +{ +	return
>>>> i2c_smbus_read_word_swapped(opt->client, reg); +}
>>> I'm very much against wrapper functions unless they add something. These really do not.
> 
>> as I mentioned before, it helps a lot when using function tracer. I can just "echo opt3001* > set_trace_function".
>> But if you guys are so much against it, I'll remove it.
> 
>>>> +static const struct iio_chan_spec opt3001_channels[] = { +	{ +		.type = IIO_LIGHT, +		.info_mask_separate =
>>>> BIT(IIO_CHAN_INFO_RAW) | +				BIT(IIO_CHAN_INFO_SCALE) | +				BIT(IIO_CHAN_INFO_INT_TIME), +		.channel = 0,
>>>
>>> No buffering at the moment so can drop scan_type and scan_index.
> 
>> will do
> 
>>>> +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); +
>>> So these will return the same value whether the value attribute is read or the hysteresis one?
> 
>> yeah, the comparison done is the same.
> 
>>>> +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; + +	u16 *thresh_mantissa; +	u16 mantissa; +	u16 value; +
>>>> u16 reg; + +	u8 *thresh_exp; + +	u8 exponent; + +	mutex_lock(&opt->lock); + +	ret = opt3001_read(opt,
>>>> OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +
>>>> OPT3001_CONFIGURATION); +		goto err; +	} + +	reg = ret; +	if (info == IIO_EV_INFO_HYSTERESIS)
>>> Without a datasheet I'm not entirely sure how the hysteresis is applied here and what it's connection to the
>>> thresholds is...
> 
>> yeah, that'll still take a while to be fully released.
> 
>>>
>>>> +		opt->hysteresis = true; +	else +		opt->hysteresis = false; + +	switch (dir) { +	case IIO_EV_DIR_RISING: +
>>>> reg = OPT3001_HIGH_LIMIT;
>>> Why bother with the indirection via a pointer. Just have a second switch statement at the end of the function
>>> that sets the right ones. Easier to follow than this.
> 
>> sure
> 
>>>> +		thresh_mantissa = &opt->high_thresh_mantissa; +		thresh_exp = &opt->high_thresh_exp; +		break; +	case
>>>> IIO_EV_DIR_FALLING: +		reg = OPT3001_LOW_LIMIT; +		thresh_mantissa = &opt->low_thresh_mantissa; +		thresh_exp =
>>>> &opt->low_thresh_exp; +		break; +	default: +		ret = -EINVAL; +		goto err; +	} + +	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); +
>>>> goto err; +	} + +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; +	value = exponent << 12 |
>>>> mantissa; +	ret = opt3001_write(opt, reg, value); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read
>>>> register %02x\n", reg); +		goto err; +	} +
>>> As stated above an additional switch statement here with direct assignment of the relevant threshold values would
>>> be clearer.
>>>> +	*thresh_mantissa = mantissa; +	*thresh_exp = exponent; + +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; + +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS +		:
>>>> OPT3001_CONFIGURATION_M_SHUTDOWN; + +	ret = opt3001_read(opt, 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, &reg, mode); + +	if (opt->hysteresis) +		reg |= OPT3001_CONFIGURATION_L; +	else +
>>>> reg &= ~OPT3001_CONFIGURATION_L; + +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +
>>>> dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_CONFIGURATION); +		return ret; +	} + +	/* wait
>>>> for mode change to go through */ +	usleep_range(opt->int_time + 5000, opt->int_time + 10000); + +	return 0; +} 
>>>> + +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 =
>>>> opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt,
>>>> OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +
>>>> OPT3001_CONFIGURATION); +		return ret; +	} + +	reg = ret; + +	if (reg & OPT3001_CONFIGURATION_CT) +
>>>> opt->int_time = 800000; +	else +		opt->int_time = 100000; + +	reg &= ~OPT3001_CONFIGURATION_L; +	reg &=
>>>> ~OPT3001_CONFIGURATION_RN_MASK; +	reg |= OPT3001_CONFIGURATION_RN_AUTO; + +	ret = opt3001_write(opt,
>>>> OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +		dev_err(opt->dev, "failed to write register %02x\n", +
>>>> OPT3001_CONFIGURATION); +		return ret; +	} + +	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt) +{
>>>
>>> Pass the iio_dev in to the irq setup instead of opt and you will then not need the opt reference to the struct
>>> iio_dev.
> 
>> What's the difference ? When used this way we have:
> 
>> struct opt3001 *opt = _opt; struct iio_dev *iio = opt->dev;
> 
>> Done your way we will have:
> 
>> struct iio_dev *iio = _iio; struct opt3001 *opt = iio_priv(iio);
> 
>> I don't see the benefit of changing this as I'll access to both opt and iio.
> Because one way you get it from the IIO core support.  The other way around
> you have a local copy.  Basically it's better from a consistency point
> of view to have all drivers do it one way around than end up with a mixture
> of the two (which way around doesn't really matter - but rather late to
> change now!)
> 
>>>> +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; + +	iio =
>>>> devm_iio_device_alloc(dev, sizeof(*opt)); +	if (!iio) +		return -ENOMEM; + +	opt = iio_priv(iio); +	opt->client
>>>> = client; +	opt->dev = dev;
>>>
>>> This is very circular and it rarely makes sense to have the device private structure have a reference to the 
>>> iio_dev.  Here it is only used in the interrupt handler. Avoid that by making the private data passed to the
>>> interrupt handler the struct iio_dev instead of the struct opt3001.
>>>
>>>> +	opt->iio = iio; + +	mutex_init(&opt->lock); +	i2c_set_clientdata(client, opt); + +	ret =
>>>> opt3001_read_id(opt); +	if (ret) +		return ret; + +	ret = opt3001_configure(opt); +	if (ret) +		return ret; + +
>>>> 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 = devm_iio_device_register(dev, iio); +	if (ret) { +		dev_err(dev, "failed to register
>>>> IIO device\n"); +		return ret; +	} +
>>> Normally we'd expect the devm_iio_device_register (that provides the userspace interfaces) to be after the irq
>>> request.
> 
>> that's wrong. By the time the IRQ is requested, an IRQ can actually fire and you better have a valid e.g. iio_dev
>> by then.
> Its very unusual to bring up a device with the IRQ already in a position
> to fire.  Normally that would require some enabling from userspace after the
> driver is loaded?  How can it fire here before that point?
> 
> Unless there is something odd going on, that would normally imply a bug.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 11852 bytes --]

Hi,

On Thu, Aug 07, 2014 at 05:26:28PM +0100, Jonathan Cameron wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 07/08/14 15:28, Felipe Balbi wrote:
> > Hi,
> > 
> > On Thu, Aug 07, 2014 at 11:01:04AM +0100, Jonathan Cameron wrote:
> >> On 06/08/14 17:10, Felipe Balbi 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.
> >>> 
> >>> Signed-off-by: Felipe Balbi <balbi@ti.com>
> >> I don't suppose there is a datasheet available?
> > 
> > There is a summary datasheet available. The device isn't released yet:
> > 
> > http://www.ti.com/product/OPT3001?keyMatch=opt3001&tisearch=Search-EN
> > 
> Ah well...  Not much on that ;)

yeah, when the thing is released the full datasheet will be available,
I'm sure. Until then, you'd have to sign an NDA with TI to get the full
datasheet, I'm afraid :-s

> >>> Also, there are a couple details about this device which I need to know if it looks for you:
> >>> 
> >>> This device has a single enable bit which will enable both rising and falling edge triggers, but the limits are
> >>> separate.
> >>> 
> >>> The same limits are also used for hysteresis-style capture and that's controlled by a single bit flip.
> >> With this on, it generates an event whenever the value changes by more than a certain amount?  If so this is
> >> better handled by providing a trigger and a buffer.  Note that the events path carries no data other than an
> >> event code.
> > 
> > It's still a bit weird to me. So, without Latch bit, then yeah, it'll be more of a trigger e.g:
> IIO triggers are actually more similar to those you find in video
> systems than the osciloscope type triggers you are perhaps thinking of.
> They cause a 'scan' of data to be captured every time the fire.
> 
> A 'scope trigger would cause a set of data to be captured everytime it
> first (e.g. multiple scans of the available channels).  We have talked
> a few times about adding this option, but it hasn't happened yet...

ok

> > I can set low limit to 200 lux and high limit to 50 lux, then I'll get a rising edge IRQ when I have more than 50
> > lux and a falling edge when I get less than 200 lux.
> > 
> > If Latch bit is set, however, then it's almost like it ignores low/high limit altogether and I get an IRQ ever
> > $int_time ms or so.
> That would be similar to the motion triggers we have for accelerometers.
> They capture every sample as long as the threshold is met (during motion).
> No idea why you'd really want to do that on a light sensor though.  Perhaps
> not worth capturing light info if the cover is closed on a phone?

Yeah, but that's something else altogether. This device doesn't really
care about a phone cover. Sounds like policy which should be in
userland, or part of an iio_gpio_trigger...

> >>> How do you want this to look on sysfs ? Currently, enable/disabling any of rising/falling edges, will disable
> >>> both.
> >> That's pretty common.  The ABI basically allows for any setting to change any other (as there are much weirder
> >> connections than this in some devices).
> > 
> > ok, so it's alright to have two "enable" files and have they refer to the same bit ?
> Absolutely.  Depending on the combinations it is sometimes possible to
> construct an 'EITHER' enable file - for example
> driver/iio/adc/xilinix-xadc-core.c

will read

> >>> +		thresh_mantissa = &opt->high_thresh_mantissa; +		thresh_exp = &opt->high_thresh_exp; +		break; +	case
> >>> IIO_EV_DIR_FALLING: +		reg = OPT3001_LOW_LIMIT; +		thresh_mantissa = &opt->low_thresh_mantissa; +		thresh_exp =
> >>> &opt->low_thresh_exp; +		break; +	default: +		ret = -EINVAL; +		goto err; +	} + +	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); +
> >>> goto err; +	} + +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; +	value = exponent << 12 |
> >>> mantissa; +	ret = opt3001_write(opt, reg, value); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read
> >>> register %02x\n", reg); +		goto err; +	} +
> >> As stated above an additional switch statement here with direct assignment of the relevant threshold values would
> >> be clearer.
> >>> +	*thresh_mantissa = mantissa; +	*thresh_exp = exponent; + +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; + +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS +		:
> >>> OPT3001_CONFIGURATION_M_SHUTDOWN; + +	ret = opt3001_read(opt, 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, &reg, mode); + +	if (opt->hysteresis) +		reg |= OPT3001_CONFIGURATION_L; +	else +
> >>> reg &= ~OPT3001_CONFIGURATION_L; + +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +
> >>> dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_CONFIGURATION); +		return ret; +	} + +	/* wait
> >>> for mode change to go through */ +	usleep_range(opt->int_time + 5000, opt->int_time + 10000); + +	return 0; +} 
> >>> + +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 =
> >>> opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt,
> >>> OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read register %02x\n", +
> >>> OPT3001_CONFIGURATION); +		return ret; +	} + +	reg = ret; + +	if (reg & OPT3001_CONFIGURATION_CT) +
> >>> opt->int_time = 800000; +	else +		opt->int_time = 100000; + +	reg &= ~OPT3001_CONFIGURATION_L; +	reg &=
> >>> ~OPT3001_CONFIGURATION_RN_MASK; +	reg |= OPT3001_CONFIGURATION_RN_AUTO; + +	ret = opt3001_write(opt,
> >>> OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +		dev_err(opt->dev, "failed to write register %02x\n", +
> >>> OPT3001_CONFIGURATION); +		return ret; +	} + +	ret = opt3001_read(opt, 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 = opt3001_read(opt, 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 *_opt) +{
> >> 
> >> Pass the iio_dev in to the irq setup instead of opt and you will then not need the opt reference to the struct
> >> iio_dev.
> > 
> > What's the difference ? When used this way we have:
> > 
> > struct opt3001 *opt = _opt; struct iio_dev *iio = opt->dev;
> > 
> > Done your way we will have:
> > 
> > struct iio_dev *iio = _iio; struct opt3001 *opt = iio_priv(iio);
> > 
> > I don't see the benefit of changing this as I'll access to both opt and iio.
> Because one way you get it from the IIO core support.  The other way around
> you have a local copy.  Basically it's better from a consistency point
> of view to have all drivers do it one way around than end up with a mixture
> of the two (which way around doesn't really matter - but rather late to
> change now!)

don't really care, frankly. i'll just change it

> >>> +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; + +	iio =
> >>> devm_iio_device_alloc(dev, sizeof(*opt)); +	if (!iio) +		return -ENOMEM; + +	opt = iio_priv(iio); +	opt->client
> >>> = client; +	opt->dev = dev;
> >> 
> >> This is very circular and it rarely makes sense to have the device private structure have a reference to the 
> >> iio_dev.  Here it is only used in the interrupt handler. Avoid that by making the private data passed to the
> >> interrupt handler the struct iio_dev instead of the struct opt3001.
> >> 
> >>> +	opt->iio = iio; + +	mutex_init(&opt->lock); +	i2c_set_clientdata(client, opt); + +	ret =
> >>> opt3001_read_id(opt); +	if (ret) +		return ret; + +	ret = opt3001_configure(opt); +	if (ret) +		return ret; + +
> >>> 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 = devm_iio_device_register(dev, iio); +	if (ret) { +		dev_err(dev, "failed to register
> >>> IIO device\n"); +		return ret; +	} +
> >> Normally we'd expect the devm_iio_device_register (that provides the userspace interfaces) to be after the irq
> >> request.
> > 
> > that's wrong. By the time the IRQ is requested, an IRQ can actually fire and you better have a valid e.g. iio_dev
> > by then.
> Its very unusual to bring up a device with the IRQ already in a position
> to fire.  Normally that would require some enabling from userspace after the
> driver is loaded?  How can it fire here before that point?

at the moment you call request_irq() (or any of its friends), that IRQ
line is unmasked and ready to fire. Remember that IRQ lines can be
shared and your IRQ handler will be called even if a separate device
triggered the IRQ. Heck, if you have a bad board design, even noise can
assert the IRQ line but let's not go there :-)

In any case, you better have valid pointers around so that by the time
your IRQ is triggered, you don't crash the kernel. Another way would be
to mask the IRQ at your device level, but that still doesn't solve
shared IRQs.

I usually request the IRQ as the last step for that very reason.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hi,

On Thu, Aug 07, 2014 at 05:27:56PM +0100, Jonathan Cameron wrote:
> >>>>> + +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) +		return -EBUSY; + +	if (chan->type != IIO_LIGHT)
> >>>>> + return -EINVAL; + +	mutex_lock(&opt->lock); + +	switch (mask) {
> >>>> 
> >>>> is there no way to control the scale?
> >>> 
> >>> yeah, but the HW does auto-scaling which works pretty well. Why would you want to control the scale manually ?
> >> On a slow device like this you probably wouldn't but for quicker devices the overhead of converting into a
> >> standard form in kernel might be excessive.
> > 
> > hehe, even when you _do_ set the scale, you still have the top 4
> > bits as exponent, the difference is that you know what they are
> > before the conversion is done :-)
> Then you can junk them and let userspace use the known value to deal
> with the raw data.

alright, I'll leave that to a future patch then. Auto-ranging is so much
simpler.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/08/14 17:35, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Aug 07, 2014 at 05:26:28PM +0100, Jonathan Cameron wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 07/08/14 15:28, Felipe Balbi wrote:
>>> Hi,
>>> 
>>> On Thu, Aug 07, 2014 at 11:01:04AM +0100, Jonathan Cameron wrote:
>>>> On 06/08/14 17:10, Felipe Balbi 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.
>>>>> 
>>>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>>> I don't suppose there is a datasheet available?
>>> 
>>> There is a summary datasheet available. The device isn't released yet:
>>> 
>>> http://www.ti.com/product/OPT3001?keyMatch=opt3001&tisearch=Search-EN
>>> 
>> Ah well...  Not much on that ;)
> 
> yeah, when the thing is released the full datasheet will be available, I'm sure. Until then, you'd have to sign an
> NDA with TI to get the full datasheet, I'm afraid :-s
> 
>>>>> Also, there are a couple details about this device which I need to know if it looks for you:
>>>>> 
>>>>> This device has a single enable bit which will enable both rising and falling edge triggers, but the limits
>>>>> are separate.
>>>>> 
>>>>> The same limits are also used for hysteresis-style capture and that's controlled by a single bit flip.
>>>> With this on, it generates an event whenever the value changes by more than a certain amount?  If so this is 
>>>> better handled by providing a trigger and a buffer.  Note that the events path carries no data other than an 
>>>> event code.
>>> 
>>> It's still a bit weird to me. So, without Latch bit, then yeah, it'll be more of a trigger e.g:
>> IIO triggers are actually more similar to those you find in video systems than the osciloscope type triggers you
>> are perhaps thinking of. They cause a 'scan' of data to be captured every time the fire.
>> 
>> A 'scope trigger would cause a set of data to be captured everytime it first (e.g. multiple scans of the
>> available channels).  We have talked a few times about adding this option, but it hasn't happened yet...
> 
> ok
> 
>>> I can set low limit to 200 lux and high limit to 50 lux, then I'll get a rising edge IRQ when I have more than
>>> 50 lux and a falling edge when I get less than 200 lux.
>>> 
>>> If Latch bit is set, however, then it's almost like it ignores low/high limit altogether and I get an IRQ ever 
>>> $int_time ms or so.
>> That would be similar to the motion triggers we have for accelerometers. They capture every sample as long as the
>> threshold is met (during motion). No idea why you'd really want to do that on a light sensor though.  Perhaps not
>> worth capturing light info if the cover is closed on a phone?
> 
> Yeah, but that's something else altogether. This device doesn't really care about a phone cover. Sounds like policy
> which should be in userland, or part of an iio_gpio_trigger...
Agreed.  Can you think of a use for the non latched version? :)
> 
>>>>> How do you want this to look on sysfs ? Currently, enable/disabling any of rising/falling edges, will
>>>>> disable both.
>>>> That's pretty common.  The ABI basically allows for any setting to change any other (as there are much
>>>> weirder connections than this in some devices).
>>> 
>>> ok, so it's alright to have two "enable" files and have they refer to the same bit ?
>> Absolutely.  Depending on the combinations it is sometimes possible to construct an 'EITHER' enable file - for
>> example driver/iio/adc/xilinix-xadc-core.c
> 
> will read
> 
>>>>> +		thresh_mantissa = &opt->high_thresh_mantissa; +		thresh_exp = &opt->high_thresh_exp; +		break; +	case 
>>>>> IIO_EV_DIR_FALLING: +		reg = OPT3001_LOW_LIMIT; +		thresh_mantissa = &opt->low_thresh_mantissa; +
>>>>> thresh_exp = &opt->low_thresh_exp; +		break; +	default: +		ret = -EINVAL; +		goto err; +	} + +	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); + goto err; +	} + +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent; +
>>>>> value = exponent << 12 | mantissa; +	ret = opt3001_write(opt, reg, value); +	if (ret < 0) { +
>>>>> dev_err(opt->dev, "failed to read register %02x\n", reg); +		goto err; +	} +
>>>> As stated above an additional switch statement here with direct assignment of the relevant threshold values
>>>> would be clearer.
>>>>> +	*thresh_mantissa = mantissa; +	*thresh_exp = exponent; + +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; + +	mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS +		: 
>>>>> OPT3001_CONFIGURATION_M_SHUTDOWN; + +	ret = opt3001_read(opt, 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, &reg, mode); + +	if (opt->hysteresis) +		reg |= OPT3001_CONFIGURATION_L; +
>>>>> else + reg &= ~OPT3001_CONFIGURATION_L; + +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); +	if (ret
>>>>> < 0) { + dev_err(opt->dev, "failed to read register %02x\n", +				OPT3001_CONFIGURATION); +		return ret; +
>>>>> } + +	/* wait for mode change to go through */ +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
>>>>> + +	return 0; +} + +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 = opt3001_read(opt, 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 = opt3001_read(opt, 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 = opt3001_read(opt, OPT3001_CONFIGURATION); +	if (ret < 0) { +		dev_err(opt->dev, "failed to read
>>>>> register %02x\n", + OPT3001_CONFIGURATION); +		return ret; +	} + +	reg = ret; + +	if (reg &
>>>>> OPT3001_CONFIGURATION_CT) + opt->int_time = 800000; +	else +		opt->int_time = 100000; + +	reg &=
>>>>> ~OPT3001_CONFIGURATION_L; +	reg &= ~OPT3001_CONFIGURATION_RN_MASK; +	reg |= OPT3001_CONFIGURATION_RN_AUTO;
>>>>> + +	ret = opt3001_write(opt, OPT3001_CONFIGURATION, reg); +	if (ret < 0) { +		dev_err(opt->dev, "failed to
>>>>> write register %02x\n", + OPT3001_CONFIGURATION); +		return ret; +	} + +	ret = opt3001_read(opt,
>>>>> 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 = opt3001_read(opt, 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 *_opt) +{
>>>> 
>>>> Pass the iio_dev in to the irq setup instead of opt and you will then not need the opt reference to the
>>>> struct iio_dev.
>>> 
>>> What's the difference ? When used this way we have:
>>> 
>>> struct opt3001 *opt = _opt; struct iio_dev *iio = opt->dev;
>>> 
>>> Done your way we will have:
>>> 
>>> struct iio_dev *iio = _iio; struct opt3001 *opt = iio_priv(iio);
>>> 
>>> I don't see the benefit of changing this as I'll access to both opt and iio.
>> Because one way you get it from the IIO core support.  The other way around you have a local copy.  Basically
>> it's better from a consistency point of view to have all drivers do it one way around than end up with a mixture 
>> of the two (which way around doesn't really matter - but rather late to change now!)
> 
> don't really care, frankly. i'll just change it
> 
>>>>> +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; +
>>>>> +	iio = devm_iio_device_alloc(dev, sizeof(*opt)); +	if (!iio) +		return -ENOMEM; + +	opt = iio_priv(iio); +
>>>>> opt->client = client; +	opt->dev = dev;
>>>> 
>>>> This is very circular and it rarely makes sense to have the device private structure have a reference to the
>>>>  iio_dev.  Here it is only used in the interrupt handler. Avoid that by making the private data passed to
>>>> the interrupt handler the struct iio_dev instead of the struct opt3001.
>>>> 
>>>>> +	opt->iio = iio; + +	mutex_init(&opt->lock); +	i2c_set_clientdata(client, opt); + +	ret = 
>>>>> opt3001_read_id(opt); +	if (ret) +		return ret; + +	ret = opt3001_configure(opt); +	if (ret) +		return ret;
>>>>> + + 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 = devm_iio_device_register(dev, iio); +	if (ret) { +		dev_err(dev, "failed to
>>>>> register IIO device\n"); +		return ret; +	} +
>>>> Normally we'd expect the devm_iio_device_register (that provides the userspace interfaces) to be after the
>>>> irq request.
>>> 
>>> that's wrong. By the time the IRQ is requested, an IRQ can actually fire and you better have a valid e.g.
>>> iio_dev by then.
>> Its very unusual to bring up a device with the IRQ already in a position to fire.  Normally that would require
>> some enabling from userspace after the driver is loaded?  How can it fire here before that point?
> 
> at the moment you call request_irq() (or any of its friends), that IRQ line is unmasked and ready to fire. Remember
> that IRQ lines can be shared and your IRQ handler will be called even if a separate device triggered the IRQ. Heck,
> if you have a bad board design, even noise can assert the IRQ line but let's not go there :-)
IIRC Sharing is only possible if the device driver explicitly allows it?
Bad board design can get you in many many ways ;)
> 
> In any case, you better have valid pointers around so that by the time your IRQ is triggered, you don't crash the
> kernel. Another way would be to mask the IRQ at your device level, but that still doesn't solve shared IRQs.
> 
> I usually request the IRQ as the last step for that very reason.
Fair enough - easy way to be sure I guess.  Though by just before the
register of the device all relevant pointers should be available.  It
won't do anything particularly helpful but it should not crash. If it
does then we have a bug we should probably close.
> 
> cheers
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJT47BRAAoJEFSFNJnE9BaI3GQP/3PPNLqjiXLHlV0LeVuNEpW5
r9HVbu8JpA4i0k5Aau6ak88nP4LDUXOcPWRtOFfz+5hHIvr9kFUp+FYLitMRML28
5yIvAa+TDijjSJwrlcIZdTDAXL8552S1NY/4+QcX4DJtYV8nnkSBOwDQkSPtxll/
Fia9LIKEvxOHkXlofNMhnGVwQcazVp7usiXxL07orBtPwCTUxKXpEuadnjIAdsiJ
CSiDt9YVR5mw3RJYrGYDHLnBPlo0kw3R/DElkii30N0K7bqh6CGSvXJv04j7sjky
gwvBmu1qbwymWny0Z50cWqoWPDady2/yrI57f/dbyUJJayLyC6roGor6e7h5SQuY
ShNAb+QOZLIFvLPEjsTA2pjju0vbNKN+JhNeRx8/GOMo1o6gzwHs966J7m8zJPXK
SfJ+FHGJoNS1vHEEEYtJ/qHdIa17Z7d5BUCdaV3V7pe20KYuFGRVn3arldND9dR+
PoTDKziNKzW9YPRPKiLNZbbmTgYxtbcIZE9EKBnkACm8C+W6jr3i4amwKgZH2WSu
81kTxVxfrHftY3L2eDMJVuULgZsAjYqK8zKcRzlyza2pVu1wWxfy15fLYt2/QY/f
gyPvTS2IvkMOGwLJerpr2wlMEKDn8gADUD/7dUs8g/2n3tmLPqOzq0F0umfHlAPh
eSwAiVDG6TfDeamnutQN
=/khk
-----END PGP SIGNATURE-----

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Jonathan Cameron]

On 07/08/14 17:36, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Aug 07, 2014 at 05:27:56PM +0100, Jonathan Cameron wrote:
>>>>>>> + +	if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS) +		return -EBUSY; + +	if (chan->type != IIO_LIGHT)
>>>>>>> + return -EINVAL; + +	mutex_lock(&opt->lock); + +	switch (mask) {
>>>>>>
>>>>>> is there no way to control the scale?
>>>>>
>>>>> yeah, but the HW does auto-scaling which works pretty well. Why would you want to control the scale manually ?
>>>> On a slow device like this you probably wouldn't but for quicker devices the overhead of converting into a
>>>> standard form in kernel might be excessive.
>>>
>>> hehe, even when you _do_ set the scale, you still have the top 4
>>> bits as exponent, the difference is that you know what they are
>>> before the conversion is done :-)
>> Then you can junk them and let userspace use the known value to deal
>> with the raw data.
> 
> alright, I'll leave that to a future patch then. Auto-ranging is so much
> simpler.
> 
Agreed - it's just unusual for it to actually be present on the device.

Re: [RFC/PATCH] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3108 bytes --]

Hi,

(your email setup is still messed up, I had to rewrap the whole email)

On Thu, Aug 07, 2014 at 05:58:57PM +0100, Jonathan Cameron wrote:
> >>> I can set low limit to 200 lux and high limit to 50 lux, then I'll
> >>> get a rising edge IRQ when I have more than 50 lux and a falling
> >>> edge when I get less than 200 lux.
> >>> 
> >>> If Latch bit is set, however, then it's almost like it ignores
> >>> low/high limit altogether and I get an IRQ ever $int_time ms or
> >>> so.
> >> That would be similar to the motion triggers we have for
> >> accelerometers. They capture every sample as long as the threshold
> >> is met (during motion). No idea why you'd really want to do that on
> >> a light sensor though.  Perhaps not worth capturing light info if
> >> the cover is closed on a phone?
> > 
> > Yeah, but that's something else altogether. This device doesn't
> > really care about a phone cover. Sounds like policy which should be
> > in userland, or part of an iio_gpio_trigger...
> Agreed.  Can you think of a use for the non latched version? :)

Sure, you might just want a notification when illuminance reaches a
certain level (rising) or drop below another (or the same) level
(falling).

> >>>>> +	ret = devm_iio_device_register(dev, iio);
> >>>>> +	if (ret) {
> >>>>> +		dev_err(dev, "failed to register IIO device\n");
> >>>>> +		return ret;
> >>>>> +	}
> >>>>> +
> >>>> Normally we'd expect the devm_iio_device_register (that provides
> >>>> the userspace interfaces) to be after the irq request.
> >>> 
> >>> that's wrong. By the time the IRQ is requested, an IRQ can
> >>> actually fire and you better have a valid e.g.  iio_dev by then.
> >> Its very unusual to bring up a device with the IRQ already in a
> >> position to fire.  Normally that would require some enabling from
> >> userspace after the driver is loaded?  How can it fire here before
> >> that point?
> > 
> > at the moment you call request_irq() (or any of its friends), that
> > IRQ line is unmasked and ready to fire. Remember that IRQ lines can
> > be shared and your IRQ handler will be called even if a separate
> > device triggered the IRQ. Heck, if you have a bad board design, even
> > noise can assert the IRQ line but let's not go there :-)
> IIRC Sharing is only possible if the device driver explicitly allows
> it?

not, necessarily. It can be set by the arch support code.

> Bad board design can get you in many many ways ;)
> > 
> > In any case, you better have valid pointers around so that by the
> > time your IRQ is triggered, you don't crash the kernel. Another way
> > would be to mask the IRQ at your device level, but that still
> > doesn't solve shared IRQs.
> > 
> > I usually request the IRQ as the last step for that very reason.
> Fair enough - easy way to be sure I guess.  Though by just before the
> register of the device all relevant pointers should be available.  It
> won't do anything particularly helpful but it should not crash. If it
> does then we have a bug we should probably close.

alright.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC/PATCH v2] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

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.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/iio/light/Kconfig   |  12 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/opt3001.c | 739 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 752 insertions(+)
 create mode 100644 drivers/iio/light/opt3001.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index bf05ca5..e4582d7 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -128,6 +128,18 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config OPT3001
+	tristate "Texas Instruments OPT3001 Light Sensor"
+	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	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 TCS3414
 	tristate "TAOS TCS3414 digital color sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 8b8c09f..898ef13 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
 obj-$(CONFIG_ISL29125)		+= isl29125.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..36737ca
--- /dev/null
+++ b/drivers/iio/light/opt3001.c
@@ -0,0 +1,739 @@
+/**
+ * opt3001.c - Texas Instruments OPT3001 Light Sensor
+ *
+ * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Felipe Balbi <balbi@ti.com>
+ *
+ * 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)
+
+#define OPT3001_REG_EXPONENT(n)	((n) >> 12)
+#define OPT3001_REG_MANTISSA(n)	((n) & 0xfff)
+
+struct opt3001 {
+	struct i2c_client	*client;
+	struct device		*dev;
+
+	struct mutex		lock;
+
+	u32			int_time;
+	u32			mode;
+
+	u16			high_thresh_mantissa;
+	u16			low_thresh_mantissa;
+
+	u8			high_thresh_exp;
+	u8			low_thresh_exp;
+
+	unsigned int		hysteresis:1;
+};
+
+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) {
+			*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_HYSTERESIS) |
+			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_HYSTERESIS) |
+			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;
+
+	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, &reg, 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 conversion and give it an extra 5ms */
+	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
+
+	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;
+	if (!(reg & OPT3001_CONFIGURATION_CRF))
+		return -EPIPE;
+
+	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);
+		return ret;
+	}
+
+	exponent = OPT3001_REG_EXPONENT(ret);
+	mantissa = OPT3001_REG_MANTISSA(ret);
+
+	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;
+
+	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;
+
+	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);
+	ret = opt3001_set_int_time(opt, val);
+	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;
+
+	u16 mantissa;
+	u16 value;
+	u16 reg;
+
+	u8 exponent;
+
+	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 err;
+	}
+
+	reg = ret;
+	if (info == IIO_EV_INFO_HYSTERESIS)
+		opt->hysteresis = true;
+	else
+		opt->hysteresis = false;
+
+	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);
+		goto err;
+	}
+
+	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
+	value = exponent << 12 | mantissa;
+
+	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;
+
+	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);
+		return ret;
+	}
+
+	reg = ret;
+	opt3001_set_mode(opt, &reg, mode);
+
+	if (opt->hysteresis)
+		reg |= OPT3001_CONFIGURATION_L;
+	else
+		reg &= ~OPT3001_CONFIGURATION_L;
+
+	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 mode change to go through */
+	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
+
+	return 0;
+}
+
+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;
+
+	if (reg & OPT3001_CONFIGURATION_CT)
+		opt->int_time = 800000;
+	else
+		opt->int_time = 100000;
+
+	reg &= ~OPT3001_CONFIGURATION_L;
+	reg &= ~OPT3001_CONFIGURATION_RN_MASK;
+	reg |= OPT3001_CONFIGURATION_RN_AUTO;
+
+	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;
+
+	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_CT))
+		goto out;
+
+	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());
+
+out:
+	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;
+
+	iio = devm_iio_device_alloc(dev, sizeof(*opt));
+	if (!iio)
+		return -ENOMEM;
+
+	opt = iio_priv(iio);
+	opt->client = client;
+	opt->dev = dev;
+
+	mutex_init(&opt->lock);
+	i2c_set_clientdata(client, opt);
+
+	ret = opt3001_read_id(opt);
+	if (ret)
+		return ret;
+
+	ret = opt3001_configure(opt);
+	if (ret)
+		return ret;
+
+	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 = devm_iio_device_register(dev, iio);
+	if (ret) {
+		dev_err(dev, "failed to register IIO device\n");
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
+			| IRQF_ONESHOT, "opt3001", iio);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ #%d\n", irq);
+		return ret;
+	}
+
+	return 0;
+
+}
+
+static int opt3001_remove(struct i2c_client *client)
+{
+	struct opt3001 *opt = i2c_get_clientdata(client);
+	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;
+	opt3001_set_mode(opt, &reg, 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;
+	}
+
+	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("Felipe Balbi <balbi@ti.com>");
+MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver");
-- 
2.0.1.563.g66f467c

Re: [RFC/PATCH v2] iio: light: add support for TI's opt3001 ligth sensor

[Peter Meerwald]

> 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.

moving in the right direction, the driver got simpler :)

light is still misspelled in the subject, the Kconfig needs to be fixed, 
see below

p.

> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/iio/light/Kconfig   |  12 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/opt3001.c | 739 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 752 insertions(+)
>  create mode 100644 drivers/iio/light/opt3001.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index bf05ca5..e4582d7 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -128,6 +128,18 @@ config LTR501
>  	 This driver can also be built as a module.  If so, the module
>           will be called ltr501.
>  
> +config OPT3001
> +	tristate "Texas Instruments OPT3001 Light Sensor"
> +	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER

IIO_BUFFER, IIO_TRIGGERED_BUFFER not needed since not supported by driver

> +	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 TCS3414
>  	tristate "TAOS TCS3414 digital color sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 8b8c09f..898ef13 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
>  obj-$(CONFIG_ISL29125)		+= isl29125.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..36737ca
> --- /dev/null
> +++ b/drivers/iio/light/opt3001.c
> @@ -0,0 +1,739 @@
> +/**
> + * opt3001.c - Texas Instruments OPT3001 Light Sensor
> + *
> + * Copyright (C) 2014 Texas Instruments Incorporated - http://www.ti.com
> + *
> + * Author: Felipe Balbi <balbi@ti.com>
> + *
> + * 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)
> +
> +#define OPT3001_REG_EXPONENT(n)	((n) >> 12)
> +#define OPT3001_REG_MANTISSA(n)	((n) & 0xfff)
> +
> +struct opt3001 {
> +	struct i2c_client	*client;
> +	struct device		*dev;
> +
> +	struct mutex		lock;
> +
> +	u32			int_time;
> +	u32			mode;
> +
> +	u16			high_thresh_mantissa;
> +	u16			low_thresh_mantissa;
> +
> +	u8			high_thresh_exp;
> +	u8			low_thresh_exp;
> +
> +	unsigned int		hysteresis:1;
> +};
> +
> +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) {
> +			*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_HYSTERESIS) |
> +			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_HYSTERESIS) |
> +			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;
> +
> +	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, &reg, 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 conversion and give it an extra 5ms */
> +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> +
> +	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;
> +	if (!(reg & OPT3001_CONFIGURATION_CRF))
> +		return -EPIPE;
> +
> +	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);
> +		return ret;
> +	}
> +
> +	exponent = OPT3001_REG_EXPONENT(ret);
> +	mantissa = OPT3001_REG_MANTISSA(ret);
> +
> +	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;
> +
> +	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;
> +
> +	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);
> +	ret = opt3001_set_int_time(opt, val);
> +	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;
> +
> +	u16 mantissa;
> +	u16 value;
> +	u16 reg;
> +
> +	u8 exponent;
> +
> +	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 err;
> +	}
> +
> +	reg = ret;
> +	if (info == IIO_EV_INFO_HYSTERESIS)
> +		opt->hysteresis = true;
> +	else
> +		opt->hysteresis = false;
> +
> +	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);
> +		goto err;
> +	}
> +
> +	mantissa = (((val * 1000) + (val2 / 1000)) / 10) >> exponent;
> +	value = exponent << 12 | mantissa;
> +
> +	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;
> +
> +	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);
> +		return ret;
> +	}
> +
> +	reg = ret;
> +	opt3001_set_mode(opt, &reg, mode);
> +
> +	if (opt->hysteresis)
> +		reg |= OPT3001_CONFIGURATION_L;
> +	else
> +		reg &= ~OPT3001_CONFIGURATION_L;
> +
> +	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 mode change to go through */
> +	usleep_range(opt->int_time + 5000, opt->int_time + 10000);
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	if (reg & OPT3001_CONFIGURATION_CT)
> +		opt->int_time = 800000;
> +	else
> +		opt->int_time = 100000;
> +
> +	reg &= ~OPT3001_CONFIGURATION_L;
> +	reg &= ~OPT3001_CONFIGURATION_RN_MASK;
> +	reg |= OPT3001_CONFIGURATION_RN_AUTO;
> +
> +	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;
> +
> +	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_CT))
> +		goto out;
> +
> +	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());
> +
> +out:
> +	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;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*opt));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	opt = iio_priv(iio);
> +	opt->client = client;
> +	opt->dev = dev;
> +
> +	mutex_init(&opt->lock);
> +	i2c_set_clientdata(client, opt);
> +
> +	ret = opt3001_read_id(opt);
> +	if (ret)
> +		return ret;
> +
> +	ret = opt3001_configure(opt);
> +	if (ret)
> +		return ret;
> +
> +	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 = devm_iio_device_register(dev, iio);
> +	if (ret) {
> +		dev_err(dev, "failed to register IIO device\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> +			| IRQF_ONESHOT, "opt3001", iio);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ #%d\n", irq);
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +}
> +
> +static int opt3001_remove(struct i2c_client *client)
> +{
> +	struct opt3001 *opt = i2c_get_clientdata(client);
> +	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;
> +	opt3001_set_mode(opt, &reg, 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;
> +	}
> +
> +	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("Felipe Balbi <balbi@ti.com>");
> +MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [RFC/PATCH v2] iio: light: add support for TI's opt3001 ligth sensor

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Mon, Aug 11, 2014 at 04:46:07PM +0200, Peter Meerwald 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.
> 
> moving in the right direction, the driver got simpler :)
> 
> light is still misspelled in the subject, the Kconfig needs to be fixed, 
> see below

hah! missed that. Fixed.

> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/iio/light/Kconfig   |  12 +
> >  drivers/iio/light/Makefile  |   1 +
> >  drivers/iio/light/opt3001.c | 739 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 752 insertions(+)
> >  create mode 100644 drivers/iio/light/opt3001.c
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index bf05ca5..e4582d7 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -128,6 +128,18 @@ config LTR501
> >  	 This driver can also be built as a module.  If so, the module
> >           will be called ltr501.
> >  
> > +config OPT3001
> > +	tristate "Texas Instruments OPT3001 Light Sensor"
> > +	depends on I2C
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> 
> IIO_BUFFER, IIO_TRIGGERED_BUFFER not needed since not supported by driver

fixed.

both pushed to my branch at kernel.org

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]