Added LTR501 Interrupt support

PATCH Superseded Kuppuswamy Sathyanarayanan 5 participants 21 messages Apr 1, 2015 – Apr 22, 2015 IIO

lore

Share & tools

lore

Kuppuswamy Sathyanarayanan

author

2015-04-01 02:55 UTC #0 lore raw

cover Added LTR501 Interrupt support

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

Signature · 1 line

1.9.1

Kuppuswamy Sathyanarayanan

author

2015-04-01 02:55 UTC #1 lore raw

PATCH 1/3 iio: ltr501: Add interrupt support

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 <[email protected]>

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

drivers/iio/light/ltr501.c +299 -3

index 62b7072..16c3f7a 100644
@@ -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)

Signature · 1 line

1.9.1

Kuppuswamy Sathyanarayanan

author

2015-04-01 02:55 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/iio/light/ltr501.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

drivers/iio/light/ltr501.c +85 -0

index 16c3f7a..e8f5019 100644
@@ -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),
 };

Signature · 1 line

1.9.1

Daniel Baluta

2015-04-01 14:04 UTC #3 lore raw

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

Quoted 10 lines · 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.

<snip>

drivers/iio/light/ltr501.c static int ltr501_read_ps(struct ltr501_data *data) · line 145

Quoted 13 lines

drivers/iio/light/ltr501.c +13 -0

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

drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248

Quoted 10 lines

drivers/iio/light/ltr501.c +10 -0

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

Lars-Peter Clausen

2015-04-01 14:39 UTC #4 lore raw

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

On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]

Quoted 12 lines

drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 11 lines
drivers/iio/light/ltr501.c +10 -0
+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.

Daniel Baluta

2015-04-01 15:02 UTC #5 lore raw

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

Quoted 19 lines · Lars-Peter Clausen

On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 12 lines
drivers/iio/light/ltr501.c +10 -0
+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).

Quoted 5 lines

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

Lars-Peter Clausen

2015-04-01 15:15 UTC #6 lore raw

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

Quoted 41 lines · Daniel Baluta

Quoted 19 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 12 lines
drivers/iio/light/ltr501.c +10 -0
+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).
Quoted 5 lines
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.
]

Quoted 5 lines

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

sathyanarayanan kuppuswamy

author

2015-04-01 17:45 UTC #7 lore raw

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

Quoted 43 lines · Lars-Peter Clausen

Quoted 42 lines · Daniel Baluta
Quoted 19 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 12 lines
drivers/iio/light/ltr501.c +10 -0
+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).
Quoted 5 lines
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.
]

Quoted 7 lines

Quoted 5 lines
* 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.

Quoted 2 lines

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Android kernel developer

Lars-Peter Clausen

2015-04-01 17:48 UTC #8 lore raw

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

Quoted 60 lines · sathyanarayanan kuppuswamy

Quoted 50 lines · Lars-Peter Clausen
Quoted 42 lines · Daniel Baluta
Quoted 19 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 12 lines
drivers/iio/light/ltr501.c +10 -0
+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).
Quoted 5 lines
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.
]
Quoted 6 lines
* 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.
Quoted 6 lines
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.

Quoted 2 lines

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?

sathyanarayanan kuppuswamy

author

2015-04-01 19:06 UTC #9 lore raw

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

Quoted 61 lines · Lars-Peter Clausen

Quoted 60 lines · sathyanarayanan kuppuswamy
Quoted 56 lines · Lars-Peter Clausen
Quoted 42 lines · Daniel Baluta
Quoted 19 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
Review
Quoted 12 lines
(quoted diff) +10 -0
+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).
Quoted 5 lines
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.
]
Quoted 6 lines
* 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.

Quoted 4 lines

Quoted 2 lines
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()

Quoted 6 lines

Signature · 4 lines

-- 
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Android kernel developer

sathyanarayanan kuppuswamy

author

2015-04-01 19:22 UTC #10 lore raw

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

Quoted 64 lines · sathyanarayanan kuppuswamy

Quoted 61 lines · Lars-Peter Clausen
Quoted 60 lines · sathyanarayanan kuppuswamy
Quoted 56 lines · Lars-Peter Clausen
Quoted 42 lines · Daniel Baluta
Quoted 19 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
Review
Quoted 12 lines
(quoted diff) +10 -0
+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).
Quoted 5 lines
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.
]
Quoted 6 lines
* 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.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
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()
    }
}

Quoted 7 lines

Quoted 6 lines

