[PATCH v1 0/3] Added LTR501 Interrupt support

[PATCH v1 0/3] Added LTR501 Interrupt support

[Kuppuswamy Sathyanarayanan]

This patchset adds interrupt support for LTR501 chip. Also
added acpi enumaration support.

Please let me know your review comments.

v1:
1. Added support to enable ALS & PS intterupts based 
   on threshold settings.
2. Added support to control intrrupt rate.
3. Added acpi enumeration support.

Kuppuswamy Sathyanarayanan (3):
  iio: ltr501: Add interrupt support
  iio: ltr501: Add interrupt rate control support
  iio: ltr501: Add ACPI enumeration support

 drivers/iio/light/ltr501.c | 395 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 392 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v1 1/3] iio: ltr501: Add interrupt support

[Kuppuswamy Sathyanarayanan]

This patch adds interrupt support for Liteon 501 chip.

Interrupt will be generated whenever ALS or proximity
data exceeds values given in upper and lower threshold
register settings.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/iio/light/ltr501.c | 302 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 299 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 62b7072..16c3f7a 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -9,7 +9,7 @@
  *
  * 7-bit I2C slave address 0x23
  *
- * TODO: interrupt, threshold, measurement rate, IR LED characteristics
+ * TODO: measurement rate, IR LED characteristics
  */
 
 #include <linux/module.h>
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 
 #include <linux/iio/iio.h>
+#include <linux/iio/events.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
@@ -33,6 +34,11 @@
 #define LTR501_ALS_DATA0 0x8a /* 16-bit, little endian */
 #define LTR501_ALS_PS_STATUS 0x8c
 #define LTR501_PS_DATA 0x8d /* 16-bit, little endian */
+#define LTR501_INTR 0x8f /* output mode, polarity, mode */
+#define LTR501_PS_THRESH_UP 0x90 /* 11 bit, ps upper threshold */
+#define LTR501_PS_THRESH_LOW 0x92 /* 11 bit, ps lower threshold */
+#define LTR501_ALS_THRESH_UP 0x97 /* 16 bit, ALS upper threshold */
+#define LTR501_ALS_THRESH_LOW 0x99 /* 16 bit, ALS lower threshold */
 
 #define LTR501_ALS_CONTR_SW_RESET BIT(2)
 #define LTR501_CONTR_PS_GAIN_MASK (BIT(3) | BIT(2))
@@ -40,10 +46,28 @@
 #define LTR501_CONTR_ALS_GAIN_MASK BIT(3)
 #define LTR501_CONTR_ACTIVE BIT(1)
 
+#define LTR501_STATUS_ALS_INTR BIT(3)
 #define LTR501_STATUS_ALS_RDY BIT(2)
+#define LTR501_STATUS_PS_INTR BIT(1)
 #define LTR501_STATUS_PS_RDY BIT(0)
 
+#define LTR501_INTR_OUTPUT_MODE_MASK BIT(3)
+#define LTR501_INTR_OUTPUT_MODE_LATCHED ~BIT(3)
+#define LTR501_INTR_OUTPUT_MODE_DIRECT BIT(3)
+#define LTR501_INTR_POLARITY_MASK BIT(2)
+#define LTR501_INTR_POLARITY_LOGIC_0 ~BIT(2)
+#define LTR501_INTR_POLARITY_LOGIC_1 BIT(2)
+#define LTR501_INTR_MODE_MASK (BIT(1) | BIT(0))
+#define LTR501_INTR_MODE_NONE 0
+#define LTR501_INTR_MODE_PS_MASK BIT(0)
+#define LTR501_INTR_MODE_PS BIT(0)
+#define LTR501_INTR_MODE_ALS_MASK BIT(1)
+#define LTR501_INTR_MODE_ALS BIT(1)
+#define LTR501_INTR_MODE_ALS_PS 3
+
 #define LTR501_PS_DATA_MASK 0x7ff
+#define LTR501_PS_THRESH_MASK 0x7ff
+#define LTR501_ALS_THRESH_MASK 0xffff
 
 struct ltr501_data {
 	struct i2c_client *client;
@@ -70,6 +94,20 @@ static int ltr501_drdy(struct ltr501_data *data, u8 drdy_mask)
 	return -EIO;
 }
 
+static int ltr501_set_intr_reg(struct ltr501_data *data, u8 mask, u8 val)
+{
+	int ret;
+	u8 new_val;
+
+	ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR);
+	if (ret < 0)
+		return ret;
+
+	new_val = (ret & ~mask) | (val & mask);
+
+	return i2c_smbus_write_byte_data(data->client, LTR501_INTR, new_val);
+}
+
 static int ltr501_read_als(struct ltr501_data *data, __le16 buf[2])
 {
 	int ret = ltr501_drdy(data, LTR501_STATUS_ALS_RDY);
@@ -88,6 +126,43 @@ static int ltr501_read_ps(struct ltr501_data *data)
 	return i2c_smbus_read_word_data(data->client, LTR501_PS_DATA);
 }
 
+static const struct iio_event_spec ltr501_als_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+
+};
+
+static const struct iio_event_spec ltr501_pxs_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 #define LTR501_INTENSITY_CHANNEL(_idx, _addr, _mod, _shared) { \
 	.type = IIO_INTENSITY, \
 	.modified = 1, \
@@ -101,7 +176,9 @@ static int ltr501_read_ps(struct ltr501_data *data)
 		.realbits = 16, \
 		.storagebits = 16, \
 		.endianness = IIO_CPU, \
