[PATCH v6 1/6] iio: imu: st_lsm6dsx: move interrupt thread to core

[PATCH v6 1/6] iio: imu: st_lsm6dsx: move interrupt thread to core

[Sean Nyekjaer]

This prepares the interrupt to be used for other stuff than
fifo reading + event readings.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 78 +---------------
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 88 +++++++++++++++++++
 2 files changed, 89 insertions(+), 77 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index b0f3da1976e4..ef579650fd52 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -30,8 +30,6 @@
  * Denis Ciocca <denis.ciocca@st.com>
  */
 #include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -42,10 +40,6 @@
 
 #include "st_lsm6dsx.h"
 
-#define ST_LSM6DSX_REG_HLACTIVE_ADDR		0x12
-#define ST_LSM6DSX_REG_HLACTIVE_MASK		BIT(5)
-#define ST_LSM6DSX_REG_PP_OD_ADDR		0x12
-#define ST_LSM6DSX_REG_PP_OD_MASK		BIT(4)
 #define ST_LSM6DSX_REG_FIFO_MODE_ADDR		0x0a
 #define ST_LSM6DSX_FIFO_MODE_MASK		GENMASK(2, 0)
 #define ST_LSM6DSX_FIFO_ODR_MASK		GENMASK(6, 3)
@@ -654,25 +648,6 @@ int st_lsm6dsx_update_fifo(struct st_lsm6dsx_sensor *sensor, bool enable)
 	return err;
 }
 
-static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
-{
-	struct st_lsm6dsx_hw *hw = private;
-
-	return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE;
-}
-
-static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
-{
-	struct st_lsm6dsx_hw *hw = private;
-	int count;
-
-	mutex_lock(&hw->fifo_lock);
-	count = hw->settings->fifo_ops.read_fifo(hw);
-	mutex_unlock(&hw->fifo_lock);
-
-	return count ? IRQ_HANDLED : IRQ_NONE;
-}
-
 static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
@@ -702,59 +677,8 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
 
 int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 {
-	struct device_node *np = hw->dev->of_node;
-	struct st_sensors_platform_data *pdata;
 	struct iio_buffer *buffer;
-	unsigned long irq_type;
-	bool irq_active_low;
-	int i, err;
-
-	irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
-
-	switch (irq_type) {
-	case IRQF_TRIGGER_HIGH:
-	case IRQF_TRIGGER_RISING:
-		irq_active_low = false;
-		break;
-	case IRQF_TRIGGER_LOW:
-	case IRQF_TRIGGER_FALLING:
-		irq_active_low = true;
-		break;
-	default:
-		dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
-		return -EINVAL;
-	}
-
-	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
-				 ST_LSM6DSX_REG_HLACTIVE_MASK,
-				 FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
-					    irq_active_low));
-	if (err < 0)
-		return err;
-
-	pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
-	if ((np && of_property_read_bool(np, "drive-open-drain")) ||
-	    (pdata && pdata->open_drain)) {
-		err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
-					 ST_LSM6DSX_REG_PP_OD_MASK,
-					 FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
-						    1));
-		if (err < 0)
-			return err;
-
-		irq_type |= IRQF_SHARED;
-	}
-
-	err = devm_request_threaded_irq(hw->dev, hw->irq,
-					st_lsm6dsx_handler_irq,
-					st_lsm6dsx_handler_thread,
-					irq_type | IRQF_ONESHOT,
-					"lsm6dsx", hw);
-	if (err) {
-		dev_err(hw->dev, "failed to request trigger irq %d\n",
-			hw->irq);
-		return err;
-	}
+	int i;
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
 		if (!hw->iio_devs[i])
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 2d3495560136..d0bcbbfb6297 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -50,6 +50,8 @@
 #include <linux/delay.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/bitfield.h>
@@ -65,6 +67,11 @@
 #define ST_LSM6DSX_REG_BDU_ADDR			0x12
 #define ST_LSM6DSX_REG_BDU_MASK			BIT(6)
 
+#define ST_LSM6DSX_REG_HLACTIVE_ADDR		0x12
+#define ST_LSM6DSX_REG_HLACTIVE_MASK		BIT(5)
+#define ST_LSM6DSX_REG_PP_OD_ADDR		0x12
+#define ST_LSM6DSX_REG_PP_OD_MASK		BIT(4)
+
 static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
 	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
 	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
@@ -1466,6 +1473,83 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	return iio_dev;
 }
 
