[PATCH v3 0/6] Added LTR501 Interrupt support

[PATCH v3 0/6] Added LTR501 Interrupt support

[Kuppuswamy Sathyanarayanan]

This patchset adds interrupt support and acpi enumeration support for
LTR-501 chip.

Also added a new event option (IIO_EV_INFO_PERSISTENCE) to specifcy
threshold interrupt persistence filter value. Currently, we don't
have any way to specify the persistence filter value in iio event
framework. Many drivers use channel ext info to work around this
issue. This patch adds this option to event info list.

I was suggested to use IIO_EV_INFO_PERIOD for this purpose. But it
is not a valid option for this use case because, IIO_EV_INFO_PERIOD
specifies period value in 'seconds'. But this option needs to represent
the value as "counts".

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.

v2:
1. Removed persistance attribute from ext_channel and
   added support for IIO_EV_INFO_PERIOD.
2. Rebased my patches on top of Daniel's alignment fix.

v3:
1. Added a new ABI to define threshold interrupt persistence
   filter value.
2. Added Documentation for persistence filter iio ABI
3. Used IIO_EV_INFO_PERSISTENCE instead of IIO_EV_INFO_PERIOD
   in ltr501 driver.

Daniel Baluta (1):
  iio: light: ltr501: Fix alignment to match open parenthesis

Kuppuswamy Sathyanarayanan (5):
  iio: core : events ABI for specifying interrupt persistence
  iio: documentation: Add ABI info for iio event persistence filter
  iio: ltr501: Add interrupt support
  iio: ltr501: Add interrupt rate control support
  iio: ltr501: Add ACPI enumeration support

 Documentation/ABI/testing/sysfs-bus-iio |  82 ++++++
 drivers/iio/industrialio-event.c        |   1 +
 drivers/iio/light/ltr501.c              | 469 ++++++++++++++++++++++++++++++--
 include/linux/iio/types.h               |   1 +
 4 files changed, 529 insertions(+), 24 deletions(-)

-- 
1.9.1

[PATCH v3 1/6] iio: core : events ABI for specifying interrupt persistence

[Kuppuswamy Sathyanarayanan]

This iio sysfs ABI defines a way to specify interrupt
persistence filter value for threshold based interrupts.

Example:

What: /sys/.../events/in_intensity0_thresh_persistence
What: /sys/.../events/in_proximity0_thresh_persistence

Currently, we don't have any way to specify the persistence
filter value in iio event framework. Many drivers use channel
ext info to work around this issue. This patch adds this option
to event info list.

Added IIO_EV_INFO_PERSISTENCE and corresponding string.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/iio/industrialio-event.c | 1 +
 include/linux/iio/types.h        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index a4b3970..7bd55b7 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -211,6 +211,7 @@ static const char * const iio_ev_info_text[] = {
 	[IIO_EV_INFO_VALUE] = "value",
 	[IIO_EV_INFO_HYSTERESIS] = "hysteresis",
 	[IIO_EV_INFO_PERIOD] = "period",
+	[IIO_EV_INFO_PERSISTENCE] = "persistence",
 };
 
 static enum iio_event_direction iio_ev_attr_dir(struct iio_dev_attr *attr)
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 580ed5b..5240dc6 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -86,6 +86,7 @@ enum iio_event_info {
 	IIO_EV_INFO_VALUE,
 	IIO_EV_INFO_HYSTERESIS,
 	IIO_EV_INFO_PERIOD,
+	IIO_EV_INFO_PERSISTENCE,
 };
 
 enum iio_event_direction {
-- 
1.9.1

[PATCH v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[Kuppuswamy Sathyanarayanan]

Add ABI info for iio event persistence filter.

Setting <n> to persistence filter would need atleast
<n> data values outside the upper/lower threshold
limits before generating an interrupt.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 82 +++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 9a70c31..590b1d4 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -856,6 +856,88 @@ Description:
 		met before an event is generated. If direction is not
 		specified then this period applies to both directions.
 
+What:		/sys/.../events/in_accel_x_thresh_rising_persistence
+What:		/sys/.../events/in_accel_x_thresh_falling_persistence
+hat:		/sys/.../events/in_accel_x_roc_rising_persistence
+What:		/sys/.../events/in_accel_x_roc_falling_persistence
+What:		/sys/.../events/in_accel_y_thresh_rising_persistence
+What:		/sys/.../events/in_accel_y_thresh_falling_persistence
+What:		/sys/.../events/in_accel_y_roc_rising_persistence
+What:		/sys/.../events/in_accel_y_roc_falling_persistence
+What:		/sys/.../events/in_accel_z_thresh_rising_persistence
+What:		/sys/.../events/in_accel_z_thresh_falling_persistence
+What:		/sys/.../events/in_accel_z_roc_rising_persistence
+What:		/sys/.../events/in_accel_z_roc_falling_persistence
+What:		/sys/.../events/in_anglvel_x_thresh_rising_persistence
+What:		/sys/.../events/in_anglvel_x_thresh_falling_persistence
+What:		/sys/.../events/in_anglvel_x_roc_rising_persistence
+What:		/sys/.../events/in_anglvel_x_roc_falling_persistence
+What:		/sys/.../events/in_anglvel_y_thresh_rising_persistence
+What:		/sys/.../events/in_anglvel_y_thresh_falling_persistence
+What:		/sys/.../events/in_anglvel_y_roc_rising_persistence
+What:		/sys/.../events/in_anglvel_y_roc_falling_persistence
+What:		/sys/.../events/in_anglvel_z_thresh_rising_persistence
+What:		/sys/.../events/in_anglvel_z_thresh_falling_persistence
+What:		/sys/.../events/in_anglvel_z_roc_rising_persistence
+What:		/sys/.../events/in_anglvel_z_roc_falling_persistence
+What:		/sys/.../events/in_magn_x_thresh_rising_persistence
+What:		/sys/.../events/in_magn_x_thresh_falling_persistence
+What:		/sys/.../events/in_magn_x_roc_rising_persistence
+What:		/sys/.../events/in_magn_x_roc_falling_persistence
+What:		/sys/.../events/in_magn_y_thresh_rising_persistence
+What:		/sys/.../events/in_magn_y_thresh_falling_persistence
+What:		/sys/.../events/in_magn_y_roc_rising_persistence
+What:		/sys/.../events/in_magn_y_roc_falling_persistence
+What:		/sys/.../events/in_magn_z_thresh_rising_persistence
+What:		/sys/.../events/in_magn_z_thresh_falling_persistence
+What:		/sys/.../events/in_magn_z_roc_rising_persistence
+What:		/sys/.../events/in_magn_z_roc_falling_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
+What:		/sys/.../events/in_rot_from_north_true_thresh_rising_persistence
+What:		/sys/.../events/in_rot_from_north_true_thresh_falling_persistence
+What:		/sys/.../events/in_rot_from_north_true_roc_rising_persistence
+What:		/sys/.../events/in_rot_from_north_true_roc_falling_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
+What:		/sys/.../events/in_voltageY_supply_thresh_rising_persistence
+What:		/sys/.../events/in_voltageY_supply_thresh_falling_persistence
+What:		/sys/.../events/in_voltageY_supply_roc_rising_persistence
+What:		/sys/.../events/in_voltageY_supply_roc_falling_persistence
+What:		/sys/.../events/in_voltageY_thresh_rising_persistence
+What:		/sys/.../events/in_voltageY_thresh_falling_persistence
+What:		/sys/.../events/in_voltageY_roc_rising_persistence
+What:		/sys/.../events/in_voltageY_roc_falling_persistence
+What:		/sys/.../events/in_tempY_thresh_rising_persistence
+What:		/sys/.../events/in_tempY_thresh_falling_persistence
+What:		/sys/.../events/in_tempY_roc_rising_persistence
+What:		/sys/.../events/in_tempY_roc_falling_persistence
+What:		/sys/.../events/in_accel_x&y&z_mag_falling_persistence
+What:		/sys/.../events/in_intensity0_thresh_persistence
+What:		/sys/.../events/in_proximity0_thresh_persistence
+What:		/sys/.../events/in_activity_still_thresh_rising_persistence
+What:		/sys/.../events/in_activity_still_thresh_falling_persistence
+What:		/sys/.../events/in_activity_walking_thresh_rising_persistence
+What:		/sys/.../events/in_activity_walking_thresh_falling_persistence
+What:		/sys/.../events/in_activity_jogging_thresh_rising_persistence
+What:		/sys/.../events/in_activity_jogging_thresh_falling_persistence
+What:		/sys/.../events/in_activity_running_thresh_rising_persistence
+What:		/sys/.../events/in_activity_running_thresh_falling_persistence
+KernelVersion:	4.0
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Number of times an event should occur before generating an
+		interrupt. Persistence filter value can be applied for both
+		rising/falling threshold based interrupts.
+
 What:		/sys/.../events/in_activity_still_thresh_rising_en
 What:		/sys/.../events/in_activity_still_thresh_falling_en
 What:		/sys/.../events/in_activity_walking_thresh_rising_en
-- 
1.9.1

[PATCH v3 3/6] iio: light: ltr501: Fix alignment to match open parenthesis

[Kuppuswamy Sathyanarayanan]

From: Daniel Baluta <daniel.baluta@intel.com>

This makes ltr501 code consistent with the coding style adopted
for the new drivers added to IIO.
We prepare the path for adding support for LTR559 chip.

Reported by checkpatch.pl

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
 drivers/iio/light/ltr501.c | 46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 62b7072..883855a 100644
--- a/drivers/iio/light/ltr501.c
+++ b/drivers/iio/light/ltr501.c
@@ -58,7 +58,7 @@ static int ltr501_drdy(struct ltr501_data *data, u8 drdy_mask)
 
 	while (tries--) {
 		ret = i2c_smbus_read_byte_data(data->client,
-			LTR501_ALS_PS_STATUS);
+					       LTR501_ALS_PS_STATUS);
 		if (ret < 0)
 			return ret;
 		if ((ret & drdy_mask) == drdy_mask)
@@ -77,7 +77,8 @@ static int ltr501_read_als(struct ltr501_data *data, __le16 buf[2])
 		return ret;
 	/* always read both ALS channels in given order */
 	return i2c_smbus_read_i2c_block_data(data->client,
-		LTR501_ALS_DATA1, 2 * sizeof(__le16), (u8 *) buf);
+					     LTR501_ALS_DATA1,
+					     2 * sizeof(__le16), (u8 *)buf);
 }
 
 static int ltr501_read_ps(struct ltr501_data *data)