-	} \
+	}, \
+	.event_spec = ltr501_als_event_spec,\
+	.num_event_specs = ARRAY_SIZE(ltr501_als_event_spec),\
 }
 
 static const struct iio_chan_spec ltr501_channels[] = {
@@ -120,6 +197,8 @@ static const struct iio_chan_spec ltr501_channels[] = {
 			.storagebits = 16,
 			.endianness = IIO_CPU,
 		},
+		.event_spec = ltr501_pxs_event_spec,
+		.num_event_specs = ARRAY_SIZE(ltr501_pxs_event_spec),
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
@@ -235,6 +314,167 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
+static int ltr501_read_thresh(struct iio_dev *indio_dev,
+		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 ltr501_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (chan->type) {
+	case IIO_INTENSITY:
+		mutex_lock(&data->lock_als);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = i2c_smbus_read_word_data(data->client,
+						       LTR501_ALS_THRESH_UP);
+			break;
+		case IIO_EV_DIR_FALLING:
+			ret = i2c_smbus_read_word_data(data->client,
+						       LTR501_ALS_THRESH_LOW);
+			break;
+		default:
+			break;
+		}
+		mutex_unlock(&data->lock_als);
+		*val = ret & LTR501_ALS_THRESH_MASK;
+		break;
+	case IIO_PROXIMITY:
+		mutex_lock(&data->lock_ps);
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = i2c_smbus_read_word_data(data->client,
+						       LTR501_PS_THRESH_UP);
+			break;
+		case IIO_EV_DIR_FALLING:
+			ret = i2c_smbus_read_word_data(data->client,
+						       LTR501_PS_THRESH_LOW);
+			break;
+		default:
+			break;
+		}
+		mutex_unlock(&data->lock_ps);
+		*val = ret & LTR501_PS_THRESH_MASK;
+		break;
+	default:
+		break;
+	}
+
+	return ret < 0 ? ret : IIO_VAL_INT;
+}
+
+static int ltr501_write_thresh(struct iio_dev *indio_dev,
+		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 ltr501_data *data = iio_priv(indio_dev);
+	u16 new_val = 0;
+	int ret = -EINVAL;
+
+	switch (chan->type) {
+	case IIO_INTENSITY:
+		mutex_lock(&data->lock_als);
+		new_val = val & LTR501_ALS_THRESH_MASK;
+		switch (dir) {
+		case IIO_EV_DIR_RISING:
+			ret = i2c_smbus_write_word_data(data->client,
+							LTR501_ALS_THRESH_UP,
+							new_val);
+			break;
+		case IIO_EV_DIR_FALLING:
+			ret = i2c_smbus_write_word_data(data->client,
+							LTR501_ALS_THRESH_LOW,
+							new_val);
+			break;
+		default:
+			break;
+		}
+		mutex_unlock(&data->lock_als);
+		break;
+	case IIO_PROXIMITY:
+		switch (dir) {
+		mutex_lock(&data->lock_ps);
+		new_val = val & LTR501_PS_THRESH_MASK;
+		case IIO_EV_DIR_RISING:
+			ret = i2c_smbus_write_word_data(data->client,
+							LTR501_PS_THRESH_UP,
+							new_val);
+			break;
+		case IIO_EV_DIR_FALLING:
+			ret = i2c_smbus_write_word_data(data->client,
+							LTR501_PS_THRESH_LOW,
+							new_val);
+			break;
+		default:
+			break;
+		}
+		mutex_unlock(&data->lock_ps);
+		break;
+
+	default:
+		break;
+	}
+
+	return ret < 0 ? ret : IIO_VAL_INT;
+}
+
+static int ltr501_read_event_config(struct iio_dev *indio_dev,
+		const struct iio_chan_spec *chan,
+		enum iio_event_type type,
+		enum iio_event_direction dir)
+{
+	struct ltr501_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (chan->type) {
+	case IIO_INTENSITY:
+		mutex_lock(&data->lock_als);
+		ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR);
+		mutex_unlock(&data->lock_als);
+		return ret < 0 ? ret : ret & LTR501_INTR_MODE_ALS_MASK;
+	case IIO_PROXIMITY:
+		mutex_lock(&data->lock_ps);
+		ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR);
+		mutex_unlock(&data->lock_ps);
+		return ret < 0 ? ret : ret & LTR501_INTR_MODE_PS_MASK;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int ltr501_write_event_config(struct iio_dev *indio_dev,
+		const struct iio_chan_spec *chan, enum iio_event_type type,
+		enum iio_event_direction dir, int state)
+{
+	struct ltr501_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (chan->type) {
+	case IIO_INTENSITY:
+		mutex_lock(&data->lock_als);
+		ret = ltr501_set_intr_reg(data, LTR501_INTR_MODE_ALS_MASK,
+					  (state ? LTR501_INTR_MODE_ALS :
+					  LTR501_INTR_MODE_NONE));
+		mutex_unlock(&data->lock_als);
+		break;
+	case IIO_PROXIMITY:
+		mutex_lock(&data->lock_ps);
+		ret = ltr501_set_intr_reg(data, LTR501_INTR_MODE_PS_MASK,
+					  (state ? LTR501_INTR_MODE_PS :
+					  LTR501_INTR_MODE_NONE));
+		mutex_unlock(&data->lock_ps);
+		break;
+	default:
+		break;
+	}
+
+	return ret < 0 ? ret : IIO_VAL_INT;
+}
+
 static IIO_CONST_ATTR(in_proximity_scale_available, "1 0.25 0.125 0.0625");
 static IIO_CONST_ATTR(in_intensity_scale_available, "1 0.005");
 
@@ -248,10 +488,21 @@ static const struct attribute_group ltr501_attribute_group = {
 	.attrs = ltr501_attributes,
 };
 
+static const struct iio_info ltr501_info_no_irq = {
+	.read_raw = ltr501_read_raw,
+	.write_raw = ltr501_write_raw,
+	.attrs = &ltr501_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
 static const struct iio_info ltr501_info = {
 	.read_raw = ltr501_read_raw,
 	.write_raw = ltr501_write_raw,
 	.attrs = &ltr501_attribute_group,
+	.read_event_value	= &ltr501_read_thresh,
+	.write_event_value	= &ltr501_write_thresh,
+	.read_event_config	= &ltr501_read_event_config,
+	.write_event_config	= &ltr501_write_event_config,
 	.driver_module = THIS_MODULE,
 };
 
@@ -315,6 +566,36 @@ done:
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t ltr501_interrupt_handler(int irq, void *private)
+{
+	struct iio_dev *dev_info = private;
+	struct ltr501_data *data = iio_priv(dev_info);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, LTR501_ALS_PS_STATUS);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"irq read int reg failed\n");
+		return IRQ_HANDLED;
+	}
+
+	if (ret & LTR501_STATUS_ALS_INTR)
+		iio_push_event(dev_info,
+			       IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
+			       IIO_EV_TYPE_THRESH,
+			       IIO_EV_DIR_EITHER),
+			       iio_get_time_ns());
+
+	if (ret & LTR501_STATUS_PS_INTR)
+		iio_push_event(dev_info,
+			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+			       IIO_EV_TYPE_THRESH,
+			       IIO_EV_DIR_EITHER),
+			       iio_get_time_ns());
+
+	return IRQ_HANDLED;
+}
+
 static int ltr501_init(struct ltr501_data *data)
 {
 	int ret;
@@ -357,7 +638,6 @@ static int ltr501_probe(struct i2c_client *client,
 		return -ENODEV;
 
 	indio_dev->dev.parent = &client->dev;
-	indio_dev->info = &ltr501_info;
 	indio_dev->channels = ltr501_channels;
 	indio_dev->num_channels = ARRAY_SIZE(ltr501_channels);
 	indio_dev->name = LTR501_DRV_NAME;
@@ -367,6 +647,22 @@ static int ltr501_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	if (client->irq > 0) {
+		indio_dev->info = &ltr501_info;
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						NULL, ltr501_interrupt_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						"ltr501_thresh_event",
+						indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "request irq (%d) failed\n",
+					client->irq);
+			return ret;
+		}
+	} else
+		indio_dev->info = &ltr501_info_no_irq;
+
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		ltr501_trigger_handler, NULL);
 	if (ret)
-- 
1.9.1

[PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Kuppuswamy Sathyanarayanan]

Added rate control support for ALS and proximity
threshold interrupts.