+static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
+{
+	struct st_lsm6dsx_hw *hw = private;
+
+	return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE;
+}
+
+static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
+{
+	struct st_lsm6dsx_hw *hw = private;
+	int count;
+
+	mutex_lock(&hw->fifo_lock);
+	count = hw->settings->fifo_ops.read_fifo(hw);
+	mutex_unlock(&hw->fifo_lock);
+
+	return count ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
+{
+	struct st_sensors_platform_data *pdata;
+	struct device_node *np = hw->dev->of_node;
+	unsigned long irq_type;
+	bool irq_active_low;
+	int err;
+
+	irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
+
+	switch (irq_type) {
+	case IRQF_TRIGGER_HIGH:
+	case IRQF_TRIGGER_RISING:
+		irq_active_low = false;
+		break;
+	case IRQF_TRIGGER_LOW:
+	case IRQF_TRIGGER_FALLING:
+		irq_active_low = true;
+		break;
+	default:
+		dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
+		return -EINVAL;
+	}
+
+	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
+				 ST_LSM6DSX_REG_HLACTIVE_MASK,
+				 FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
+					    irq_active_low));
+	if (err < 0)
+		return err;
+
+	pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
+	if ((np && of_property_read_bool(np, "drive-open-drain")) ||
+	    (pdata && pdata->open_drain)) {
+		err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
+					 ST_LSM6DSX_REG_PP_OD_MASK,
+					 FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
+						    1));
+		if (err < 0)
+			return err;
+
+		irq_type |= IRQF_SHARED;
+	}
+
+	err = devm_request_threaded_irq(hw->dev, hw->irq,
+					st_lsm6dsx_handler_irq,
+					st_lsm6dsx_handler_thread,
+					irq_type | IRQF_ONESHOT,
+					"lsm6dsx", hw);
+	if (err) {
+		dev_err(hw->dev, "failed to request trigger irq %d\n",
+			hw->irq);
+		return err;
+	}
+
+	return 0;
+}
+
 int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 		     struct regmap *regmap)
 {
@@ -1514,6 +1598,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 	}
 
 	if (hw->irq > 0) {
+		err = st_lsm6dsx_irq_setup(hw);
+		if (err < 0)
+			return err;
+
 		err = st_lsm6dsx_fifo_setup(hw);
 		if (err < 0)
 			return err;
-- 
2.23.0

[PATCH v6 2/6] iio: imu: st_lsm6dsx: add motion events

[Sean Nyekjaer]

Add event channels that controls the creation of motion events.
Tested on ISM330DLC

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v4:
 * Added check for event support
 * Added registers for more devices that support this event

Changes since v5:
 * Moved wakeup_src masks to PATCH 5/6

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  41 ++++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 194 ++++++++++++++++++-
 2 files changed, 231 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 5e3cd96b0059..d04473861fba 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -12,6 +12,7 @@
 #define ST_LSM6DSX_H
 
 #include <linux/device.h>
+#include <linux/iio/iio.h>
 
 #define ST_LSM6DS3_DEV_NAME	"lsm6ds3"
 #define ST_LSM6DS3H_DEV_NAME	"lsm6ds3h"
@@ -54,6 +55,26 @@ enum st_lsm6dsx_hw_id {
 					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
 #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
 
+#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
+{									\
+	.type = chan_type,						\
+	.address = addr,						\
+	.modified = 1,							\
+	.channel2 = mod,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = scan_idx,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+	.event_spec = &st_lsm6dsx_event,				\
+	.num_event_specs = 1,						\
+}
+
 #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
 {									\
 	.type = chan_type,						\
@@ -162,6 +183,11 @@ struct st_lsm6dsx_shub_settings {
 	u8 batch_en;
 };
 
+struct st_lsm6dsx_event_settings {
+	struct st_lsm6dsx_reg enable_reg;
+	struct st_lsm6dsx_reg wakeup_reg;
+};
+
 enum st_lsm6dsx_ext_sensor_id {
 	ST_LSM6DSX_ID_MAGN,
 };
@@ -223,6 +249,9 @@ struct st_lsm6dsx_settings {
 	u8 wai;
 	u8 int1_addr;
 	u8 int2_addr;
+	u8 int1_func_addr;
+	u8 int2_func_addr;
+	u8 int_func_mask;
 	u8 reset_addr;
 	u16 max_fifo_size;
 	struct {
@@ -240,6 +269,7 @@ struct st_lsm6dsx_settings {
 	struct st_lsm6dsx_fifo_ops fifo_ops;
 	struct st_lsm6dsx_hw_ts_settings ts_settings;
 	struct st_lsm6dsx_shub_settings shub_settings;
+	struct st_lsm6dsx_event_settings event_settings;
 };
 
 enum st_lsm6dsx_sensor_id {
@@ -320,6 +350,10 @@ struct st_lsm6dsx_hw {
 	u8 ts_sip;
 	u8 sip;
 
+	u8 event_threshold;
+	bool enable_event;
+	struct st_lsm6dsx_reg irq_routing;
+
 	u8 *buff;
 
 	struct iio_dev *iio_devs[ST_LSM6DSX_ID_MAX];
@@ -327,6 +361,13 @@ struct st_lsm6dsx_hw {
 	const struct st_lsm6dsx_settings *settings;
 };
 
+static const struct iio_event_spec st_lsm6dsx_event = {
+	.type = IIO_EV_TYPE_THRESH,
+	.dir = IIO_EV_DIR_EITHER,
+	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			 BIT(IIO_EV_INFO_ENABLE)
+};
+
 static const unsigned long st_lsm6dsx_available_scan_masks[] = {0x7, 0x0};
 extern const struct dev_pm_ops st_lsm6dsx_pm_ops;
 
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index d0bcbbfb6297..d58683e13465 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -73,9 +73,9 @@
 #define ST_LSM6DSX_REG_PP_OD_MASK		BIT(4)
 
 static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
-	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
-	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
-	ST_LSM6DSX_CHANNEL(IIO_ACCEL, 0x2c, IIO_MOD_Z, 2),
+	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x28, IIO_MOD_X, 0),
+	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2a, IIO_MOD_Y, 1),
+	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, 0x2c, IIO_MOD_Z, 2),
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
@@ -168,6 +168,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 		.wai = 0x69,
 		.int1_addr = 0x0d,
 		.int2_addr = 0x0e,
+		.int1_func_addr = 0x5e,
+		.int2_func_addr = 0x5f,
+		.int_func_mask = BIT(5),
 		.reset_addr = 0x12,
 		.max_fifo_size = 1365,
 		.id = {
@@ -275,11 +278,20 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.mask = GENMASK(5, 3),
 			},
 		},
+		.event_settings = {
+			.wakeup_reg = {
+				.addr = 0x5B,
+				.mask = GENMASK(5, 0),
+			},
+		},
 	},
 	{
 		.wai = 0x69,
 		.int1_addr = 0x0d,
 		.int2_addr = 0x0e,
+		.int1_func_addr = 0x5e,
+		.int2_func_addr = 0x5f,
+		.int_func_mask = BIT(5),
 		.reset_addr = 0x12,
 		.max_fifo_size = 682,
 		.id = {
@@ -387,11 +399,20 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.mask = GENMASK(5, 3),
 			},
 		},
+		.event_settings = {
+			.wakeup_reg = {
+				.addr = 0x5B,
+				.mask = GENMASK(5, 0),
+			},
+		},
 	},
 	{
 		.wai = 0x6a,
 		.int1_addr = 0x0d,
 		.int2_addr = 0x0e,
+		.int1_func_addr = 0x5e,
+		.int2_func_addr = 0x5f,
+		.int_func_mask = BIT(5),
 		.reset_addr = 0x12,
 		.max_fifo_size = 682,
 		.id = {
@@ -508,6 +529,16 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.mask = GENMASK(5, 3),
 			},
 		},
+		.event_settings = {
+			.enable_reg = {
+				.addr = 0x58,
+				.mask = BIT(7),
+			},
+			.wakeup_reg = {
+				.addr = 0x5B,
+				.mask = GENMASK(5, 0),
+			},
+		},
 	},
 	{
 		.wai = 0x6c,
@@ -646,6 +677,9 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 		.wai = 0x6b,
 		.int1_addr = 0x0d,
 		.int2_addr = 0x0e,
+		.int1_func_addr = 0x5e,
+		.int2_func_addr = 0x5f,
+		.int_func_mask = BIT(5),
 		.reset_addr = 0x12,
 		.max_fifo_size = 512,
 		.id = {
@@ -745,11 +779,24 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.mask = GENMASK(7, 6),
 			},
 		},
