[RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[RESEND PATCH] iio: light: add support for TI's opt3001 light 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>
---

Resending as I saw no changes on the thread.

 drivers/iio/light/Kconfig   |  10 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/opt3001.c | 753 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 764 insertions(+)
 create mode 100644 drivers/iio/light/opt3001.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index bf05ca5..f196996 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -128,6 +128,16 @@ 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
+	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..8c4a153
--- /dev/null
+++ b/drivers/iio/light/opt3001.c
@@ -0,0 +1,753 @@
+/**
+ * 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, val2);
+	mutex_unlock(&opt->lock);
+
+	return ret;
+}
+
+static int opt3001_read_event_value(struct iio_dev *iio,
+		const struct iio_chan_spec *chan, enum iio_event_type type,
+		enum iio_event_direction dir, enum iio_event_info info,
+		int *val, int *val2)
+{
+	struct opt3001 *opt = iio_priv(iio);
+	int ret = IIO_VAL_INT_PLUS_MICRO;
+
+	mutex_lock(&opt->lock);
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		opt3001_to_iio_ret(opt, opt->high_thresh_exp,
+				opt->high_thresh_mantissa, val, val2);
+		break;
+	case IIO_EV_DIR_FALLING:
+		opt3001_to_iio_ret(opt, opt->low_thresh_exp,
+				opt->low_thresh_mantissa, val, val2);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&opt->lock);
+
+	return ret;
+}
+
+static int opt3001_write_event_value(struct iio_dev *iio,
+		const struct iio_chan_spec *chan, enum iio_event_type type,
+		enum iio_event_direction dir, enum iio_event_info info,
+		int val, int val2)
+{
+	struct opt3001 *opt = iio_priv(iio);
+	int ret = 0;
+
+	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 = -ENOMEM;
+
+	iio = iio_device_alloc(sizeof(*opt));
+	if (!iio)
+		goto err_iio_alloc;
+
+	opt = iio_priv(iio);
+	opt->client = client;
+	opt->dev = dev;
+
+	mutex_init(&opt->lock);
+	i2c_set_clientdata(client, iio);
+
+	ret = opt3001_read_id(opt);
+	if (ret)
+		goto err_read;
+
+	ret = opt3001_configure(opt);
+	if (ret)
+		goto err_read;
+
+	iio->name = client->name;
+	iio->channels = opt3001_channels;
+	iio->num_channels = ARRAY_SIZE(opt3001_channels);
+	iio->dev.parent = dev;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->info = &opt3001_info;
+
+	ret = iio_device_register(iio);
+	if (ret) {
+		dev_err(dev, "failed to register IIO device\n");
+		goto err_read;
+	}
+
+	ret = request_threaded_irq(irq, NULL, opt3001_irq,
+			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
+			| IRQF_ONESHOT, "opt3001", iio);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ #%d\n", irq);
+		goto err_request_irq;
+	}
+
+	return 0;
+
+err_request_irq:
+	iio_device_unregister(iio);
+
+err_read:
+	iio_device_free(iio);
+
+err_iio_alloc:
+	return ret;
+}
+
+static int opt3001_remove(struct i2c_client *client)
+{
+	struct iio_dev *iio = i2c_get_clientdata(client);
+	struct opt3001 *opt = iio_priv(iio);
+	int ret;
+	u16 reg;
+
+	free_irq(client->irq, iio);
+	iio_device_unregister(iio);
+
+	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
+	if (ret < 0) {
+		dev_err(opt->dev, "failed to read register %02x\n",
+				OPT3001_CONFIGURATION);
+		return ret;
+	}
+
+	reg = ret;
+	opt3001_set_mode(opt, &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;
+	}
+
+	iio_device_free(iio);
+
+	return 0;
+}
+
+static const struct i2c_device_id opt3001_id[] = {
+	{ "opt3001", 0 },
+	{ } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(i2c, opt3001_id);
+
+static struct i2c_driver opt3001_driver = {
+	.probe = opt3001_probe,
+	.remove = opt3001_remove,
+	.id_table = opt3001_id,
+
+	.driver = {
+		.name = "opt3001",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_i2c_driver(opt3001_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Felipe Balbi <balbi@ti.com>");
+MODULE_DESCRIPTION("Texas Instruments OPT3001 Light Sensor Driver");
-- 
2.0.1.563.g66f467c

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

On 02/09/14 16:17, 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>
> ---
> 
> Resending as I saw no changes on the thread.
Hi Felipe,

Sorry for the delay on this, entirely my fault - been busy and forgot
I still had questions about what was going on in here (yup its the hysteresis bit
again!)

Anyhow, I'm afraid I am still a little confused about the meaning you have assigned
to Hysteresis in this driver.

Let me conjecture on what might be going on here (I may be entirely wrong).

Normally a hysteresis value in IIO is defined as the 'distance' back from a threshold
that a signal must go before it may retrip the threshold.
This threshold value is separately controlled. Thus if we have a rising threshold
of 10 and an hysteresis of 2 - to get two events the signal must first rise
past 10, then drop back below 8 and rise again past 10.
If it drops below 10 but not 8 and rises again past 10 then we will not get an
event.

So having the same register for both the hysteresis and the threshold doesn't
with this description make much sense.  It would mean that you could only
have a threshold of say 10 and a hysteresis of 10, thus in effect meaning the
signal would always have to cross 0 before the next event whatever the combined
threshold / hysteresis value?

Perhaps instead the device is automatically adjusting the threshold when we cross
it and the 'hysteresis' here is with respect to a the previous threshold?

Thus if we start with a value of 0 and hysteresis is set to 2 it will trigger an
event at:

2, 4, 6, 8, 10 as we rise?

This sort of auto adjustment of levels isn't uncommon in light sensors (where
the point of the interrupt is to notify the operating system that a 'significant'
change has occurred and things like screen brightness may need adjusting.

If so then the current hysteresis interface does not apply, nor does the Rate
of Change (ROC) interface as this is dependent on amount of change, not how
fast it changed.  Hence we needs something new to handle this cleanly. I would
suggest a new event type.  Perhaps something with sysfs attr naming along
the lines of
What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
What:           /sys/.../iio:deviceX/events/in_light_change_rising_value

etc?
> 
>  drivers/iio/light/Kconfig   |  10 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/opt3001.c | 753 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 764 insertions(+)
>  create mode 100644 drivers/iio/light/opt3001.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index bf05ca5..f196996 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -128,6 +128,16 @@ 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
> +	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..8c4a153
> --- /dev/null
> +++ b/drivers/iio/light/opt3001.c
> @@ -0,0 +1,753 @@
> +/**
> + * 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 */
> +

I guess this naming is straight off the datasheet, but it is rather more
cryptic than perhaps it needs to be!  That's kind of an issue with the
datasheet choices rather than what you have here however!

> +#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, val2);
> +	mutex_unlock(&opt->lock);
> +
> +	return ret;
> +}
> +
> +static int opt3001_read_event_value(struct iio_dev *iio,
> +		const struct iio_chan_spec *chan, enum iio_event_type type,
> +		enum iio_event_direction dir, enum iio_event_info info,
> +		int *val, int *val2)
> +{
> +	struct opt3001 *opt = iio_priv(iio);
> +	int ret = IIO_VAL_INT_PLUS_MICRO;
> +
> +	mutex_lock(&opt->lock);
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		opt3001_to_iio_ret(opt, opt->high_thresh_exp,
> +				opt->high_thresh_mantissa, val, val2);
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		opt3001_to_iio_ret(opt, opt->low_thresh_exp,
> +				opt->low_thresh_mantissa, val, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&opt->lock);
> +
> +	return ret;
> +}
> +
> +static int opt3001_write_event_value(struct iio_dev *iio,
> +		const struct iio_chan_spec *chan, enum iio_event_type type,
> +		enum iio_event_direction dir, enum iio_event_info info,
> +		int val, int val2)
> +{
> +	struct opt3001 *opt = iio_priv(iio);
> +	int ret = 0;
> +
> +	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 = -ENOMEM;
> +
> +	iio = iio_device_alloc(sizeof(*opt));
> +	if (!iio)
> +		goto err_iio_alloc;
> +
> +	opt = iio_priv(iio);
> +	opt->client = client;
> +	opt->dev = dev;
> +
> +	mutex_init(&opt->lock);
> +	i2c_set_clientdata(client, iio);
> +
> +	ret = opt3001_read_id(opt);
> +	if (ret)
> +		goto err_read;
> +
> +	ret = opt3001_configure(opt);
> +	if (ret)
> +		goto err_read;
> +
> +	iio->name = client->name;
> +	iio->channels = opt3001_channels;
> +	iio->num_channels = ARRAY_SIZE(opt3001_channels);
> +	iio->dev.parent = dev;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &opt3001_info;
> +
> +	ret = iio_device_register(iio);
> +	if (ret) {
> +		dev_err(dev, "failed to register IIO device\n");
> +		goto err_read;
> +	}
> +
> +	ret = request_threaded_irq(irq, NULL, opt3001_irq,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
> +			| IRQF_ONESHOT, "opt3001", iio);
> +	if (ret) {
> +		dev_err(dev, "failed to request IRQ #%d\n", irq);
> +		goto err_request_irq;
> +	}
> +
> +	return 0;
> +
> +err_request_irq:
> +	iio_device_unregister(iio);
> +
> +err_read:
> +	iio_device_free(iio);
> +
> +err_iio_alloc:
> +	return ret;
> +}
> +
> +static int opt3001_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *iio = i2c_get_clientdata(client);
> +	struct opt3001 *opt = iio_priv(iio);
> +	int ret;
> +	u16 reg;
> +
> +	free_irq(client->irq, iio);
> +	iio_device_unregister(iio);
> +
> +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> +	if (ret < 0) {
> +		dev_err(opt->dev, "failed to read register %02x\n",
> +				OPT3001_CONFIGURATION);
> +		return ret;
> +	}
> +
> +	reg = ret;
> +	opt3001_set_mode(opt, &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;
> +	}
> +
> +	iio_device_free(iio);
Use the devm_iio_device_alloc and you can drop the need to free it.
I don't really mind, but I'll almost guarantee that someone will post
a follow up patch doing this if you don't.  As it will be ever so
slightly cleaner, I'll probably take that patch.
> +
> +	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: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

Hi,

On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> On 02/09/14 16:17, 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>
> > ---
> > 
> > Resending as I saw no changes on the thread.
> Hi Felipe,
> 
> Sorry for the delay on this, entirely my fault - been busy and forgot
> I still had questions about what was going on in here (yup its the
> hysteresis bit again!)

right, this is starting to become way too much headache for such a
simple device. Sorry will not help me getting this driver upstream. When
I first sent this (August 6), we didn't even have v3.17-rc1, now we're
about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
window.