Setting <n> to ALS intr_persist sysfs node would
generate interrupt whenever ALS data cross either
upper or lower threshold limits <n> number of times.
Similarly setting <m> to proximity intr_persist sysfs
node would genere interrupt whenever proximity data falls
outside threshold limit <m> number of times.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/iio/light/ltr501.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 16c3f7a..e8f5019 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -39,6 +39,7 @@
 #define LTR501_PS_THRESH_LOW 0x92 /* 11 bit, ps lower threshold */
 #define LTR501_ALS_THRESH_UP 0x97 /* 16 bit, ALS upper threshold */
 #define LTR501_ALS_THRESH_LOW 0x99 /* 16 bit, ALS lower threshold */
+#define LTR501_INTR_PRST 0x9e /* ps thresh, als thresh */
 
 #define LTR501_ALS_CONTR_SW_RESET BIT(2)
 #define LTR501_CONTR_PS_GAIN_MASK (BIT(3) | BIT(2))
@@ -65,6 +66,9 @@
 #define LTR501_INTR_MODE_ALS BIT(1)
 #define LTR501_INTR_MODE_ALS_PS 3
 
+#define LTR501_INTR_PRST_ALS_MASK 0x0f
+#define LTR501_INTR_PRST_PS_MASK 0xf0
+
 #define LTR501_PS_DATA_MASK 0x7ff
 #define LTR501_PS_THRESH_MASK 0x7ff
 #define LTR501_ALS_THRESH_MASK 0xffff
@@ -126,6 +130,75 @@ static int ltr501_read_ps(struct ltr501_data *data)
 	return i2c_smbus_read_word_data(data->client, LTR501_PS_DATA);
 }
 
+static ssize_t ltr501_read_intr_prst(struct iio_dev *indio_dev,
+				   uintptr_t private,
+				   const struct iio_chan_spec *chan,
+				   char *buf)
+{
+	struct ltr501_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (chan->type) {
+	case IIO_INTENSITY:
+		mutex_lock(&data->lock_als);
+		ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
+		mutex_unlock(&data->lock_als);
+		return ret < 0 ? ret : sprintf(buf, "%u\n",
+				ret & LTR501_INTR_PRST_ALS_MASK);
+	case IIO_PROXIMITY:
+		mutex_lock(&data->lock_ps);
+		ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
+		mutex_unlock(&data->lock_ps);
+		return ret < 0 ? ret : sprintf(buf, "%u\n",
+				(ret & LTR501_INTR_PRST_PS_MASK) >> 4);
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static ssize_t ltr501_write_intr_prst(struct iio_dev *indio_dev,
+				    uintptr_t private,
+				    const struct iio_chan_spec *chan,
+				    const char *buf, size_t len)
+{
+	struct ltr501_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+	u8 new_val;
+
+	ret = kstrtou8(buf, 10, &new_val);
+	if (ret)
+		return ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
+	if (ret < 0)
+		return ret;
+
+	switch (chan->type) {
+	case IIO_INTENSITY:
+		mutex_lock(&data->lock_als);
+		new_val = (ret & ~LTR501_INTR_PRST_ALS_MASK) |
+				(new_val & LTR501_INTR_PRST_ALS_MASK);
+		ret = i2c_smbus_write_byte_data(data->client,
+					LTR501_INTR_PRST, new_val);
+		mutex_unlock(&data->lock_als);
+		break;
+	case IIO_PROXIMITY:
+		mutex_lock(&data->lock_ps);
+		new_val = (ret & ~LTR501_INTR_PRST_PS_MASK) |
+				((new_val << 4) & LTR501_INTR_PRST_PS_MASK);
+		ret = i2c_smbus_write_byte_data(data->client,
+					LTR501_INTR_PRST, new_val);
+		mutex_unlock(&data->lock_ps);
+		break;
+	default:
+		break;
+	}
+
+	return ret < 0 ? ret : len;
+}
+
 static const struct iio_event_spec ltr501_als_event_spec[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -163,6 +236,16 @@ static const struct iio_event_spec ltr501_pxs_event_spec[] = {
 	},
 };
 
+static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
+	{
+		.name = "intr_persist",
+		.read = ltr501_read_intr_prst,
+		.write = ltr501_write_intr_prst,
+		.shared = IIO_SHARED_BY_TYPE,
+	},
+	{},
+};
+
 #define LTR501_INTENSITY_CHANNEL(_idx, _addr, _mod, _shared) { \
 	.type = IIO_INTENSITY, \
 	.modified = 1, \
@@ -179,6 +262,7 @@ static const struct iio_event_spec ltr501_pxs_event_spec[] = {
 	}, \
 	.event_spec = ltr501_als_event_spec,\
 	.num_event_specs = ARRAY_SIZE(ltr501_als_event_spec),\
+	.ext_info = ltr501_ext_info,\
 }
 
 static const struct iio_chan_spec ltr501_channels[] = {
@@ -199,6 +283,7 @@ static const struct iio_chan_spec ltr501_channels[] = {
 		},
 		.event_spec = ltr501_pxs_event_spec,
 		.num_event_specs = ARRAY_SIZE(ltr501_pxs_event_spec),
+		.ext_info = ltr501_ext_info,
 	},
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
-- 
1.9.1

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Daniel Baluta]

On Wed, Apr 1, 2015 at 5:55 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> Added rate control support for ALS and proximity
> threshold interrupts.
>
> Setting <n> to ALS intr_persist sysfs node would
> generate interrupt whenever ALS data cross either
> upper or lower threshold limits <n> number of times.
> Similarly setting <m> to proximity intr_persist sysfs
> node would genere interrupt whenever proximity data falls
> outside threshold limit <m> number of times.
>

<snip>

> +static ssize_t ltr501_read_intr_prst(struct iio_dev *indio_dev,
> +                                  uintptr_t private,
> +                                  const struct iio_chan_spec *chan,
> +                                  char *buf)
> +{
> +       struct ltr501_data *data = iio_priv(indio_dev);
> +       int ret = -EINVAL;
> +
> +       switch (chan->type) {
> +       case IIO_INTENSITY:
> +               mutex_lock(&data->lock_als);
> +               ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
> +               mutex_unlock(&data->lock_als);

I am not sure I understand why is the mutex needed here? AFAIK I2C transactions
are serialized.

<snip>

> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
> +       {
> +               .name = "intr_persist",
> +               .read = ltr501_read_intr_prst,
> +               .write = ltr501_write_intr_prst,
> +               .shared = IIO_SHARED_BY_TYPE,
> +       },
> +       {},
> +};
> +
Would be nice to standardize persistence attribute (IIO_CHAN_INFO_PERSISTENCE).

thanks,
Daniel.

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Lars-Peter Clausen]

On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
>
>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>> +       {
>> +               .name = "intr_persist",
>> +               .read = ltr501_read_intr_prst,
>> +               .write = ltr501_write_intr_prst,
>> +               .shared = IIO_SHARED_BY_TYPE,
>> +       },
>> +       {},
>> +};
>> +
> Would be nice to standardize persistence attribute (IIO_CHAN_INFO_PERSISTENCE).