@@ -107,7 +108,7 @@ static int ltr501_read_ps(struct ltr501_data *data)
 static const struct iio_chan_spec ltr501_channels[] = {
 	LTR501_INTENSITY_CHANNEL(0, LTR501_ALS_DATA0, IIO_MOD_LIGHT_BOTH, 0),
 	LTR501_INTENSITY_CHANNEL(1, LTR501_ALS_DATA1, IIO_MOD_LIGHT_IR,
-		BIT(IIO_CHAN_INFO_SCALE)),
+				 BIT(IIO_CHAN_INFO_SCALE)),
 	{
 		.type = IIO_PROXIMITY,
 		.address = LTR501_PS_DATA,
@@ -129,8 +130,8 @@ static const int ltr501_ps_gain[4][2] = {
 };
 
 static int ltr501_read_raw(struct iio_dev *indio_dev,
-				struct iio_chan_spec const *chan,
-				int *val, int *val2, long mask)
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
 {
 	struct ltr501_data *data = iio_priv(indio_dev);
 	__le16 buf[2];
@@ -149,7 +150,7 @@ static int ltr501_read_raw(struct iio_dev *indio_dev,
 			if (ret < 0)
 				return ret;
 			*val = le16_to_cpu(chan->address == LTR501_ALS_DATA1 ?
-				buf[0] : buf[1]);
+					   buf[0] : buf[1]);
 			return IIO_VAL_INT;
 		case IIO_PROXIMITY:
 			mutex_lock(&data->lock_ps);
@@ -199,8 +200,8 @@ static int ltr501_get_ps_gain_index(int val, int val2)
 }
 
 static int ltr501_write_raw(struct iio_dev *indio_dev,
-			       struct iio_chan_spec const *chan,
-			       int val, int val2, long mask)
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
 {
 	struct ltr501_data *data = iio_priv(indio_dev);
 	int i;
@@ -219,7 +220,8 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
 			else
 				return -EINVAL;
 			return i2c_smbus_write_byte_data(data->client,
-				LTR501_ALS_CONTR, data->als_contr);
+							 LTR501_ALS_CONTR,
+							 data->als_contr);
 		case IIO_PROXIMITY:
 			i = ltr501_get_ps_gain_index(val, val2);
 			if (i < 0)
@@ -227,7 +229,8 @@ static int ltr501_write_raw(struct iio_dev *indio_dev,
 			data->ps_contr &= ~LTR501_CONTR_PS_GAIN_MASK;
 			data->ps_contr |= i << LTR501_CONTR_PS_GAIN_SHIFT;
 			return i2c_smbus_write_byte_data(data->client,
-				LTR501_PS_CONTR, data->ps_contr);
+							 LTR501_PS_CONTR,
+							 data->ps_contr);
 		default:
 			return -EINVAL;
 		}
@@ -279,7 +282,7 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
 
 	/* figure out which data needs to be ready */
 	if (test_bit(0, indio_dev->active_scan_mask) ||
-		test_bit(1, indio_dev->active_scan_mask))
+	    test_bit(1, indio_dev->active_scan_mask))
 		mask |= LTR501_STATUS_ALS_RDY;
 	if (test_bit(2, indio_dev->active_scan_mask))
 		mask |= LTR501_STATUS_PS_RDY;
@@ -290,7 +293,9 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
 
 	if (mask & LTR501_STATUS_ALS_RDY) {
 		ret = i2c_smbus_read_i2c_block_data(data->client,
-			LTR501_ALS_DATA1, sizeof(als_buf), (u8 *) als_buf);
+						    LTR501_ALS_DATA1,
+						    sizeof(als_buf),
+						    (u8 *)als_buf);
 		if (ret < 0)
 			return ret;
 		if (test_bit(0, indio_dev->active_scan_mask))
@@ -306,8 +311,7 @@ static irqreturn_t ltr501_trigger_handler(int irq, void *p)
 		buf[j++] = ret & LTR501_PS_DATA_MASK;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, buf,
-		iio_get_time_ns());
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
 
 done:
 	iio_trigger_notify_done(indio_dev->trig);
@@ -330,11 +334,11 @@ static int ltr501_init(struct ltr501_data *data)
 	data->ps_contr = ret | LTR501_CONTR_ACTIVE;
 
 	return ltr501_write_contr(data->client, data->als_contr,
-		data->ps_contr);
+				  data->ps_contr);
 }
 
 static int ltr501_probe(struct i2c_client *client,
-			  const struct i2c_device_id *id)
+			const struct i2c_device_id *id)
 {
 	struct ltr501_data *data;
 	struct iio_dev *indio_dev;
@@ -368,7 +372,7 @@ static int ltr501_probe(struct i2c_client *client,
 		return ret;
 
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-		ltr501_trigger_handler, NULL);
+					 ltr501_trigger_handler, NULL);
 	if (ret)
 		return ret;
 
@@ -386,8 +390,8 @@ error_unreg_buffer:
 static int ltr501_powerdown(struct ltr501_data *data)
 {
 	return ltr501_write_contr(data->client,
-		data->als_contr & ~LTR501_CONTR_ACTIVE,
-		data->ps_contr & ~LTR501_CONTR_ACTIVE);
+				  data->als_contr & ~LTR501_CONTR_ACTIVE,
+				  data->ps_contr & ~LTR501_CONTR_ACTIVE);
 }
 
 static int ltr501_remove(struct i2c_client *client)
@@ -405,14 +409,14 @@ static int ltr501_remove(struct i2c_client *client)
 static int ltr501_suspend(struct device *dev)
 {
 	struct ltr501_data *data = iio_priv(i2c_get_clientdata(
-		to_i2c_client(dev)));
+					    to_i2c_client(dev)));
 	return ltr501_powerdown(data);
 }
 
 static int ltr501_resume(struct device *dev)
 {
 	struct ltr501_data *data = iio_priv(i2c_get_clientdata(
-		to_i2c_client(dev)));
+					    to_i2c_client(dev)));
 
 	return ltr501_write_contr(data->client, data->als_contr,
 		data->ps_contr);
-- 
1.9.1

[PATCH v3 4/6] 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 883855a..8672962 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);
@@ -89,6 +127,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, \
@@ -102,7 +177,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[] = {
@@ -121,6 +198,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),
 };
@@ -238,6 +317,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");
 
@@ -251,10 +491,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,
 };
 
@@ -319,6 +570,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;
@@ -361,7 +642,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;
@@ -371,6 +651,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 v3 5/6] 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 | 121 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 117 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
index 8672962..1b314f3 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
@@ -127,6 +131,70 @@ static int ltr501_read_ps(struct ltr501_data *data)
 	return i2c_smbus_read_word_data(data->client, LTR501_PS_DATA);
 }
 
+static int ltr501_read_intr_prst(struct iio_dev *indio_dev,
+				 const struct iio_chan_spec *chan,
+				 int *val)
+{
+	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);
+		if (ret >= 0)
+			*val = ret & LTR501_INTR_PRST_ALS_MASK;
+		break;
+	case IIO_PROXIMITY:
+		mutex_lock(&data->lock_ps);
+		ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
+		mutex_unlock(&data->lock_ps);
+		if (ret >= 0)
+			*val = (ret & LTR501_INTR_PRST_PS_MASK) >> 4;
+		break;
+	default:
+		break;
+	}
+
+	return ret < 0 ? ret : IIO_VAL_INT;
+}
+
+static int ltr501_write_intr_prst(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  u8 new_val)
+{
+	struct ltr501_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	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;
+}
+
 static const struct iio_event_spec ltr501_als_event_spec[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -141,7 +209,8 @@ static const struct iio_event_spec ltr501_als_event_spec[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
 		.dir = IIO_EV_DIR_EITHER,
-		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_PERSISTENCE),
 	},
 
 };
@@ -160,7 +229,8 @@ static const struct iio_event_spec ltr501_pxs_event_spec[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
 		.dir = IIO_EV_DIR_EITHER,
-		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_PERSISTENCE),
 	},
 };
 
@@ -423,6 +493,49 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev,
 	return ret < 0 ? ret : IIO_VAL_INT;
 }
 