> Anyhow, I'm afraid I am still a little confused about the meaning you
> have assigned to Hysteresis in this driver.
> 
> Let me conjecture on what might be going on here (I may be entirely
> wrong).
> 
> Normally a hysteresis value in IIO is defined as the 'distance' back
> from a threshold that a signal must go before it may retrip the
> threshold.
> This threshold value is separately controlled. Thus if we have a
> rising threshold of 10 and an hysteresis of 2 - to get two events the
> signal must first rise past 10, then drop back below 8 and rise again
> past 10.
> If it drops below 10 but not 8 and rises again past 10 then we will
> not get an event.
> 
> So having the same register for both the hysteresis and the threshold
> doesn't with this description make much sense.  It would mean that you
> could only have a threshold of say 10 and a hysteresis of 10, thus in
> effect meaning the signal would always have to cross 0 before the next
> event whatever the combined threshold / hysteresis value?
> 
> Perhaps instead the device is automatically adjusting the threshold
> when we cross it and the 'hysteresis' here is with respect to a the
> previous threshold?
> 
> Thus if we start with a value of 0 and hysteresis is set to 2 it will
> trigger an event at:
> 
> 2, 4, 6, 8, 10 as we rise?
> 
> This sort of auto adjustment of levels isn't uncommon in light sensors
> (where the point of the interrupt is to notify the operating system
> that a 'significant' change has occurred and things like screen
> brightness may need adjusting.
> 
> If so then the current hysteresis interface does not apply, nor does
> the Rate of Change (ROC) interface as this is dependent on amount of
> change, not how fast it changed.  Hence we needs something new to
> handle this cleanly. I would suggest a new event type.  Perhaps
> something with sysfs attr naming along the lines of
> What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> 
> etc?

will you just tell me what you want ? I really cannot give a crap
anymore. This has already taken me over a month of my time for such a
simple little device, not to mention your confusing and contradicting
change requests.

(could you also trim your responses ? it's very annoying to scroll so
much)

> > +#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 */
> > +
> 
> I guess this naming is straight off the datasheet, but it is rather
> more cryptic than perhaps it needs to be!  That's kind of an issue
> with the datasheet choices rather than what you have here however!

man, are you nit-picky!! These are named as such to make grepping on
documentation easier. It's better to have something that matches
documentation, don't you think ? otherwise, future users/developers of
these driver will need either a shit ton of comments explaining that A
maps to B in docs, or will need a fscking crystal ball to read my mind.
Assuming I'll still remember what I meant.

> > +static int opt3001_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > +	struct opt3001 *opt = iio_priv(iio);
> > +	int ret;
> > +	u16 reg;
> > +
> > +	free_irq(client->irq, iio);
> > +	iio_device_unregister(iio);
> > +
> > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > +	if (ret < 0) {
> > +		dev_err(opt->dev, "failed to read register %02x\n",
> > +				OPT3001_CONFIGURATION);
> > +		return ret;
> > +	}
> > +
> > +	reg = ret;
> > +	opt3001_set_mode(opt, &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;
> > +	}
> > +
> > +	iio_device_free(iio);
>
> Use the devm_iio_device_alloc and you can drop the need to free it.
> I don't really mind, but I'll almost guarantee that someone will post
> a follow up patch doing this if you don't.  As it will be ever so
> slightly cleaner, I'll probably take that patch.

here's the original driver:

http://www.spinics.net/lists/linux-iio/msg14331.html

notice that it *WAS* *USING* devm_iio_device_alloc(), until:

http://www.spinics.net/lists/linux-iio/msg14421.html

you *SPECIFICALLY* asked for *NON* *DEVM* versions!!

So figure out what you really want, let me know and I'll code it all up
quickly and hopefully still get this into v3.18 merge window. I sent
this driver *very* early to be doubly sure it would hit v3.18 and there
was a long hiatus from yourself which is now jeopardizing what I was
expecting. That, coupled with your contradicting requests, has just put
me in a bad mood, even before Monday, hurray.

cheers

-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

On September 15, 2014 6:21:37 AM GMT+01:00, Felipe Balbi <balbi@ti.com> wrote:
>Hi,
>
>On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>> On 02/09/14 16:17, 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>
>> > ---
>> > 
>> > Resending as I saw no changes on the thread.
>> Hi Felipe,
>> 
>> Sorry for the delay on this, entirely my fault - been busy and forgot
>> I still had questions about what was going on in here (yup its the
>> hysteresis bit again!)
>
>right, this is starting to become way too much headache for such a
>simple device. Sorry will not help me getting this driver upstream.
>When
>I first sent this (August 6), we didn't even have v3.17-rc1, now we're
>about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
>window.
>
>> Anyhow, I'm afraid I am still a little confused about the meaning you
>> have assigned to Hysteresis in this driver.
>> 
>> Let me conjecture on what might be going on here (I may be entirely
>> wrong).
>> 
>> Normally a hysteresis value in IIO is defined as the 'distance' back
>> from a threshold that a signal must go before it may retrip the
>> threshold.
>> This threshold value is separately controlled. Thus if we have a
>> rising threshold of 10 and an hysteresis of 2 - to get two events the
>> signal must first rise past 10, then drop back below 8 and rise again
>> past 10.
>> If it drops below 10 but not 8 and rises again past 10 then we will
>> not get an event.
>> 
>> So having the same register for both the hysteresis and the threshold
>> doesn't with this description make much sense.  It would mean that
>you
>> could only have a threshold of say 10 and a hysteresis of 10, thus in
>> effect meaning the signal would always have to cross 0 before the
>next
>> event whatever the combined threshold / hysteresis value?
>> 
>> Perhaps instead the device is automatically adjusting the threshold
>> when we cross it and the 'hysteresis' here is with respect to a the
>> previous threshold?
>> 
>> Thus if we start with a value of 0 and hysteresis is set to 2 it will
>> trigger an event at:
>> 
>> 2, 4, 6, 8, 10 as we rise?
>> 
>> This sort of auto adjustment of levels isn't uncommon in light
>sensors
>> (where the point of the interrupt is to notify the operating system
>> that a 'significant' change has occurred and things like screen
>> brightness may need adjusting.
>> 
>> If so then the current hysteresis interface does not apply, nor does
>> the Rate of Change (ROC) interface as this is dependent on amount of
>> change, not how fast it changed.  Hence we needs something new to
>> handle this cleanly. I would suggest a new event type.  Perhaps
>> something with sysfs attr naming along the lines of
>> What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>> What:          
>/sys/.../iio:deviceX/events/in_light_change_rising_value
>> 
>> etc?
>
>will you just tell me what you want ? I really cannot give a crap
>anymore. This has already taken me over a month of my time for such a
>simple little device, not to mention your confusing and contradicting
>change requests.
I cannot tell you what i want because no data sheet is available so I have to rely on what
 you tell me. What you have right now does not make sense or appear to correctly obey
 the ABI so I cannot accept it.  

I have spent considerable time asking you to explain the use of hysteresis in this driver
 which does not appear to be remotely standard.  Either explain it or drop that
 element and we can revisit it when the data sheet becomes available. Yes, writing drivers
 where no data sheet is available does add a burden to the author when they submit it.
 Tough.

>
>(could you also trim your responses ? it's very annoying to scroll so
>much)
>
>> > +#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
>*/
>> > +
>> 
>> I guess this naming is straight off the datasheet, but it is rather
>> more cryptic than perhaps it needs to be!  That's kind of an issue
>> with the datasheet choices rather than what you have here however!
>
>man, are you nit-picky!! These are named as such to make grepping on
>documentation easier. It's better to have something that matches
>documentation, don't you think ? otherwise, future users/developers of
>these driver will need either a shit ton of comments explaining that A
>maps to B in docs, or will need a fscking crystal ball to read my mind.
>Assuming I'll still remember what I meant.
Fine. State that and we can move on.
>
>> > +static int opt3001_remove(struct i2c_client *client)
>> > +{
>> > +	struct iio_dev *iio = i2c_get_clientdata(client);
>> > +	struct opt3001 *opt = iio_priv(iio);
>> > +	int ret;
>> > +	u16 reg;
>> > +
>> > +	free_irq(client->irq, iio);
>> > +	iio_device_unregister(iio);
>> > +
>> > +	ret = i2c_smbus_read_word_swapped(opt->client,
>OPT3001_CONFIGURATION);
>> > +	if (ret < 0) {
>> > +		dev_err(opt->dev, "failed to read register %02x\n",
>> > +				OPT3001_CONFIGURATION);
>> > +		return ret;
>> > +	}
>> > +
>> > +	reg = ret;
>> > +	opt3001_set_mode(opt, &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;
>> > +	}
>> > +
>> > +	iio_device_free(iio);
>>
>> Use the devm_iio_device_alloc and you can drop the need to free it.
>> I don't really mind, but I'll almost guarantee that someone will post
>> a follow up patch doing this if you don't.  As it will be ever so
>> slightly cleaner, I'll probably take that patch.
>
>here's the original driver:
>
>http://www.spinics.net/lists/linux-iio/msg14331.html
>
>notice that it *WAS* *USING* devm_iio_device_alloc(), until:
>
>http://www.spinics.net/lists/linux-iio/msg14421.html
>
>you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
Read that email again.  I specifically say the register function. I do not mention allocation.
>
>So figure out what you really want, let me know and I'll code it all up
>quickly and hopefully still get this into v3.18 merge window.
When you coherently explain what the hardware actually does rather than
 assuming it is obvious we may be able to make some progress.


 I sent
>this driver *very* early to be doubly sure it would hit v3.18 and there
>was a long hiatus from yourself which is now jeopardizing what I was
>expecting. That, coupled with your contradicting requests, has just put
>me in a bad mood, even bfore Monday, hurray.

I would like nothing better than not having to ask you the same question again and again 
Why should I conjecture what your hardware is doing to try to allow us to move forward?

If you wish to proceed please either answer my questions or drop the hysteresis support
 until we can use the docs to figure it out for ourselves.

Jonathan



>chee

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Hartmut Knaack]

Felipe Balbi schrieb, Am 15.09.2014 07:21:
> Hi,
> 
> On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>> On 02/09/14 16:17, 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>
>>> ---
>>>
>>> Resending as I saw no changes on the thread.
>> Hi Felipe,
>>
>> Sorry for the delay on this, entirely my fault - been busy and forgot
>> I still had questions about what was going on in here (yup its the
>> hysteresis bit again!)
> 
> right, this is starting to become way too much headache for such a
> simple device. Sorry will not help me getting this driver upstream. When
> I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> window.
> 
Easy, dude. I don't know how much you followed this list, but during that mentioned time, there were over 500 mails exchanged. As the maintainer, I expect Jonathan to have read all of them more or less intensively. The time he sacrifices for this project is quite massive, considering that he doesn't do this for a living. So please respect, that he also has other activities, which may have higher priorities, and that he doesn't schedule his life according to kernel deadlines. He is doing a pretty good job here.
Thanks
Hartmut 
>> Anyhow, I'm afraid I am still a little confused about the meaning you
>> have assigned to Hysteresis in this driver.
>>
>> Let me conjecture on what might be going on here (I may be entirely
>> wrong).
>>
>> Normally a hysteresis value in IIO is defined as the 'distance' back
>> from a threshold that a signal must go before it may retrip the
>> threshold.
>> This threshold value is separately controlled. Thus if we have a
>> rising threshold of 10 and an hysteresis of 2 - to get two events the
>> signal must first rise past 10, then drop back below 8 and rise again
>> past 10.
>> If it drops below 10 but not 8 and rises again past 10 then we will
>> not get an event.
>>
>> So having the same register for both the hysteresis and the threshold
>> doesn't with this description make much sense.  It would mean that you
>> could only have a threshold of say 10 and a hysteresis of 10, thus in
>> effect meaning the signal would always have to cross 0 before the next
>> event whatever the combined threshold / hysteresis value?
>>
>> Perhaps instead the device is automatically adjusting the threshold
>> when we cross it and the 'hysteresis' here is with respect to a the
>> previous threshold?
>>
>> Thus if we start with a value of 0 and hysteresis is set to 2 it will
>> trigger an event at:
>>
>> 2, 4, 6, 8, 10 as we rise?
>>
>> This sort of auto adjustment of levels isn't uncommon in light sensors
>> (where the point of the interrupt is to notify the operating system
>> that a 'significant' change has occurred and things like screen
>> brightness may need adjusting.
>>
>> If so then the current hysteresis interface does not apply, nor does
>> the Rate of Change (ROC) interface as this is dependent on amount of
>> change, not how fast it changed.  Hence we needs something new to
>> handle this cleanly. I would suggest a new event type.  Perhaps
>> something with sysfs attr naming along the lines of
>> What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>> What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
>>
>> etc?
> 
> will you just tell me what you want ? I really cannot give a crap
> anymore. This has already taken me over a month of my time for such a
> simple little device, not to mention your confusing and contradicting
> change requests.
> 
> (could you also trim your responses ? it's very annoying to scroll so
> much)
> 
>>> +#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 */
>>> +
>>
>> I guess this naming is straight off the datasheet, but it is rather
>> more cryptic than perhaps it needs to be!  That's kind of an issue
>> with the datasheet choices rather than what you have here however!
> 
> man, are you nit-picky!! These are named as such to make grepping on
> documentation easier. It's better to have something that matches
> documentation, don't you think ? otherwise, future users/developers of
> these driver will need either a shit ton of comments explaining that A
> maps to B in docs, or will need a fscking crystal ball to read my mind.
> Assuming I'll still remember what I meant.
> 
>>> +static int opt3001_remove(struct i2c_client *client)
>>> +{
>>> +	struct iio_dev *iio = i2c_get_clientdata(client);
>>> +	struct opt3001 *opt = iio_priv(iio);
>>> +	int ret;
>>> +	u16 reg;
>>> +
>>> +	free_irq(client->irq, iio);
>>> +	iio_device_unregister(iio);
>>> +
>>> +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>> +	if (ret < 0) {
>>> +		dev_err(opt->dev, "failed to read register %02x\n",
>>> +				OPT3001_CONFIGURATION);
>>> +		return ret;
>>> +	}
>>> +
>>> +	reg = ret;
>>> +	opt3001_set_mode(opt, &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;
>>> +	}
>>> +
>>> +	iio_device_free(iio);
>>
>> Use the devm_iio_device_alloc and you can drop the need to free it.
>> I don't really mind, but I'll almost guarantee that someone will post
>> a follow up patch doing this if you don't.  As it will be ever so
>> slightly cleaner, I'll probably take that patch.
> 
> here's the original driver:
> 
> http://www.spinics.net/lists/linux-iio/msg14331.html
> 
> notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> 
> http://www.spinics.net/lists/linux-iio/msg14421.html
> 
> you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> 
> So figure out what you really want, let me know and I'll code it all up
> quickly and hopefully still get this into v3.18 merge window. I sent
> this driver *very* early to be doubly sure it would hit v3.18 and there
> was a long hiatus from yourself which is now jeopardizing what I was
> expecting. That, coupled with your contradicting requests, has just put
> me in a bad mood, even before Monday, hurray.
> 
> cheers
> 

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > On 02/09/14 16:17, 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>
> > > ---
> > > 
> > > Resending as I saw no changes on the thread.
> > Hi Felipe,
> > 
> > Sorry for the delay on this, entirely my fault - been busy and forgot
> > I still had questions about what was going on in here (yup its the
> > hysteresis bit again!)
> 
> right, this is starting to become way too much headache for such a
> simple device. Sorry will not help me getting this driver upstream. When
> I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> window.
> 
> > Anyhow, I'm afraid I am still a little confused about the meaning you
> > have assigned to Hysteresis in this driver.
> > 
> > Let me conjecture on what might be going on here (I may be entirely
> > wrong).
> > 
> > Normally a hysteresis value in IIO is defined as the 'distance' back
> > from a threshold that a signal must go before it may retrip the
> > threshold.
> > This threshold value is separately controlled. Thus if we have a
> > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > signal must first rise past 10, then drop back below 8 and rise again
> > past 10.
> > If it drops below 10 but not 8 and rises again past 10 then we will
> > not get an event.
> > 
> > So having the same register for both the hysteresis and the threshold
> > doesn't with this description make much sense.  It would mean that you
> > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > effect meaning the signal would always have to cross 0 before the next
> > event whatever the combined threshold / hysteresis value?
> > 
> > Perhaps instead the device is automatically adjusting the threshold
> > when we cross it and the 'hysteresis' here is with respect to a the
> > previous threshold?
> > 
> > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > trigger an event at:
> > 
> > 2, 4, 6, 8, 10 as we rise?
> > 
> > This sort of auto adjustment of levels isn't uncommon in light sensors
> > (where the point of the interrupt is to notify the operating system
> > that a 'significant' change has occurred and things like screen
> > brightness may need adjusting.
> > 
> > If so then the current hysteresis interface does not apply, nor does
> > the Rate of Change (ROC) interface as this is dependent on amount of
> > change, not how fast it changed.  Hence we needs something new to
> > handle this cleanly. I would suggest a new event type.  Perhaps
> > something with sysfs attr naming along the lines of
> > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > 
> > etc?
> 
> will you just tell me what you want ? I really cannot give a crap
> anymore. This has already taken me over a month of my time for such a
> simple little device, not to mention your confusing and contradicting
> change requests.
> 
> (could you also trim your responses ? it's very annoying to scroll so
> much)
> 
> > > +#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 */
> > > +
> > 
> > I guess this naming is straight off the datasheet, but it is rather
> > more cryptic than perhaps it needs to be!  That's kind of an issue
> > with the datasheet choices rather than what you have here however!
> 
> man, are you nit-picky!! These are named as such to make grepping on
> documentation easier. It's better to have something that matches
> documentation, don't you think ? otherwise, future users/developers of
> these driver will need either a shit ton of comments explaining that A
> maps to B in docs, or will need a fscking crystal ball to read my mind.
> Assuming I'll still remember what I meant.
> 
> > > +static int opt3001_remove(struct i2c_client *client)
> > > +{
> > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > +	struct opt3001 *opt = iio_priv(iio);
> > > +	int ret;
> > > +	u16 reg;
> > > +
> > > +	free_irq(client->irq, iio);
> > > +	iio_device_unregister(iio);
> > > +
> > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > +	if (ret < 0) {
> > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > +				OPT3001_CONFIGURATION);
> > > +		return ret;
> > > +	}
> > > +
> > > +	reg = ret;
> > > +	opt3001_set_mode(opt, &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;
> > > +	}
> > > +
> > > +	iio_device_free(iio);
> >
> > Use the devm_iio_device_alloc and you can drop the need to free it.
> > I don't really mind, but I'll almost guarantee that someone will post
> > a follow up patch doing this if you don't.  As it will be ever so
> > slightly cleaner, I'll probably take that patch.
> 
> here's the original driver:
> 
> http://www.spinics.net/lists/linux-iio/msg14331.html
> 
> notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> 
> http://www.spinics.net/lists/linux-iio/msg14421.html
> 
> you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> 
> So figure out what you really want, let me know and I'll code it all up
> quickly and hopefully still get this into v3.18 merge window. I sent
> this driver *very* early to be doubly sure it would hit v3.18 and there
> was a long hiatus from yourself which is now jeopardizing what I was
> expecting. That, coupled with your contradicting requests, has just put
> me in a bad mood, even before Monday, hurray.
> 
> cheers
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

http://marc.info/?l=linux-iio&m=141077611429500&w=2

On September 16, 2014 6:03:16 PM GMT+01:00, Felipe Balbi <balbi@ti.com> wrote:
>ping
>
>On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
>> Hi,
>> 
>> On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>> > On 02/09/14 16:17, 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>
>> > > ---
>> > > 
>> > > Resending as I saw no changes on the thread.
>> > Hi Felipe,
>> > 
>> > Sorry for the delay on this, entirely my fault - been busy and
>forgot
>> > I still had questions about what was going on in here (yup its the
>> > hysteresis bit again!)
>> 
>> right, this is starting to become way too much headache for such a
>> simple device. Sorry will not help me getting this driver upstream.
>When
>> I first sent this (August 6), we didn't even have v3.17-rc1, now
>we're
>> about to tag -rc5 and I'm worried this driver will not hit v3.18
>merge
>> window.
>> 
>> > Anyhow, I'm afraid I am still a little confused about the meaning
>you
>> > have assigned to Hysteresis in this driver.
>> > 
>> > Let me conjecture on what might be going on here (I may be entirely
>> > wrong).
>> > 
>> > Normally a hysteresis value in IIO is defined as the 'distance'
>back
>> > from a threshold that a signal must go before it may retrip the
>> > threshold.
>> > This threshold value is separately controlled. Thus if we have a
>> > rising threshold of 10 and an hysteresis of 2 - to get two events
>the
>> > signal must first rise past 10, then drop back below 8 and rise
>again
>> > past 10.
>> > If it drops below 10 but not 8 and rises again past 10 then we will
>> > not get an event.
>> > 
>> > So having the same register for both the hysteresis and the
>threshold
>> > doesn't with this description make much sense.  It would mean that
>you
>> > could only have a threshold of say 10 and a hysteresis of 10, thus
>in
>> > effect meaning the signal would always have to cross 0 before the
>next
>> > event whatever the combined threshold / hysteresis value?
>> > 
>> > Perhaps instead the device is automatically adjusting the threshold
>> > when we cross it and the 'hysteresis' here is with respect to a the
>> > previous threshold?
>> > 
>> > Thus if we start with a value of 0 and hysteresis is set to 2 it
>will
>> > trigger an event at:
>> > 
>> > 2, 4, 6, 8, 10 as we rise?
>> > 
>> > This sort of auto adjustment of levels isn't uncommon in light
>sensors
>> > (where the point of the interrupt is to notify the operating system
>> > that a 'significant' change has occurred and things like screen
>> > brightness may need adjusting.
>> > 
>> > If so then the current hysteresis interface does not apply, nor
>does
>> > the Rate of Change (ROC) interface as this is dependent on amount
>of
>> > change, not how fast it changed.  Hence we needs something new to
>> > handle this cleanly. I would suggest a new event type.  Perhaps
>> > something with sysfs attr naming along the lines of
>> > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>> > What:          
>/sys/.../iio:deviceX/events/in_light_change_rising_value
>> > 
>> > etc?
>> 
>> will you just tell me what you want ? I really cannot give a crap
>> anymore. This has already taken me over a month of my time for such a
>> simple little device, not to mention your confusing and contradicting
>> change requests.
>> 
>> (could you also trim your responses ? it's very annoying to scroll so
>> much)
>> 
>> > > +#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 */
>> > > +
>> > 
>> > I guess this naming is straight off the datasheet, but it is rather
>> > more cryptic than perhaps it needs to be!  That's kind of an issue
>> > with the datasheet choices rather than what you have here however!
>> 
>> man, are you nit-picky!! These are named as such to make grepping on
>> documentation easier. It's better to have something that matches
>> documentation, don't you think ? otherwise, future users/developers
>of
>> these driver will need either a shit ton of comments explaining that
>A
>> maps to B in docs, or will need a fscking crystal ball to read my
>mind.
>> Assuming I'll still remember what I meant.
>> 
>> > > +static int opt3001_remove(struct i2c_client *client)
>> > > +{
>> > > +	struct iio_dev *iio = i2c_get_clientdata(client);
>> > > +	struct opt3001 *opt = iio_priv(iio);
>> > > +	int ret;
>> > > +	u16 reg;
>> > > +
>> > > +	free_irq(client->irq, iio);
>> > > +	iio_device_unregister(iio);
>> > > +
>> > > +	ret = i2c_smbus_read_word_swapped(opt->client,
>OPT3001_CONFIGURATION);
>> > > +	if (ret < 0) {
>> > > +		dev_err(opt->dev, "failed to read register %02x\n",
>> > > +				OPT3001_CONFIGURATION);
>> > > +		return ret;
>> > > +	}
>> > > +
>> > > +	reg = ret;
>> > > +	opt3001_set_mode(opt, &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;
>> > > +	}
>> > > +
>> > > +	iio_device_free(iio);
>> >
>> > Use the devm_iio_device_alloc and you can drop the need to free it.
>> > I don't really mind, but I'll almost guarantee that someone will
>post
>> > a follow up patch doing this if you don't.  As it will be ever so
>> > slightly cleaner, I'll probably take that patch.
>> 
>> here's the original driver:
>> 
>> http://www.spinics.net/lists/linux-iio/msg14331.html
>> 
>> notice that it *WAS* *USING* devm_iio_device_alloc(), until:
>> 
>> http://www.spinics.net/lists/linux-iio/msg14421.html
>> 
>> you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
>> 
>> So figure out what you really want, let me know and I'll code it all
>up
>> quickly and hopefully still get this into v3.18 merge window. I sent
>> this driver *very* early to be doubly sure it would hit v3.18 and
>there
>> was a long hiatus from yourself which is now jeopardizing what I was
>> expecting. That, coupled with your contradicting requests, has just
>put
>> me in a bad mood, even before Monday, hurray.
>> 
>> cheers
>> 
>> -- 
>> balbi

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> ping
> 
> On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > On 02/09/14 16:17, 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>
> > > > ---
> > > > 
> > > > Resending as I saw no changes on the thread.
> > > Hi Felipe,
> > > 
> > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > I still had questions about what was going on in here (yup its the
> > > hysteresis bit again!)
> > 
> > right, this is starting to become way too much headache for such a
> > simple device. Sorry will not help me getting this driver upstream. When
> > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > window.
> > 
> > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > have assigned to Hysteresis in this driver.
> > > 
> > > Let me conjecture on what might be going on here (I may be entirely
> > > wrong).
> > > 
> > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > from a threshold that a signal must go before it may retrip the
> > > threshold.
> > > This threshold value is separately controlled. Thus if we have a
> > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > signal must first rise past 10, then drop back below 8 and rise again
> > > past 10.
> > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > not get an event.
> > > 
> > > So having the same register for both the hysteresis and the threshold
> > > doesn't with this description make much sense.  It would mean that you
> > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > effect meaning the signal would always have to cross 0 before the next
> > > event whatever the combined threshold / hysteresis value?
> > > 
> > > Perhaps instead the device is automatically adjusting the threshold
> > > when we cross it and the 'hysteresis' here is with respect to a the
> > > previous threshold?
> > > 
> > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > trigger an event at:
> > > 
> > > 2, 4, 6, 8, 10 as we rise?
> > > 
> > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > (where the point of the interrupt is to notify the operating system
> > > that a 'significant' change has occurred and things like screen
> > > brightness may need adjusting.
> > > 
> > > If so then the current hysteresis interface does not apply, nor does
> > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > change, not how fast it changed.  Hence we needs something new to
> > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > something with sysfs attr naming along the lines of
> > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > 
> > > etc?
> > 
> > will you just tell me what you want ? I really cannot give a crap
> > anymore. This has already taken me over a month of my time for such a
> > simple little device, not to mention your confusing and contradicting
> > change requests.
> > 
> > (could you also trim your responses ? it's very annoying to scroll so
> > much)
> > 
> > > > +#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 */
> > > > +
> > > 
> > > I guess this naming is straight off the datasheet, but it is rather
> > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > with the datasheet choices rather than what you have here however!
> > 
> > man, are you nit-picky!! These are named as such to make grepping on
> > documentation easier. It's better to have something that matches
> > documentation, don't you think ? otherwise, future users/developers of
> > these driver will need either a shit ton of comments explaining that A
> > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > Assuming I'll still remember what I meant.
> > 
> > > > +static int opt3001_remove(struct i2c_client *client)
> > > > +{
> > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > +	int ret;
> > > > +	u16 reg;
> > > > +
> > > > +	free_irq(client->irq, iio);
> > > > +	iio_device_unregister(iio);
> > > > +
> > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > +	if (ret < 0) {
> > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > +				OPT3001_CONFIGURATION);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	reg = ret;
> > > > +	opt3001_set_mode(opt, &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;
> > > > +	}
> > > > +
> > > > +	iio_device_free(iio);
> > >
> > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > I don't really mind, but I'll almost guarantee that someone will post
> > > a follow up patch doing this if you don't.  As it will be ever so
> > > slightly cleaner, I'll probably take that patch.
> > 
> > here's the original driver:
> > 
> > http://www.spinics.net/lists/linux-iio/msg14331.html
> > 
> > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > 
> > http://www.spinics.net/lists/linux-iio/msg14421.html
> > 
> > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > 
> > So figure out what you really want, let me know and I'll code it all up
> > quickly and hopefully still get this into v3.18 merge window. I sent
> > this driver *very* early to be doubly sure it would hit v3.18 and there
> > was a long hiatus from yourself which is now jeopardizing what I was
> > expecting. That, coupled with your contradicting requests, has just put
> > me in a bad mood, even before Monday, hurray.
> > 
> > cheers
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

I'm confused. Are you not receiving my emails?

On September 17, 2014 4:00:41 PM GMT+01:00, Felipe Balbi <balbi@ti.com> wrote:
>ping
>
>On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
>> ping
>> 
>> On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
>> > Hi,
>> > 
>> > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>> > > On 02/09/14 16:17, 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>
>> > > > ---
>> > > > 
>> > > > Resending as I saw no changes on the thread.
>> > > Hi Felipe,
>> > > 
>> > > Sorry for the delay on this, entirely my fault - been busy and
>forgot
>> > > I still had questions about what was going on in here (yup its
>the
>> > > hysteresis bit again!)
>> > 
>> > right, this is starting to become way too much headache for such a
>> > simple device. Sorry will not help me getting this driver upstream.
>When
>> > I first sent this (August 6), we didn't even have v3.17-rc1, now
>we're
>> > about to tag -rc5 and I'm worried this driver will not hit v3.18
>merge
>> > window.
>> > 
>> > > Anyhow, I'm afraid I am still a little confused about the meaning
>you
>> > > have assigned to Hysteresis in this driver.
>> > > 
>> > > Let me conjecture on what might be going on here (I may be
>entirely
>> > > wrong).
>> > > 
>> > > Normally a hysteresis value in IIO is defined as the 'distance'
>back
>> > > from a threshold that a signal must go before it may retrip the
>> > > threshold.
>> > > This threshold value is separately controlled. Thus if we have a
>> > > rising threshold of 10 and an hysteresis of 2 - to get two events
>the
>> > > signal must first rise past 10, then drop back below 8 and rise
>again
>> > > past 10.
>> > > If it drops below 10 but not 8 and rises again past 10 then we
>will
>> > > not get an event.
>> > > 
>> > > So having the same register for both the hysteresis and the
>threshold
>> > > doesn't with this description make much sense.  It would mean
>that you
>> > > could only have a threshold of say 10 and a hysteresis of 10,
>thus in
>> > > effect meaning the signal would always have to cross 0 before the
>next
>> > > event whatever the combined threshold / hysteresis value?
>> > > 
>> > > Perhaps instead the device is automatically adjusting the
>threshold
>> > > when we cross it and the 'hysteresis' here is with respect to a
>the
>> > > previous threshold?
>> > > 
>> > > Thus if we start with a value of 0 and hysteresis is set to 2 it
>will
>> > > trigger an event at:
>> > > 
>> > > 2, 4, 6, 8, 10 as we rise?
>> > > 
>> > > This sort of auto adjustment of levels isn't uncommon in light
>sensors
>> > > (where the point of the interrupt is to notify the operating
>system
>> > > that a 'significant' change has occurred and things like screen
>> > > brightness may need adjusting.
>> > > 
>> > > If so then the current hysteresis interface does not apply, nor
>does
>> > > the Rate of Change (ROC) interface as this is dependent on amount
>of
>> > > change, not how fast it changed.  Hence we needs something new to
>> > > handle this cleanly. I would suggest a new event type.  Perhaps
>> > > something with sysfs attr naming along the lines of
>> > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>> > > What:          
>/sys/.../iio:deviceX/events/in_light_change_rising_value
>> > > 
>> > > etc?
>> > 
>> > will you just tell me what you want ? I really cannot give a crap
>> > anymore. This has already taken me over a month of my time for such
>a
>> > simple little device, not to mention your confusing and
>contradicting
>> > change requests.
>> > 
>> > (could you also trim your responses ? it's very annoying to scroll
>so
>> > much)
>> > 
>> > > > +#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 */
>> > > > +
>> > > 
>> > > I guess this naming is straight off the datasheet, but it is
>rather
>> > > more cryptic than perhaps it needs to be!  That's kind of an
>issue
>> > > with the datasheet choices rather than what you have here
>however!
>> > 
>> > man, are you nit-picky!! These are named as such to make grepping
>on
>> > documentation easier. It's better to have something that matches
>> > documentation, don't you think ? otherwise, future users/developers
>of
>> > these driver will need either a shit ton of comments explaining
>that A
>> > maps to B in docs, or will need a fscking crystal ball to read my
>mind.
>> > Assuming I'll still remember what I meant.
>> > 
>> > > > +static int opt3001_remove(struct i2c_client *client)
>> > > > +{
>> > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
>> > > > +	struct opt3001 *opt = iio_priv(iio);
>> > > > +	int ret;
>> > > > +	u16 reg;
>> > > > +
>> > > > +	free_irq(client->irq, iio);
>> > > > +	iio_device_unregister(iio);
>> > > > +
>> > > > +	ret = i2c_smbus_read_word_swapped(opt->client,
>OPT3001_CONFIGURATION);
>> > > > +	if (ret < 0) {
>> > > > +		dev_err(opt->dev, "failed to read register %02x\n",
>> > > > +				OPT3001_CONFIGURATION);
>> > > > +		return ret;
>> > > > +	}
>> > > > +
>> > > > +	reg = ret;
>> > > > +	opt3001_set_mode(opt, &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;
>> > > > +	}
>> > > > +
>> > > > +	iio_device_free(iio);
>> > >
>> > > Use the devm_iio_device_alloc and you can drop the need to free
>it.
>> > > I don't really mind, but I'll almost guarantee that someone will
>post
>> > > a follow up patch doing this if you don't.  As it will be ever so
>> > > slightly cleaner, I'll probably take that patch.
>> > 
>> > here's the original driver:
>> > 
>> > http://www.spinics.net/lists/linux-iio/msg14331.html
>> > 
>> > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
>> > 
>> > http://www.spinics.net/lists/linux-iio/msg14421.html
>> > 
>> > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
>> > 
>> > So figure out what you really want, let me know and I'll code it
>all up
>> > quickly and hopefully still get this into v3.18 merge window. I
>sent
>> > this driver *very* early to be doubly sure it would hit v3.18 and
>there
>> > was a long hiatus from yourself which is now jeopardizing what I
>was
>> > expecting. That, coupled with your contradicting requests, has just
>put
>> > me in a bad mood, even before Monday, hurray.
>> > 
>> > cheers
>> > 
>> > -- 
>> > balbi
>> 
>> 
>> 
>> -- 
>> balbi

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> ping
> 
> On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > On 02/09/14 16:17, 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>
> > > > > ---
> > > > > 
> > > > > Resending as I saw no changes on the thread.
> > > > Hi Felipe,
> > > > 
> > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > I still had questions about what was going on in here (yup its the
> > > > hysteresis bit again!)
> > > 
> > > right, this is starting to become way too much headache for such a
> > > simple device. Sorry will not help me getting this driver upstream. When
> > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > window.
> > > 
> > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > have assigned to Hysteresis in this driver.
> > > > 
> > > > Let me conjecture on what might be going on here (I may be entirely
> > > > wrong).
> > > > 
> > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > from a threshold that a signal must go before it may retrip the
> > > > threshold.
> > > > This threshold value is separately controlled. Thus if we have a
> > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > past 10.
> > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > not get an event.
> > > > 
> > > > So having the same register for both the hysteresis and the threshold
> > > > doesn't with this description make much sense.  It would mean that you
> > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > effect meaning the signal would always have to cross 0 before the next
> > > > event whatever the combined threshold / hysteresis value?
> > > > 
> > > > Perhaps instead the device is automatically adjusting the threshold
> > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > previous threshold?
> > > > 
> > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > trigger an event at:
> > > > 
> > > > 2, 4, 6, 8, 10 as we rise?
> > > > 
> > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > (where the point of the interrupt is to notify the operating system
> > > > that a 'significant' change has occurred and things like screen
> > > > brightness may need adjusting.
> > > > 
> > > > If so then the current hysteresis interface does not apply, nor does
> > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > change, not how fast it changed.  Hence we needs something new to
> > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > something with sysfs attr naming along the lines of
> > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > 
> > > > etc?
> > > 
> > > will you just tell me what you want ? I really cannot give a crap
> > > anymore. This has already taken me over a month of my time for such a
> > > simple little device, not to mention your confusing and contradicting
> > > change requests.
> > > 
> > > (could you also trim your responses ? it's very annoying to scroll so
> > > much)
> > > 
> > > > > +#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 */
> > > > > +
> > > > 
> > > > I guess this naming is straight off the datasheet, but it is rather
> > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > with the datasheet choices rather than what you have here however!
> > > 
> > > man, are you nit-picky!! These are named as such to make grepping on
> > > documentation easier. It's better to have something that matches
> > > documentation, don't you think ? otherwise, future users/developers of
> > > these driver will need either a shit ton of comments explaining that A
> > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > Assuming I'll still remember what I meant.
> > > 
> > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > +{
> > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > +	int ret;
> > > > > +	u16 reg;
> > > > > +
> > > > > +	free_irq(client->irq, iio);
> > > > > +	iio_device_unregister(iio);
> > > > > +
> > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > +				OPT3001_CONFIGURATION);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	reg = ret;
> > > > > +	opt3001_set_mode(opt, &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;
> > > > > +	}
> > > > > +
> > > > > +	iio_device_free(iio);
> > > >
> > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > slightly cleaner, I'll probably take that patch.
> > > 
> > > here's the original driver:
> > > 
> > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > 
> > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > 
> > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > 
> > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > 
> > > So figure out what you really want, let me know and I'll code it all up
> > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > was a long hiatus from yourself which is now jeopardizing what I was
> > > expecting. That, coupled with your contradicting requests, has just put
> > > me in a bad mood, even before Monday, hurray.
> > > 
> > > cheers
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Hartmut Knaack]

Problems receiving mails from Jonathan and the list?

http://marc.info/?l=linux-iio&m=141077611429500&w=2

Felipe Balbi schrieb, Am 18.09.2014 15:36:
> ping
> 
> On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
>> ping
>>
>> On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
>>> ping
>>>
>>> On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
>>>> Hi,
>>>>
>>>> On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>>>>> On 02/09/14 16:17, 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>
>>>>>> ---
>>>>>>
>>>>>> Resending as I saw no changes on the thread.
>>>>> Hi Felipe,
>>>>>
>>>>> Sorry for the delay on this, entirely my fault - been busy and forgot
>>>>> I still had questions about what was going on in here (yup its the
>>>>> hysteresis bit again!)
>>>>
>>>> right, this is starting to become way too much headache for such a
>>>> simple device. Sorry will not help me getting this driver upstream. When
>>>> I first sent this (August 6), we didn't even have v3.17-rc1, now we're
>>>> about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
>>>> window.
>>>>
>>>>> Anyhow, I'm afraid I am still a little confused about the meaning you
>>>>> have assigned to Hysteresis in this driver.
>>>>>
>>>>> Let me conjecture on what might be going on here (I may be entirely
>>>>> wrong).
>>>>>
>>>>> Normally a hysteresis value in IIO is defined as the 'distance' back
>>>>> from a threshold that a signal must go before it may retrip the
>>>>> threshold.
>>>>> This threshold value is separately controlled. Thus if we have a
>>>>> rising threshold of 10 and an hysteresis of 2 - to get two events the
>>>>> signal must first rise past 10, then drop back below 8 and rise again
>>>>> past 10.
>>>>> If it drops below 10 but not 8 and rises again past 10 then we will
>>>>> not get an event.
>>>>>
>>>>> So having the same register for both the hysteresis and the threshold
>>>>> doesn't with this description make much sense.  It would mean that you
>>>>> could only have a threshold of say 10 and a hysteresis of 10, thus in
>>>>> effect meaning the signal would always have to cross 0 before the next
>>>>> event whatever the combined threshold / hysteresis value?
>>>>>
>>>>> Perhaps instead the device is automatically adjusting the threshold
>>>>> when we cross it and the 'hysteresis' here is with respect to a the
>>>>> previous threshold?
>>>>>
>>>>> Thus if we start with a value of 0 and hysteresis is set to 2 it will
>>>>> trigger an event at:
>>>>>
>>>>> 2, 4, 6, 8, 10 as we rise?
>>>>>
>>>>> This sort of auto adjustment of levels isn't uncommon in light sensors
>>>>> (where the point of the interrupt is to notify the operating system
>>>>> that a 'significant' change has occurred and things like screen
>>>>> brightness may need adjusting.
>>>>>
>>>>> If so then the current hysteresis interface does not apply, nor does
>>>>> the Rate of Change (ROC) interface as this is dependent on amount of
>>>>> change, not how fast it changed.  Hence we needs something new to
>>>>> handle this cleanly. I would suggest a new event type.  Perhaps
>>>>> something with sysfs attr naming along the lines of
>>>>> What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>>>>> What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
>>>>>
>>>>> etc?
>>>>
>>>> will you just tell me what you want ? I really cannot give a crap
>>>> anymore. This has already taken me over a month of my time for such a
>>>> simple little device, not to mention your confusing and contradicting
>>>> change requests.
>>>>
>>>> (could you also trim your responses ? it's very annoying to scroll so
>>>> much)
>>>>
>>>>>> +#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 */
>>>>>> +
>>>>>
>>>>> I guess this naming is straight off the datasheet, but it is rather
>>>>> more cryptic than perhaps it needs to be!  That's kind of an issue
>>>>> with the datasheet choices rather than what you have here however!
>>>>
>>>> man, are you nit-picky!! These are named as such to make grepping on
>>>> documentation easier. It's better to have something that matches
>>>> documentation, don't you think ? otherwise, future users/developers of
>>>> these driver will need either a shit ton of comments explaining that A
>>>> maps to B in docs, or will need a fscking crystal ball to read my mind.
>>>> Assuming I'll still remember what I meant.
>>>>
>>>>>> +static int opt3001_remove(struct i2c_client *client)
>>>>>> +{
>>>>>> +	struct iio_dev *iio = i2c_get_clientdata(client);
>>>>>> +	struct opt3001 *opt = iio_priv(iio);
>>>>>> +	int ret;
>>>>>> +	u16 reg;
>>>>>> +
>>>>>> +	free_irq(client->irq, iio);
>>>>>> +	iio_device_unregister(iio);
>>>>>> +
>>>>>> +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>>>>> +	if (ret < 0) {
>>>>>> +		dev_err(opt->dev, "failed to read register %02x\n",
>>>>>> +				OPT3001_CONFIGURATION);
>>>>>> +		return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	reg = ret;
>>>>>> +	opt3001_set_mode(opt, &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;
>>>>>> +	}
>>>>>> +
>>>>>> +	iio_device_free(iio);
>>>>>
>>>>> Use the devm_iio_device_alloc and you can drop the need to free it.
>>>>> I don't really mind, but I'll almost guarantee that someone will post
>>>>> a follow up patch doing this if you don't.  As it will be ever so
>>>>> slightly cleaner, I'll probably take that patch.
>>>>
>>>> here's the original driver:
>>>>
>>>> http://www.spinics.net/lists/linux-iio/msg14331.html
>>>>
>>>> notice that it *WAS* *USING* devm_iio_device_alloc(), until:
>>>>
>>>> http://www.spinics.net/lists/linux-iio/msg14421.html
>>>>
>>>> you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
>>>>
>>>> So figure out what you really want, let me know and I'll code it all up
>>>> quickly and hopefully still get this into v3.18 merge window. I sent
>>>> this driver *very* early to be doubly sure it would hit v3.18 and there
>>>> was a long hiatus from yourself which is now jeopardizing what I was
>>>> expecting. That, coupled with your contradicting requests, has just put
>>>> me in a bad mood, even before Monday, hurray.
>>>>
>>>> cheers
>>>>
>>>> -- 
>>>> balbi
>>>
>>>
>>>
>>> -- 
>>> balbi
>>
>>
>>
>> -- 
>> balbi
> 
> 
> 

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> ping
> 
> On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > Hi,
> > > > 
> > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > On 02/09/14 16:17, 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>
> > > > > > ---
> > > > > > 
> > > > > > Resending as I saw no changes on the thread.
> > > > > Hi Felipe,
> > > > > 
> > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > I still had questions about what was going on in here (yup its the
> > > > > hysteresis bit again!)
> > > > 
> > > > right, this is starting to become way too much headache for such a
> > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > window.
> > > > 
> > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > have assigned to Hysteresis in this driver.
> > > > > 
> > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > wrong).
> > > > > 
> > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > from a threshold that a signal must go before it may retrip the
> > > > > threshold.
> > > > > This threshold value is separately controlled. Thus if we have a
> > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > past 10.
> > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > not get an event.
> > > > > 
> > > > > So having the same register for both the hysteresis and the threshold
> > > > > doesn't with this description make much sense.  It would mean that you
> > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > event whatever the combined threshold / hysteresis value?
> > > > > 
> > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > previous threshold?
> > > > > 
> > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > trigger an event at:
> > > > > 
> > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > 
> > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > (where the point of the interrupt is to notify the operating system
> > > > > that a 'significant' change has occurred and things like screen
> > > > > brightness may need adjusting.
> > > > > 
> > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > something with sysfs attr naming along the lines of
> > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > 
> > > > > etc?
> > > > 
> > > > will you just tell me what you want ? I really cannot give a crap
> > > > anymore. This has already taken me over a month of my time for such a
> > > > simple little device, not to mention your confusing and contradicting
> > > > change requests.
> > > > 
> > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > much)
> > > > 
> > > > > > +#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 */
> > > > > > +
> > > > > 
> > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > with the datasheet choices rather than what you have here however!
> > > > 
> > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > documentation easier. It's better to have something that matches
> > > > documentation, don't you think ? otherwise, future users/developers of
> > > > these driver will need either a shit ton of comments explaining that A
> > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > Assuming I'll still remember what I meant.
> > > > 
> > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > +{
> > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > +	int ret;
> > > > > > +	u16 reg;
> > > > > > +
> > > > > > +	free_irq(client->irq, iio);
> > > > > > +	iio_device_unregister(iio);
> > > > > > +
> > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > +	if (ret < 0) {
> > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > +				OPT3001_CONFIGURATION);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	reg = ret;
> > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > +	}
> > > > > > +
> > > > > > +	iio_device_free(iio);
> > > > >
> > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > slightly cleaner, I'll probably take that patch.
> > > > 
> > > > here's the original driver:
> > > > 
> > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > 
> > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > 
> > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > 
> > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > 
> > > > So figure out what you really want, let me know and I'll code it all up
> > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > expecting. That, coupled with your contradicting requests, has just put
> > > > me in a bad mood, even before Monday, hurray.
> > > > 
> > > > cheers
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> ping
> 
> On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > On 02/09/14 16:17, 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>
> > > > > > > ---
> > > > > > > 
> > > > > > > Resending as I saw no changes on the thread.
> > > > > > Hi Felipe,
> > > > > > 
> > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > I still had questions about what was going on in here (yup its the
> > > > > > hysteresis bit again!)
> > > > > 
> > > > > right, this is starting to become way too much headache for such a
> > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > window.
> > > > > 
> > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > have assigned to Hysteresis in this driver.
> > > > > > 
> > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > wrong).
> > > > > > 
> > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > threshold.
> > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > past 10.
> > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > not get an event.
> > > > > > 
> > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > 
> > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > previous threshold?
> > > > > > 
> > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > trigger an event at:
> > > > > > 
> > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > 
> > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > that a 'significant' change has occurred and things like screen
> > > > > > brightness may need adjusting.
> > > > > > 
> > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > something with sysfs attr naming along the lines of
> > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > 
> > > > > > etc?
> > > > > 
> > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > anymore. This has already taken me over a month of my time for such a
> > > > > simple little device, not to mention your confusing and contradicting
> > > > > change requests.
> > > > > 
> > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > much)
> > > > > 
> > > > > > > +#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 */
> > > > > > > +
> > > > > > 
> > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > with the datasheet choices rather than what you have here however!
> > > > > 
> > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > documentation easier. It's better to have something that matches
> > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > these driver will need either a shit ton of comments explaining that A
> > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > Assuming I'll still remember what I meant.
> > > > > 
> > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > +{
> > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > +	int ret;
> > > > > > > +	u16 reg;
> > > > > > > +
> > > > > > > +	free_irq(client->irq, iio);
> > > > > > > +	iio_device_unregister(iio);
> > > > > > > +
> > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > +	if (ret < 0) {
> > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > +		return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	reg = ret;
> > > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	iio_device_free(iio);
> > > > > >
> > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > slightly cleaner, I'll probably take that patch.
> > > > > 
> > > > > here's the original driver:
> > > > > 
> > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > 
> > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > 
> > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > 
> > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > 
> > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > me in a bad mood, even before Monday, hurray.
> > > > > 
> > > > > cheers
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
> ping
> 
> On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > > ping
> > > > > 
> > > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > > On 02/09/14 16:17, 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>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Resending as I saw no changes on the thread.
> > > > > > > Hi Felipe,
> > > > > > > 
> > > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > > I still had questions about what was going on in here (yup its the
> > > > > > > hysteresis bit again!)
> > > > > > 
> > > > > > right, this is starting to become way too much headache for such a
> > > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > > window.
> > > > > > 
> > > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > > have assigned to Hysteresis in this driver.
> > > > > > > 
> > > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > > wrong).
> > > > > > > 
> > > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > > threshold.
> > > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > > past 10.
> > > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > > not get an event.
> > > > > > > 
> > > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > > 
> > > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > > previous threshold?
> > > > > > > 
> > > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > > trigger an event at:
> > > > > > > 
> > > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > > 
> > > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > > that a 'significant' change has occurred and things like screen
> > > > > > > brightness may need adjusting.
> > > > > > > 
> > > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > > something with sysfs attr naming along the lines of
> > > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > > 
> > > > > > > etc?
> > > > > > 
> > > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > > anymore. This has already taken me over a month of my time for such a
> > > > > > simple little device, not to mention your confusing and contradicting
> > > > > > change requests.
> > > > > > 
> > > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > > much)
> > > > > > 
> > > > > > > > +#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 */
> > > > > > > > +
> > > > > > > 
> > > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > > with the datasheet choices rather than what you have here however!
> > > > > > 
> > > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > > documentation easier. It's better to have something that matches
> > > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > > these driver will need either a shit ton of comments explaining that A
> > > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > > Assuming I'll still remember what I meant.
> > > > > > 
> > > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > > +{
> > > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > > +	int ret;
> > > > > > > > +	u16 reg;
> > > > > > > > +
> > > > > > > > +	free_irq(client->irq, iio);
> > > > > > > > +	iio_device_unregister(iio);
> > > > > > > > +
> > > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > > +	if (ret < 0) {
> > > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > +		return ret;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	reg = ret;
> > > > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	iio_device_free(iio);
> > > > > > >
> > > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > > slightly cleaner, I'll probably take that patch.
> > > > > > 
> > > > > > here's the original driver:
> > > > > > 
> > > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > > 
> > > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > > 
> > > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > > 
> > > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > > 
> > > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > > me in a bad mood, even before Monday, hurray.
> > > > > > 
> > > > > > cheers
> > > > > > 
> > > > > > -- 
> > > > > > balbi
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
> ping
> 
> On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > > > ping
> > > > > 
> > > > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > > > ping
> > > > > > 
> > > > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > > > On 02/09/14 16:17, 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>
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > Resending as I saw no changes on the thread.
> > > > > > > > Hi Felipe,
> > > > > > > > 
> > > > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > > > I still had questions about what was going on in here (yup its the
> > > > > > > > hysteresis bit again!)
> > > > > > > 
> > > > > > > right, this is starting to become way too much headache for such a
> > > > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > > > window.
> > > > > > > 
> > > > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > > > have assigned to Hysteresis in this driver.
> > > > > > > > 
> > > > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > > > wrong).
> > > > > > > > 
> > > > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > > > threshold.
> > > > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > > > past 10.
> > > > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > > > not get an event.
> > > > > > > > 
> > > > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > > > 
> > > > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > > > previous threshold?
> > > > > > > > 
> > > > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > > > trigger an event at:
> > > > > > > > 
> > > > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > > > 
> > > > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > > > that a 'significant' change has occurred and things like screen
> > > > > > > > brightness may need adjusting.
> > > > > > > > 
> > > > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > > > something with sysfs attr naming along the lines of
> > > > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > > > 
> > > > > > > > etc?
> > > > > > > 
> > > > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > > > anymore. This has already taken me over a month of my time for such a
> > > > > > > simple little device, not to mention your confusing and contradicting
> > > > > > > change requests.
> > > > > > > 
> > > > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > > > much)
> > > > > > > 
> > > > > > > > > +#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 */
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > > > with the datasheet choices rather than what you have here however!
> > > > > > > 
> > > > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > > > documentation easier. It's better to have something that matches
> > > > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > > > these driver will need either a shit ton of comments explaining that A
> > > > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > > > Assuming I'll still remember what I meant.
> > > > > > > 
> > > > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > > > +{
> > > > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > > > +	int ret;
> > > > > > > > > +	u16 reg;
> > > > > > > > > +
> > > > > > > > > +	free_irq(client->irq, iio);
> > > > > > > > > +	iio_device_unregister(iio);
> > > > > > > > > +
> > > > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > > > +	if (ret < 0) {
> > > > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > > +		return ret;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	reg = ret;
> > > > > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > > +	iio_device_free(iio);
> > > > > > > >
> > > > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > > > slightly cleaner, I'll probably take that patch.
> > > > > > > 
> > > > > > > here's the original driver:
> > > > > > > 
> > > > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > > > 
> > > > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > > > 
> > > > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > > > 
> > > > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > > > 
> > > > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > > > me in a bad mood, even before Monday, hurray.
> > > > > > > 
> > > > > > > cheers
> > > > > > > 
> > > > > > > -- 
> > > > > > > balbi
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > balbi
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Wed, Sep 24, 2014 at 09:36:10AM -0500, Felipe Balbi wrote:
> ping
> 
> On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > > > > ping
> > > > > 
> > > > > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > > > > ping
> > > > > > 
> > > > > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > > > > ping
> > > > > > > 
> > > > > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > > > > On 02/09/14 16:17, 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>
> > > > > > > > > > ---
> > > > > > > > > > 
> > > > > > > > > > Resending as I saw no changes on the thread.
> > > > > > > > > Hi Felipe,
> > > > > > > > > 
> > > > > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > > > > I still had questions about what was going on in here (yup its the
> > > > > > > > > hysteresis bit again!)
> > > > > > > > 
> > > > > > > > right, this is starting to become way too much headache for such a
> > > > > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > > > > window.
> > > > > > > > 
> > > > > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > > > > have assigned to Hysteresis in this driver.
> > > > > > > > > 
> > > > > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > > > > wrong).
> > > > > > > > > 
> > > > > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > > > > threshold.
> > > > > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > > > > past 10.
> > > > > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > > > > not get an event.
> > > > > > > > > 
> > > > > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > > > > 
> > > > > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > > > > previous threshold?
> > > > > > > > > 
> > > > > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > > > > trigger an event at:
> > > > > > > > > 
> > > > > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > > > > 
> > > > > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > > > > that a 'significant' change has occurred and things like screen
> > > > > > > > > brightness may need adjusting.
> > > > > > > > > 
> > > > > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > > > > something with sysfs attr naming along the lines of
> > > > > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > > > > 
> > > > > > > > > etc?
> > > > > > > > 
> > > > > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > > > > anymore. This has already taken me over a month of my time for such a
> > > > > > > > simple little device, not to mention your confusing and contradicting
> > > > > > > > change requests.
> > > > > > > > 
> > > > > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > > > > much)
> > > > > > > > 
> > > > > > > > > > +#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 */
> > > > > > > > > > +
> > > > > > > > > 
> > > > > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > > > > with the datasheet choices rather than what you have here however!
> > > > > > > > 
> > > > > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > > > > documentation easier. It's better to have something that matches
> > > > > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > > > > these driver will need either a shit ton of comments explaining that A
> > > > > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > > > > Assuming I'll still remember what I meant.
> > > > > > > > 
> > > > > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > > > > +{
> > > > > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > > > > +	int ret;
> > > > > > > > > > +	u16 reg;
> > > > > > > > > > +
> > > > > > > > > > +	free_irq(client->irq, iio);
> > > > > > > > > > +	iio_device_unregister(iio);
> > > > > > > > > > +
> > > > > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > > > +		return ret;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	reg = ret;
> > > > > > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > > > > > +	}
> > > > > > > > > > +
> > > > > > > > > > +	iio_device_free(iio);
> > > > > > > > >
> > > > > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > > > > slightly cleaner, I'll probably take that patch.
> > > > > > > > 
> > > > > > > > here's the original driver:
> > > > > > > > 
> > > > > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > > > > 
> > > > > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > > > > 
> > > > > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > > > > 
> > > > > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > > > > 
> > > > > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > > > > me in a bad mood, even before Monday, hurray.
> > > > > > > > 
> > > > > > > > cheers
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > balbi
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > -- 
> > > > > > > balbi
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > balbi
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

ping

On Thu, Sep 25, 2014 at 05:16:19PM -0500, Felipe Balbi wrote:
> ping
> 
> On Wed, Sep 24, 2014 at 09:36:10AM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> > > > > ping
> > > > > 
> > > > > On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > > > > > ping
> > > > > > 
> > > > > > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > > > > > ping
> > > > > > > 
> > > > > > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > > > > > ping
> > > > > > > > 
> > > > > > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > > > > > Hi,
> > > > > > > > > 
> > > > > > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > > > > > On 02/09/14 16:17, 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>
> > > > > > > > > > > ---
> > > > > > > > > > > 
> > > > > > > > > > > Resending as I saw no changes on the thread.
> > > > > > > > > > Hi Felipe,
> > > > > > > > > > 
> > > > > > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > > > > > I still had questions about what was going on in here (yup its the
> > > > > > > > > > hysteresis bit again!)
> > > > > > > > > 
> > > > > > > > > right, this is starting to become way too much headache for such a
> > > > > > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > > > > > window.
> > > > > > > > > 
> > > > > > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > > > > > have assigned to Hysteresis in this driver.
> > > > > > > > > > 
> > > > > > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > > > > > wrong).
> > > > > > > > > > 
> > > > > > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > > > > > threshold.
> > > > > > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > > > > > past 10.
> > > > > > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > > > > > not get an event.
> > > > > > > > > > 
> > > > > > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > > > > > 
> > > > > > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > > > > > previous threshold?
> > > > > > > > > > 
> > > > > > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > > > > > trigger an event at:
> > > > > > > > > > 
> > > > > > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > > > > > 
> > > > > > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > > > > > that a 'significant' change has occurred and things like screen
> > > > > > > > > > brightness may need adjusting.
> > > > > > > > > > 
> > > > > > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > > > > > something with sysfs attr naming along the lines of
> > > > > > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > > > > > 
> > > > > > > > > > etc?
> > > > > > > > > 
> > > > > > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > > > > > anymore. This has already taken me over a month of my time for such a
> > > > > > > > > simple little device, not to mention your confusing and contradicting
> > > > > > > > > change requests.
> > > > > > > > > 
> > > > > > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > > > > > much)
> > > > > > > > > 
> > > > > > > > > > > +#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 */
> > > > > > > > > > > +
> > > > > > > > > > 
> > > > > > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > > > > > with the datasheet choices rather than what you have here however!
> > > > > > > > > 
> > > > > > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > > > > > documentation easier. It's better to have something that matches
> > > > > > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > > > > > these driver will need either a shit ton of comments explaining that A
> > > > > > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > > > > > Assuming I'll still remember what I meant.
> > > > > > > > > 
> > > > > > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > > > > > +{
> > > > > > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > > > > > +	int ret;
> > > > > > > > > > > +	u16 reg;
> > > > > > > > > > > +
> > > > > > > > > > > +	free_irq(client->irq, iio);
> > > > > > > > > > > +	iio_device_unregister(iio);
> > > > > > > > > > > +
> > > > > > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > > > > +		return ret;
> > > > > > > > > > > +	}
> > > > > > > > > > > +
> > > > > > > > > > > +	reg = ret;
> > > > > > > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > > > > > > +	}
> > > > > > > > > > > +
> > > > > > > > > > > +	iio_device_free(iio);
> > > > > > > > > >
> > > > > > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > > > > > slightly cleaner, I'll probably take that patch.
> > > > > > > > > 
> > > > > > > > > here's the original driver:
> > > > > > > > > 
> > > > > > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > > > > > 
> > > > > > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > > > > > 
> > > > > > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > > > > > 
> > > > > > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > > > > > 
> > > > > > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > > > > > me in a bad mood, even before Monday, hurray.
> > > > > > > > > 
> > > > > > > > > cheers
> > > > > > > > > 
> > > > > > > > > -- 
> > > > > > > > > balbi
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > balbi
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > -- 
> > > > > > > balbi
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > balbi
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

Alright, this is already ridiculous. Andrew, if I resend the patch can
you apply it since maintainer has been ignoring this thread anyway ?

On Fri, Sep 26, 2014 at 11:19:59PM -0500, Felipe Balbi wrote:
> ping
> 
> On Thu, Sep 25, 2014 at 05:16:19PM -0500, Felipe Balbi wrote:
> > ping
> > 
> > On Wed, Sep 24, 2014 at 09:36:10AM -0500, Felipe Balbi wrote:
> > > ping
> > > 
> > > On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
> > > > ping
> > > > 
> > > > On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
> > > > > ping
> > > > > 
> > > > > On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
> > > > > > ping
> > > > > > 
> > > > > > On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
> > > > > > > ping
> > > > > > > 
> > > > > > > On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
> > > > > > > > ping
> > > > > > > > 
> > > > > > > > On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
> > > > > > > > > ping
> > > > > > > > > 
> > > > > > > > > On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
> > > > > > > > > > Hi,
> > > > > > > > > > 
> > > > > > > > > > On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
> > > > > > > > > > > On 02/09/14 16:17, 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>
> > > > > > > > > > > > ---
> > > > > > > > > > > > 
> > > > > > > > > > > > Resending as I saw no changes on the thread.
> > > > > > > > > > > Hi Felipe,
> > > > > > > > > > > 
> > > > > > > > > > > Sorry for the delay on this, entirely my fault - been busy and forgot
> > > > > > > > > > > I still had questions about what was going on in here (yup its the
> > > > > > > > > > > hysteresis bit again!)
> > > > > > > > > > 
> > > > > > > > > > right, this is starting to become way too much headache for such a
> > > > > > > > > > simple device. Sorry will not help me getting this driver upstream. When
> > > > > > > > > > I first sent this (August 6), we didn't even have v3.17-rc1, now we're
> > > > > > > > > > about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
> > > > > > > > > > window.
> > > > > > > > > > 
> > > > > > > > > > > Anyhow, I'm afraid I am still a little confused about the meaning you
> > > > > > > > > > > have assigned to Hysteresis in this driver.
> > > > > > > > > > > 
> > > > > > > > > > > Let me conjecture on what might be going on here (I may be entirely
> > > > > > > > > > > wrong).
> > > > > > > > > > > 
> > > > > > > > > > > Normally a hysteresis value in IIO is defined as the 'distance' back
> > > > > > > > > > > from a threshold that a signal must go before it may retrip the
> > > > > > > > > > > threshold.
> > > > > > > > > > > This threshold value is separately controlled. Thus if we have a
> > > > > > > > > > > rising threshold of 10 and an hysteresis of 2 - to get two events the
> > > > > > > > > > > signal must first rise past 10, then drop back below 8 and rise again
> > > > > > > > > > > past 10.
> > > > > > > > > > > If it drops below 10 but not 8 and rises again past 10 then we will
> > > > > > > > > > > not get an event.
> > > > > > > > > > > 
> > > > > > > > > > > So having the same register for both the hysteresis and the threshold
> > > > > > > > > > > doesn't with this description make much sense.  It would mean that you
> > > > > > > > > > > could only have a threshold of say 10 and a hysteresis of 10, thus in
> > > > > > > > > > > effect meaning the signal would always have to cross 0 before the next
> > > > > > > > > > > event whatever the combined threshold / hysteresis value?
> > > > > > > > > > > 
> > > > > > > > > > > Perhaps instead the device is automatically adjusting the threshold
> > > > > > > > > > > when we cross it and the 'hysteresis' here is with respect to a the
> > > > > > > > > > > previous threshold?
> > > > > > > > > > > 
> > > > > > > > > > > Thus if we start with a value of 0 and hysteresis is set to 2 it will
> > > > > > > > > > > trigger an event at:
> > > > > > > > > > > 
> > > > > > > > > > > 2, 4, 6, 8, 10 as we rise?
> > > > > > > > > > > 
> > > > > > > > > > > This sort of auto adjustment of levels isn't uncommon in light sensors
> > > > > > > > > > > (where the point of the interrupt is to notify the operating system
> > > > > > > > > > > that a 'significant' change has occurred and things like screen
> > > > > > > > > > > brightness may need adjusting.
> > > > > > > > > > > 
> > > > > > > > > > > If so then the current hysteresis interface does not apply, nor does
> > > > > > > > > > > the Rate of Change (ROC) interface as this is dependent on amount of
> > > > > > > > > > > change, not how fast it changed.  Hence we needs something new to
> > > > > > > > > > > handle this cleanly. I would suggest a new event type.  Perhaps
> > > > > > > > > > > something with sysfs attr naming along the lines of
> > > > > > > > > > > What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
> > > > > > > > > > > What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
> > > > > > > > > > > 
> > > > > > > > > > > etc?
> > > > > > > > > > 
> > > > > > > > > > will you just tell me what you want ? I really cannot give a crap
> > > > > > > > > > anymore. This has already taken me over a month of my time for such a
> > > > > > > > > > simple little device, not to mention your confusing and contradicting
> > > > > > > > > > change requests.
> > > > > > > > > > 
> > > > > > > > > > (could you also trim your responses ? it's very annoying to scroll so
> > > > > > > > > > much)
> > > > > > > > > > 
> > > > > > > > > > > > +#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 */
> > > > > > > > > > > > +
> > > > > > > > > > > 
> > > > > > > > > > > I guess this naming is straight off the datasheet, but it is rather
> > > > > > > > > > > more cryptic than perhaps it needs to be!  That's kind of an issue
> > > > > > > > > > > with the datasheet choices rather than what you have here however!
> > > > > > > > > > 
> > > > > > > > > > man, are you nit-picky!! These are named as such to make grepping on
> > > > > > > > > > documentation easier. It's better to have something that matches
> > > > > > > > > > documentation, don't you think ? otherwise, future users/developers of
> > > > > > > > > > these driver will need either a shit ton of comments explaining that A
> > > > > > > > > > maps to B in docs, or will need a fscking crystal ball to read my mind.
> > > > > > > > > > Assuming I'll still remember what I meant.
> > > > > > > > > > 
> > > > > > > > > > > > +static int opt3001_remove(struct i2c_client *client)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +	struct iio_dev *iio = i2c_get_clientdata(client);
> > > > > > > > > > > > +	struct opt3001 *opt = iio_priv(iio);
> > > > > > > > > > > > +	int ret;
> > > > > > > > > > > > +	u16 reg;
> > > > > > > > > > > > +
> > > > > > > > > > > > +	free_irq(client->irq, iio);
> > > > > > > > > > > > +	iio_device_unregister(iio);
> > > > > > > > > > > > +
> > > > > > > > > > > > +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
> > > > > > > > > > > > +	if (ret < 0) {
> > > > > > > > > > > > +		dev_err(opt->dev, "failed to read register %02x\n",
> > > > > > > > > > > > +				OPT3001_CONFIGURATION);
> > > > > > > > > > > > +		return ret;
> > > > > > > > > > > > +	}
> > > > > > > > > > > > +
> > > > > > > > > > > > +	reg = ret;
> > > > > > > > > > > > +	opt3001_set_mode(opt, &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;
> > > > > > > > > > > > +	}
> > > > > > > > > > > > +
> > > > > > > > > > > > +	iio_device_free(iio);
> > > > > > > > > > >
> > > > > > > > > > > Use the devm_iio_device_alloc and you can drop the need to free it.
> > > > > > > > > > > I don't really mind, but I'll almost guarantee that someone will post
> > > > > > > > > > > a follow up patch doing this if you don't.  As it will be ever so
> > > > > > > > > > > slightly cleaner, I'll probably take that patch.
> > > > > > > > > > 
> > > > > > > > > > here's the original driver:
> > > > > > > > > > 
> > > > > > > > > > http://www.spinics.net/lists/linux-iio/msg14331.html
> > > > > > > > > > 
> > > > > > > > > > notice that it *WAS* *USING* devm_iio_device_alloc(), until:
> > > > > > > > > > 
> > > > > > > > > > http://www.spinics.net/lists/linux-iio/msg14421.html
> > > > > > > > > > 
> > > > > > > > > > you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
> > > > > > > > > > 
> > > > > > > > > > So figure out what you really want, let me know and I'll code it all up
> > > > > > > > > > quickly and hopefully still get this into v3.18 merge window. I sent
> > > > > > > > > > this driver *very* early to be doubly sure it would hit v3.18 and there
> > > > > > > > > > was a long hiatus from yourself which is now jeopardizing what I was
> > > > > > > > > > expecting. That, coupled with your contradicting requests, has just put
> > > > > > > > > > me in a bad mood, even before Monday, hurray.
> > > > > > > > > > 
> > > > > > > > > > cheers
> > > > > > > > > > 
> > > > > > > > > > -- 
> > > > > > > > > > balbi
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > -- 
> > > > > > > > > balbi
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > balbi
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > -- 
> > > > > > > balbi
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -- 
> > > > > > balbi
> > > > > 
> > > > > 
> > > > > 
> > > > > -- 
> > > > > balbi
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > balbi
> > > 
> > > 
> > > 
> > > -- 
> > > balbi
> > 
> > 
> > 
> > -- 
> > balbi
> 
> 
> 
> -- 
> balbi



-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

Hi Andrew.

I would prefer to assume there is some issue with emails from myself
to Felipe than he is completely ignoring the requests to actually
respond in a technical fashion to my review.

http://marc.info/?l=linux-iio&m=141077611429500&w=2
was my last request for clarification on ABI usage within driver.
Given Felipe's last substantial email I think the lack of
communications may be a generous interpretation...
http://marc.info/?l=linux-iio&m=141075853024132&w=2

Harmut also emailed to see if Felipe was having communication problems
and does not seem to have received a public response.

The driver as proposed, under review with the information available,
does not appear to conform to the published ABI and without further
clarification or restriction to those elements within the ABI (as
suggested as an easy way of moving forward), should not be applied.

Rather than getting worked up, I'll leave interested readers to draw
their own conclusions from the email thread.

Sorry that this is wasting everyone's time.  I am quite happy to
either personally respond to any technical response to the review
with the assistance of anyone else who wishes to review the code.

Greg cc'd as upstream maintainer for IIO to keep him informed.

Thanks,

Jonathan

On 29/09/14 15:02, Felipe Balbi wrote:
> Alright, this is already ridiculous. Andrew, if I resend the patch can
> you apply it since maintainer has been ignoring this thread anyway ?
> 
> On Fri, Sep 26, 2014 at 11:19:59PM -0500, Felipe Balbi wrote:
>> ping
>>
>> On Thu, Sep 25, 2014 at 05:16:19PM -0500, Felipe Balbi wrote:
>>> ping
>>>
>>> On Wed, Sep 24, 2014 at 09:36:10AM -0500, Felipe Balbi wrote:
>>>> ping
>>>>
>>>> On Tue, Sep 23, 2014 at 09:09:24AM -0500, Felipe Balbi wrote:
>>>>> ping
>>>>>
>>>>> On Mon, Sep 22, 2014 at 09:09:14AM -0500, Felipe Balbi wrote:
>>>>>> ping
>>>>>>
>>>>>> On Fri, Sep 19, 2014 at 11:21:29AM -0500, Felipe Balbi wrote:
>>>>>>> ping
>>>>>>>
>>>>>>> On Thu, Sep 18, 2014 at 08:36:31AM -0500, Felipe Balbi wrote:
>>>>>>>> ping
>>>>>>>>
>>>>>>>> On Wed, Sep 17, 2014 at 10:00:41AM -0500, Felipe Balbi wrote:
>>>>>>>>> ping
>>>>>>>>>
>>>>>>>>> On Tue, Sep 16, 2014 at 12:03:16PM -0500, Felipe Balbi wrote:
>>>>>>>>>> ping
>>>>>>>>>>
>>>>>>>>>> On Mon, Sep 15, 2014 at 12:21:37AM -0500, Felipe Balbi wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> On Sat, Sep 13, 2014 at 05:52:02PM +0100, Jonathan Cameron wrote:
>>>>>>>>>>>> On 02/09/14 16:17, 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>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> Resending as I saw no changes on the thread.
>>>>>>>>>>>> Hi Felipe,
>>>>>>>>>>>>
>>>>>>>>>>>> Sorry for the delay on this, entirely my fault - been busy and forgot
>>>>>>>>>>>> I still had questions about what was going on in here (yup its the
>>>>>>>>>>>> hysteresis bit again!)
>>>>>>>>>>>
>>>>>>>>>>> right, this is starting to become way too much headache for such a
>>>>>>>>>>> simple device. Sorry will not help me getting this driver upstream. When
>>>>>>>>>>> I first sent this (August 6), we didn't even have v3.17-rc1, now we're
>>>>>>>>>>> about to tag -rc5 and I'm worried this driver will not hit v3.18 merge
>>>>>>>>>>> window.
>>>>>>>>>>>
>>>>>>>>>>>> Anyhow, I'm afraid I am still a little confused about the meaning you
>>>>>>>>>>>> have assigned to Hysteresis in this driver.
>>>>>>>>>>>>
>>>>>>>>>>>> Let me conjecture on what might be going on here (I may be entirely
>>>>>>>>>>>> wrong).
>>>>>>>>>>>>
>>>>>>>>>>>> Normally a hysteresis value in IIO is defined as the 'distance' back
>>>>>>>>>>>> from a threshold that a signal must go before it may retrip the
>>>>>>>>>>>> threshold.
>>>>>>>>>>>> This threshold value is separately controlled. Thus if we have a
>>>>>>>>>>>> rising threshold of 10 and an hysteresis of 2 - to get two events the
>>>>>>>>>>>> signal must first rise past 10, then drop back below 8 and rise again
>>>>>>>>>>>> past 10.
>>>>>>>>>>>> If it drops below 10 but not 8 and rises again past 10 then we will
>>>>>>>>>>>> not get an event.
>>>>>>>>>>>>
>>>>>>>>>>>> So having the same register for both the hysteresis and the threshold
>>>>>>>>>>>> doesn't with this description make much sense.  It would mean that you
>>>>>>>>>>>> could only have a threshold of say 10 and a hysteresis of 10, thus in
>>>>>>>>>>>> effect meaning the signal would always have to cross 0 before the next
>>>>>>>>>>>> event whatever the combined threshold / hysteresis value?
>>>>>>>>>>>>
>>>>>>>>>>>> Perhaps instead the device is automatically adjusting the threshold
>>>>>>>>>>>> when we cross it and the 'hysteresis' here is with respect to a the
>>>>>>>>>>>> previous threshold?
>>>>>>>>>>>>
>>>>>>>>>>>> Thus if we start with a value of 0 and hysteresis is set to 2 it will
>>>>>>>>>>>> trigger an event at:
>>>>>>>>>>>>
>>>>>>>>>>>> 2, 4, 6, 8, 10 as we rise?
>>>>>>>>>>>>
>>>>>>>>>>>> This sort of auto adjustment of levels isn't uncommon in light sensors
>>>>>>>>>>>> (where the point of the interrupt is to notify the operating system
>>>>>>>>>>>> that a 'significant' change has occurred and things like screen
>>>>>>>>>>>> brightness may need adjusting.
>>>>>>>>>>>>
>>>>>>>>>>>> If so then the current hysteresis interface does not apply, nor does
>>>>>>>>>>>> the Rate of Change (ROC) interface as this is dependent on amount of
>>>>>>>>>>>> change, not how fast it changed.  Hence we needs something new to
>>>>>>>>>>>> handle this cleanly. I would suggest a new event type.  Perhaps
>>>>>>>>>>>> something with sysfs attr naming along the lines of
>>>>>>>>>>>> What:		/sys/.../iio:deviceX/events/in_light_change_rising_en
>>>>>>>>>>>> What:           /sys/.../iio:deviceX/events/in_light_change_rising_value
>>>>>>>>>>>>
>>>>>>>>>>>> etc?
>>>>>>>>>>>
>>>>>>>>>>> will you just tell me what you want ? I really cannot give a crap
>>>>>>>>>>> anymore. This has already taken me over a month of my time for such a
>>>>>>>>>>> simple little device, not to mention your confusing and contradicting
>>>>>>>>>>> change requests.
>>>>>>>>>>>
>>>>>>>>>>> (could you also trim your responses ? it's very annoying to scroll so
>>>>>>>>>>> much)
>>>>>>>>>>>
>>>>>>>>>>>>> +#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 */
>>>>>>>>>>>>> +
>>>>>>>>>>>>
>>>>>>>>>>>> I guess this naming is straight off the datasheet, but it is rather
>>>>>>>>>>>> more cryptic than perhaps it needs to be!  That's kind of an issue
>>>>>>>>>>>> with the datasheet choices rather than what you have here however!
>>>>>>>>>>>
>>>>>>>>>>> man, are you nit-picky!! These are named as such to make grepping on
>>>>>>>>>>> documentation easier. It's better to have something that matches
>>>>>>>>>>> documentation, don't you think ? otherwise, future users/developers of
>>>>>>>>>>> these driver will need either a shit ton of comments explaining that A
>>>>>>>>>>> maps to B in docs, or will need a fscking crystal ball to read my mind.
>>>>>>>>>>> Assuming I'll still remember what I meant.
>>>>>>>>>>>
>>>>>>>>>>>>> +static int opt3001_remove(struct i2c_client *client)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +	struct iio_dev *iio = i2c_get_clientdata(client);
>>>>>>>>>>>>> +	struct opt3001 *opt = iio_priv(iio);
>>>>>>>>>>>>> +	int ret;
>>>>>>>>>>>>> +	u16 reg;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	free_irq(client->irq, iio);
>>>>>>>>>>>>> +	iio_device_unregister(iio);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	ret = i2c_smbus_read_word_swapped(opt->client, OPT3001_CONFIGURATION);
>>>>>>>>>>>>> +	if (ret < 0) {
>>>>>>>>>>>>> +		dev_err(opt->dev, "failed to read register %02x\n",
>>>>>>>>>>>>> +				OPT3001_CONFIGURATION);
>>>>>>>>>>>>> +		return ret;
>>>>>>>>>>>>> +	}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	reg = ret;
>>>>>>>>>>>>> +	opt3001_set_mode(opt, &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;
>>>>>>>>>>>>> +	}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +	iio_device_free(iio);
>>>>>>>>>>>>
>>>>>>>>>>>> Use the devm_iio_device_alloc and you can drop the need to free it.
>>>>>>>>>>>> I don't really mind, but I'll almost guarantee that someone will post
>>>>>>>>>>>> a follow up patch doing this if you don't.  As it will be ever so
>>>>>>>>>>>> slightly cleaner, I'll probably take that patch.
>>>>>>>>>>>
>>>>>>>>>>> here's the original driver:
>>>>>>>>>>>
>>>>>>>>>>> http://www.spinics.net/lists/linux-iio/msg14331.html
>>>>>>>>>>>
>>>>>>>>>>> notice that it *WAS* *USING* devm_iio_device_alloc(), until:
>>>>>>>>>>>
>>>>>>>>>>> http://www.spinics.net/lists/linux-iio/msg14421.html
>>>>>>>>>>>
>>>>>>>>>>> you *SPECIFICALLY* asked for *NON* *DEVM* versions!!
>>>>>>>>>>>
>>>>>>>>>>> So figure out what you really want, let me know and I'll code it all up
>>>>>>>>>>> quickly and hopefully still get this into v3.18 merge window. I sent
>>>>>>>>>>> this driver *very* early to be doubly sure it would hit v3.18 and there
>>>>>>>>>>> was a long hiatus from yourself which is now jeopardizing what I was
>>>>>>>>>>> expecting. That, coupled with your contradicting requests, has just put
>>>>>>>>>>> me in a bad mood, even before Monday, hurray.
>>>>>>>>>>>
>>>>>>>>>>> cheers
>>>>>>>>>>>
>>>>>>>>>>> -- 
>>>>>>>>>>> balbi
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -- 
>>>>>>>>>> balbi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> -- 
>>>>>>>>> balbi
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> -- 
>>>>>>>> balbi
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> -- 
>>>>>>> balbi
>>>>>>
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> balbi
>>>>>
>>>>>
>>>>>
>>>>> -- 
>>>>> balbi
>>>>
>>>>
>>>>
>>>> -- 
>>>> balbi
>>>
>>>
>>>
>>> -- 
>>> balbi
>>
>>
>>
>> -- 
>> balbi
> 
> 
> 

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi wrote:
> Alright, this is already ridiculous. Andrew, if I resend the patch can
> you apply it since maintainer has been ignoring this thread anyway ?
> 

Do you reall think this is the best way to approach this?

Perhaps it would be better to explain what each field of the
configuration register does and then we can move on.

In particular the OPT3001_CONFIGURATION_L field needs to be explained
such that the ABI can be properly applied.

Looking at the docs for the Windows demo program the field is associated
with a latch configuration. What does this bit field actually do?

Either have TI release the documentation or add comments to each of the
fields of each of the registers such that we can understand what exactly
they are doing.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

Hi,

On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
> On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi wrote:
> > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > you apply it since maintainer has been ignoring this thread anyway ?
> > 
> 
> Do you reall think this is the best way to approach this?

when maintainer doesn't respond for weeks, yeah! Sure it is.

> Perhaps it would be better to explain what each field of the
> configuration register does and then we can move on.

perhaps Jonathan could tell me exactly what he wants because I can't
handle back-and-forth anymore. Specially when he complains about stuff
he asked me to modify himself.

> In particular the OPT3001_CONFIGURATION_L field needs to be explained
> such that the ABI can be properly applied.
> 
> Looking at the docs for the Windows demo program the field is associated
> with a latch configuration. What does this bit field actually do?
> 
> Either have TI release the documentation or add comments to each of the
> fields of each of the registers such that we can understand what exactly
> they are doing.

TI will release the documentation when that's all cleared up with Legal.
You can't expect it to be any earlier than that.

-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Daniel Baluta]

On Mon, Sep 29, 2014 at 9:53 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
>> On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi wrote:
>> > Alright, this is already ridiculous. Andrew, if I resend the patch can
>> > you apply it since maintainer has been ignoring this thread anyway ?
>> >
>>
>> Do you reall think this is the best way to approach this?
>
> when maintainer doesn't respond for weeks, yeah! Sure it is.

This is ridiculous! Jonathan asked you some questions about the implementation,
but you ignored the request. Instead you've send 9 ping emails!

Daniel.

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

On Mon, Sep 29, 2014 at 10:07:32PM +0300, Daniel Baluta wrote:
> On Mon, Sep 29, 2014 at 9:53 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
> >> On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi wrote:
> >> > Alright, this is already ridiculous. Andrew, if I resend the patch can
> >> > you apply it since maintainer has been ignoring this thread anyway ?
> >> >
> >>
> >> Do you reall think this is the best way to approach this?
> >
> > when maintainer doesn't respond for weeks, yeah! Sure it is.
> 
> This is ridiculous! Jonathan asked you some questions about the implementation,
> but you ignored the request. Instead you've send 9 ping emails!

I ignored ? I replied straight away asking him to tell me exactly what
he needed and pointing out all the things which he's complaining about
that he asked me to get rid of himself. For example, original driver was
using devm_*, Jonathan asked to remove, I tried arguing and he said he
wanted them out, so I removed, now he's asking me why I didn't use
devm_* to start with.

That's ridiculous, that's what pointless and a total waste of time.

Not to mention that there was a long hiatus from Jonathan for weeks
until he said "yeah, sorry, I had forgotten about this". Because of
Jonathan alone, we lost a merge window. I send the original driver
because v3.17-rc1 was even tagged and now you tell me that it's
ridiculous I'm upset with Jonathan's lazy responses ?

gimme a break

-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

On Mon, Sep 29, 2014 at 01:53:25PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Sep 29, 2014 at 01:38:56PM -0500, Michael Welling wrote:
> > On Mon, Sep 29, 2014 at 09:02:27AM -0500, Felipe Balbi wrote:
> > > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > > you apply it since maintainer has been ignoring this thread anyway ?
> > > 
> > 
> > Do you reall think this is the best way to approach this?
> 
> when maintainer doesn't respond for weeks, yeah! Sure it is.
> 
> > Perhaps it would be better to explain what each field of the
> > configuration register does and then we can move on.
> 
> perhaps Jonathan could tell me exactly what he wants because I can't
> handle back-and-forth anymore. Specially when he complains about stuff
> he asked me to modify himself.
>
> > In particular the OPT3001_CONFIGURATION_L field needs to be explained
> > such that the ABI can be properly applied.
> > 
> > Looking at the docs for the Windows demo program the field is associated
> > with a latch configuration. What does this bit field actually do?

Still no technical information. Without all the facts how can you expect
him to tell you what he wants?

> > 
> > Either have TI release the documentation or add comments to each of the
> > fields of each of the registers such that we can understand what exactly
> > they are doing.
> 
> TI will release the documentation when that's all cleared up with Legal.
> You can't expect it to be any earlier than that.

I am a little fuzzy on how the source code can be released when an NDA
is required to access the datasheet.

Isn't the source code going to be breaking the NDA by releasing information
that is in the datasheet?

> 
> -- 
> balbi

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

Hi,

On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
> > > > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > > > you apply it since maintainer has been ignoring this thread anyway ?
> > > > 
> > > 
> > > Do you reall think this is the best way to approach this?
> > 
> > when maintainer doesn't respond for weeks, yeah! Sure it is.
> > 
> > > Perhaps it would be better to explain what each field of the
> > > configuration register does and then we can move on.
> > 
> > perhaps Jonathan could tell me exactly what he wants because I can't
> > handle back-and-forth anymore. Specially when he complains about stuff
> > he asked me to modify himself.
> >
> > > In particular the OPT3001_CONFIGURATION_L field needs to be explained
> > > such that the ABI can be properly applied.
> > > 
> > > Looking at the docs for the Windows demo program the field is associated
> > > with a latch configuration. What does this bit field actually do?
> 
> Still no technical information. Without all the facts how can you expect
> him to tell you what he wants?

yeah, because clearly he doesn't know himself, right ?

> > > Either have TI release the documentation or add comments to each of the
> > > fields of each of the registers such that we can understand what exactly
> > > they are doing.
> > 
> > TI will release the documentation when that's all cleared up with Legal.
> > You can't expect it to be any earlier than that.
> 
> I am a little fuzzy on how the source code can be released when an NDA
> is required to access the datasheet.
>
> Isn't the source code going to be breaking the NDA by releasing information
> that is in the datasheet?

that's really not my role inside TI, though. I have no degree by any law
school from any country. When I get asked to write a driver, all I do is
request permission to release it, if that says "okay, release it", I
don't go after the Lawyer who decided it was okay to release the driver.

On top of that, what does that has anything to do with anything ? I'm
pretty sure many have released code based off of either simulation or
pre-release HW. Lack of public documentation does not prevent source
code from being released at all.

Try to get documentation for most of SoCs supported under the ARM tree
and you'll see at least 80% of them will require NDA and/or a big
purchase order of many SoCs before you can get documentation.

-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
> > > > > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > > > > you apply it since maintainer has been ignoring this thread anyway ?
> > > > > 
> > > > 
> > > > Do you reall think this is the best way to approach this?
> > > 
> > > when maintainer doesn't respond for weeks, yeah! Sure it is.
> > > 
> > > > Perhaps it would be better to explain what each field of the
> > > > configuration register does and then we can move on.
> > > 
> > > perhaps Jonathan could tell me exactly what he wants because I can't
> > > handle back-and-forth anymore. Specially when he complains about stuff
> > > he asked me to modify himself.
> > >
> > > > In particular the OPT3001_CONFIGURATION_L field needs to be explained
> > > > such that the ABI can be properly applied.
> > > > 
> > > > Looking at the docs for the Windows demo program the field is associated
> > > > with a latch configuration. What does this bit field actually do?
> > 
> > Still no technical information. Without all the facts how can you expect
> > him to tell you what he wants?
> 
> yeah, because clearly he doesn't know himself, right ?

Could you explain how it works to me then?

> 
> > > > Either have TI release the documentation or add comments to each of the
> > > > fields of each of the registers such that we can understand what exactly
> > > > they are doing.
> > > 
> > > TI will release the documentation when that's all cleared up with Legal.
> > > You can't expect it to be any earlier than that.
> > 
> > I am a little fuzzy on how the source code can be released when an NDA
> > is required to access the datasheet.
> >
> > Isn't the source code going to be breaking the NDA by releasing information
> > that is in the datasheet?
>
> that's really not my role inside TI, though. I have no degree by any law
> school from any country. When I get asked to write a driver, all I do is
> request permission to release it, if that says "okay, release it", I
> don't go after the Lawyer who decided it was okay to release the driver.
> 
> On top of that, what does that has anything to do with anything ? I'm
> pretty sure many have released code based off of either simulation or
> pre-release HW. Lack of public documentation does not prevent source
> code from being released at all.
>
> Try to get documentation for most of SoCs supported under the ARM tree
> and you'll see at least 80% of them will require NDA and/or a big
> purchase order of many SoCs before you can get documentation.
>

I don't know I am just trying to the more facts so my mailbox will stop
pinging. :)

> -- 
> balbi

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

On Mon, Sep 29, 2014 at 06:41:59PM -0500, Michael Welling wrote:
> On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
> > > > > > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > > > > > you apply it since maintainer has been ignoring this thread anyway ?
> > > > > > 
> > > > > 
> > > > > Do you reall think this is the best way to approach this?
> > > > 
> > > > when maintainer doesn't respond for weeks, yeah! Sure it is.
> > > > 
> > > > > Perhaps it would be better to explain what each field of the
> > > > > configuration register does and then we can move on.
> > > > 
> > > > perhaps Jonathan could tell me exactly what he wants because I can't
> > > > handle back-and-forth anymore. Specially when he complains about stuff
> > > > he asked me to modify himself.
> > > >
> > > > > In particular the OPT3001_CONFIGURATION_L field needs to be explained
> > > > > such that the ABI can be properly applied.
> > > > > 
> > > > > Looking at the docs for the Windows demo program the field is associated
> > > > > with a latch configuration. What does this bit field actually do?
> > > 
> > > Still no technical information. Without all the facts how can you expect
> > > him to tell you what he wants?
> > 
> > yeah, because clearly he doesn't know himself, right ?
> 
> Could you explain how it works to me then?

checking how much of the docs I can expose now, gimme a couple days.

-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

Hi,

On Tue, Sep 30, 2014 at 04:22:04PM -0500, Felipe Balbi wrote:
> On Mon, Sep 29, 2014 at 06:41:59PM -0500, Michael Welling wrote:
> > On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
> > > > > > > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > > > > > > you apply it since maintainer has been ignoring this thread anyway ?
> > > > > > > 
> > > > > > 
> > > > > > Do you reall think this is the best way to approach this?
> > > > > 
> > > > > when maintainer doesn't respond for weeks, yeah! Sure it is.
> > > > > 
> > > > > > Perhaps it would be better to explain what each field of the
> > > > > > configuration register does and then we can move on.
> > > > > 
> > > > > perhaps Jonathan could tell me exactly what he wants because I can't
> > > > > handle back-and-forth anymore. Specially when he complains about stuff
> > > > > he asked me to modify himself.
> > > > >
> > > > > > In particular the OPT3001_CONFIGURATION_L field needs to be explained
> > > > > > such that the ABI can be properly applied.
> > > > > > 
> > > > > > Looking at the docs for the Windows demo program the field is associated
> > > > > > with a latch configuration. What does this bit field actually do?
> > > > 
> > > > Still no technical information. Without all the facts how can you expect
> > > > him to tell you what he wants?
> > > 
> > > yeah, because clearly he doesn't know himself, right ?
> > 
> > Could you explain how it works to me then?
> 
> checking how much of the docs I can expose now, gimme a couple days.

alright, here's a snippet of what's on preliminary docs:

Latch field (read or write).
The latch field controls the functionality of the interrupt reporting
mechanisms: the INT pin, the flag high field (FH), and flag low field (FL).
This bit selects the reporting style between a latched window-style comparison
and a transparent hysteresis-style comparison.

0 = The device functions in transparent hysteresis-style comparison operation,
where the three interrupt reporting mechanisms directly reflect the comparison
of the result register with the high- and low-limit registers with no
user-controlled clearing event. See the Interrupt Operation, INT Pin, and
Interrupt Reporting Mechanisms section for further details.

1 = The device functions in latched window-style comparison operation, latching
the interrupt reporting mechanisms until a user-controlled clearing event.

[...]

7.4.2.2 Transparent Hysteresis-Style Comparison Mode
The transparent hysteresis-style comparison mode is typically used when a
single digital signal is desired that indicates whether the input light is
higher than or lower than a light level of interest. If the result register is
higher than the high-limit register for a consecutive number of events set by
the fault count field, the INT line is set to active, the flag high field is
set to 1, and the flag low field is set to 0. If the result register is lower
than the lowlimit register for a consecutive number of events set by the fault
count field, the INT line is set to inactive, the flag low field is set to 1,
and the flag high field is set to 0. The INT pin and flag high and flag low
fields do not change state with configuration reads and writes. The INT pin and
flag fields continually report the appropriate comparison of the light to the
low-limit and high-limit registers. The device does not respond to the SMBus
alert response protocol while in either of the two transparent comparison modes
(configuration register, latch field = 0).

-- 
balbi

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

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Michael Welling]

On Thu, Oct 02, 2014 at 01:05:53PM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Sep 30, 2014 at 04:22:04PM -0500, Felipe Balbi wrote:
> > On Mon, Sep 29, 2014 at 06:41:59PM -0500, Michael Welling wrote:
> > > On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
> > > > Hi,
> > > > 
> > > > On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
> > > > > > > > Alright, this is already ridiculous. Andrew, if I resend the patch can
> > > > > > > > you apply it since maintainer has been ignoring this thread anyway ?
> > > > > > > > 
> > > > > > > 
> > > > > > > Do you reall think this is the best way to approach this?
> > > > > > 
> > > > > > when maintainer doesn't respond for weeks, yeah! Sure it is.
> > > > > > 
> > > > > > > Perhaps it would be better to explain what each field of the
> > > > > > > configuration register does and then we can move on.
> > > > > > 
> > > > > > perhaps Jonathan could tell me exactly what he wants because I can't
> > > > > > handle back-and-forth anymore. Specially when he complains about stuff
> > > > > > he asked me to modify himself.
> > > > > >
> > > > > > > In particular the OPT3001_CONFIGURATION_L field needs to be explained
> > > > > > > such that the ABI can be properly applied.
> > > > > > > 
> > > > > > > Looking at the docs for the Windows demo program the field is associated
> > > > > > > with a latch configuration. What does this bit field actually do?
> > > > > 
> > > > > Still no technical information. Without all the facts how can you expect
> > > > > him to tell you what he wants?
> > > > 
> > > > yeah, because clearly he doesn't know himself, right ?
> > > 
> > > Could you explain how it works to me then?
> > 
> > checking how much of the docs I can expose now, gimme a couple days.
> 
> alright, here's a snippet of what's on preliminary docs:
> 

Firstly, there is logical error in the latest code.
The hysteresis setting is 0 not 1.

+       if (opt->hysteresis)
+               reg |= OPT3001_CONFIGURATION_L;
+       else
+               reg &= ~OPT3001_CONFIGURATION_L;


> Latch field (read or write).
> The latch field controls the functionality of the interrupt reporting
> mechanisms: the INT pin, the flag high field (FH), and flag low field (FL).
> This bit selects the reporting style between a latched window-style comparison
> and a transparent hysteresis-style comparison.
> 
> 0 = The device functions in transparent hysteresis-style comparison operation,
> where the three interrupt reporting mechanisms directly reflect the comparison
> of the result register with the high- and low-limit registers with no
> user-controlled clearing event. See the Interrupt Operation, INT Pin, and
> Interrupt Reporting Mechanisms section for further details.
> 
> 1 = The device functions in latched window-style comparison operation, latching
> the interrupt reporting mechanisms until a user-controlled clearing event.
> 

How is the interrupt cleared in mode 1 in the latest version of your
code?

> [...]
> 
> 7.4.2.2 Transparent Hysteresis-Style Comparison Mode
> The transparent hysteresis-style comparison mode is typically used when a
> single digital signal is desired that indicates whether the input light is
> higher than or lower than a light level of interest. If the result register is
> higher than the high-limit register for a consecutive number of events set by
> the fault count field, the INT line is set to active, the flag high field is
> set to 1, and the flag low field is set to 0. If the result register is lower
> than the lowlimit register for a consecutive number of events set by the fault
> count field, the INT line is set to inactive, the flag low field is set to 1,
> and the flag high field is set to 0. The INT pin and flag high and flag low
> fields do not change state with configuration reads and writes. The INT pin and
> flag fields continually report the appropriate comparison of the light to the
> low-limit and high-limit registers. The device does not respond to the SMBus
> alert response protocol while in either of the two transparent comparison modes
> (configuration register, latch field = 0).
> 

The ABI confusion starts here.

You are dealing with a mode enable and IIO_EV_INFO_HYSTERESIS is associated
with a the hysteresis level of a threshold event.

http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L605

You are going to have abstract the register access to conform to the ABI or add
to the ABI as Jonathan suggests.

> -- 
> balbi

Re: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Jonathan Cameron]

On 04/10/14 04:17, Michael Welling wrote:
> On Thu, Oct 02, 2014 at 01:05:53PM -0500, Felipe Balbi wrote:
>> Hi,
>>
>> On Tue, Sep 30, 2014 at 04:22:04PM -0500, Felipe Balbi wrote:
>>> On Mon, Sep 29, 2014 at 06:41:59PM -0500, Michael Welling wrote:
>>>> On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
>>>>>>>>> Alright, this is already ridiculous. Andrew, if I resend the patch can
>>>>>>>>> you apply it since maintainer has been ignoring this thread anyway ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Do you reall think this is the best way to approach this?
>>>>>>>
>>>>>>> when maintainer doesn't respond for weeks, yeah! Sure it is.
>>>>>>>
>>>>>>>> Perhaps it would be better to explain what each field of the
>>>>>>>> configuration register does and then we can move on.
>>>>>>>
>>>>>>> perhaps Jonathan could tell me exactly what he wants because I can't
>>>>>>> handle back-and-forth anymore. Specially when he complains about stuff
>>>>>>> he asked me to modify himself.
>>>>>>>
>>>>>>>> In particular the OPT3001_CONFIGURATION_L field needs to be explained
>>>>>>>> such that the ABI can be properly applied.
>>>>>>>>
>>>>>>>> Looking at the docs for the Windows demo program the field is associated
>>>>>>>> with a latch configuration. What does this bit field actually do?
>>>>>>
>>>>>> Still no technical information. Without all the facts how can you expect
>>>>>> him to tell you what he wants?
>>>>>
>>>>> yeah, because clearly he doesn't know himself, right ?
>>>>
>>>> Could you explain how it works to me then?
>>>
>>> checking how much of the docs I can expose now, gimme a couple days.
>>
>> alright, here's a snippet of what's on preliminary docs:
>>
Firstly, thanks Felipe for going through the process to get permission
to release the snippets.  I know this can be a real pain to do.
> 
> Firstly, there is logical error in the latest code.
> The hysteresis setting is 0 not 1.
> 
> +       if (opt->hysteresis)
> +               reg |= OPT3001_CONFIGURATION_L;
> +       else
> +               reg &= ~OPT3001_CONFIGURATION_L;
> 
> 
>> Latch field (read or write).
>> The latch field controls the functionality of the interrupt reporting
>> mechanisms: the INT pin, the flag high field (FH), and flag low field (FL).
>> This bit selects the reporting style between a latched window-style comparison
>> and a transparent hysteresis-style comparison.
>>
>> 0 = The device functions in transparent hysteresis-style comparison operation,
>> where the three interrupt reporting mechanisms directly reflect the comparison
>> of the result register with the high- and low-limit registers with no
>> user-controlled clearing event. See the Interrupt Operation, INT Pin, and
>> Interrupt Reporting Mechanisms section for further details.
>>
>> 1 = The device functions in latched window-style comparison operation, latching
>> the interrupt reporting mechanisms until a user-controlled clearing event.
>>
> 
> How is the interrupt cleared in mode 1 in the latest version of your
> code?
> 
>> [...]
>>
>> 7.4.2.2 Transparent Hysteresis-Style Comparison Mode
>> The transparent hysteresis-style comparison mode is typically used when a
>> single digital signal is desired that indicates whether the input light is
>> higher than or lower than a light level of interest. If the result register is
>> higher than the high-limit register for a consecutive number of events set by
>> the fault count field, the INT line is set to active, the flag high field is
>> set to 1, and the flag low field is set to 0. If the result register is lower
>> than the lowlimit register for a consecutive number of events set by the fault
>> count field, the INT line is set to inactive, the flag low field is set to 1,
>> and the flag high field is set to 0. The INT pin and flag high and flag low
>> fields do not change state with configuration reads and writes. The INT pin and
>> flag fields continually report the appropriate comparison of the light to the
>> low-limit and high-limit registers. The device does not respond to the SMBus
>> alert response protocol while in either of the two transparent comparison modes
>> (configuration register, latch field = 0).
>>
> 
> The ABI confusion starts here.
> 
> You are dealing with a mode enable and IIO_EV_INFO_HYSTERESIS is associated
> with a the hysteresis level of a threshold event.
> 
> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L605
> 
> You are going to have abstract the register access to conform to the ABI or add
> to the ABI as Jonathan suggests.

Hysteresis in IIO is the equivalent of the action of a Schmitt trigger
Clearly there are other more complex forms of hysteresis, though a
purely temporal one is an unusual use of the term.

As I read this description what we have here corresponds to
*_period though that ABI requires the units to be seconds rather
than samples hence a conversion will be necessary.

I guess that the OPT3001_CONFIGURATION_FC_MASK relates to this
fault count? Hence your *_period controls IIO_EV_INFO_PERIOD
will presumably write that.

Michael has noted the other complexity above.  In non hysteresis
mode we have a conventional interrupt that can be cleared one
the driver has dealt with it. That simple interrupt corresponds
to a period of 0 (though that might also be a valid value for your
fault count field).

For these others cases that 'interrupt' line has become a simple
indicator of state. What state is it in if the value is between the
two thresholds?

Anyhow, you handle this by triggering on either rising or falling edge,
thus catching any change of state but not causing an interrupt storm.
This works in most cases but would it not miss a change of state from
initially between the thresholds to then being outside of them?
Perhaps I'm missing something here.

Thanks,

Jonathan



> 
>> -- 
>> balbi
> 
> 
> --
> 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: [RESEND PATCH] iio: light: add support for TI's opt3001 light sensor

[Felipe Balbi]

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

On Sat, Oct 04, 2014 at 10:43:57AM +0100, Jonathan Cameron wrote:
> On 04/10/14 04:17, Michael Welling wrote:
> > On Thu, Oct 02, 2014 at 01:05:53PM -0500, Felipe Balbi wrote:
> >> Hi,
> >>
> >> On Tue, Sep 30, 2014 at 04:22:04PM -0500, Felipe Balbi wrote:
> >>> On Mon, Sep 29, 2014 at 06:41:59PM -0500, Michael Welling wrote:
> >>>> On Mon, Sep 29, 2014 at 05:46:38PM -0500, Felipe Balbi wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Mon, Sep 29, 2014 at 05:38:33PM -0500, Michael Welling wrote:
> >>>>>>>>> Alright, this is already ridiculous. Andrew, if I resend the patch can
> >>>>>>>>> you apply it since maintainer has been ignoring this thread anyway ?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Do you reall think this is the best way to approach this?
> >>>>>>>
> >>>>>>> when maintainer doesn't respond for weeks, yeah! Sure it is.
> >>>>>>>
> >>>>>>>> Perhaps it would be better to explain what each field of the
> >>>>>>>> configuration register does and then we can move on.
> >>>>>>>
> >>>>>>> perhaps Jonathan could tell me exactly what he wants because I can't
> >>>>>>> handle back-and-forth anymore. Specially when he complains about stuff
> >>>>>>> he asked me to modify himself.
> >>>>>>>
> >>>>>>>> In particular the OPT3001_CONFIGURATION_L field needs to be explained
> >>>>>>>> such that the ABI can be properly applied.
> >>>>>>>>
> >>>>>>>> Looking at the docs for the Windows demo program the field is associated
> >>>>>>>> with a latch configuration. What does this bit field actually do?
> >>>>>>
> >>>>>> Still no technical information. Without all the facts how can you expect
> >>>>>> him to tell you what he wants?
> >>>>>
> >>>>> yeah, because clearly he doesn't know himself, right ?
> >>>>
> >>>> Could you explain how it works to me then?
> >>>
> >>> checking how much of the docs I can expose now, gimme a couple days.
> >>
> >> alright, here's a snippet of what's on preliminary docs:
> >>
> Firstly, thanks Felipe for going through the process to get permission
> to release the snippets.  I know this can be a real pain to do.

alright, np. Device will be launched soonish, so full datasheet should
be in TI's website in the coming weeks or so.

I'll wait until v3.18-rc1 is tagged before working on this again.

-- 
balbi

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