If I understand the behavior correctly it causes that the event needs to be 
triggered at least n times before the event is reported by the chip. In my 
opinion 'persistence' is not a good term for that. I'm not sure what a 
better term is but I think it should go more in the direction of ratelimit 
or something.

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Daniel Baluta]

On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
> [...]
>>
>>
>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>> +       {
>>> +               .name = "intr_persist",
>>> +               .read = ltr501_read_intr_prst,
>>> +               .write = ltr501_write_intr_prst,
>>> +               .shared = IIO_SHARED_BY_TYPE,
>>> +       },
>>> +       {},
>>> +};
>>> +
>>
>> Would be nice to standardize persistence attribute
>> (IIO_CHAN_INFO_PERSISTENCE).
>
>
> If I understand the behavior correctly it causes that the event needs to be
> triggered at least n times before the event is reported by the chip. In my
> opinion 'persistence' is not a good term for that. I'm not sure what a
> better term is but I think it should go more in the direction of ratelimit
> or something.

I've seen this term used for many devices:

*  TSL25911 ambient light sensor [1]

[ One set of thresholds can be configured to trigger an interrupt only when
 the ambient light exceeds them for a configurable amount of time (persistence)
]

* TAOS TCS34725 ambient light sensor [2]
[
The interrupt persistence filter allows the user to define the number
of consecutive
 out-of-threshold events necessary before generating an interrupt.
]

* Avago SAPDS-9950, Sensortek STK3310

I think the TSL25911 datasheet best describes this parameter, as the
amount of time
that ambient light should exceed a threshold until an interrupt is generated.

thanks,
Daniel.

[1] http://www.adafruit.com/datasheets/TSL25911_Datasheet_EN_v1.pdf
[2] http://www.adafruit.com/datasheets/TCS34725.pdf

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Lars-Peter Clausen]

On 04/01/2015 05:02 PM, Daniel Baluta wrote:
> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>> [...]
>>>
>>>
>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>> +       {
>>>> +               .name = "intr_persist",
>>>> +               .read = ltr501_read_intr_prst,
>>>> +               .write = ltr501_write_intr_prst,
>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>> +       },
>>>> +       {},
>>>> +};
>>>> +
>>>
>>> Would be nice to standardize persistence attribute
>>> (IIO_CHAN_INFO_PERSISTENCE).
>>
>>
>> If I understand the behavior correctly it causes that the event needs to be
>> triggered at least n times before the event is reported by the chip. In my
>> opinion 'persistence' is not a good term for that. I'm not sure what a
>> better term is but I think it should go more in the direction of ratelimit
>> or something.
>
> I've seen this term used for many devices:
>
> *  TSL25911 ambient light sensor [1]
>
> [ One set of thresholds can be configured to trigger an interrupt only when
>   the ambient light exceeds them for a configurable amount of time (persistence)
> ]
>
> * TAOS TCS34725 ambient light sensor [2]
> [
> The interrupt persistence filter allows the user to define the number
> of consecutive
>   out-of-threshold events necessary before generating an interrupt.
> ]
>
> * Avago SAPDS-9950, Sensortek STK3310
>
> I think the TSL25911 datasheet best describes this parameter, as the
> amount of time
> that ambient light should exceed a threshold until an interrupt is generated.

Ok, that makes more sense. I misunderstood the initial description as that 
the signal would have to go first above the threshold then below the 
threshold, and this for a number of times. Whereas it needs to exceed the 
threshold for a certain amount of time before the event is triggered. If it 
goes below the threshold before the persistence interval no event is 
triggered and the counter is reset.

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[sathyanarayanan kuppuswamy]

On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de> 
>> wrote:
>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>> [...]
>>>>
>>>>
>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>> +       {
>>>>> +               .name = "intr_persist",
>>>>> +               .read = ltr501_read_intr_prst,
>>>>> +               .write = ltr501_write_intr_prst,
>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>> +       },
>>>>> +       {},
>>>>> +};
>>>>> +
>>>>
>>>> Would be nice to standardize persistence attribute
>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>
>>>
>>> If I understand the behavior correctly it causes that the event 
>>> needs to be
>>> triggered at least n times before the event is reported by the chip. 
>>> In my
>>> opinion 'persistence' is not a good term for that. I'm not sure what a
>>> better term is but I think it should go more in the direction of 
>>> ratelimit
>>> or something.
>>
>> I've seen this term used for many devices:
>>
>> *  TSL25911 ambient light sensor [1]
>>
>> [ One set of thresholds can be configured to trigger an interrupt 
>> only when
>>   the ambient light exceeds them for a configurable amount of time 
>> (persistence)
>> ]
>>
>> * TAOS TCS34725 ambient light sensor [2]
>> [
>> The interrupt persistence filter allows the user to define the number
>> of consecutive
>>   out-of-threshold events necessary before generating an interrupt.
>> ]
>>
>> * Avago SAPDS-9950, Sensortek STK3310
>>
>> I think the TSL25911 datasheet best describes this parameter, as the
>> amount of time
>> that ambient light should exceed a threshold until an interrupt is 
>> generated.
>
> Ok, that makes more sense. I misunderstood the initial description as 
> that the signal would have to go first above the threshold then below 
> the threshold, and this for a number of times. Whereas it needs to 
> exceed the threshold for a certain amount of time before the event is 
> triggered. If it goes below the threshold before the persistence 
> interval no event is triggered and the counter is reset.

Yes, it needs to cross the threshold n number of times before a event is 
generated.
>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Lars-Peter Clausen]

On 04/01/2015 07:45 PM, sathyanarayanan kuppuswamy wrote:
>
>
> On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
>> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>>> [...]
>>>>>
>>>>>
>>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>>> +       {
>>>>>> +               .name = "intr_persist",
>>>>>> +               .read = ltr501_read_intr_prst,
>>>>>> +               .write = ltr501_write_intr_prst,
>>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>>> +       },
>>>>>> +       {},
>>>>>> +};
>>>>>> +
>>>>>
>>>>> Would be nice to standardize persistence attribute
>>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>>
>>>>
>>>> If I understand the behavior correctly it causes that the event needs to be
>>>> triggered at least n times before the event is reported by the chip. In my
>>>> opinion 'persistence' is not a good term for that. I'm not sure what a
>>>> better term is but I think it should go more in the direction of ratelimit
>>>> or something.
>>>
>>> I've seen this term used for many devices:
>>>
>>> *  TSL25911 ambient light sensor [1]
>>>
>>> [ One set of thresholds can be configured to trigger an interrupt only when
>>>   the ambient light exceeds them for a configurable amount of time
>>> (persistence)
>>> ]
>>>
>>> * TAOS TCS34725 ambient light sensor [2]
>>> [
>>> The interrupt persistence filter allows the user to define the number
>>> of consecutive
>>>   out-of-threshold events necessary before generating an interrupt.
>>> ]
>>>
>>> * Avago SAPDS-9950, Sensortek STK3310
>>>
>>> I think the TSL25911 datasheet best describes this parameter, as the
>>> amount of time
>>> that ambient light should exceed a threshold until an interrupt is
>>> generated.
>>
>> Ok, that makes more sense. I misunderstood the initial description as that
>> the signal would have to go first above the threshold then below the
>> threshold, and this for a number of times. Whereas it needs to exceed the
>> threshold for a certain amount of time before the event is triggered. If
>> it goes below the threshold before the persistence interval no event is
>> triggered and the counter is reset.
>
> Yes, it needs to cross the threshold n number of times before a event is
> generated.