Signature · 4 lines

-- 
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Android kernel developer

Jonathan Cameron

maintainer

2015-04-09 10:16 UTC #11 lore raw

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

Quoted 78 lines · sathyanarayanan kuppuswamy

Quoted 64 lines · sathyanarayanan kuppuswamy
Quoted 61 lines · Lars-Peter Clausen
Quoted 60 lines · sathyanarayanan kuppuswamy
Quoted 56 lines · Lars-Peter Clausen
Quoted 42 lines · Daniel Baluta
Quoted 19 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 15 lines
Review
Quoted 12 lines
(quoted diff) +10 -0
+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).
Quoted 5 lines
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.
]
Quoted 6 lines
* 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.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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

Quoted 8 lines

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)

Quoted 8 lines

Quoted 7 lines

Quoted 6 lines

Signature · 4 lines

-- 
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

sathyanarayanan kuppuswamy

author

2015-04-09 22:34 UTC #12 lore raw

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

Comments inline.

Quoted 76 lines · Jonathan Cameron

Quoted 67 lines · sathyanarayanan kuppuswamy
Quoted 56 lines · sathyanarayanan kuppuswamy
Quoted 54 lines · Lars-Peter Clausen
Quoted 53 lines · sathyanarayanan kuppuswamy
Quoted 51 lines · Lars-Peter Clausen
Quoted 38 lines · Daniel Baluta
Quoted 16 lines · Lars-Peter Clausen
On 04/01/2015 04:04 PM, Daniel Baluta wrote:
[...]
Quoted 13 lines
Review
Quoted 11 lines
(quoted diff) +10 -0
+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).
Quoted 5 lines
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.
]
Quoted 6 lines
* 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.
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
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
Quoted 8 lines
If ( data > Upper_threshold or data  < Lower_threshold) {
     increment_count;
     if (count >= n) {
         generate_event()
         reset_count()
     }
}

Quoted 5 lines

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.

Quoted 6 lines

Quoted 5 lines

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Android kernel developer

Jonathan Cameron

maintainer

2015-04-10 05:51 UTC #13 lore raw

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

Quoted 104 lines · sathyanarayanan kuppuswamy

Comments inline.
Quoted 10 lines · Jonathan Cameron
Quoted 9 lines · sathyanarayanan kuppuswamy
Quoted 7 lines · sathyanarayanan kuppuswamy
Quoted 5 lines · Lars-Peter Clausen
Quoted 4 lines · sathyanarayanan kuppuswamy
Quoted 2 lines · Lars-Peter Clausen
Quoted 1 line · Daniel Baluta
On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen
Quoted 4 lines · <[email protected]> wrote:
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Daniel Baluta
[...]
Quoted 2 lines
Quoted 2 lines
+static const struct iio_chan_spec_ext_info
ltr501_ext_info[] = {
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Review
Quoted 9 lines
(quoted diff) +9 -0
+       {
+               .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).
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
If I understand the behavior correctly it causes that the
event needs to be
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
triggered at least n times before the event is reported by the
chip. In my
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
opinion 'persistence' is not a good term for that. I'm not
sure what a
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
better term is but I think it should go more in the direction
of ratelimit
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
the ambient light exceeds them for a configurable amount of
time
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(persistence)
]
* TAOS TCS34725 ambient light sensor [2]
[
The interrupt persistence filter allows the user to define the
number
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
of consecutive
out-of-threshold events necessary before generating an
interrupt.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
]
* Avago SAPDS-9950, Sensortek STK3310
I think the TSL25911 datasheet best describes this parameter,
as the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
amount of time
that ambient light should exceed a threshold until an interrupt
is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
generated.
Ok, that makes more sense. I misunderstood the initial
description as that
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
the signal would have to go first above the threshold then below
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
threshold, and this for a number of times. Whereas it needs to
exceed the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
threshold for a certain amount of time before the event is
triggered. If
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
it goes below the threshold before the persistence interval no
event is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
triggered and the counter is reset.
Yes, it needs to cross the threshold n number of times before a
event is
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
generated.
Wait. It needs to cross the threshold or it needs to stay above
the threshold?
Quoted 14 lines
Quoted 6 lines
Quoted 4 lines
Following is the logic for this chip.
If ( data > Upper_threshold or data < Lower_threshold)
generate_event()
Missed to add <n> times logic
Quoted 8 lines
If ( data > Upper_threshold or data  < Lower_threshold) {
     increment_count;
     if (count >= n) {
         generate_event()
         reset_count()
     }
}
Quoted 2 lines
So level rather than edge triggered. Definitely what we put _period in for in the first place. Admittedly persistence might have been a
better
Quoted 3 lines
name, but too late now! (dratted ABI compatibility)
J