+		.event_settings = {
+			.enable_reg = {
+				.addr = 0x58,
+				.mask = BIT(7),
+			},
+			.wakeup_reg = {
+				.addr = 0x5B,
+				.mask = GENMASK(5, 0),
+			},
+		},
 	},
 	{
 		.wai = 0x6b,
 		.int1_addr = 0x0d,
 		.int2_addr = 0x0e,
+		.int1_func_addr = 0x5e,
+		.int2_func_addr = 0x5f,
+		.int_func_mask = BIT(5),
 		.reset_addr = 0x12,
 		.max_fifo_size = 512,
 		.id = {
@@ -877,6 +924,16 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 			.slv0_addr = 0x15,
 			.dw_slv0_addr = 0x21,
 			.batch_en = BIT(3),
+		},
+		.event_settings = {
+			.enable_reg = {
+				.addr = 0x58,
+				.mask = BIT(7),
+			},
+			.wakeup_reg = {
+				.addr = 0x5B,
+				.mask = GENMASK(5, 0),
+			},
 		}
 	},
 };
@@ -1083,7 +1140,8 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
 	if (err < 0)
 		return err;
 
-	st_lsm6dsx_sensor_set_enable(sensor, false);
+	if (!hw->enable_event)
+		st_lsm6dsx_sensor_set_enable(sensor, false);
 
 	*val = (s16)le16_to_cpu(data);
 
@@ -1156,6 +1214,126 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
 	return err;
 }
 
+int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw, int state)
+{
+	int err;
+	u8 enable = 0;
+
+	if (!hw->settings->int1_func_addr)
+		return -ENOTSUPP;
+
+	enable = state ? hw->settings->event_settings.enable_reg.mask : 0;
+
+	err = regmap_update_bits(hw->regmap,
+				 hw->settings->event_settings.enable_reg.addr,
+				 hw->settings->event_settings.enable_reg.mask,
+				 enable);
+	if (err < 0)
+		return err;
+
+	enable = state ? hw->irq_routing.mask : 0;
+
+	/* Enable wakeup interrupt */
+	return regmap_update_bits(hw->regmap, hw->irq_routing.addr,
+				  hw->irq_routing.mask,
+				  enable);
+}
+
+static int st_lsm6dsx_read_event(struct iio_dev *iio_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 st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	*val2 = 0;
+	*val = hw->event_threshold;
+
+	return IIO_VAL_INT;
+}
+
+static int st_lsm6dsx_write_event(struct iio_dev *iio_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 st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+	int err;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	if (val < 0 || val > 31)
+		return -EINVAL;
+
+	err = regmap_update_bits(hw->regmap,
+				 hw->settings->event_settings.wakeup_reg.addr,
+				 hw->settings->event_settings.wakeup_reg.mask,
+				 val);
+	if (err)
+		return -EINVAL;
+
+	hw->event_threshold = val;
+
+	return 0;
+}
+
+static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
+					  const struct iio_chan_spec *chan,
+					  enum iio_event_type type,
+					  enum iio_event_direction dir)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	return hw->enable_event;
+}
+
+static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
+					   const struct iio_chan_spec *chan,
+					   enum iio_event_type type,
+					   enum iio_event_direction dir,
+					   int state)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+	int err = 0;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
+		return -EBUSY;
+
+	/* do not enable events if they are already enabled */
+	if (state && hw->enable_event)
+		return 0;
+
+	err = st_lsm6dsx_event_setup(hw, state);
+	if (err < 0)
+		return err;
+
+	err = st_lsm6dsx_sensor_set_enable(sensor, state);
+	if (err < 0)
+		return err;
+
+	hw->enable_event = state;
+
+	return 0;
+}
+
 int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
@@ -1240,6 +1418,10 @@ static const struct iio_info st_lsm6dsx_acc_info = {
 	.attrs = &st_lsm6dsx_acc_attribute_group,
 	.read_raw = st_lsm6dsx_read_raw,
 	.write_raw = st_lsm6dsx_write_raw,
+	.read_event_value = st_lsm6dsx_read_event,
+	.write_event_value = st_lsm6dsx_write_event,
+	.read_event_config = st_lsm6dsx_read_event_config,
+	.write_event_config = st_lsm6dsx_write_event_config,
 	.hwfifo_set_watermark = st_lsm6dsx_set_watermark,
 };
 
@@ -1285,9 +1467,13 @@ static int st_lsm6dsx_get_drdy_reg(struct st_lsm6dsx_hw *hw, u8 *drdy_reg)
 	switch (drdy_pin) {
 	case 1:
 		*drdy_reg = hw->settings->int1_addr;
+		hw->irq_routing.addr = hw->settings->int1_func_addr;
+		hw->irq_routing.mask = hw->settings->int_func_mask;
 		break;
 	case 2:
 		*drdy_reg = hw->settings->int2_addr;
+		hw->irq_routing.addr = hw->settings->int2_func_addr;
+		hw->irq_routing.mask = hw->settings->int_func_mask;
 		break;
 	default:
 		dev_err(hw->dev, "unsupported data ready pin\n");
-- 
2.23.0

[PATCH v6 3/6] iio: imu: st_lsm6dsx: add wakeup-source option

[Sean Nyekjaer]

This add ways for the SoC to wake from accelerometer wake events.

In the suspend function we skip disabling the sensor if wakeup-source
and events are activated.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v4:
 * More devices supports wakeup

Changes since v5:
 * None

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index d58683e13465..9d023b99139f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1802,6 +1802,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 			return err;
 	}
 