+static int ltr501_read_event(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)
+{
+	*val2 = 0;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		return ltr501_read_thresh(indio_dev, chan, type, dir,
+					  info, val, val2);
+	case IIO_EV_INFO_PERSISTENCE:
+		return ltr501_read_intr_prst(indio_dev, chan, val);
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int ltr501_write_event(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)
+{
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		return ltr501_write_thresh(indio_dev, chan, type, dir,
+					   info, val, val2);
+	case IIO_EV_INFO_PERSISTENCE:
+		return ltr501_write_intr_prst(indio_dev, chan, val);
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+
 static int ltr501_read_event_config(struct iio_dev *indio_dev,
 		const struct iio_chan_spec *chan,
 		enum iio_event_type type,
@@ -502,8 +615,8 @@ 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_value	= &ltr501_read_event,
+	.write_event_value	= &ltr501_write_event,
 	.read_event_config	= &ltr501_read_event_config,
 	.write_event_config	= &ltr501_write_event_config,
 	.driver_module = THIS_MODULE,
-- 
1.9.1

[PATCH v3 6/6] 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 1b314f3..c8b375f 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>
@@ -834,6 +835,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 },
 	{ }
@@ -844,6 +851,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 v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[Jonathan Cameron]

On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
> Add ABI info for iio event persistence filter.
> 
> Setting <n> to persistence filter would need atleast
> <n> data values outside the upper/lower threshold
> limits before generating an interrupt.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 82 +++++++++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 9a70c31..590b1d4 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -856,6 +856,88 @@ Description:
>  		met before an event is generated. If direction is not
>  		specified then this period applies to both directions.
>  
Please don't mass document cases unless they are actually in use.  These should
get added as and when they become so.  Often a particular type of event
characteristic is actually only implemented on a small subset of channel types
(where it makes sense).
> +What:		/sys/.../events/in_accel_x_thresh_rising_persistence
> +What:		/sys/.../events/in_accel_x_thresh_falling_persistence
> +hat:		/sys/.../events/in_accel_x_roc_rising_persistence
> +What:		/sys/.../events/in_accel_x_roc_falling_persistence
> +What:		/sys/.../events/in_accel_y_thresh_rising_persistence
> +What:		/sys/.../events/in_accel_y_thresh_falling_persistence
> +What:		/sys/.../events/in_accel_y_roc_rising_persistence
> +What:		/sys/.../events/in_accel_y_roc_falling_persistence
> +What:		/sys/.../events/in_accel_z_thresh_rising_persistence
> +What:		/sys/.../events/in_accel_z_thresh_falling_persistence
> +What:		/sys/.../events/in_accel_z_roc_rising_persistence
> +What:		/sys/.../events/in_accel_z_roc_falling_persistence
> +What:		/sys/.../events/in_anglvel_x_thresh_rising_persistence
> +What:		/sys/.../events/in_anglvel_x_thresh_falling_persistence
> +What:		/sys/.../events/in_anglvel_x_roc_rising_persistence
> +What:		/sys/.../events/in_anglvel_x_roc_falling_persistence
> +What:		/sys/.../events/in_anglvel_y_thresh_rising_persistence
> +What:		/sys/.../events/in_anglvel_y_thresh_falling_persistence
> +What:		/sys/.../events/in_anglvel_y_roc_rising_persistence
> +What:		/sys/.../events/in_anglvel_y_roc_falling_persistence
> +What:		/sys/.../events/in_anglvel_z_thresh_rising_persistence
> +What:		/sys/.../events/in_anglvel_z_thresh_falling_persistence
> +What:		/sys/.../events/in_anglvel_z_roc_rising_persistence
> +What:		/sys/.../events/in_anglvel_z_roc_falling_persistence
> +What:		/sys/.../events/in_magn_x_thresh_rising_persistence
> +What:		/sys/.../events/in_magn_x_thresh_falling_persistence
> +What:		/sys/.../events/in_magn_x_roc_rising_persistence
> +What:		/sys/.../events/in_magn_x_roc_falling_persistence
> +What:		/sys/.../events/in_magn_y_thresh_rising_persistence
> +What:		/sys/.../events/in_magn_y_thresh_falling_persistence
> +What:		/sys/.../events/in_magn_y_roc_rising_persistence
> +What:		/sys/.../events/in_magn_y_roc_falling_persistence
> +What:		/sys/.../events/in_magn_z_thresh_rising_persistence
> +What:		/sys/.../events/in_magn_z_thresh_falling_persistence
> +What:		/sys/.../events/in_magn_z_roc_rising_persistence
> +What:		/sys/.../events/in_magn_z_roc_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_true_thresh_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_true_thresh_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_true_roc_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_true_roc_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
> +What:		/sys/.../events/in_voltageY_supply_thresh_rising_persistence
> +What:		/sys/.../events/in_voltageY_supply_thresh_falling_persistence
> +What:		/sys/.../events/in_voltageY_supply_roc_rising_persistence
> +What:		/sys/.../events/in_voltageY_supply_roc_falling_persistence
> +What:		/sys/.../events/in_voltageY_thresh_rising_persistence
> +What:		/sys/.../events/in_voltageY_thresh_falling_persistence
> +What:		/sys/.../events/in_voltageY_roc_rising_persistence
> +What:		/sys/.../events/in_voltageY_roc_falling_persistence
> +What:		/sys/.../events/in_tempY_thresh_rising_persistence
> +What:		/sys/.../events/in_tempY_thresh_falling_persistence
> +What:		/sys/.../events/in_tempY_roc_rising_persistence
> +What:		/sys/.../events/in_tempY_roc_falling_persistence
> +What:		/sys/.../events/in_accel_x&y&z_mag_falling_persistence
> +What:		/sys/.../events/in_intensity0_thresh_persistence
> +What:		/sys/.../events/in_proximity0_thresh_persistence
> +What:		/sys/.../events/in_activity_still_thresh_rising_persistence
> +What:		/sys/.../events/in_activity_still_thresh_falling_persistence
> +What:		/sys/.../events/in_activity_walking_thresh_rising_persistence
> +What:		/sys/.../events/in_activity_walking_thresh_falling_persistence
> +What:		/sys/.../events/in_activity_jogging_thresh_rising_persistence
> +What:		/sys/.../events/in_activity_jogging_thresh_falling_persistence
> +What:		/sys/.../events/in_activity_running_thresh_rising_persistence
> +What:		/sys/.../events/in_activity_running_thresh_falling_persistence
> +KernelVersion:	4.0
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Number of times an event should occur before generating an
> +		interrupt. Persistence filter value can be applied for both
> +		rising/falling threshold based interrupts.
This still needs clarification as reading the discussion I'm still a little unsure
what the condition is.  I don't think Lars' original query ever got cleanly answered.

Taking just the upper threshold...
The interpretations I can think of are:

1) tsl2591 use case (maps directly to period with the sampling frequency taken into
account).  
 - Too much light for N ALS cycling samples.  If it drops below the threshold
   the count is reset.
the tcs part that Daniel referenced is also this use case.

2) A new option which doesn't take account of the signal dropping below the threshold.
   So a count of number of times above the threshold.
 i.e. On each cycle, if over threshold, increment N.
      Evaluate if N is greater than 'persistence' then trigger an event.
  Why this would ever make sense on a light sensor is beyond me ;)  but there
  we are.  We have a related abi for in_steps_change_value, which fires
  an event, only every N steps.  It doesn't really adapt to this case though.

  The name persistence is definitely misleading if this is what the liteon parts
  do as that definitely implies case 1).

Welcome to my life, bludgeoning what appears to new ABI into what we already have!
(not a lot of point in standardized ABI otherwise!)

Jonathan


> +
>  What:		/sys/.../events/in_activity_still_thresh_rising_en
>  What:		/sys/.../events/in_activity_still_thresh_falling_en
>  What:		/sys/.../events/in_activity_walking_thresh_rising_en
> 

Re: [PATCH v3 4/6] iio: ltr501: Add interrupt support

[Jonathan Cameron]

On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
> 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>
Various bits and bobs inline.

Sorry it took me so long to get to this series.

> ---
>  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 883855a..8672962 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)
>
I think you got a bit carried away in here!
Some of these probably make sense as 'documenation'
but not all ofthem.

> +#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)
I'd just set the above to 0.  Same result, perhaps simpler
to read.
> +#define LTR501_INTR_POLARITY_LOGIC_1 BIT(2)
> +#define LTR501_INTR_MODE_MASK (BIT(1) | BIT(0))
Not used and obvious from other defs so drop the above.

> +#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
Never actually used.. Drop this last one.
> +
>  #define LTR501_PS_DATA_MASK 0x7ff
> +#define LTR501_PS_THRESH_MASK 0x7ff
> +#define LTR501_ALS_THRESH_MASK 0xffff
You define these and don't actually enforce them...
You should check the values being written and error out appropriately
if out of range.
>  
>  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;
> +
Hmm. Beginning to look like this driver could benefit from regmap
with it's caching and utility functions for this sort of thing.
Just a thought ;)
> +	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);
> @@ -89,6 +127,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),
> +	},
> +	{
Personally I prefer
}, {
but not that bothered so feel free to ignore if you are particular attached
to the newline.
> +		.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, \
> @@ -102,7 +177,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),\
It's pretty unusual for a device to support thresholds on both intensity
sensors...  Is that actually the case here given you only set one
threshold etc?
>  }
>  
>  static const struct iio_chan_spec ltr501_channels[] = {
> @@ -121,6 +198,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),
>  };
> @@ -238,6 +317,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);
Probably being a little paranoid about locking in the read path.
If it is racing with an upate, should still give a valid answer either
before or after the update just as it will with the race.
Dropping the locking will make the code flow a little simpler, particularly
when you handle the errors from i2c calls.

> +		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);
Hmm. Might be giberish, but as we would return the error I don't suppose
it matters. Not intuitive code though.
> +		*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) {
Bounds checking on the new_val?
> +		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;
return 0 for the write function not 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;
Just return this where it makes sense, don't bother with the
preassignment.
> +
> +	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);
  		if ret < 0
		   return ret;
		return ret & LTR501_INTR_MODE_ALS_MASK;

Would be slightly easier to read.  Can get carried away
with the ? operator and error path handling and sacrifice readability
to save a line or two.
		