Quoted 8 lines

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?

Quoted 13 lines

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.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Signature · 1 line
To unsubscribe from this list: send the line "unsubscribe
linux-iio" in
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

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

Lars-Peter Clausen

2015-04-22 15:59 UTC #14 lore raw

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

On 04/10/2015 07:51 AM, Jonathan Cameron wrote:
[...]

Quoted 14 lines

Quoted 6 lines
Quoted 2 lines
So level rather than edge triggered. Definitely what we put _period in for in the first place. Admittedly persistence might have been a
better
Quoted 3 lines
name, but too late now! (dratted ABI compatibility)
J
Quoted 8 lines
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.

Quoted 3 lines

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

Jonathan Cameron

maintainer

2015-04-22 20:58 UTC #15 lore raw

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

Quoted 16 lines · Lars-Peter Clausen

On 04/10/2015 07:51 AM, Jonathan Cameron wrote:
[...]
Quoted 14 lines
Quoted 14 lines
Quoted 2 lines
So level rather than edge triggered. Definitely what we put _period in for in the first place. Admittedly persistence might have been a
better
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 8 lines

Thanks,
- Lars

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jonathan Cameron

2015-04-01 20:19 UTC #16 lore raw

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

In reply to Lars-Peter Clausen

Quoted 56 lines · Lars-Peter Clausen

Quoted 1 line · Daniel Baluta
On Wed, Apr 1, 2015 at 5:39 PM, Lars-Peter Clausen <[email protected]>
Quoted 19 lines · wrote:
Quoted 19 lines · Daniel Baluta
[...]
Quoted 15 lines
drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 12 lines
drivers/iio/light/ltr501.c +10 -0
+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).
Quoted 1 line
Quoted 1 line
If I understand the behavior correctly it causes that the event
needs to be
Quoted 1 line
Quoted 1 line
triggered at least n times before the event is reported by the chip.
In my
Quoted 1 line
Quoted 1 line
opinion 'persistence' is not a good term for that. I'm not sure what
a
Quoted 1 line
Quoted 1 line
better term is but I think it should go more in the direction of
ratelimit
Quoted 7 lines
Quoted 1 line
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
Quoted 1 line
the ambient light exceeds them for a configurable amount of time
(persistence)
Quoted 9 lines
]
* 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.
]
Quoted 5 lines
* 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.

Quoted 9 lines

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

Signature · 1 line

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

sathyanarayanan kuppuswamy

author

2015-04-01 17:33 UTC #17 lore raw

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

In reply to Daniel Baluta

Quoted 14 lines · Daniel Baluta

Quoted 10 lines · 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.
<snip>

Quoted 15 lines

drivers/iio/light/ltr501.c static int ltr501_read_ps(struct ltr501_data *data) · line 145

Quoted 13 lines

drivers/iio/light/ltr501.c +13 -0

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

Quoted 14 lines

<snip>
drivers/iio/light/ltr501.c static const struct iio_event_spec ltr501_pxs_event_spec[] = { · line 248
Quoted 10 lines
drivers/iio/light/ltr501.c +10 -0
+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.

Quoted 4 lines

thanks,
Daniel.

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Android kernel developer

Kuppuswamy Sathyanarayanan

author

2015-04-01 02:55 UTC #18 lore raw

PATCH 3/3 iio: ltr501: Add ACPI enumeration support

In reply to Kuppuswamy Sathyanarayanan

Added ACPI enumeration support for LTR501 chip.

Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>

---
 drivers/iio/light/ltr501.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/iio/light/ltr501.c +8 -0

index e8f5019..fc32036 100644
@@ -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,

Signature · 1 line

1.9.1

Daniel Baluta

2015-04-01 13:02 UTC #19 lore raw

In reply to Kuppuswamy Sathyanarayanan

Quoted 11 lines · 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.

Quoted 7 lines

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.

sathyanarayanan kuppuswamy

author

2015-04-01 17:29 UTC #20 lore raw

Quoted 20 lines · Daniel Baluta

Quoted 11 lines · 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.
Quoted 7 lines
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(-)

Quoted 7 lines

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.

Quoted 8 lines

thanks,
Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Sathyanarayanan Kuppuswamy
Android kernel developer