+	if (dev->of_node && of_property_read_bool(dev->of_node, "wakeup-source"))
+		device_init_wakeup(dev, true);
+
 	return 0;
 }
 EXPORT_SYMBOL(st_lsm6dsx_probe);
@@ -1820,6 +1823,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
 		if (!(hw->enable_mask & BIT(sensor->id)))
 			continue;
 
+		if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) {
+			/* Enable wake from IRQ */
+			enable_irq_wake(hw->irq);
+			continue;
+		}
+
 		if (sensor->id == ST_LSM6DSX_ID_EXT0 ||
 		    sensor->id == ST_LSM6DSX_ID_EXT1 ||
 		    sensor->id == ST_LSM6DSX_ID_EXT2)
@@ -1852,6 +1861,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
 		if (!(hw->suspend_mask & BIT(sensor->id)))
 			continue;
 
+		if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC) {
+			disable_irq_wake(hw->irq);
+			continue;
+		}
+
 		if (sensor->id == ST_LSM6DSX_ID_EXT0 ||
 		    sensor->id == ST_LSM6DSX_ID_EXT1 ||
 		    sensor->id == ST_LSM6DSX_ID_EXT2)
-- 
2.23.0

[PATCH v6 4/6] iio: imu: st_lsm6dsx: always enter interrupt thread

[Sean Nyekjaer]

The interrupt source can come from multiple sources,
fifo and wake interrupts.
Enter interrupt thread to check which interrupt that has fired.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 9d023b99139f..5b8bcd9cc4d2 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1661,9 +1661,7 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 
 static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
 {
-	struct st_lsm6dsx_hw *hw = private;
-
-	return hw->sip > 0 ? IRQ_WAKE_THREAD : IRQ_NONE;
+	return IRQ_WAKE_THREAD;
 }
 
 static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
-- 
2.23.0

[PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Sean Nyekjaer]

Report iio motion events to iio subsystem

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v4:
 * Updated bitmask as pr Jonathans comments

Changes since v5:
 * None

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index d04473861fba..015b837f366f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -186,6 +186,11 @@ struct st_lsm6dsx_shub_settings {
 struct st_lsm6dsx_event_settings {
 	struct st_lsm6dsx_reg enable_reg;
 	struct st_lsm6dsx_reg wakeup_reg;
+	u8 wakeup_src_reg;
+	u8 wakeup_src_status_mask;
+	u8 wakeup_src_z_mask;
+	u8 wakeup_src_y_mask;
+	u8 wakeup_src_x_mask;
 };
 
 enum st_lsm6dsx_ext_sensor_id {
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 5b8bcd9cc4d2..bd5c7c65a519 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -48,6 +48,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/delay.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/interrupt.h>
@@ -283,6 +284,11 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -404,6 +410,11 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -538,6 +549,11 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -788,6 +804,11 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		},
 	},
 	{
@@ -934,6 +955,11 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 				.addr = 0x5B,
 				.mask = GENMASK(5, 0),
 			},
+			.wakeup_src_reg = 0x1b,
+			.wakeup_src_status_mask = BIT(3),
+			.wakeup_src_z_mask = BIT(0),
+			.wakeup_src_y_mask = BIT(1),
+			.wakeup_src_x_mask = BIT(2),
 		}
 	},
 };
@@ -1659,6 +1685,38 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	return iio_dev;
 }
 
+void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
+{
+	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
+
+	if (data & hw->settings->event_settings.wakeup_src_z_mask)
+		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_Z,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_EITHER),
+						  timestamp);
+
+	if (data & hw->settings->event_settings.wakeup_src_x_mask)
+		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_Y,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_EITHER),
+						  timestamp);
+
+	if (data & hw->settings->event_settings.wakeup_src_x_mask)
+		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
+			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
+						  0,
+						  IIO_MOD_X,
+						  IIO_EV_TYPE_THRESH,
+						  IIO_EV_DIR_EITHER),
+						  timestamp);
+}
+
 static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
 {
 	return IRQ_WAKE_THREAD;
@@ -1668,6 +1726,18 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
 {
 	struct st_lsm6dsx_hw *hw = private;
 	int count;
+	int data, err;
+
+	if (hw->enable_event) {
+		err = regmap_read(hw->regmap,
+				  hw->settings->event_settings.wakeup_src_reg,
+				  &data);
+		if (err < 0)
+			return IRQ_NONE;
+
+		if (data & hw->settings->event_settings.wakeup_src_status_mask)
+			st_lsm6dsx_report_motion_event(hw, data);
+	}
 
 	mutex_lock(&hw->fifo_lock);
 	count = hw->settings->fifo_ops.read_fifo(hw);
-- 
2.23.0

[PATCH v6 6/6] iio: imu: st_lsm6dsx: prohibit the use of events and buffered reads simultaneously

[Sean Nyekjaer]

When events and buffered reads is enabled simultaneously, and the first
event accours the interrupt pin stays high.

This can be reverted when we find a solution to allow events and
buffered reads simultaneously.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v4:
 * Use fifo configuration mutex to prevent a race in hw->enable_event check.

Changes since v5:
 * Updated do not return without unlocking mutexes
 * Lock mutex before unlock
 * Runtime tested ;-)

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c |  5 +++++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c   | 13 ++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index ef579650fd52..b87a1872bc60 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -603,6 +603,11 @@ int st_lsm6dsx_update_fifo(struct st_lsm6dsx_sensor *sensor, bool enable)
 
 	mutex_lock(&hw->conf_lock);
 
+	if (hw->enable_event) {
+		err = -EBUSY;
+		goto out;
+	}
+
 	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
 		err = st_lsm6dsx_flush_fifo(hw);
 		if (err < 0)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 00ba14d15c13..2616036ce953 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1340,8 +1340,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 	if (type != IIO_EV_TYPE_THRESH)
 		return -EINVAL;
 
-	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
-		return -EBUSY;
+	mutex_lock(&hw->conf_lock);
+
+	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
+		err = -EBUSY;
+		goto out;
+	}
 
 	/* do not enable events if they are already enabled */
 	if (state && hw->enable_event)
@@ -1357,7 +1361,10 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 
 	hw->enable_event = state;
 