> +		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:
		return -EINVAL;
> +		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));
This had me a little confused that it was clearing both interrupts initially
till I looked at the implemetnation of ltr501_set_intr_reg.

Perhaps clearer would be to use ~LTR501_INTR_MODE_ALS_MASK instead
of LTR501_INTR_MODE_NONE.  Mind you that's pretty convoluted so maybe just
an explicit 0 would be clearer.

> +		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");
>  
> @@ -251,10 +491,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,
>  };
>  
> @@ -319,6 +570,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);
Roll the two lines above into one as you never use the iio_dev in
this function.

struct ltr501_data *data = iio_priv(private);

That also avoids the fact that you aren't being consistent with existing
naming within the driver wehre the struct iio_dev is called indio_dev
(which somehow snuck through to become the standard choice in IIO - no
idea how ;)

> +	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,
The intensity channels both have modifiers.  The event code
should reflect that.
> +			       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;
> @@ -361,7 +642,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;
> @@ -371,6 +651,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)
> 

Re: [PATCH v3 5/6] iio: ltr501: Add interrupt rate control support

[Jonathan Cameron]

On 09/04/15 01:06, Kuppuswamy Sathyanarayanan 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.
> 
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Putting aside the exact interface question, various bits inline.
Be much more wary of incorrect inputs from userspace...
> ---
>  drivers/iio/light/ltr501.c | 121 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 117 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c
> index 8672962..1b314f3 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 */
Bit late to mention it but if these had been named to make it clear
they were register addresses would have made code reading slightly
easier.