Wait. It needs to cross the threshold or it needs to stay above the threshold?

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[sathyanarayanan kuppuswamy]

On 04/01/2015 10:48 AM, Lars-Peter Clausen wrote:
> On 04/01/2015 07:45 PM, sathyanarayanan kuppuswamy wrote:
>>
>>
>> On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
>>> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>>>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen 
>>>> <lars@metafoo.de> wrote:
>>>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>>>> [...]
>>>>>>
>>>>>>
>>>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>>>> +       {
>>>>>>> +               .name = "intr_persist",
>>>>>>> +               .read = ltr501_read_intr_prst,
>>>>>>> +               .write = ltr501_write_intr_prst,
>>>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>>>> +       },
>>>>>>> +       {},
>>>>>>> +};
>>>>>>> +
>>>>>>
>>>>>> Would be nice to standardize persistence attribute
>>>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>>>
>>>>>
>>>>> If I understand the behavior correctly it causes that the event 
>>>>> needs to be
>>>>> triggered at least n times before the event is reported by the 
>>>>> chip. In my
>>>>> opinion 'persistence' is not a good term for that. I'm not sure 
>>>>> what a
>>>>> better term is but I think it should go more in the direction of 
>>>>> ratelimit
>>>>> or something.
>>>>
>>>> I've seen this term used for many devices:
>>>>
>>>> *  TSL25911 ambient light sensor [1]
>>>>
>>>> [ One set of thresholds can be configured to trigger an interrupt 
>>>> only when
>>>>   the ambient light exceeds them for a configurable amount of time
>>>> (persistence)
>>>> ]
>>>>
>>>> * TAOS TCS34725 ambient light sensor [2]
>>>> [
>>>> The interrupt persistence filter allows the user to define the number
>>>> of consecutive
>>>>   out-of-threshold events necessary before generating an interrupt.
>>>> ]
>>>>
>>>> * Avago SAPDS-9950, Sensortek STK3310
>>>>
>>>> I think the TSL25911 datasheet best describes this parameter, as the
>>>> amount of time
>>>> that ambient light should exceed a threshold until an interrupt is
>>>> generated.
>>>
>>> Ok, that makes more sense. I misunderstood the initial description 
>>> as that
>>> the signal would have to go first above the threshold then below the
>>> threshold, and this for a number of times. Whereas it needs to 
>>> exceed the
>>> threshold for a certain amount of time before the event is 
>>> triggered. If
>>> it goes below the threshold before the persistence interval no event is
>>> triggered and the counter is reset.
>>
>> Yes, it needs to cross the threshold n number of times before a event is
>> generated.
>
> Wait. It needs to cross the threshold or it needs to stay above the 
> threshold?
Following is the logic for this chip.

If ( data > Upper_threshold or data  < Lower_threshold)
     generate_event()
>
> -- 
> 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
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[sathyanarayanan kuppuswamy]

On 04/01/2015 12:06 PM, sathyanarayanan kuppuswamy wrote:
>
>
> On 04/01/2015 10:48 AM, Lars-Peter Clausen wrote:
>> On 04/01/2015 07:45 PM, sathyanarayanan kuppuswamy wrote:
>>>
>>>
>>> On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
>>>> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>>>>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen 
>>>>> <lars@metafoo.de> wrote:
>>>>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>>>>> [...]
>>>>>>>
>>>>>>>
>>>>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>>>>> +       {
>>>>>>>> +               .name = "intr_persist",
>>>>>>>> +               .read = ltr501_read_intr_prst,
>>>>>>>> +               .write = ltr501_write_intr_prst,
>>>>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>>>>> +       },
>>>>>>>> +       {},
>>>>>>>> +};
>>>>>>>> +
>>>>>>>
>>>>>>> Would be nice to standardize persistence attribute
>>>>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>>>>
>>>>>>
>>>>>> If I understand the behavior correctly it causes that the event 
>>>>>> needs to be
>>>>>> triggered at least n times before the event is reported by the 
>>>>>> chip. In my
>>>>>> opinion 'persistence' is not a good term for that. I'm not sure 
>>>>>> what a
>>>>>> better term is but I think it should go more in the direction of 
>>>>>> ratelimit
>>>>>> or something.
>>>>>
>>>>> I've seen this term used for many devices:
>>>>>
>>>>> *  TSL25911 ambient light sensor [1]
>>>>>
>>>>> [ One set of thresholds can be configured to trigger an interrupt 
>>>>> only when
>>>>>   the ambient light exceeds them for a configurable amount of time
>>>>> (persistence)
>>>>> ]
>>>>>
>>>>> * TAOS TCS34725 ambient light sensor [2]
>>>>> [
>>>>> The interrupt persistence filter allows the user to define the number
>>>>> of consecutive
>>>>>   out-of-threshold events necessary before generating an interrupt.
>>>>> ]
>>>>>
>>>>> * Avago SAPDS-9950, Sensortek STK3310
>>>>>
>>>>> I think the TSL25911 datasheet best describes this parameter, as the
>>>>> amount of time
>>>>> that ambient light should exceed a threshold until an interrupt is
>>>>> generated.
>>>>
>>>> Ok, that makes more sense. I misunderstood the initial description 
>>>> as that
>>>> the signal would have to go first above the threshold then below the
>>>> threshold, and this for a number of times. Whereas it needs to 
>>>> exceed the
>>>> threshold for a certain amount of time before the event is 
>>>> triggered. If
>>>> it goes below the threshold before the persistence interval no 
>>>> event is
>>>> triggered and the counter is reset.
>>>
>>> Yes, it needs to cross the threshold n number of times before a 
>>> event is
>>> generated.
>>
>> Wait. It needs to cross the threshold or it needs to stay above the 
>> threshold?
> Following is the logic for this chip.
>
> If ( data > Upper_threshold or data  < Lower_threshold)
>     generate_event()

Missed to add <n> times logic

If ( data > Upper_threshold or data  < Lower_threshold) {
     increment_count;
     if (count >= n) {
         generate_event()
         reset_count()
     }
}

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

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Jonathan Cameron]