-	return 0;
+out:
+	mutex_unlock(&hw->conf_lock);
+
+	return err;
 }
 
 int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
-- 
2.23.0

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Sean Nyekjaer]

On 09/09/2019 13.28, Sean Nyekjaer wrote:
> Report iio motion events to iio subsystem
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v4:
>   * Updated bitmask as pr Jonathans comments
> 
> Changes since v5:
>   * None
> 
>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
>   2 files changed, 75 insertions(+)
> 
[...]
>   
> +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
> +{
> +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> +
> +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_Z,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +						  timestamp);
> +
> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_Y,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +						  timestamp);
> +
> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +						  0,
> +						  IIO_MOD_X,
> +						  IIO_EV_TYPE_THRESH,
> +						  IIO_EV_DIR_EITHER),
> +						  timestamp);
> +}
> +

I was looking at this again, and if the user enables events for channel 
x, we continue to report events for y, z.
Is it okay or is it better to filter them out?

/Sean

Re: [PATCH v6 6/6] iio: imu: st_lsm6dsx: prohibit the use of events and buffered reads simultaneously

[Lorenzo Bianconi]

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

> When events and buffered reads is enabled simultaneously, and the first
> event accours the interrupt pin stays high.
> 
> This can be reverted when we find a solution to allow events and
> buffered reads simultaneously.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v4:
>  * Use fifo configuration mutex to prevent a race in hw->enable_event check.
> 
> Changes since v5:
>  * Updated do not return without unlocking mutexes
>  * Lock mutex before unlock
>  * Runtime tested ;-)
> 

[...]

> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1340,8 +1340,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>  	if (type != IIO_EV_TYPE_THRESH)
>  		return -EINVAL;
>  
> -	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
> -		return -EBUSY;
> +	mutex_lock(&hw->conf_lock);
> +
> +	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
> +		err = -EBUSY;
> +		goto out;
> +	}

This patch is still broken!!! Returning in case of error you need to relase the
lock.

>  
>  	/* do not enable events if they are already enabled */
>  	if (state && hw->enable_event)
> @@ -1357,7 +1361,10 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>  
>  	hw->enable_event = state;
>  
> -	return 0;
> +out:
> +	mutex_unlock(&hw->conf_lock);
> +
> +	return err;
>  }
>  
>  int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
> -- 
> 2.23.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 6/6] iio: imu: st_lsm6dsx: prohibit the use of events and buffered reads simultaneously

[Sean Nyekjaer]

On 09/09/2019 13.55, Lorenzo Bianconi wrote:
>> When events and buffered reads is enabled simultaneously, and the first
>> event accours the interrupt pin stays high.
>>
>> This can be reverted when we find a solution to allow events and
>> buffered reads simultaneously.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>> Changes since v4:
>>   * Use fifo configuration mutex to prevent a race in hw->enable_event check.
>>
>> Changes since v5:
>>   * Updated do not return without unlocking mutexes
>>   * Lock mutex before unlock
>>   * Runtime tested ;-)
>>
> 
> [...]
> 
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -1340,8 +1340,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>>   	if (type != IIO_EV_TYPE_THRESH)
>>   		return -EINVAL;
>>   
>> -	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
>> -		return -EBUSY;
>> +	mutex_lock(&hw->conf_lock);
>> +
>> +	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
>> +		err = -EBUSY;
>> +		goto out;
>> +	}
> 
> This patch is still broken!!! Returning in case of error you need to relase the
> lock.
> 

Hmm,

please read below this:

-	return 0;
+out:
+	mutex_unlock(&hw->conf_lock);
+
+	return err;


>>   
>>   	/* do not enable events if they are already enabled */
>>   	if (state && hw->enable_event)
>> @@ -1357,7 +1361,10 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>>   
>>   	hw->enable_event = state;
>>   
>> -	return 0;
>> +out:
>> +	mutex_unlock(&hw->conf_lock);
>> +
>> +	return err;
>>   }
>>   
>>   int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
>> -- 
>> 2.23.0
>>

Re: [PATCH v6 6/6] iio: imu: st_lsm6dsx: prohibit the use of events and buffered reads simultaneously

[Sean Nyekjaer]

On 09/09/2019 13.59, Sean Nyekjaer wrote:
> 
> 
> On 09/09/2019 13.55, Lorenzo Bianconi wrote:
>>> When events and buffered reads is enabled simultaneously, and the first
>>> event accours the interrupt pin stays high.
>>>
>>> This can be reverted when we find a solution to allow events and
>>> buffered reads simultaneously.
>>>
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> ---
>>> Changes since v4:
>>>   * Use fifo configuration mutex to prevent a race in 
>>> hw->enable_event check.
>>>
>>> Changes since v5:
>>>   * Updated do not return without unlocking mutexes
>>>   * Lock mutex before unlock
>>>   * Runtime tested ;-)
>>>
>>
>> [...]
>>
>>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>> @@ -1340,8 +1340,12 @@ static int 
>>> st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>>>       if (type != IIO_EV_TYPE_THRESH)
>>>           return -EINVAL;
>>> -    if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS)
>>> -        return -EBUSY;
>>> +    mutex_lock(&hw->conf_lock);
>>> +
>>> +    if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
>>> +        err = -EBUSY;
>>> +        goto out;
>>> +    }
>>
>> This patch is still broken!!! Returning in case of error you need to 
>> relase the
>> lock.
>>
> 
> Hmm,
> 
> please read below this:
> 
> -    return 0;
> +out:
> +    mutex_unlock(&hw->conf_lock);
> +
> +    return err;
> >

I see what you mean...
I will update this function with:
goto's :-)

/Sean

>>>       /* do not enable events if they are already enabled */
>>>       if (state && hw->enable_event)
>>> @@ -1357,7 +1361,10 @@ static int 
>>> st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>>>       hw->enable_event = state;
>>> -    return 0;
>>> +out:
>>> +    mutex_unlock(&hw->conf_lock);
>>> +
>>> +    return err;
>>>   }
>>>   int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int 
>>> val)
>>> -- 
>>> 2.23.0
>>>

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Lorenzo Bianconi]

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

> Report iio motion events to iio subsystem
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Changes since v4:
>  * Updated bitmask as pr Jonathans comments
> 
> Changes since v5:
>  * None
> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 