LTR501_ADDR_ALS_THRESH_UP etc perhaps.
>  
>  #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
> @@ -127,6 +131,70 @@ static int ltr501_read_ps(struct ltr501_data *data)
>  	return i2c_smbus_read_word_data(data->client, LTR501_PS_DATA);
>  }
>  
> +static int ltr501_read_intr_prst(struct iio_dev *indio_dev,
> +				 const struct iio_chan_spec *chan,
> +				 int *val)
> +{
> +	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);
> +		if (ret >= 0)
> +			*val = ret & LTR501_INTR_PRST_ALS_MASK;
> +		break;
> +	case IIO_PROXIMITY:
> +		mutex_lock(&data->lock_ps);
> +		ret = i2c_smbus_read_byte_data(data->client, LTR501_INTR_PRST);
> +		mutex_unlock(&data->lock_ps);
Reverse this logic.  Always easier to review if the error path is the
if, adds a line, but worth it for readability and then return directly
from all case statements.
> +		if (ret >= 0)
> +			*val = (ret & LTR501_INTR_PRST_PS_MASK) >> 4;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret < 0 ? ret : IIO_VAL_INT;
> +}
> +
> +static int ltr501_write_intr_prst(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  u8 new_val)
> +{
> +	struct ltr501_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
Unnecessary assignment as overwritten in next line.
Are all of 0-255 valid for new_val? If not, then you need bounds checking here.
> +
> +	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;
> +}
> +
>  static const struct iio_event_spec ltr501_als_event_spec[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -141,7 +209,8 @@ static const struct iio_event_spec ltr501_als_event_spec[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
>  		.dir = IIO_EV_DIR_EITHER,
> -		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				 BIT(IIO_EV_INFO_PERSISTENCE),
>  	},
>  
>  };
> @@ -160,7 +229,8 @@ static const struct iio_event_spec ltr501_pxs_event_spec[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
>  		.dir = IIO_EV_DIR_EITHER,
> -		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> +				 BIT(IIO_EV_INFO_PERSISTENCE),
>  	},
>  };
>  
> @@ -423,6 +493,49 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev,
>  	return ret < 0 ? ret : IIO_VAL_INT;
>  }
>  
> +static int ltr501_read_event(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)
> +{
> +	*val2 = 0;
Why is this needed?  If you are returning IIO_INT_VAL it should be ignored
anyway.
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		return ltr501_read_thresh(indio_dev, chan, type, dir,
> +					  info, val, val2);
> +	case IIO_EV_INFO_PERSISTENCE:
> +		return ltr501_read_intr_prst(indio_dev, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ltr501_write_event(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)
> +{
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		return ltr501_write_thresh(indio_dev, chan, type, dir,
> +					   info, val, val2);
> +	case IIO_EV_INFO_PERSISTENCE:
Should sanity check that val2 is not provided and error out if it is not 0.
> +		return ltr501_write_intr_prst(indio_dev, chan, val);
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
Bonus white line to remove.
> +
>  static int ltr501_read_event_config(struct iio_dev *indio_dev,
>  		const struct iio_chan_spec *chan,
>  		enum iio_event_type type,
> @@ -502,8 +615,8 @@ 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_value	= &ltr501_read_event,
> +	.write_event_value	= &ltr501_write_event,
>  	.read_event_config	= &ltr501_read_event_config,
>  	.write_event_config	= &ltr501_write_event_config,
>  	.driver_module = THIS_MODULE,
> 

Re: [PATCH v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[sathyanarayanan kuppuswamy]

On 04/09/2015 03:33 AM, Jonathan Cameron wrote:
> On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
>> Add ABI info for iio event persistence filter.
>>
>> Setting <n> to persistence filter would need atleast
>> <n> data values outside the upper/lower threshold
>> limits before generating an interrupt.
>>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>   Documentation/ABI/testing/sysfs-bus-iio | 82 +++++++++++++++++++++++++++++++++
>>   1 file changed, 82 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index 9a70c31..590b1d4 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -856,6 +856,88 @@ Description:
>>   		met before an event is generated. If direction is not
>>   		specified then this period applies to both directions.
>>   
> Please don't mass document cases unless they are actually in use.  These should
> get added as and when they become so.  Often a particular type of event
> characteristic is actually only implemented on a small subset of channel types
> (where it makes sense).
IMO, persistence filter can be applied for all these cases. But if you 
think its better to
add ABI for what we currently use then I will leave only ALS/proxmity 
related ABI's here.

+What:		/sys/.../events/in_intensity0_thresh_persistence
+What:		/sys/.../events/in_proximity0_thresh_persistence

>> +What:		/sys/.../events/in_accel_x_thresh_rising_persistence
>> +What:		/sys/.../events/in_accel_x_thresh_falling_persistence
>> +hat:		/sys/.../events/in_accel_x_roc_rising_persistence
>> +What:		/sys/.../events/in_accel_x_roc_falling_persistence
>> +What:		/sys/.../events/in_accel_y_thresh_rising_persistence
>> +What:		/sys/.../events/in_accel_y_thresh_falling_persistence
>> +What:		/sys/.../events/in_accel_y_roc_rising_persistence
>> +What:		/sys/.../events/in_accel_y_roc_falling_persistence
>> +What:		/sys/.../events/in_accel_z_thresh_rising_persistence
>> +What:		/sys/.../events/in_accel_z_thresh_falling_persistence
>> +What:		/sys/.../events/in_accel_z_roc_rising_persistence
>> +What:		/sys/.../events/in_accel_z_roc_falling_persistence
>> +What:		/sys/.../events/in_anglvel_x_thresh_rising_persistence
>> +What:		/sys/.../events/in_anglvel_x_thresh_falling_persistence
>> +What:		/sys/.../events/in_anglvel_x_roc_rising_persistence
>> +What:		/sys/.../events/in_anglvel_x_roc_falling_persistence
>> +What:		/sys/.../events/in_anglvel_y_thresh_rising_persistence
>> +What:		/sys/.../events/in_anglvel_y_thresh_falling_persistence
>> +What:		/sys/.../events/in_anglvel_y_roc_rising_persistence
>> +What:		/sys/.../events/in_anglvel_y_roc_falling_persistence
>> +What:		/sys/.../events/in_anglvel_z_thresh_rising_persistence
>> +What:		/sys/.../events/in_anglvel_z_thresh_falling_persistence
>> +What:		/sys/.../events/in_anglvel_z_roc_rising_persistence
>> +What:		/sys/.../events/in_anglvel_z_roc_falling_persistence
>> +What:		/sys/.../events/in_magn_x_thresh_rising_persistence
>> +What:		/sys/.../events/in_magn_x_thresh_falling_persistence
>> +What:		/sys/.../events/in_magn_x_roc_rising_persistence
>> +What:		/sys/.../events/in_magn_x_roc_falling_persistence
>> +What:		/sys/.../events/in_magn_y_thresh_rising_persistence
>> +What:		/sys/.../events/in_magn_y_thresh_falling_persistence
>> +What:		/sys/.../events/in_magn_y_roc_rising_persistence
>> +What:		/sys/.../events/in_magn_y_roc_falling_persistence
>> +What:		/sys/.../events/in_magn_z_thresh_rising_persistence
>> +What:		/sys/.../events/in_magn_z_thresh_falling_persistence
>> +What:		/sys/.../events/in_magn_z_roc_rising_persistence
>> +What:		/sys/.../events/in_magn_z_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
>> +What:		/sys/.../events/in_voltageY_supply_thresh_rising_persistence
>> +What:		/sys/.../events/in_voltageY_supply_thresh_falling_persistence
>> +What:		/sys/.../events/in_voltageY_supply_roc_rising_persistence
>> +What:		/sys/.../events/in_voltageY_supply_roc_falling_persistence
>> +What:		/sys/.../events/in_voltageY_thresh_rising_persistence
>> +What:		/sys/.../events/in_voltageY_thresh_falling_persistence
>> +What:		/sys/.../events/in_voltageY_roc_rising_persistence
>> +What:		/sys/.../events/in_voltageY_roc_falling_persistence
>> +What:		/sys/.../events/in_tempY_thresh_rising_persistence
>> +What:		/sys/.../events/in_tempY_thresh_falling_persistence
>> +What:		/sys/.../events/in_tempY_roc_rising_persistence
>> +What:		/sys/.../events/in_tempY_roc_falling_persistence
>> +What:		/sys/.../events/in_accel_x&y&z_mag_falling_persistence
>> +What:		/sys/.../events/in_intensity0_thresh_persistence
>> +What:		/sys/.../events/in_proximity0_thresh_persistence
>> +What:		/sys/.../events/in_activity_still_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_still_thresh_falling_persistence
>> +What:		/sys/.../events/in_activity_walking_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_walking_thresh_falling_persistence
>> +What:		/sys/.../events/in_activity_jogging_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_jogging_thresh_falling_persistence
>> +What:		/sys/.../events/in_activity_running_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_running_thresh_falling_persistence
>> +KernelVersion:	4.0
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +		Number of times an event should occur before generating an
>> +		interrupt. Persistence filter value can be applied for both
>> +		rising/falling threshold based interrupts.
> This still needs clarification as reading the discussion I'm still a little unsure
> what the condition is.  I don't think Lars' original query ever got cleanly answered
Sorry for being unclear. Even in this case, the use case is "number of 
consecutive" measurement data
outside the range.
>
> Taking just the upper threshold...
> The interpretations I can think of are:
>
> 1) tsl2591 use case (maps directly to period with the sampling frequency taken into
> account).
>   - Too much light for N ALS cycling samples.  If it drops below the threshold
>     the count is reset.
> the tcs part that Daniel referenced is also this use case.
I agree that it can be mapped to _period if you take the sampling 
frequency into account. But if you do that then it becomes dependent on 
integration time ( for ALS case). Since _period is calculated based on 
frequency, each time you change _integration_time then internally you 
need to change the _period value to keep the ratio constant. Also you 
need to consider what happens if your frequency > period. I think this 
makes is bit complicated.

Don't you think its better to add another representation instead of 
trying to fit it in existing ABI's ?

Following are the persistence filter descriptions from data sheets of 
tsl2591 and ltr501.

LTR501: " The INTERRUPT PERSIST register controls the N number of times 
the measurement data is outside the range defined by the upper and lower 
threshold limits before asserting the INT".
TSL2591: " The Interrupt persistence filter sets the number of 
consecutive out-of-range ALS cycles necessary to generate an interrupt"

>
> 2) A new option which doesn't take account of the signal dropping below the threshold.
>     So a count of number of times above the threshold.
>   i.e. On each cycle, if over threshold, increment N.
>        Evaluate if N is greater than 'persistence' then trigger an event.
>    Why this would ever make sense on a light sensor is beyond me ;)  but there
>    we are.  We have a related abi for in_steps_change_value, which fires
>    an event, only every N steps.  It doesn't really adapt to this case though.
>
>    The name persistence is definitely misleading if this is what the liteon parts
>    do as that definitely implies case 1).
>
> Welcome to my life, bludgeoning what appears to new ABI into what we already have!
> (not a lot of point in standardized ABI otherwise!)
>
> Jonathan
>
>
>> +
>>   What:		/sys/.../events/in_activity_still_thresh_rising_en
>>   What:		/sys/.../events/in_activity_still_thresh_falling_en
>>   What:		/sys/.../events/in_activity_walking_thresh_rising_en
>>
>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[Jonathan Cameron]

On 10 April 2015 00:30:53 BST, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>On 04/09/2015 03:33 AM, Jonathan Cameron wrote:
>> On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
>>> Add ABI info for iio event persistence filter.
>>>
>>> Setting <n> to persistence filter would need atleast
>>> <n> data values outside the upper/lower threshold
>>> limits before generating an interrupt.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
><sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>   Documentation/ABI/testing/sysfs-bus-iio | 82
>+++++++++++++++++++++++++++++++++
>>>   1 file changed, 82 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 9a70c31..590b1d4 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -856,6 +856,88 @@ Description:
>>>   		met before an event is generated. If direction is not
>>>   		specified then this period applies to both directions.
>>>   
>> Please don't mass document cases unless they are actually in use. 
>These should
>> get added as and when they become so.  Often a particular type of
>event
>> characteristic is actually only implemented on a small subset of
>channel types
>> (where it makes sense).
>IMO, persistence filter can be applied for all these cases. But if you 
>think its better to
>add ABI for what we currently use then I will leave only ALS/proxmity 
>related ABI's here.
>
>+What:		/sys/.../events/in_intensity0_thresh_persistence
>+What:		/sys/.../events/in_proximity0_thresh_persistence
>
>>> +What:		/sys/.../events/in_accel_x_thresh_rising_persistence
>>> +What:		/sys/.../events/in_accel_x_thresh_falling_persistence
>>> +hat:		/sys/.../events/in_accel_x_roc_rising_persistence
>>> +What:		/sys/.../events/in_accel_x_roc_falling_persistence
>>> +What:		/sys/.../events/in_accel_y_thresh_rising_persistence
>>> +What:		/sys/.../events/in_accel_y_thresh_falling_persistence
>>> +What:		/sys/.../events/in_accel_y_roc_rising_persistence
>>> +What:		/sys/.../events/in_accel_y_roc_falling_persistence
>>> +What:		/sys/.../events/in_accel_z_thresh_rising_persistence
>>> +What:		/sys/.../events/in_accel_z_thresh_falling_persistence
>>> +What:		/sys/.../events/in_accel_z_roc_rising_persistence
>>> +What:		/sys/.../events/in_accel_z_roc_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_x_thresh_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_x_thresh_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_x_roc_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_x_roc_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_y_thresh_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_y_thresh_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_y_roc_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_y_roc_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_z_thresh_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_z_thresh_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_z_roc_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_z_roc_falling_persistence
>>> +What:		/sys/.../events/in_magn_x_thresh_rising_persistence
>>> +What:		/sys/.../events/in_magn_x_thresh_falling_persistence
>>> +What:		/sys/.../events/in_magn_x_roc_rising_persistence
>>> +What:		/sys/.../events/in_magn_x_roc_falling_persistence
>>> +What:		/sys/.../events/in_magn_y_thresh_rising_persistence
>>> +What:		/sys/.../events/in_magn_y_thresh_falling_persistence
>>> +What:		/sys/.../events/in_magn_y_roc_rising_persistence
>>> +What:		/sys/.../events/in_magn_y_roc_falling_persistence
>>> +What:		/sys/.../events/in_magn_z_thresh_rising_persistence
>>> +What:		/sys/.../events/in_magn_z_thresh_falling_persistence
>>> +What:		/sys/.../events/in_magn_z_roc_rising_persistence
>>> +What:		/sys/.../events/in_magn_z_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_supply_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_voltageY_supply_thresh_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_supply_roc_rising_persistence
>>> +What:		/sys/.../events/in_voltageY_supply_roc_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_thresh_rising_persistence
>>> +What:		/sys/.../events/in_voltageY_thresh_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_roc_rising_persistence
>>> +What:		/sys/.../events/in_voltageY_roc_falling_persistence
>>> +What:		/sys/.../events/in_tempY_thresh_rising_persistence
>>> +What:		/sys/.../events/in_tempY_thresh_falling_persistence
>>> +What:		/sys/.../events/in_tempY_roc_rising_persistence
>>> +What:		/sys/.../events/in_tempY_roc_falling_persistence
>>> +What:		/sys/.../events/in_accel_x&y&z_mag_falling_persistence
>>> +What:		/sys/.../events/in_intensity0_thresh_persistence
>>> +What:		/sys/.../events/in_proximity0_thresh_persistence
>>> +What:		/sys/.../events/in_activity_still_thresh_rising_persistence
>>> +What:		/sys/.../events/in_activity_still_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_activity_walking_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_activity_walking_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_activity_jogging_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_activity_jogging_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_activity_running_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_activity_running_thresh_falling_persistence
>>> +KernelVersion:	4.0
>>> +Contact:	linux-iio@vger.kernel.org
>>> +Description:
>>> +		Number of times an event should occur before generating an
>>> +		interrupt. Persistence filter value can be applied for both
>>> +		rising/falling threshold based interrupts.
>> This still needs clarification as reading the discussion I'm still a
>little unsure
>> what the condition is.  I don't think Lars' original query ever got
>cleanly answered
>Sorry for being unclear. Even in this case, the use case is "number of 
>consecutive" measurement data
>outside the range.
>>
>> Taking just the upper threshold...
>> The interpretations I can think of are:
>>
>> 1) tsl2591 use case (maps directly to period with the sampling
>frequency taken into
>> account).
>>   - Too much light for N ALS cycling samples.  If it drops below the
>threshold
>>     the count is reset.
>> the tcs part that Daniel referenced is also this use case.
>I agree that it can be mapped to _period if you take the sampling 
>frequency into account. But if you do that then it becomes dependent on
>
>integration time ( for ALS case). Since _period is calculated based on 
>frequency, each time you change _integration_time then internally you 
>need to change the _period value to keep the ratio constant. Also you 
>need to consider what happens if your frequency > period. I think this 
>makes is bit complicated.

Indeed. Such is life. This isn't the first such case and it won't be the last where
 the driver becomes more complex to conform to the existing ABI.


>
>Don't you think its better to add another representation instead of 
>trying to fit it in existing ABI's ?
No
This is a common situation. All over the place we have cases where changing one
 abi element effects the value of another.
We cope with it to provide a consistent
 and minimal interface to userspace.
Where the sampling frequency is directly
 controlled it is perfectly acceptable to not automatically adjust the period.