On 01/04/15 20:22, sathyanarayanan kuppuswamy wrote:
> 
> 
> On 04/01/2015 12:06 PM, sathyanarayanan kuppuswamy wrote:
>>
>>
>> On 04/01/2015 10:48 AM, Lars-Peter Clausen wrote:
>>> On 04/01/2015 07:45 PM, sathyanarayanan kuppuswamy wrote:
>>>>
>>>>
>>>> On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
>>>>> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>>>>>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>>>>>> [...]
>>>>>>>>
>>>>>>>>
>>>>>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>>>>>> +       {
>>>>>>>>> +               .name = "intr_persist",
>>>>>>>>> +               .read = ltr501_read_intr_prst,
>>>>>>>>> +               .write = ltr501_write_intr_prst,
>>>>>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>>>>>> +       },
>>>>>>>>> +       {},
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Would be nice to standardize persistence attribute
>>>>>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>>>>>
>>>>>>>
>>>>>>> If I understand the behavior correctly it causes that the event needs to be
>>>>>>> triggered at least n times before the event is reported by the chip. In my
>>>>>>> opinion 'persistence' is not a good term for that. I'm not sure what a
>>>>>>> better term is but I think it should go more in the direction of ratelimit
>>>>>>> or something.
>>>>>>
>>>>>> I've seen this term used for many devices:
>>>>>>
>>>>>> *  TSL25911 ambient light sensor [1]
>>>>>>
>>>>>> [ One set of thresholds can be configured to trigger an interrupt only when
>>>>>>   the ambient light exceeds them for a configurable amount of time
>>>>>> (persistence)
>>>>>> ]
>>>>>>
>>>>>> * TAOS TCS34725 ambient light sensor [2]
>>>>>> [
>>>>>> The interrupt persistence filter allows the user to define the number
>>>>>> of consecutive
>>>>>>   out-of-threshold events necessary before generating an interrupt.
>>>>>> ]
>>>>>>
>>>>>> * Avago SAPDS-9950, Sensortek STK3310
>>>>>>
>>>>>> I think the TSL25911 datasheet best describes this parameter, as the
>>>>>> amount of time
>>>>>> that ambient light should exceed a threshold until an interrupt is
>>>>>> generated.
>>>>>
>>>>> Ok, that makes more sense. I misunderstood the initial description as that
>>>>> the signal would have to go first above the threshold then below the
>>>>> threshold, and this for a number of times. Whereas it needs to exceed the
>>>>> threshold for a certain amount of time before the event is triggered. If
>>>>> it goes below the threshold before the persistence interval no event is
>>>>> triggered and the counter is reset.
>>>>
>>>> Yes, it needs to cross the threshold n number of times before a event is
>>>> generated.
>>>
>>> Wait. It needs to cross the threshold or it needs to stay above the threshold?
>> Following is the logic for this chip.
>>
>> If ( data > Upper_threshold or data  < Lower_threshold)
>>     generate_event()
> 
> Missed to add <n> times logic
> 
> If ( data > Upper_threshold or data  < Lower_threshold) {
>     increment_count;
>     if (count >= n) {
>         generate_event()
>         reset_count()
>     }
> }
> 
So level rather than edge triggered.  Definitely what we put _period
in for in the first place.  Admittedly persistence might have been a better
name, but too late now! (dratted ABI compatibility)

J
>>>
>>> -- 
>>> 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: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[sathyanarayanan kuppuswamy]

Comments inline.

On 04/09/2015 03:16 AM, Jonathan Cameron wrote:
> On 01/04/15 20:22, sathyanarayanan kuppuswamy wrote:
>>
>> On 04/01/2015 12:06 PM, sathyanarayanan kuppuswamy wrote:
>>>
>>> On 04/01/2015 10:48 AM, Lars-Peter Clausen wrote:
>>>> On 04/01/2015 07:45 PM, sathyanarayanan kuppuswamy wrote:
>>>>>
>>>>> On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
>>>>>> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>>>>>>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>>>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>>>>>>> +       {
>>>>>>>>>> +               .name = "intr_persist",
>>>>>>>>>> +               .read = ltr501_read_intr_prst,
>>>>>>>>>> +               .write = ltr501_write_intr_prst,
>>>>>>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>>>>>>> +       },
>>>>>>>>>> +       {},
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>> Would be nice to standardize persistence attribute
>>>>>>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>>>>>>
>>>>>>>> If I understand the behavior correctly it causes that the event needs to be
>>>>>>>> triggered at least n times before the event is reported by the chip. In my
>>>>>>>> opinion 'persistence' is not a good term for that. I'm not sure what a
>>>>>>>> better term is but I think it should go more in the direction of ratelimit
>>>>>>>> or something.
>>>>>>> I've seen this term used for many devices:
>>>>>>>
>>>>>>> *  TSL25911 ambient light sensor [1]
>>>>>>>
>>>>>>> [ One set of thresholds can be configured to trigger an interrupt only when
>>>>>>>    the ambient light exceeds them for a configurable amount of time
>>>>>>> (persistence)
>>>>>>> ]
>>>>>>>
>>>>>>> * TAOS TCS34725 ambient light sensor [2]
>>>>>>> [
>>>>>>> The interrupt persistence filter allows the user to define the number
>>>>>>> of consecutive
>>>>>>>    out-of-threshold events necessary before generating an interrupt.
>>>>>>> ]
>>>>>>>
>>>>>>> * Avago SAPDS-9950, Sensortek STK3310
>>>>>>>
>>>>>>> I think the TSL25911 datasheet best describes this parameter, as the
>>>>>>> amount of time
>>>>>>> that ambient light should exceed a threshold until an interrupt is
>>>>>>> generated.
>>>>>> Ok, that makes more sense. I misunderstood the initial description as that
>>>>>> the signal would have to go first above the threshold then below the
>>>>>> threshold, and this for a number of times. Whereas it needs to exceed the
>>>>>> threshold for a certain amount of time before the event is triggered. If
>>>>>> it goes below the threshold before the persistence interval no event is
>>>>>> triggered and the counter is reset.
>>>>> Yes, it needs to cross the threshold n number of times before a event is
>>>>> generated.
>>>> Wait. It needs to cross the threshold or it needs to stay above the threshold?
>>> Following is the logic for this chip.
>>>
>>> If ( data > Upper_threshold or data  < Lower_threshold)
>>>      generate_event()
>> Missed to add <n> times logic
>>
>> If ( data > Upper_threshold or data  < Lower_threshold) {
>>      increment_count;
>>      if (count >= n) {
>>          generate_event()
>>          reset_count()
>>      }
>> }
>>
> So level rather than edge triggered.  Definitely what we put _period
> in for in the first place.  Admittedly persistence might have been a better
> name, but too late now! (dratted ABI compatibility)
>
> J
But we cannot use period for this use case. According to _period ABI 
description,
It specifies the period of time for which the condition must be true for 
getting a
valid event. But here, we are checking for number of times a condition 
must be
true for generating a valid event.

Description:
                 Period of time (in seconds) for which the condition must be
                 met before an event is generated. If direction is not
                 specified then this period applies to both directions.

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

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Jonathan Cameron]