[...]

>  static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>  {
>  	return IRQ_WAKE_THREAD;
> @@ -1668,6 +1726,18 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>  {
>  	struct st_lsm6dsx_hw *hw = private;
>  	int count;
> +	int data, err;
> +
> +	if (hw->enable_event) {


Maybe I understood the issue between the buffered reading and event generation.
I guess it is a race here between when the device is generating the interrupt
and when you set enable_event. I think there are two solutions:
1- trivial one: always read wakeup_src_reg
2- set hw->enable_event as first instruction in st_lsm6dsx_write_event_config()
and roll back in case of error.

Could you please try that changes and double check if you are still able to
trigger the issue?

Regards,
Lorenzo

> +		err = regmap_read(hw->regmap,
> +				  hw->settings->event_settings.wakeup_src_reg,
> +				  &data);
> +		if (err < 0)
> +			return IRQ_NONE;
> +
> +		if (data & hw->settings->event_settings.wakeup_src_status_mask)
> +			st_lsm6dsx_report_motion_event(hw, data);
> +	}
>  
>  	mutex_lock(&hw->fifo_lock);
>  	count = hw->settings->fifo_ops.read_fifo(hw);
> -- 
> 2.23.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Sean Nyekjaer]

On 09/09/2019 14.05, Lorenzo Bianconi wrote:
>>   static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>>   {
>>   	return IRQ_WAKE_THREAD;
>> @@ -1668,6 +1726,18 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>>   {
>>   	struct st_lsm6dsx_hw *hw = private;
>>   	int count;
>> +	int data, err;
>> +
>> +	if (hw->enable_event) {
> 
> Maybe I understood the issue between the buffered reading and event generation.
> I guess it is a race here between when the device is generating the interrupt
> and when you set enable_event. I think there are two solutions:
> 1- trivial one: always read wakeup_src_reg
> 2- set hw->enable_event as first instruction in st_lsm6dsx_write_event_config()
> and roll back in case of error.
> 
> Could you please try that changes and double check if you are still able to
> trigger the issue?
> 

1. Without the last [PATCH v6 6/6] and this diff:

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c 
b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index bd5c7c65a519..b303459e0d31 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1728,7 +1728,6 @@ static irqreturn_t st_lsm6dsx_handler_thread(int 
irq, void *private)
         int count;
         int data, err;

-       if (hw->enable_event) {
                 err = regmap_read(hw->regmap,
 
hw->settings->event_settings.wakeup_src_reg,
                                   &data);
@@ -1737,7 +1736,6 @@ static irqreturn_t st_lsm6dsx_handler_thread(int 
irq, void *private)

                 if (data & 
hw->settings->event_settings.wakeup_src_status_mask)
                         st_lsm6dsx_report_motion_event(hw, data);
-       }

         mutex_lock(&hw->fifo_lock);
         count = hw->settings->fifo_ops.read_fifo(hw);

$ cd /sys/bus/iio/devices/iio:device2
$ echo 1 > events/in_accel_x_thresh_either_en
$ echo 1 > events/in_accel_x_thresh_either_value
$ echo 1 > scan_elements/in_accel_x_en
$ echo 1 > buffer/enable

FIFO interrupts ticking in... until I trigger the first event. :-(
The event is reported correctly. The interrupt pin is staying high.
The result is the same if I enable the FIFO first.
I don't think we have a race in the driver around this, to me it looks 
like something in the ism330 device should be cleared.
Could the device go into sleep or power down mode?

2. Seems like an okay idea, do you want this in v7?

/Sean

[PATCH v6.1 6/6] iio: imu: st_lsm6dsx: prohibit the use of events and buffered reads simultaneously

[Sean Nyekjaer]

When events and buffered reads is enabled simultaneously, and the first
event accours the interrupt pin stays high.

This can be reverted when we find a solution to allow events and
buffered reads simultaneously.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since v4:
 * Use fifo configuration mutex to prevent a race in hw->enable_event check.

Changes since v5:
 * Updated do not return without unlocking mutexes
 * Lock mutex before unlock
 * Runtime tested 
 * Make sure we unlock the mutex in case of failure

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c |  5 +++++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c   | 18 ++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index ef579650fd52..b87a1872bc60 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -603,6 +603,11 @@ int st_lsm6dsx_update_fifo(struct st_lsm6dsx_sensor *sensor, bool enable)
 
 	mutex_lock(&hw->conf_lock);
 
+	if (hw->enable_event) {
+		err = -EBUSY;
+		goto out;
+	}
+
 	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
 		err = st_lsm6dsx_flush_fifo(hw);
 		if (err < 0)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index de6bddd91471..fb658cb69c9d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1340,21 +1340,31 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 	if (type != IIO_EV_TYPE_THRESH)
 		return -EINVAL;
 
+	mutex_lock(&hw->conf_lock);
+
+	if (hw->fifo_mode != ST_LSM6DSX_FIFO_BYPASS) {
+		err = -EBUSY;
+		goto out;
+	}
+
 	/* do not enable events if they are already enabled */
 	if (state && hw->enable_event)
-		return 0;
+		goto out;
 
 	err = st_lsm6dsx_event_setup(hw, state);
 	if (err < 0)
-		return err;
+		goto out;
 
 	err = st_lsm6dsx_sensor_set_enable(sensor, state);
 	if (err < 0)
-		return err;
+		goto out;
 
 	hw->enable_event = state;
 
-	return 0;
+out:
+	mutex_unlock(&hw->conf_lock);
+
+	return err;
 }
 
 int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
-- 
2.23.0

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Lorenzo Bianconi]

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

> > Maybe I understood the issue between the buffered reading and event generation.
> > I guess it is a race here between when the device is generating the interrupt
> > and when you set enable_event. I think there are two solutions:
> > 1- trivial one: always read wakeup_src_reg
> > 2- set hw->enable_event as first instruction in st_lsm6dsx_write_event_config()
> > and roll back in case of error.
> > 
> > Could you please try that changes and double check if you are still able to
> > trigger the issue?
> > 

[...]

> $ cd /sys/bus/iio/devices/iio:device2
> $ echo 1 > events/in_accel_x_thresh_either_en
> $ echo 1 > events/in_accel_x_thresh_either_value
> $ echo 1 > scan_elements/in_accel_x_en
> $ echo 1 > buffer/enable
> 
> FIFO interrupts ticking in... until I trigger the first event. :-(
> The event is reported correctly. The interrupt pin is staying high.
> The result is the same if I enable the FIFO first.
> I don't think we have a race in the driver around this, to me it looks like
> something in the ism330 device should be cleared.
> Could the device go into sleep or power down mode?

probably a silly question..are you tracing the interrupt line with an
oscilloscope or a logical analyser? If you dump interrupt counters in
/proc/interrupts will you see an interrupt storm for the selected irq
pin?

Regards,
Lorenzo

> 
> 2. Seems like an okay idea, do you want this in v7?
> 
> /Sean

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Sean Nyekjaer]

On 10/09/2019 09.12, Lorenzo Bianconi wrote:
> probably a silly question..are you tracing the interrupt line with an
> oscilloscope or a logical analyser? If you dump interrupt counters in
> /proc/interrupts will you see an interrupt storm for the selected irq
> pin?
> 

Not a silly question ;-)

An Oscilloscope :-)
The interrupt counter is stopping.
If I switch to IRQ_TYPE_LEVEL_HIGH, (to test if additional readings of 
the event and FIFO registers would help. It results in interrupt storm 
and the line continues to stay high.

/Sean

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Lorenzo Bianconi]

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

> 
> 
> On 10/09/2019 09.12, Lorenzo Bianconi wrote:
> > probably a silly question..are you tracing the interrupt line with an
> > oscilloscope or a logical analyser? If you dump interrupt counters in
> > /proc/interrupts will you see an interrupt storm for the selected irq
> > pin?
> > 
> 
> Not a silly question ;-)
> 
> An Oscilloscope :-)