We frequently rely on the rule that userspce must check all attrs if it changes any.
>
>Following are the persistence filter descriptions from data sheets of 
>tsl2591 and ltr501.
>
>LTR501: " The INTERRUPT PERSIST register controls the N number of times
>
>the measurement data is outside the range defined by the upper and
>lower 
>threshold limits before asserting the INT".
>TSL2591: " The Interrupt persistence filter sets the number of 
>consecutive out-of-range ALS cycles necessary to generate an interrupt"
>
>>
>> 2) A new option which doesn't take account of the signal dropping
>below the threshold.
>>     So a count of number of times above the threshold.
>>   i.e. On each cycle, if over threshold, increment N.
>>        Evaluate if N is greater than 'persistence' then trigger an
>event.
>>    Why this would ever make sense on a light sensor is beyond me ;) 
>but there
>>    we are.  We have a related abi for in_steps_change_value, which
>fires
>>    an event, only every N steps.  It doesn't really adapt to this
>case though.
>>
>>    The name persistence is definitely misleading if this is what the
>liteon parts
>>    do as that definitely implies case 1).
>>
>> Welcome to my life, bludgeoning what appears to new ABI into what we
>already have!
>> (not a lot of point in standardized ABI otherwise!)
>>
>> Jonathan
>>
>>
>>> +
>>>   What:		/sys/.../events/in_activity_still_thresh_rising_en
>>>   What:		/sys/.../events/in_activity_still_thresh_falling_en
>>>   What:		/sys/.../events/in_activity_walking_thresh_rising_en
>>>
>>

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

Re: [PATCH v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[Jonathan Cameron]

On 10 April 2015 00:30:53 BST, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
>On 04/09/2015 03:33 AM, Jonathan Cameron wrote:
>> On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
>>> Add ABI info for iio event persistence filter.
>>>
>>> Setting <n> to persistence filter would need atleast
>>> <n> data values outside the upper/lower threshold
>>> limits before generating an interrupt.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan
><sathyanarayanan.kuppuswamy@linux.intel.com>
>>> ---
>>>   Documentation/ABI/testing/sysfs-bus-iio | 82
>+++++++++++++++++++++++++++++++++
>>>   1 file changed, 82 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>b/Documentation/ABI/testing/sysfs-bus-iio
>>> index 9a70c31..590b1d4 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -856,6 +856,88 @@ Description:
>>>   		met before an event is generated. If direction is not
>>>   		specified then this period applies to both directions.
>>>   
>> Please don't mass document cases unless they are actually in use. 
>These should
>> get added as and when they become so.  Often a particular type of
>event
>> characteristic is actually only implemented on a small subset of
>channel types
>> (where it makes sense).
>IMO, persistence filter can be applied for all these cases. 
Sure it can, but we try to only document ABI when it is use rather than bulk adding it.
That keeps us from getting unmanageably large doc changes when a
 new type is added.
Pitty there is no neat wildcard type format for these docs!
>But if you 
>think its better to
>add ABI for what we currently use then I will leave only ALS/proxmity 
>related ABI's here.
>
>+What:		/sys/.../events/in_intensity0_thresh_persistence
>+What:		/sys/.../events/in_proximity0_thresh_persistence
>
>>> +What:		/sys/.../events/in_accel_x_thresh_rising_persistence
>>> +What:		/sys/.../events/in_accel_x_thresh_falling_persistence
>>> +hat:		/sys/.../events/in_accel_x_roc_rising_persistence
>>> +What:		/sys/.../events/in_accel_x_roc_falling_persistence
>>> +What:		/sys/.../events/in_accel_y_thresh_rising_persistence
>>> +What:		/sys/.../events/in_accel_y_thresh_falling_persistence
>>> +What:		/sys/.../events/in_accel_y_roc_rising_persistence
>>> +What:		/sys/.../events/in_accel_y_roc_falling_persistence
>>> +What:		/sys/.../events/in_accel_z_thresh_rising_persistence
>>> +What:		/sys/.../events/in_accel_z_thresh_falling_persistence
>>> +What:		/sys/.../events/in_accel_z_roc_rising_persistence
>>> +What:		/sys/.../events/in_accel_z_roc_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_x_thresh_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_x_thresh_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_x_roc_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_x_roc_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_y_thresh_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_y_thresh_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_y_roc_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_y_roc_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_z_thresh_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_z_thresh_falling_persistence
>>> +What:		/sys/.../events/in_anglvel_z_roc_rising_persistence
>>> +What:		/sys/.../events/in_anglvel_z_roc_falling_persistence
>>> +What:		/sys/.../events/in_magn_x_thresh_rising_persistence
>>> +What:		/sys/.../events/in_magn_x_thresh_falling_persistence
>>> +What:		/sys/.../events/in_magn_x_roc_rising_persistence
>>> +What:		/sys/.../events/in_magn_x_roc_falling_persistence
>>> +What:		/sys/.../events/in_magn_y_thresh_rising_persistence
>>> +What:		/sys/.../events/in_magn_y_thresh_falling_persistence
>>> +What:		/sys/.../events/in_magn_y_roc_rising_persistence
>>> +What:		/sys/.../events/in_magn_y_roc_falling_persistence
>>> +What:		/sys/.../events/in_magn_z_thresh_rising_persistence
>>> +What:		/sys/.../events/in_magn_z_thresh_falling_persistence
>>> +What:		/sys/.../events/in_magn_z_roc_rising_persistence
>>> +What:		/sys/.../events/in_magn_z_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
>>>
>+What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_supply_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_voltageY_supply_thresh_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_supply_roc_rising_persistence
>>> +What:		/sys/.../events/in_voltageY_supply_roc_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_thresh_rising_persistence
>>> +What:		/sys/.../events/in_voltageY_thresh_falling_persistence
>>> +What:		/sys/.../events/in_voltageY_roc_rising_persistence
>>> +What:		/sys/.../events/in_voltageY_roc_falling_persistence
>>> +What:		/sys/.../events/in_tempY_thresh_rising_persistence
>>> +What:		/sys/.../events/in_tempY_thresh_falling_persistence
>>> +What:		/sys/.../events/in_tempY_roc_rising_persistence
>>> +What:		/sys/.../events/in_tempY_roc_falling_persistence
>>> +What:		/sys/.../events/in_accel_x&y&z_mag_falling_persistence
>>> +What:		/sys/.../events/in_intensity0_thresh_persistence
>>> +What:		/sys/.../events/in_proximity0_thresh_persistence
>>> +What:		/sys/.../events/in_activity_still_thresh_rising_persistence
>>> +What:		/sys/.../events/in_activity_still_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_activity_walking_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_activity_walking_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_activity_jogging_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_activity_jogging_thresh_falling_persistence
>>>
>+What:		/sys/.../events/in_activity_running_thresh_rising_persistence
>>>
>+What:		/sys/.../events/in_activity_running_thresh_falling_persistence
>>> +KernelVersion:	4.0
>>> +Contact:	linux-iio@vger.kernel.org
>>> +Description:
>>> +		Number of times an event should occur before generating an
>>> +		interrupt. Persistence filter value can be applied for both
>>> +		rising/falling threshold based interrupts.
>> This still needs clarification as reading the discussion I'm still a
>little unsure
>> what the condition is.  I don't think Lars' original query ever got
>cleanly answered
>Sorry for being unclear. Even in this case, the use case is "number of 
>consecutive" measurement data
>outside the range.
>>
>> Taking just the upper threshold...
>> The interpretations I can think of are:
>>
>> 1) tsl2591 use case (maps directly to period with the sampling
>frequency taken into
>> account).
>>   - Too much light for N ALS cycling samples.  If it drops below the
>threshold
>>     the count is reset.
>> the tcs part that Daniel referenced is also this use case.
>I agree that it can be mapped to _period if you take the sampling 
>frequency into account. But if you do that then it becomes dependent on
>
>integration time ( for ALS case). Since _period is calculated based on 
>frequency, each time you change _integration_time then internally you 
>need to change the _period value to keep the ratio constant. Also you 
>need to consider what happens if your frequency > period. I think this 
>makes is bit complicated.
>
>Don't you think its better to add another representation instead of 
>trying to fit it in existing ABI's ?
>
>Following are the persistence filter descriptions from data sheets of 
>tsl2591 and ltr501.
>
>LTR501: " The INTERRUPT PERSIST register controls the N number of times
>
>the measurement data is outside the range defined by the upper and
>lower 
>threshold limits before asserting the INT".
>TSL2591: " The Interrupt persistence filter sets the number of 
>consecutive out-of-range ALS cycles necessary to generate an interrupt"
>
>>
>> 2) A new option which doesn't take account of the signal dropping
>below the threshold.
>>     So a count of number of times above the threshold.
>>   i.e. On each cycle, if over threshold, increment N.
>>        Evaluate if N is greater than 'persistence' then trigger an
>event.
>>    Why this would ever make sense on a light sensor is beyond me ;) 
>but there
>>    we are.  We have a related abi for in_steps_change_value, which
>fires
>>    an event, only every N steps.  It doesn't really adapt to this
>case though.
>>
>>    The name persistence is definitely misleading if this is what the
>liteon parts
>>    do as that definitely implies case 1).
>>
>> Welcome to my life, bludgeoning what appears to new ABI into what we
>already have!
>> (not a lot of point in standardized ABI otherwise!)
>>
>> Jonathan
>>
>>
>>> +
>>>   What:		/sys/.../events/in_activity_still_thresh_rising_en
>>>   What:		/sys/.../events/in_activity_still_thresh_falling_en
>>>   What:		/sys/.../events/in_activity_walking_thresh_rising_en
>>>
>>

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