On 9 April 2015 23:35:03 BST, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>Comments inline.
>
>On 04/09/2015 03:16 AM, Jonathan Cameron wrote:
>> On 01/04/15 20:22, sathyanarayanan kuppuswamy wrote:
>>>
>>> On 04/01/2015 12:06 PM, sathyanarayanan kuppuswamy wrote:
>>>>
>>>> On 04/01/2015 10:48 AM, Lars-Peter Clausen wrote:
>>>>> On 04/01/2015 07:45 PM, sathyanarayanan kuppuswamy wrote:
>>>>>>
>>>>>> On 04/01/2015 08:15 AM, Lars-Peter Clausen wrote:
>>>>>>> On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>>>>>>>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen
><lars@metafoo.de> wrote:
>>>>>>>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>>>>>>>> [...]
>>>>>>>>>>
>>>>>>>>>>> +static const struct iio_chan_spec_ext_info
>ltr501_ext_info[] = {
>>>>>>>>>>> +       {
>>>>>>>>>>> +               .name = "intr_persist",
>>>>>>>>>>> +               .read = ltr501_read_intr_prst,
>>>>>>>>>>> +               .write = ltr501_write_intr_prst,
>>>>>>>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>>>>>>>> +       },
>>>>>>>>>>> +       {},
>>>>>>>>>>> +};
>>>>>>>>>>> +
>>>>>>>>>> Would be nice to standardize persistence attribute
>>>>>>>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>>>>>>>
>>>>>>>>> If I understand the behavior correctly it causes that the
>event needs to be
>>>>>>>>> triggered at least n times before the event is reported by the
>chip. In my
>>>>>>>>> opinion 'persistence' is not a good term for that. I'm not
>sure what a
>>>>>>>>> better term is but I think it should go more in the direction
>of ratelimit
>>>>>>>>> or something.
>>>>>>>> I've seen this term used for many devices:
>>>>>>>>
>>>>>>>> *  TSL25911 ambient light sensor [1]
>>>>>>>>
>>>>>>>> [ One set of thresholds can be configured to trigger an
>interrupt only when
>>>>>>>>    the ambient light exceeds them for a configurable amount of
>time
>>>>>>>> (persistence)
>>>>>>>> ]
>>>>>>>>
>>>>>>>> * TAOS TCS34725 ambient light sensor [2]
>>>>>>>> [
>>>>>>>> The interrupt persistence filter allows the user to define the
>number
>>>>>>>> of consecutive
>>>>>>>>    out-of-threshold events necessary before generating an
>interrupt.
>>>>>>>> ]
>>>>>>>>
>>>>>>>> * Avago SAPDS-9950, Sensortek STK3310
>>>>>>>>
>>>>>>>> I think the TSL25911 datasheet best describes this parameter,
>as the
>>>>>>>> amount of time
>>>>>>>> that ambient light should exceed a threshold until an interrupt
>is
>>>>>>>> generated.
>>>>>>> Ok, that makes more sense. I misunderstood the initial
>description as that
>>>>>>> the signal would have to go first above the threshold then below
>the
>>>>>>> threshold, and this for a number of times. Whereas it needs to
>exceed the
>>>>>>> threshold for a certain amount of time before the event is
>triggered. If
>>>>>>> it goes below the threshold before the persistence interval no
>event is
>>>>>>> triggered and the counter is reset.
>>>>>> Yes, it needs to cross the threshold n number of times before a
>event is
>>>>>> generated.
>>>>> Wait. It needs to cross the threshold or it needs to stay above
>the threshold?
>>>> Following is the logic for this chip.
>>>>
>>>> If ( data > Upper_threshold or data  < Lower_threshold)
>>>>      generate_event()
>>> Missed to add <n> times logic
>>>
>>> If ( data > Upper_threshold or data  < Lower_threshold) {
>>>      increment_count;
>>>      if (count >= n) {
>>>          generate_event()
>>>          reset_count()
>>>      }
>>> }
>>>
>> So level rather than edge triggered.  Definitely what we put _period
>> in for in the first place.  Admittedly persistence might have been a
>better
>> name, but too late now! (dratted ABI compatibility)
>>
>> J
>But we cannot use period for this use case. According to _period ABI 
>description,
>It specifies the period of time for which the condition must be true
>for 
>getting a
>valid event. But here, we are checking for number of times a condition 
>must be
>true for generating a valid event.
But we are checking that condition at a known sampling rate I assume?
Hence 
period = persistence x 1/sampling frequency?

>
>Description:
>            Period of time (in seconds) for which the condition must be
>                 met before an event is generated. If direction is not
>                 specified then this period applies to both directions.
>
>>>>> -- 
>>>>> 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
>>>>>
>>

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

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Lars-Peter Clausen]

On 04/10/2015 07:51 AM, Jonathan Cameron wrote:
[...]
>>> So level rather than edge triggered.  Definitely what we put _period
>>> in for in the first place.  Admittedly persistence might have been a
>> better
>>> name, but too late now! (dratted ABI compatibility)
>>>
>>> J
>> But we cannot use period for this use case. According to _period ABI
>> description,
>> It specifies the period of time for which the condition must be true
>> for
>> getting a
>> valid event. But here, we are checking for number of times a condition
>> must be
>> true for generating a valid event.
> But we are checking that condition at a known sampling rate I assume?
> Hence
> period = persistence x 1/sampling frequency?

Sorry for highjacking this thread.

So period is for the time interval it has the event has to be asserted 
before it is registered. What would we call the same thing but for the 
de-assertion of the event? The event has to be de-asserted for X amount of 
time before it gets propagated.

Thanks,
- Lars

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Jonathan Cameron]

On 22/04/15 16:59, Lars-Peter Clausen wrote:
> On 04/10/2015 07:51 AM, Jonathan Cameron wrote:
> [...]
>>>> So level rather than edge triggered.  Definitely what we put _period
>>>> in for in the first place.  Admittedly persistence might have been a
>>> better
>>>> name, but too late now! (dratted ABI compatibility)
>>>>
>>>> J
>>> But we cannot use period for this use case. According to _period ABI
>>> description,
>>> It specifies the period of time for which the condition must be true
>>> for
>>> getting a
>>> valid event. But here, we are checking for number of times a condition
>>> must be
>>> true for generating a valid event.
>> But we are checking that condition at a known sampling rate I assume?
>> Hence
>> period = persistence x 1/sampling frequency?
> 
> Sorry for highjacking this thread.
> 
> So period is for the time interval it has the event has to be asserted before it is registered. What would we call the same thing but for the de-assertion of the event? The event has to be de-asserted for X amount of time before it gets propagated.

Err, offperiod?  resetperiod? space? (on the basis of mark-space description of square waves?)

It's kind of temporal hysteresis, or a debounce filter I suppose.  Period
isn't great for the existing interface.
> 
> Thanks,
> - Lars
> 
> -- 
> 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: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[Jonathan Cameron]