ack, thx

Could you please try to carry out the following test?
1- set the FIFO watermark to a high level (e.g. 128)
   $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
   $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
2- set a low acc odr (e.g 13Hz)
   $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
3- start reading from the accel and generate a wake-upp event

Is still happen? Are you able to decode bus transaction? (register addresses,
data read, ..)

> The interrupt counter is stopping.
> If I switch to IRQ_TYPE_LEVEL_HIGH, (to test if additional readings of the
> event and FIFO registers would help. It results in interrupt storm and the
> line continues to stay high.
> 

@Denis, @Mario, @Armando: any thoughts about this?

Regards,
Lorenzo

> /Sean

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Lorenzo Bianconi]

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

> > > probably a silly question..are you tracing the interrupt line with an
> > > oscilloscope or a logical analyser? If you dump interrupt counters in
> > > /proc/interrupts will you see an interrupt storm for the selected irq
> > > pin?
> > > 
> > 
> > Not a silly question ;-)
> > 
> > An Oscilloscope :-)
> 
> ack, thx
> 
> Could you please try to carry out the following test?
> 1- set the FIFO watermark to a high level (e.g. 128)
>    $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
>    $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
> 2- set a low acc odr (e.g 13Hz)
>    $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
> 3- start reading from the accel and generate a wake-upp event
> 
> Is still happen? Are you able to decode bus transaction? (register addresses,
> data read, ..)
> 
> > The interrupt counter is stopping.
> > If I switch to IRQ_TYPE_LEVEL_HIGH, (to test if additional readings of the
> > event and FIFO registers would help. It results in interrupt storm and the
> > line continues to stay high.
> > 

Could you please try to enable the LIR (Latched interrupt - BIT(0) in 0x58)?
Please remember that on ISM330DLC the interrupt will be automatically cleared
reading the wake up src register after a time slice equals to 1/ODR so the it
will be set for longer time if you run the device at low ODR

Regards,
Lorenzo

> 
> @Denis, @Mario, @Armando: any thoughts about this?
> 
> Regards,
> Lorenzo
> 
> > /Sean



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Sean Nyekjaer]

On 10/09/2019 14.27, Lorenzo Bianconi wrote:
>> Could you please try to carry out the following test?
>> 1- set the FIFO watermark to a high level (e.g. 128)
>>     $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
>>     $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
>> 2- set a low acc odr (e.g 13Hz)
>>     $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
>> 3- start reading from the accel and generate a wake-upp event
>>
>> Is still happen? Are you able to decode bus transaction? (register addresses,
>> data read, ..)
>>

Do you still want this tested?

[...]

> Could you please try to enable the LIR (Latched interrupt - BIT(0) in 0x58)?
> Please remember that on ISM330DLC the interrupt will be automatically cleared
> reading the wake up src register after a time slice equals to 1/ODR so the it
> will be set for longer time if you run the device at low ODR


"iio: imu: st_lsm6dsx: enable LIR for sensor events"
"iio: imu: st_lsm6dsx: enable clear on read for latched interrupts"
Does allow us to get events and buffered reads simultaneously.

I will drop PATCH 6/6.

/Sean

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Lorenzo Bianconi]

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

On Sep 11, Sean Nyekjaer wrote:
> 
> On 10/09/2019 14.27, Lorenzo Bianconi wrote:
> > > Could you please try to carry out the following test?
> > > 1- set the FIFO watermark to a high level (e.g. 128)
> > >     $echo 256 > /sys/bus/iio/devices/iio:device{x}/buffer/length
> > >     $echo 128 > /sys/bus/iio/devices/iio:device{x}/buffer/watermark
> > > 2- set a low acc odr (e.g 13Hz)
> > >     $echo 13 > /sys/bus/iio/devices/iio:device{x}/sampling_frequency
> > > 3- start reading from the accel and generate a wake-upp event
> > > 
> > > Is still happen? Are you able to decode bus transaction? (register addresses,
> > > data read, ..)
> > > 
> 
> Do you still want this tested?

no, not necessary

> 
> [...]
> 
> > Could you please try to enable the LIR (Latched interrupt - BIT(0) in 0x58)?
> > Please remember that on ISM330DLC the interrupt will be automatically cleared
> > reading the wake up src register after a time slice equals to 1/ODR so the it
> > will be set for longer time if you run the device at low ODR
> 
> 
> "iio: imu: st_lsm6dsx: enable LIR for sensor events"
> "iio: imu: st_lsm6dsx: enable clear on read for latched interrupts"
> Does allow us to get events and buffered reads simultaneously.
> 

cool :)

Regards,
Lorenzo

> I will drop PATCH 6/6.
> 
> /Sean

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Jonathan Cameron]

On Mon, 9 Sep 2019 13:51:13 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 09/09/2019 13.28, Sean Nyekjaer wrote:
> > Report iio motion events to iio subsystem
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> > Changes since v4:
> >   * Updated bitmask as pr Jonathans comments
> > 
> > Changes since v5:
> >   * None
> > 
> >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
> >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
> >   2 files changed, 75 insertions(+)
> >   
> [...]
> >   
> > +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
> > +{
> > +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> > +
> > +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
> > +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> > +						  0,
> > +						  IIO_MOD_Z,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_EITHER),
> > +						  timestamp);
> > +
> > +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> > +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> > +						  0,
> > +						  IIO_MOD_Y,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_EITHER),
> > +						  timestamp);
> > +
> > +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> > +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> > +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> > +						  0,
> > +						  IIO_MOD_X,
> > +						  IIO_EV_TYPE_THRESH,
> > +						  IIO_EV_DIR_EITHER),
> > +						  timestamp);
> > +}
> > +  
> 
> I was looking at this again, and if the user enables events for channel 
> x, we continue to report events for y, z.
> Is it okay or is it better to filter them out?
Better to filter them out.  It'll be a bit of a surprise for userspace
otherwise.

Thanks,

Jonathan

> 
> /Sean

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Sean Nyekjaer]

On 15/09/2019 14.20, Jonathan Cameron wrote:
> On Mon, 9 Sep 2019 13:51:13 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
>> On 09/09/2019 13.28, Sean Nyekjaer wrote:
>>> Report iio motion events to iio subsystem
>>>
>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> ---
>>> Changes since v4:
>>>    * Updated bitmask as pr Jonathans comments
>>>
>>> Changes since v5:
>>>    * None
>>>
>>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
>>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
>>>    2 files changed, 75 insertions(+)
>>>    
>> [...]
>>>    
>>> +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
>>> +{
>>> +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
>>> +
>>> +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
>>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +						  0,
>>> +						  IIO_MOD_Z,
>>> +						  IIO_EV_TYPE_THRESH,
>>> +						  IIO_EV_DIR_EITHER),
>>> +						  timestamp);
>>> +
>>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
>>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +						  0,
>>> +						  IIO_MOD_Y,
>>> +						  IIO_EV_TYPE_THRESH,
>>> +						  IIO_EV_DIR_EITHER),
>>> +						  timestamp);
>>> +
>>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
>>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +						  0,
>>> +						  IIO_MOD_X,
>>> +						  IIO_EV_TYPE_THRESH,
>>> +						  IIO_EV_DIR_EITHER),
>>> +						  timestamp);
>>> +}
>>> +
>>
>> I was looking at this again, and if the user enables events for channel
>> x, we continue to report events for y, z.
>> Is it okay or is it better to filter them out?
> Better to filter them out.  It'll be a bit of a surprise for userspace
> otherwise.
> 
> Thanks,
> 
> Jonathan
> 
Okay, but keep in mind that we can't distinguish which channel we're 
waking up to. So even if some channel is disabled, we still wake up on 
it ...

/Sean

Re: [PATCH v6 5/6] iio: imu: st_lsm6dsx: add motion report function and call from interrupt

[Jonathan Cameron]

On Sun, 15 Sep 2019 14:24:57 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 15/09/2019 14.20, Jonathan Cameron wrote:
> > On Mon, 9 Sep 2019 13:51:13 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> >> On 09/09/2019 13.28, Sean Nyekjaer wrote:  
> >>> Report iio motion events to iio subsystem
> >>>
> >>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >>> ---
> >>> Changes since v4:
> >>>    * Updated bitmask as pr Jonathans comments
> >>>
> >>> Changes since v5:
> >>>    * None
> >>>
> >>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  5 ++
> >>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 70 ++++++++++++++++++++
> >>>    2 files changed, 75 insertions(+)
> >>>      
> >> [...]  
> >>>    
> >>> +void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
> >>> +{
> >>> +	s64 timestamp = iio_get_time_ns(hw->iio_devs[ST_LSM6DSX_ID_ACC]);
> >>> +
> >>> +	if (data & hw->settings->event_settings.wakeup_src_z_mask)
> >>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> >>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> >>> +						  0,
> >>> +						  IIO_MOD_Z,
> >>> +						  IIO_EV_TYPE_THRESH,
> >>> +						  IIO_EV_DIR_EITHER),
> >>> +						  timestamp);
> >>> +
> >>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> >>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> >>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> >>> +						  0,
> >>> +						  IIO_MOD_Y,
> >>> +						  IIO_EV_TYPE_THRESH,
> >>> +						  IIO_EV_DIR_EITHER),
> >>> +						  timestamp);
> >>> +
> >>> +	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> >>> +		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
> >>> +			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
> >>> +						  0,
> >>> +						  IIO_MOD_X,
> >>> +						  IIO_EV_TYPE_THRESH,
> >>> +						  IIO_EV_DIR_EITHER),
> >>> +						  timestamp);
> >>> +}
> >>> +  
> >>
> >> I was looking at this again, and if the user enables events for channel
> >> x, we continue to report events for y, z.
> >> Is it okay or is it better to filter them out?  
> > Better to filter them out.  It'll be a bit of a surprise for userspace
> > otherwise.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> Okay, but keep in mind that we can't distinguish which channel we're 
> waking up to. So even if some channel is disabled, we still wake up on 
> it ...

Hmm. Alternative is to not filter it, but make sure that the enable
and disable interfaces treat all of them as one control.  In theory
I think the ABI would allow for events/in_accel_thresh_both_en in a similar
fashion to the shared_by_type of the info elements for the channel, but
given I'm not sure anything implements it, it's likely to be a hole
in userspace code.  We do have devices that do
in_accel_thresh_both_en when the events are rising and falling separately
but they two can't be enabled independently.  This is kind of similar
to that but on the channel rather than the direction.

Jonathan

> 
> /Sean