Re: [PATCH v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[sathyanarayanan kuppuswamy]

Hi Jonathan,

Thanks for the comments. Comments inline.

On 04/09/2015 10:58 PM, Jonathan Cameron wrote:
>
> On 10 April 2015 00:30:53 BST, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 04/09/2015 03:33 AM, Jonathan Cameron wrote:
>>> On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
>>>> Add ABI info for iio event persistence filter.
>>>>
>>>> Setting <n> to persistence filter would need atleast
>>>> <n> data values outside the upper/lower threshold
>>>> limits before generating an interrupt.
>>>>
>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>> ---
>>>>    Documentation/ABI/testing/sysfs-bus-iio | 82
>> +++++++++++++++++++++++++++++++++
>>>>    1 file changed, 82 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>> b/Documentation/ABI/testing/sysfs-bus-iio
>>>> index 9a70c31..590b1d4 100644
>>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>>> @@ -856,6 +856,88 @@ Description:
>>>>    		met before an event is generated. If direction is not
>>>>    		specified then this period applies to both directions.
>>>>    
>>> Please don't mass document cases unless they are actually in use.
>> These should
>>> get added as and when they become so.  Often a particular type of
>> event
>>> characteristic is actually only implemented on a small subset of
>> channel types
>>> (where it makes sense).
>> IMO, persistence filter can be applied for all these cases. But if you
>> think its better to
>> add ABI for what we currently use then I will leave only ALS/proxmity
>> related ABI's here.
>>
>> +What:		/sys/.../events/in_intensity0_thresh_persistence
>> +What:		/sys/.../events/in_proximity0_thresh_persistence
>>
>>>> +What:		/sys/.../events/in_accel_x_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_accel_x_thresh_falling_persistence
>>>> +hat:		/sys/.../events/in_accel_x_roc_rising_persistence
>>>> +What:		/sys/.../events/in_accel_x_roc_falling_persistence
>>>> +What:		/sys/.../events/in_accel_y_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_accel_y_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_accel_y_roc_rising_persistence
>>>> +What:		/sys/.../events/in_accel_y_roc_falling_persistence
>>>> +What:		/sys/.../events/in_accel_z_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_accel_z_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_accel_z_roc_rising_persistence
>>>> +What:		/sys/.../events/in_accel_z_roc_falling_persistence
>>>> +What:		/sys/.../events/in_anglvel_x_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_anglvel_x_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_anglvel_x_roc_rising_persistence
>>>> +What:		/sys/.../events/in_anglvel_x_roc_falling_persistence
>>>> +What:		/sys/.../events/in_anglvel_y_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_anglvel_y_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_anglvel_y_roc_rising_persistence
>>>> +What:		/sys/.../events/in_anglvel_y_roc_falling_persistence
>>>> +What:		/sys/.../events/in_anglvel_z_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_anglvel_z_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_anglvel_z_roc_rising_persistence
>>>> +What:		/sys/.../events/in_anglvel_z_roc_falling_persistence
>>>> +What:		/sys/.../events/in_magn_x_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_magn_x_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_magn_x_roc_rising_persistence
>>>> +What:		/sys/.../events/in_magn_x_roc_falling_persistence
>>>> +What:		/sys/.../events/in_magn_y_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_magn_y_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_magn_y_roc_rising_persistence
>>>> +What:		/sys/.../events/in_magn_y_roc_falling_persistence
>>>> +What:		/sys/.../events/in_magn_z_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_magn_z_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_magn_z_roc_rising_persistence
>>>> +What:		/sys/.../events/in_magn_z_roc_falling_persistence
>>>>
>> +What:		/sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
>> +What:		/sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
>>>> +What:		/sys/.../events/in_voltageY_supply_thresh_rising_persistence
>>>>
>> +What:		/sys/.../events/in_voltageY_supply_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_voltageY_supply_roc_rising_persistence
>>>> +What:		/sys/.../events/in_voltageY_supply_roc_falling_persistence
>>>> +What:		/sys/.../events/in_voltageY_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_voltageY_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_voltageY_roc_rising_persistence
>>>> +What:		/sys/.../events/in_voltageY_roc_falling_persistence
>>>> +What:		/sys/.../events/in_tempY_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_tempY_thresh_falling_persistence
>>>> +What:		/sys/.../events/in_tempY_roc_rising_persistence
>>>> +What:		/sys/.../events/in_tempY_roc_falling_persistence
>>>> +What:		/sys/.../events/in_accel_x&y&z_mag_falling_persistence
>>>> +What:		/sys/.../events/in_intensity0_thresh_persistence
>>>> +What:		/sys/.../events/in_proximity0_thresh_persistence
>>>> +What:		/sys/.../events/in_activity_still_thresh_rising_persistence
>>>> +What:		/sys/.../events/in_activity_still_thresh_falling_persistence
>>>>
>> +What:		/sys/.../events/in_activity_walking_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_walking_thresh_falling_persistence
>> +What:		/sys/.../events/in_activity_jogging_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_jogging_thresh_falling_persistence
>> +What:		/sys/.../events/in_activity_running_thresh_rising_persistence
>> +What:		/sys/.../events/in_activity_running_thresh_falling_persistence
>>>> +KernelVersion:	4.0
>>>> +Contact:	linux-iio@vger.kernel.org
>>>> +Description:
>>>> +		Number of times an event should occur before generating an
>>>> +		interrupt. Persistence filter value can be applied for both
>>>> +		rising/falling threshold based interrupts.
>>> This still needs clarification as reading the discussion I'm still a
>> little unsure
>>> what the condition is.  I don't think Lars' original query ever got
>> cleanly answered
>> Sorry for being unclear. Even in this case, the use case is "number of
>> consecutive" measurement data
>> outside the range.
>>> Taking just the upper threshold...
>>> The interpretations I can think of are:
>>>
>>> 1) tsl2591 use case (maps directly to period with the sampling
>> frequency taken into
>>> account).
>>>    - Too much light for N ALS cycling samples.  If it drops below the
>> threshold
>>>      the count is reset.
>>> the tcs part that Daniel referenced is also this use case.
>> I agree that it can be mapped to _period if you take the sampling
>> frequency into account. But if you do that then it becomes dependent on
>>
>> integration time ( for ALS case). Since _period is calculated based on
>> frequency, each time you change _integration_time then internally you
>> need to change the _period value to keep the ratio constant. Also you
>> need to consider what happens if your frequency > period. I think this
>> makes is bit complicated.
> Indeed. Such is life. This isn't the first such case and it won't be the last where
>   the driver becomes more complex to conform to the existing ABI.
If its just the driver complexity then I would completely agree with 
you. My concern is, we are expecting the user to consider these factors 
before changing the value on either frequency/period.

If this dependency is due to the hardware then it would make sense for 
expecting the user to follow them. But here we are creating a logical 
dependency between persistence and sampling frequency to avoid the 
software complexity.

If you read the data sheet, there is no relation between persistence and 
sampling frequency. But when using the interfaces, the user should 
understand the ABI dependency created in software.

>
>> Don't you think its better to add another representation instead of
>> trying to fit it in existing ABI's ?
> No
> This is a common situation. All over the place we have cases where changing one
>   abi element effects the value of another.
> We cope with it to provide a consistent
>   and minimal interface to userspace.
> Where the sampling frequency is directly
>   controlled it is perfectly acceptable to not automatically adjust the period.
> We frequently rely on the rule that userspce must check all attrs if it changes any.
>> Following are the persistence filter descriptions from data sheets of
>> tsl2591 and ltr501.
>>
>> LTR501: " The INTERRUPT PERSIST register controls the N number of times
>>
>> the measurement data is outside the range defined by the upper and
>> lower
>> threshold limits before asserting the INT".
>> TSL2591: " The Interrupt persistence filter sets the number of
>> consecutive out-of-range ALS cycles necessary to generate an interrupt"
>>
>>> 2) A new option which doesn't take account of the signal dropping
>> below the threshold.
>>>      So a count of number of times above the threshold.
>>>    i.e. On each cycle, if over threshold, increment N.
>>>         Evaluate if N is greater than 'persistence' then trigger an
>> event.
>>>     Why this would ever make sense on a light sensor is beyond me ;)
>> but there
>>>     we are.  We have a related abi for in_steps_change_value, which
>> fires
>>>     an event, only every N steps.  It doesn't really adapt to this
>> case though.
>>>     The name persistence is definitely misleading if this is what the
>> liteon parts
>>>     do as that definitely implies case 1).
>>>
>>> Welcome to my life, bludgeoning what appears to new ABI into what we
>> already have!
>>> (not a lot of point in standardized ABI otherwise!)
>>>
>>> Jonathan
>>>
>>>
>>>> +
>>>>    What:		/sys/.../events/in_activity_still_thresh_rising_en
>>>>    What:		/sys/.../events/in_activity_still_thresh_falling_en
>>>>    What:		/sys/.../events/in_activity_walking_thresh_rising_en
>>>>

-- 
Sathyanarayanan Kuppuswamy
Android kernel developer

Re: [PATCH v3 2/6] iio: documentation: Add ABI info for iio event persistence filter

[Jonathan Cameron]

On 10/04/15 19:52, sathyanarayanan kuppuswamy wrote:
> 
> Hi Jonathan,
> 
> Thanks for the comments. Comments inline.
Hi again,
> 
> On 04/09/2015 10:58 PM, Jonathan Cameron wrote:
>>
>> On 10 April 2015 00:30:53 BST, sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>
>>> On 04/09/2015 03:33 AM, Jonathan Cameron wrote:
>>>> On 09/04/15 01:06, Kuppuswamy Sathyanarayanan wrote:
>>>>> Add ABI info for iio event persistence filter.
>>>>>
>>>>> Setting <n> to persistence filter would need atleast
>>>>> <n> data values outside the upper/lower threshold
>>>>> limits before generating an interrupt.
>>>>>
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> ---
>>>>>    Documentation/ABI/testing/sysfs-bus-iio | 82
>>> +++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 82 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio
>>> b/Documentation/ABI/testing/sysfs-bus-iio
>>>>> index 9a70c31..590b1d4 100644
>>>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>>>> @@ -856,6 +856,88 @@ Description:
>>>>>            met before an event is generated. If direction is not
>>>>>            specified then this period applies to both directions.
>>>>>    
>>>> Please don't mass document cases unless they are actually in use.
>>> These should
>>>> get added as and when they become so.  Often a particular type of
>>> event
>>>> characteristic is actually only implemented on a small subset of
>>> channel types
>>>> (where it makes sense).
>>> IMO, persistence filter can be applied for all these cases. But if you
>>> think its better to
>>> add ABI for what we currently use then I will leave only ALS/proxmity
>>> related ABI's here.
>>>
>>> +What:        /sys/.../events/in_intensity0_thresh_persistence
>>> +What:        /sys/.../events/in_proximity0_thresh_persistence
>>>
>>>>> +What:        /sys/.../events/in_accel_x_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_accel_x_thresh_falling_persistence
>>>>> +hat:        /sys/.../events/in_accel_x_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_accel_x_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_accel_y_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_accel_y_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_accel_y_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_accel_y_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_accel_z_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_accel_z_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_accel_z_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_accel_z_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_anglvel_x_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_anglvel_x_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_anglvel_x_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_anglvel_x_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_anglvel_y_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_anglvel_y_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_anglvel_y_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_anglvel_y_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_anglvel_z_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_anglvel_z_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_anglvel_z_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_anglvel_z_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_magn_x_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_magn_x_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_magn_x_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_magn_x_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_magn_y_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_magn_y_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_magn_y_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_magn_y_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_magn_z_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_magn_z_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_magn_z_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_magn_z_roc_falling_persistence
>>>>>
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_thresh_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_thresh_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_roc_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_roc_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_thresh_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_thresh_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_roc_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_roc_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_persistence
>>> +What:        /sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_voltageY_supply_thresh_rising_persistence
>>>>>
>>> +What:        /sys/.../events/in_voltageY_supply_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_voltageY_supply_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_voltageY_supply_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_voltageY_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_voltageY_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_voltageY_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_voltageY_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_tempY_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_tempY_thresh_falling_persistence
>>>>> +What:        /sys/.../events/in_tempY_roc_rising_persistence
>>>>> +What:        /sys/.../events/in_tempY_roc_falling_persistence
>>>>> +What:        /sys/.../events/in_accel_x&y&z_mag_falling_persistence
>>>>> +What:        /sys/.../events/in_intensity0_thresh_persistence
>>>>> +What:        /sys/.../events/in_proximity0_thresh_persistence
>>>>> +What:        /sys/.../events/in_activity_still_thresh_rising_persistence
>>>>> +What:        /sys/.../events/in_activity_still_thresh_falling_persistence
>>>>>
>>> +What:        /sys/.../events/in_activity_walking_thresh_rising_persistence
>>> +What:        /sys/.../events/in_activity_walking_thresh_falling_persistence
>>> +What:        /sys/.../events/in_activity_jogging_thresh_rising_persistence
>>> +What:        /sys/.../events/in_activity_jogging_thresh_falling_persistence
>>> +What:        /sys/.../events/in_activity_running_thresh_rising_persistence
>>> +What:        /sys/.../events/in_activity_running_thresh_falling_persistence
>>>>> +KernelVersion:    4.0
>>>>> +Contact:    linux-iio@vger.kernel.org
>>>>> +Description:
>>>>> +        Number of times an event should occur before generating an
>>>>> +        interrupt. Persistence filter value can be applied for both
>>>>> +        rising/falling threshold based interrupts.
>>>> This still needs clarification as reading the discussion I'm still a
>>> little unsure
>>>> what the condition is.  I don't think Lars' original query ever got
>>> cleanly answered
>>> Sorry for being unclear. Even in this case, the use case is "number of
>>> consecutive" measurement data
>>> outside the range.
>>>> Taking just the upper threshold...
>>>> The interpretations I can think of are:
>>>>
>>>> 1) tsl2591 use case (maps directly to period with the sampling
>>> frequency taken into
>>>> account).
>>>>    - Too much light for N ALS cycling samples.  If it drops below the
>>> threshold
>>>>      the count is reset.
>>>> the tcs part that Daniel referenced is also this use case.
>>> I agree that it can be mapped to _period if you take the sampling
>>> frequency into account. But if you do that then it becomes dependent on
>>>
>>> integration time ( for ALS case). Since _period is calculated based on
>>> frequency, each time you change _integration_time then internally you
>>> need to change the _period value to keep the ratio constant. Also you
>>> need to consider what happens if your frequency > period. I think this
>>> makes is bit complicated.
>> Indeed. Such is life. This isn't the first such case and it won't be the last where
>>   the driver becomes more complex to conform to the existing ABI.
> If its just the driver complexity then I would completely agree with
> you. My concern is, we are expecting the user to consider these
> factors before changing the value on either frequency/period.
Think of likely usecases for persistence. It's about avoiding getting
too many events for light level changes.  Two reasons for this
1) Wastes power and time on processor handling them.
2) Userspace will end up doing dumb things like changing screen brightness
in response to ever event.

Conceptually both of these want a time / frequency based control, not
one that is dependent on the number of times the device happens to
sample the sensor.

So if you put your persistence measurement in and the integration time
is changing regularly, then all you have done is to move the problem
into userspace which will have to constant change the persistence value
instead.

The point I am trying to make is the complexity and predictability
argument swings both ways.
> 
> If this dependency is due to the hardware then it would make sense
> for expecting the user to follow them. But here we are creating a
> logical dependency between persistence and sampling frequency to
> avoid the software complexity.
It is due to hardware. The hardware could have exposed this as
a time based element.  It chose not to because of a hardware 
design decision. 

We have numerous cases of this - often devices have a sampling frequency
that is based on number of clock cycles of some supplied clock.  Or a
low pass filter that is based on the number of sampling cycles, just
like your persistence.  We define these in terms of time, because
we had to choose to either do it in samples, or in time and userspace
is far more interested (most of the time) in the time based one.
Honestly most users of sensors, couldn't care less what the sampling
frequency is, they care how often (in units of time) they get their update.

> 
> If you read the data sheet, there is no relation between persistence
> and sampling frequency. But when using the interfaces, the user
> should understand the ABI dependency created in software.
Exactly so we are calling it period.  It is related to the
datasheet defined persistence by being multiplied persistence
by the sampling frequency. 

What I don't want is a random mixture of period and persistence based
on the way it was documented in datasheets.  These are the same thing.
The only difference is the unit (at a given setting).
Honestly the term persistence is
well defined as being i
>From an ABI point of view, we couldn't care less what the underlying
hardware is doing, what we care about is not have two interfaces for
the same thing. Period is there, it is defined and in use, so that
is what we are going to stick with

Yes it's painful. I've written a good amount of code myself
to deal with unifying the different approaches different hardware
takes to a given problem. It's fiddly but what is the point of
a unified interface if we don't put the work in to keep it
consistent.

Sorry, this is one of the main roles of a maintainer:
Keeping expose ABI consistent and minimal.

Jonathan
>>
>>> Don't you think its better to add another representation instead of
>>> trying to fit it in existing ABI's ?
>> No
>> This is a common situation. All over the place we have cases where changing one
>>   abi element effects the value of another.
>> We cope with it to provide a consistent
>>   and minimal interface to userspace.
>> Where the sampling frequency is directly
>>   controlled it is perfectly acceptable to not automatically adjust the period.
>> We frequently rely on the rule that userspce must check all attrs if it changes any.
>>> Following are the persistence filter descriptions from data sheets of
>>> tsl2591 and ltr501.
>>>
>>> LTR501: " The INTERRUPT PERSIST register controls the N number of times
>>>
>>> the measurement data is outside the range defined by the upper and
>>> lower
>>> threshold limits before asserting the INT".
>>> TSL2591: " The Interrupt persistence filter sets the number of
>>> consecutive out-of-range ALS cycles necessary to generate an interrupt"
>>>
>>>> 2) A new option which doesn't take account of the signal dropping
>>> below the threshold.
>>>>      So a count of number of times above the threshold.
>>>>    i.e. On each cycle, if over threshold, increment N.
>>>>         Evaluate if N is greater than 'persistence' then trigger an
>>> event.
>>>>     Why this would ever make sense on a light sensor is beyond me ;)
>>> but there
>>>>     we are.  We have a related abi for in_steps_change_value, which
>>> fires
>>>>     an event, only every N steps.  It doesn't really adapt to this
>>> case though.
>>>>     The name persistence is definitely misleading if this is what the
>>> liteon parts
>>>>     do as that definitely implies case 1).
>>>>
>>>> Welcome to my life, bludgeoning what appears to new ABI into what we
>>> already have!
>>>> (not a lot of point in standardized ABI otherwise!)
>>>>
>>>> Jonathan
>>>>
>>>>
>>>>> +
>>>>>    What:        /sys/.../events/in_activity_still_thresh_rising_en
>>>>>    What:        /sys/.../events/in_activity_still_thresh_falling_en
>>>>>    What:        /sys/.../events/in_activity_walking_thresh_rising_en
>>>>>
>