On 1 April 2015 16:15:33 BST, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 04/01/2015 05:02 PM, Daniel Baluta wrote:
>> On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <lars@metafoo.de>
>wrote:
>>> On 04/01/2015 04:04 PM, Daniel Baluta wrote:
>>> [...]
>>>>
>>>>
>>>>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>>>>> +       {
>>>>> +               .name = "intr_persist",
>>>>> +               .read = ltr501_read_intr_prst,
>>>>> +               .write = ltr501_write_intr_prst,
>>>>> +               .shared = IIO_SHARED_BY_TYPE,
>>>>> +       },
>>>>> +       {},
>>>>> +};
>>>>> +
>>>>
>>>> Would be nice to standardize persistence attribute
>>>> (IIO_CHAN_INFO_PERSISTENCE).
>>>
>>>
>>> If I understand the behavior correctly it causes that the event
>needs to be
>>> triggered at least n times before the event is reported by the chip.
>In my
>>> opinion 'persistence' is not a good term for that. I'm not sure what
>a
>>> better term is but I think it should go more in the direction of
>ratelimit
>>> or something.
>>
>> I've seen this term used for many devices:
>>
>> *  TSL25911 ambient light sensor [1]
>>
>> [ One set of thresholds can be configured to trigger an interrupt
>only when
>>   the ambient light exceeds them for a configurable amount of time
>(persistence)
>> ]
>>
>> * TAOS TCS34725 ambient light sensor [2]
>> [
>> The interrupt persistence filter allows the user to define the number
>> of consecutive
>>   out-of-threshold events necessary before generating an interrupt.
>> ]
>>
>> * Avago SAPDS-9950, Sensortek STK3310
>>
>> I think the TSL25911 datasheet best describes this parameter, as the
>> amount of time
>> that ambient light should exceed a threshold until an interrupt is
>generated.
>
>Ok, that makes more sense. I misunderstood the initial description as
>that 
>the signal would have to go first above the threshold then below the 
>threshold, and this for a number of times. Whereas it needs to exceed
>the 
>threshold for a certain amount of time before the event is triggered.
>If it 
>goes below the threshold before the persistence interval no event is 
>triggered and the counter is reset.

See ABI docs for _period.
This is indeed very common...


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

Re: [PATCH v1 2/3] iio: ltr501: Add interrupt rate control support

[sathyanarayanan kuppuswamy]

On 04/01/2015 07:04 AM, Daniel Baluta wrote:
> On Wed, Apr 1, 2015 at 5:55 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Added rate control support for ALS and proximity
>> threshold interrupts.
>>
>> Setting <n> to ALS intr_persist sysfs node would
>> generate interrupt whenever ALS data cross either
>> upper or lower threshold limits <n> number of times.
>> Similarly setting <m> to proximity intr_persist sysfs
>> node would genere interrupt whenever proximity data falls
>> outside threshold limit <m> number of times.
>>
> <snip>
>
>> +static ssize_t ltr501_read_intr_prst(struct iio_dev *indio_dev,
>> +                                  uintptr_t private,
>> +                                  const struct iio_chan_spec *chan,
>> +                                  char *buf)
>> +{
>> +       struct ltr501_data *data = iio_priv(indio_dev);
>> +       int ret = -EINVAL;
>> +
>> +       switch (chan->type) {
>> +       case IIO_INTENSITY:
>> +               mutex_lock(&data->lock_als);
>> +               ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
>> +               mutex_unlock(&data->lock_als);
> I am not sure I understand why is the mutex needed here? AFAIK I2C transactions
> are serialized.
That's mutex is to protect the ALS register updates. I want it 
serialized so that no data can be read when we are changing the control 
settings.
>
> <snip>
>
>> +static const struct iio_chan_spec_ext_info ltr501_ext_info[] = {
>> +       {
>> +               .name = "intr_persist",
>> +               .read = ltr501_read_intr_prst,
>> +               .write = ltr501_write_intr_prst,
>> +               .shared = IIO_SHARED_BY_TYPE,
>> +       },
>> +       {},
>> +};
>> +
> Would be nice to standardize persistence attribute (IIO_CHAN_INFO_PERSISTENCE).
Agreed. I will work on it.
>
> thanks,
> Daniel.
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

[PATCH v1 3/3] iio: ltr501: Add ACPI enumeration support

[Kuppuswamy Sathyanarayanan]

Added ACPI enumeration support for LTR501 chip.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/iio/light/ltr501.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index e8f5019..fc32036 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -16,6 +16,7 @@
 #include <linux/i2c.h>
 #include <linux/err.h>
 #include <linux/delay.h>
+#include <linux/acpi.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/events.h>
@@ -802,6 +803,12 @@ static int ltr501_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(ltr501_pm_ops, ltr501_suspend, ltr501_resume);
 
+static const struct acpi_device_id ltr_acpi_match[] = {
+	{"LTER0501", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, ltr_acpi_match);
+
 static const struct i2c_device_id ltr501_id[] = {
 	{ "ltr501", 0 },
 	{ }
@@ -812,6 +819,7 @@ static struct i2c_driver ltr501_driver = {
 	.driver = {
 		.name   = LTR501_DRV_NAME,
 		.pm	= &ltr501_pm_ops,
+		.acpi_match_table = ACPI_PTR(ltr_acpi_match),
 		.owner  = THIS_MODULE,
 	},
 	.probe  = ltr501_probe,
-- 
1.9.1

Re: [PATCH v1 0/3] Added LTR501 Interrupt support

[Daniel Baluta]

On Wed, Apr 1, 2015 at 5:55 AM, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> This patchset adds interrupt support for LTR501 chip. Also
> added acpi enumaration support.
>
> Please let me know your review comments.
>
> v1:
> 1. Added support to enable ALS & PS intterupts based
>    on threshold settings.
> 2. Added support to control intrrupt rate.
> 3. Added acpi enumeration support.
>
> Kuppuswamy Sathyanarayanan (3):
>   iio: ltr501: Add interrupt support
>   iio: ltr501: Add interrupt rate control support
>   iio: ltr501: Add ACPI enumeration support
>
>  drivers/iio/light/ltr501.c | 395 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 392 insertions(+), 3 deletions(-)

Nice :). Keep in mind that there are some pending patches for ltr501
and your code
might not apply.

I think at least this patch must be applied.

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

thanks,
Daniel.

Re: [PATCH v1 0/3] Added LTR501 Interrupt support

[sathyanarayanan kuppuswamy]

On 04/01/2015 06:02 AM, Daniel Baluta wrote:
> On Wed, Apr 1, 2015 at 5:55 AM, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> This patchset adds interrupt support for LTR501 chip. Also
>> added acpi enumaration support.
>>
>> Please let me know your review comments.
>>
>> v1:
>> 1. Added support to enable ALS & PS intterupts based
>>     on threshold settings.
>> 2. Added support to control intrrupt rate.
>> 3. Added acpi enumeration support.
>>
>> Kuppuswamy Sathyanarayanan (3):
>>    iio: ltr501: Add interrupt support
>>    iio: ltr501: Add interrupt rate control support
>>    iio: ltr501: Add ACPI enumeration support
>>
>>   drivers/iio/light/ltr501.c | 395 ++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 392 insertions(+), 3 deletions(-)
> Nice :). Keep in mind that there are some pending patches for ltr501
> and your code
> might not apply.
>
> I think at least this patch must be applied.
>
> http://marc.info/?l=linux-iio&m=142779829617041&w=2
Do you want me to include this in my patch set. I can rebase my patches 
on top of this.
>
> thanks,
> Daniel.
> --
> 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
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer