[PATCH v8 0/5] enable motion events for st_lsm6dsx

[PATCH v8 0/5] enable motion events for st_lsm6dsx

[Sean Nyekjaer]

This patchset depends on:
[PATCH 0/2] enable LIR and clear_on_read for st_lsm6dsx

@Jonathan if this is accepted, will it go in 5.4?

Sean Nyekjaer (5):
  iio: imu: st_lsm6dsx: move interrupt thread to core
  iio: imu: st_lsm6dsx: add motion events
  iio: imu: st_lsm6dsx: add wakeup-source option
  iio: imu: st_lsm6dsx: always enter interrupt thread
  iio: imu: st_lsm6dsx: add motion report function and call from
    interrupt

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  46 +++
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  78 +---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 361 +++++++++++++++++-
 3 files changed, 404 insertions(+), 81 deletions(-)

-- 
2.23.0

[PATCH v8 1/5] 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 b65a6ca775e0..ef838206b30f 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),
@@ -1525,6 +1532,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)
 {
@@ -1573,6 +1657,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 v8 2/5] 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

Changes since v6:
 * None

Changes since v7:
 * None

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index fefd9042590a..449c2798f7ed 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,
 };
@@ -225,6 +251,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 {
@@ -244,6 +273,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 {
@@ -324,6 +354,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];
@@ -331,6 +365,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 ef838206b30f..4198ba263d03 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 = {
@@ -279,11 +282,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 = {
@@ -395,11 +407,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 = {
@@ -520,6 +541,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,
@@ -666,6 +697,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 = {
@@ -773,11 +807,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 = {
@@ -913,6 +960,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),
+			},
 		}
 	},
 };
@@ -1119,7 +1176,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);
 
@@ -1192,6 +1250,123 @@ 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;
+
+	/* 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);
@@ -1276,6 +1451,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,
 };
 
@@ -1321,9 +1500,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 v8 3/5] 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

Changes since v6:
 * None

Changes since v7:
 * Add check for hw->enable_event
 * Moved disable_irq_wake section so it's called
 * Removed not neeeded continue from disable_irq_wake section

 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 4198ba263d03..810807c52d5f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1858,6 +1858,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);
@@ -1876,6 +1879,13 @@ 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 &&
+		    hw->enable_event) {
+			/* 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)
@@ -1904,6 +1914,10 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
 		if (!hw->iio_devs[i])
 			continue;
 
+		if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC &&
+		    hw->enable_event)
+			disable_irq_wake(hw->irq);
+
 		sensor = iio_priv(hw->iio_devs[i]);
 		if (!(hw->suspend_mask & BIT(sensor->id)))
 			continue;
-- 
2.23.0

[PATCH v8 4/5] 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 810807c52d5f..80a94335175f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1717,9 +1717,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 v8 5/5] 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

Changes since v6:
 * None

Changes since v7:
 * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
@@ -287,6 +288,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),
 		},
 	},
 	{
@@ -412,6 +418,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),
 		},
 	},
 	{
@@ -550,6 +561,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),
 		},
 	},
 	{
@@ -816,6 +832,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),
 		},
 	},
 	{
@@ -970,6 +991,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),
 		}
 	},
 };
@@ -1715,6 +1741,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;
@@ -1724,6 +1782,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

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

[Jonathan Cameron]

On Fri, 13 Sep 2019 11:07:08 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Report iio motion events to iio subsystem
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
I got to the earlier thread rather late so hadn't replied to your
question on filtering events that haven't been enabled.

Hence I think it's just that change outstanding
+ I want to give Lorenzo time for a final look.

Thanks,

Jonathan

> ---
> Changes since v4:
>  * Updated bitmask as pr Jonathans comments
> 
> Changes since v5:
>  * None
> 
> Changes since v6:
>  * None
> 
> Changes since v7:
>  * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
> @@ -287,6 +288,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),
>  		},
>  	},
>  	{
> @@ -412,6 +418,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),
>  		},
>  	},
>  	{
> @@ -550,6 +561,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),
>  		},
>  	},
>  	{
> @@ -816,6 +832,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),
>  		},
>  	},
>  	{
> @@ -970,6 +991,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),
>  		}
>  	},
>  };
> @@ -1715,6 +1741,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;
> @@ -1724,6 +1782,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);

Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread

[Jonathan Cameron]

On Fri, 13 Sep 2019 11:07:07 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> 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 810807c52d5f..80a94335175f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1717,9 +1717,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;

I missed this before.  Isn't this the same as just not providing a top half at all?

I.e. Pass null to request_threaded_irq where this function was.

Thanks,

Jonathan


>  }
>  
>  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)

Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread

[Lorenzo Bianconi]

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

> On Fri, 13 Sep 2019 11:07:07 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > 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 810807c52d5f..80a94335175f 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -1717,9 +1717,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;
> 
> I missed this before.  Isn't this the same as just not providing a top half at all?
> 
> I.e. Pass null to request_threaded_irq where this function was.
> 
> Thanks,
> 
> Jonathan

Right, for the moment we do not need it. It will be probably useful adding
buffering support for sensors that do not support hw timestamping in FIFO
(e.g. LSM9DS1). I am fine both ways, so up to you.

Regards,
Lorenzo

> 
> 
> >  }
> >  
> >  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> 

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

Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread

[Jonathan Cameron]

On Sun, 15 Sep 2019 14:48:40 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > On Fri, 13 Sep 2019 11:07:07 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> > > 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 810807c52d5f..80a94335175f 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -1717,9 +1717,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;  
> > 
> > I missed this before.  Isn't this the same as just not providing a top half at all?
> > 
> > I.e. Pass null to request_threaded_irq where this function was.
> > 
> > Thanks,
> > 
> > Jonathan  
> 
> Right, for the moment we do not need it. It will be probably useful adding
> buffering support for sensors that do not support hw timestamping in FIFO
> (e.g. LSM9DS1). I am fine both ways, so up to you.
Scrap it for now. I suspect someone will have a script out there that will
fire on this and generate a patch removing it.  Better to not waste people's
time!

Thanks,

Jonathan

> 
> Regards,
> Lorenzo
> 
> > 
> >   
> > >  }
> > >  
> > >  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)  
> >   

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

[Lorenzo Bianconi]

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

> On Fri, 13 Sep 2019 11:07:08 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
> 
> > Report iio motion events to iio subsystem
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> I got to the earlier thread rather late so hadn't replied to your
> question on filtering events that haven't been enabled.

IIUC, how is possible to filter events? it seems not currently supported in
hw, right?

> 
> Hence I think it's just that change outstanding
> + I want to give Lorenzo time for a final look.

Jonathan the series seems fine to me now, there are some leftover nitpicks we can
cover with some follow-up patches.

Regards,
Lorenzo

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > Changes since v4:
> >  * Updated bitmask as pr Jonathans comments
> > 
> > Changes since v5:
> >  * None
> > 
> > Changes since v6:
> >  * None
> > 
> > Changes since v7:
> >  * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
> > @@ -287,6 +288,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),
> >  		},
> >  	},
> >  	{
> > @@ -412,6 +418,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),
> >  		},
> >  	},
> >  	{
> > @@ -550,6 +561,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),
> >  		},
> >  	},
> >  	{
> > @@ -816,6 +832,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),
> >  		},
> >  	},
> >  	{
> > @@ -970,6 +991,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),
> >  		}
> >  	},
> >  };
> > @@ -1715,6 +1741,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;
> > @@ -1724,6 +1782,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);
> 

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

Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread

[Lorenzo Bianconi]

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

> On Sun, 15 Sep 2019 14:48:40 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > > On Fri, 13 Sep 2019 11:07:07 +0200
> > > Sean Nyekjaer <sean@geanix.com> wrote:
> > >   
> > > > 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 810807c52d5f..80a94335175f 100644
> > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > @@ -1717,9 +1717,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;  
> > > 
> > > I missed this before.  Isn't this the same as just not providing a top half at all?
> > > 
> > > I.e. Pass null to request_threaded_irq where this function was.
> > > 
> > > Thanks,
> > > 
> > > Jonathan  
> > 
> > Right, for the moment we do not need it. It will be probably useful adding
> > buffering support for sensors that do not support hw timestamping in FIFO
> > (e.g. LSM9DS1). I am fine both ways, so up to you.
> Scrap it for now. I suspect someone will have a script out there that will
> fire on this and generate a patch removing it.  Better to not waste people's
> time!

ack :)
@Sean: can you please fix it in v9?

Regards,
Lorenzo

> 
> Thanks,
> 
> Jonathan
> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > >   
> > > >  }
> > > >  
> > > >  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)  
> > >   
> 

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

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

[Jonathan Cameron]

On Sun, 15 Sep 2019 15:05:08 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > On Fri, 13 Sep 2019 11:07:08 +0200
> > Sean Nyekjaer <sean@geanix.com> wrote:
> >   
> > > Report iio motion events to iio subsystem
> > > 
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>  
> > I got to the earlier thread rather late so hadn't replied to your
> > question on filtering events that haven't been enabled.  
> 
> IIUC, how is possible to filter events? it seems not currently supported in
> hw, right?
Filter them in software on their way to userspace.  So you'll get an interrupt
either way, but no need to tell userspace about event's it's not interested
in.
> 
> > 
> > Hence I think it's just that change outstanding
> > + I want to give Lorenzo time for a final look.  
> 
> Jonathan the series seems fine to me now, there are some leftover nitpicks we can
> cover with some follow-up patches.

Acks/Reviewed-bys?  Let's do this formally!

Jonathan

> 
> Regards,
> Lorenzo
> 
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > > Changes since v4:
> > >  * Updated bitmask as pr Jonathans comments
> > > 
> > > Changes since v5:
> > >  * None
> > > 
> > > Changes since v6:
> > >  * None
> > > 
> > > Changes since v7:
> > >  * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
> > > @@ -287,6 +288,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),
> > >  		},
> > >  	},
> > >  	{
> > > @@ -412,6 +418,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),
> > >  		},
> > >  	},
> > >  	{
> > > @@ -550,6 +561,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),
> > >  		},
> > >  	},
> > >  	{
> > > @@ -816,6 +832,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),
> > >  		},
> > >  	},
> > >  	{
> > > @@ -970,6 +991,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),
> > >  		}
> > >  	},
> > >  };
> > > @@ -1715,6 +1741,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;
> > > @@ -1724,6 +1782,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);  
> >   

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

[Sean Nyekjaer]

On 15/09/2019 15.18, Jonathan Cameron wrote:
> On Sun, 15 Sep 2019 15:05:08 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
>>> On Fri, 13 Sep 2019 11:07:08 +0200
>>> Sean Nyekjaer <sean@geanix.com> wrote:
>>>    
>>>> Report iio motion events to iio subsystem
>>>>
>>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>>> I got to the earlier thread rather late so hadn't replied to your
>>> question on filtering events that haven't been enabled.
>>
>> IIUC, how is possible to filter events? it seems not currently supported in
>> hw, right?
> Filter them in software on their way to userspace.  So you'll get an interrupt
> either way, but no need to tell userspace about event's it's not interested
> in.

Fine by me, will submit a V9.

Which one is preferred:
in_accel_thresh_(x,y,z)_en -> in_accel_thresh_both_en
or keep in_accel_thresh_(x,y,z)_en and filter them in the driver?

We would wake on all events either way

/Sean

>>
>>>
>>> Hence I think it's just that change outstanding
>>> + I want to give Lorenzo time for a final look.
>>
>> Jonathan the series seems fine to me now, there are some leftover nitpicks we can
>> cover with some follow-up patches.
> 
> Acks/Reviewed-bys?  Let's do this formally!
> 
> Jonathan
> 
>>
>> Regards,
>> Lorenzo
>>
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>    
>>>> ---
>>>> Changes since v4:
>>>>   * Updated bitmask as pr Jonathans comments
>>>>
>>>> Changes since v5:
>>>>   * None
>>>>
>>>> Changes since v6:
>>>>   * None
>>>>
>>>> Changes since v7:
>>>>   * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
>>>> @@ -287,6 +288,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),
>>>>   		},
>>>>   	},
>>>>   	{
>>>> @@ -412,6 +418,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),
>>>>   		},
>>>>   	},
>>>>   	{
>>>> @@ -550,6 +561,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),
>>>>   		},
>>>>   	},
>>>>   	{
>>>> @@ -816,6 +832,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),
>>>>   		},
>>>>   	},
>>>>   	{
>>>> @@ -970,6 +991,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),
>>>>   		}
>>>>   	},
>>>>   };
>>>> @@ -1715,6 +1741,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;
>>>> @@ -1724,6 +1782,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);
>>>    
> 

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

[Jonathan Cameron]

On Sun, 15 Sep 2019 15:22:44 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 15/09/2019 15.18, Jonathan Cameron wrote:
> > On Sun, 15 Sep 2019 15:05:08 +0200
> > Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >   
> >>> On Fri, 13 Sep 2019 11:07:08 +0200
> >>> Sean Nyekjaer <sean@geanix.com> wrote:
> >>>      
> >>>> Report iio motion events to iio subsystem
> >>>>
> >>>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>  
> >>> I got to the earlier thread rather late so hadn't replied to your
> >>> question on filtering events that haven't been enabled.  
> >>
> >> IIUC, how is possible to filter events? it seems not currently supported in
> >> hw, right?  
> > Filter them in software on their way to userspace.  So you'll get an interrupt
> > either way, but no need to tell userspace about event's it's not interested
> > in.  
> 
> Fine by me, will submit a V9.
> 
> Which one is preferred:
> in_accel_thresh_(x,y,z)_en -> in_accel_thresh_both_en
> or keep in_accel_thresh_(x,y,z)_en and filter them in the driver?
> 
> We would wake on all events either way

I think I slightly prefer filtering in the driver.  Wakeup will
just have to be a bit strange if someone actually doesn't want to wake
based on all axis.

Thanks,

Jonathan

> 
> /Sean
> 
> >>  
> >>>
> >>> Hence I think it's just that change outstanding
> >>> + I want to give Lorenzo time for a final look.  
> >>
> >> Jonathan the series seems fine to me now, there are some leftover nitpicks we can
> >> cover with some follow-up patches.  
> > 
> > Acks/Reviewed-bys?  Let's do this formally!
> > 
> > Jonathan
> >   
> >>
> >> Regards,
> >> Lorenzo
> >>  
> >>>
> >>> Thanks,
> >>>
> >>> Jonathan
> >>>      
> >>>> ---
> >>>> Changes since v4:
> >>>>   * Updated bitmask as pr Jonathans comments
> >>>>
> >>>> Changes since v5:
> >>>>   * None
> >>>>
> >>>> Changes since v6:
> >>>>   * None
> >>>>
> >>>> Changes since v7:
> >>>>   * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
> >>>> @@ -287,6 +288,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),
> >>>>   		},
> >>>>   	},
> >>>>   	{
> >>>> @@ -412,6 +418,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),
> >>>>   		},
> >>>>   	},
> >>>>   	{
> >>>> @@ -550,6 +561,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),
> >>>>   		},
> >>>>   	},
> >>>>   	{
> >>>> @@ -816,6 +832,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),
> >>>>   		},
> >>>>   	},
> >>>>   	{
> >>>> @@ -970,6 +991,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),
> >>>>   		}
> >>>>   	},
> >>>>   };
> >>>> @@ -1715,6 +1741,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;
> >>>> @@ -1724,6 +1782,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);  
> >>>      
> >   

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

[Lorenzo Bianconi]

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

> On Sun, 15 Sep 2019 15:05:08 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> > > On Fri, 13 Sep 2019 11:07:08 +0200
> > > Sean Nyekjaer <sean@geanix.com> wrote:
> > >   
> > > > Report iio motion events to iio subsystem
> > > > 
> > > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>  
> > > I got to the earlier thread rather late so hadn't replied to your
> > > question on filtering events that haven't been enabled.  
> > 
> > IIUC, how is possible to filter events? it seems not currently supported in
> > hw, right?
> Filter them in software on their way to userspace.  So you'll get an interrupt
> either way, but no need to tell userspace about event's it's not interested
> in.

ack

> > 
> > > 
> > > Hence I think it's just that change outstanding
> > > + I want to give Lorenzo time for a final look.  
> > 
> > Jonathan the series seems fine to me now, there are some leftover nitpicks we can
> > cover with some follow-up patches.
> 
> Acks/Reviewed-bys?  Let's do this formally!

I will look at v9 and then I will add my acked-by.

Regards,
Lorenzo

> 
> Jonathan
> 
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > Thanks,
> > > 
> > > Jonathan
> > >   
> > > > ---
> > > > Changes since v4:
> > > >  * Updated bitmask as pr Jonathans comments
> > > > 
> > > > Changes since v5:
> > > >  * None
> > > > 
> > > > Changes since v6:
> > > >  * None
> > > > 
> > > > Changes since v7:
> > > >  * 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 449c2798f7ed..7c50fac7b85c 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 80a94335175f..66700c20920d 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>
> > > > @@ -287,6 +288,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),
> > > >  		},
> > > >  	},
> > > >  	{
> > > > @@ -412,6 +418,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),
> > > >  		},
> > > >  	},
> > > >  	{
> > > > @@ -550,6 +561,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),
> > > >  		},
> > > >  	},
> > > >  	{
> > > > @@ -816,6 +832,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),
> > > >  		},
> > > >  	},
> > > >  	{
> > > > @@ -970,6 +991,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),
> > > >  		}
> > > >  	},
> > > >  };
> > > > @@ -1715,6 +1741,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;
> > > > @@ -1724,6 +1782,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);  
> > >   
> 

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

Re: [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option

[Lorenzo Bianconi]

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

> 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
> 
> Changes since v6:
>  * None
> 
> Changes since v7:
>  * Add check for hw->enable_event
>  * Moved disable_irq_wake section so it's called
>  * Removed not neeeded continue from disable_irq_wake section
> 
>  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 4198ba263d03..810807c52d5f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1858,6 +1858,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);
> @@ -1876,6 +1879,13 @@ 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 &&

since you have to send v9...better to use sensor->id instead of i here


> +		    hw->enable_event) {
> +			/* 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)
> @@ -1904,6 +1914,10 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
>  		if (!hw->iio_devs[i])
>  			continue;
>  
> +		if (device_may_wakeup(dev) && i == ST_LSM6DSX_ID_ACC &&
> +		    hw->enable_event)
> +			disable_irq_wake(hw->irq);
> +
>  		sensor = iio_priv(hw->iio_devs[i]);
>  		if (!(hw->suspend_mask & BIT(sensor->id)))
>  			continue;
> -- 
> 2.23.0
> 

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

[RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Sean Nyekjaer]

Do not report non enabled motion events.
Wakeup will still be on all channels as it's not possible to do the
filtering in hw.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Hope it's okay to do this as an RFC. To get the most obvious stuff
reviewed before v9

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  1 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 20 ++++++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 938192212485..dd46209f94e8 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -362,6 +362,7 @@ struct st_lsm6dsx_hw {
 
 	u8 event_threshold;
 	bool enable_event;
+	u8 event_en_mask;
 	struct st_lsm6dsx_reg irq_routing;
 
 	u8 *buff;
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 7596a6ed7d97..2d66e3758921 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1377,9 +1377,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 	if (type != IIO_EV_TYPE_THRESH)
 		return -EINVAL;
 
+	if (hw->event_en_mask & BIT(chan->channel2))
+		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)
@@ -1391,6 +1394,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
 
 	hw->enable_event = state;
 
+out:
+	if (state)
+		hw->event_en_mask |= BIT(chan->channel2);
+	else
+		hw->event_en_mask &= ~BIT(chan->channel2);
+
 	return 0;
 }
 
@@ -1746,7 +1755,8 @@ 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)
+	if (data & hw->settings->event_settings.wakeup_src_z_mask &&
+	    hw->event_en_mask & BIT(IIO_MOD_Z))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
@@ -1755,7 +1765,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
 						  IIO_EV_DIR_EITHER),
 						  timestamp);
 
-	if (data & hw->settings->event_settings.wakeup_src_y_mask)
+	if (data & hw->settings->event_settings.wakeup_src_y_mask &&
+	    hw->event_en_mask & BIT(IIO_MOD_Y))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
@@ -1764,7 +1775,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
 						  IIO_EV_DIR_EITHER),
 						  timestamp);
 
-	if (data & hw->settings->event_settings.wakeup_src_x_mask)
+	if (data & hw->settings->event_settings.wakeup_src_x_mask &&
+	    hw->event_en_mask & BIT(IIO_MOD_X))
 		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
 			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
 						  0,
-- 
2.23.0

Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Lorenzo Bianconi]

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

> Do not report non enabled motion events.
> Wakeup will still be on all channels as it's not possible to do the
> filtering in hw.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> Hope it's okay to do this as an RFC. To get the most obvious stuff
> reviewed before v9
> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  1 +
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 20 ++++++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index 938192212485..dd46209f94e8 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -362,6 +362,7 @@ struct st_lsm6dsx_hw {
>  
>  	u8 event_threshold;
>  	bool enable_event;
> +	u8 event_en_mask;

I think we do not need it, you could just use enable_event as bitmask (just
convert it in u8)

>  	struct st_lsm6dsx_reg irq_routing;
>  
>  	u8 *buff;
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 7596a6ed7d97..2d66e3758921 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1377,9 +1377,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>  	if (type != IIO_EV_TYPE_THRESH)
>  		return -EINVAL;
>  
> +	if (hw->event_en_mask & BIT(chan->channel2))
> +		goto out;

Why do we need this?

> +
>  	/* 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)
> @@ -1391,6 +1394,12 @@ static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>  
>  	hw->enable_event = state;
>  
> +out:
> +	if (state)
> +		hw->event_en_mask |= BIT(chan->channel2);
> +	else
> +		hw->event_en_mask &= ~BIT(chan->channel2);

you can use enable_event here instead of event_en_mask

> +
>  	return 0;
>  }
>  
> @@ -1746,7 +1755,8 @@ 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)
> +	if (data & hw->settings->event_settings.wakeup_src_z_mask &&
> +	    hw->event_en_mask & BIT(IIO_MOD_Z))

please add () 

if ((data & hw->settings->event_settings.wakeup_src_z_mask) &&
    (hw->event_en_mask & BIT(IIO_MOD_Z))

>  		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>  			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>  						  0,
> @@ -1755,7 +1765,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
>  						  IIO_EV_DIR_EITHER),
>  						  timestamp);
>  
> -	if (data & hw->settings->event_settings.wakeup_src_y_mask)
> +	if (data & hw->settings->event_settings.wakeup_src_y_mask &&
> +	    hw->event_en_mask & BIT(IIO_MOD_Y))
>  		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>  			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>  						  0,
> @@ -1764,7 +1775,8 @@ void st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw, int data)
>  						  IIO_EV_DIR_EITHER),
>  						  timestamp);
>  
> -	if (data & hw->settings->event_settings.wakeup_src_x_mask)
> +	if (data & hw->settings->event_settings.wakeup_src_x_mask &&
> +	    hw->event_en_mask & BIT(IIO_MOD_X))
>  		iio_push_event(hw->iio_devs[ST_LSM6DSX_ID_ACC],
>  			       IIO_MOD_EVENT_CODE(IIO_ACCEL,
>  						  0,
> -- 
> 2.23.0
> 

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

Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Sean Nyekjaer]

On 16/09/2019 11.16, Lorenzo Bianconi wrote:
>> +	if (hw->event_en_mask & BIT(chan->channel2))
>> +		goto out;
> Why do we need this?
> 

Guess we need to check if 0 < hw->enable_event before disabling the 
sensor...
>> +
>>   	/* do not enable events if they are already enabled */
>>   	if (state && hw->enable_event)
>> -		return 0;
>> +		goto out;
>>   

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;
         u8 _enable_event;
         int err = 0;

         if (type != IIO_EV_TYPE_THRESH)
                 return -EINVAL;

         if (state) {
                 _enable_event = hw->enable_event | BIT(chan->channel2);

                 /* do not enable events if they are already enabled */
                 if (0 < hw->enable_event)
                         goto out;
         } else {
                 _enable_event = hw->enable_event & ~BIT(chan->channel2);

                 /* only turn off sensor if no events is enabled */ 
 
 
 

                 if (0 != _enable_event)
                         goto out;
         }

         /* stop here if no changes have been made */
         if (hw->enable_event == _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;

out:
         hw->enable_event = _enable_event;

         return 0;
}

Is something like this better?

Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Lorenzo Bianconi]

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

> 
> 
> On 16/09/2019 11.16, Lorenzo Bianconi wrote:
> > > +	if (hw->event_en_mask & BIT(chan->channel2))
> > > +		goto out;
> > Why do we need this?
> > 
> 
> Guess we need to check if 0 < hw->enable_event before disabling the
> sensor...
> > > +
> > >   	/* do not enable events if they are already enabled */
> > >   	if (state && hw->enable_event)
> > > -		return 0;
> > > +		goto out;
> 
> 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;
>         u8 _enable_event;

please use enable_event instead of _enable_event

>         int err = 0;
> 
>         if (type != IIO_EV_TYPE_THRESH)
>                 return -EINVAL;
> 
>         if (state) {
>                 _enable_event = hw->enable_event | BIT(chan->channel2);
> 
>                 /* do not enable events if they are already enabled */
>                 if (0 < hw->enable_event)
>                         goto out;

we do not need this since there is the check:
if (hw->enable_event == enable_event)
	return 0;

>         } else {
>                 _enable_event = hw->enable_event & ~BIT(chan->channel2);
> 
>                 /* only turn off sensor if no events is enabled */
> 
> 
> 
> 
>                 if (0 != _enable_event)
>                         goto out;

we do not need this as well

>         }
> 
>         /* stop here if no changes have been made */
>         if (hw->enable_event == _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;
> 
> out:
>         hw->enable_event = _enable_event;
> 
>         return 0;
> }
> 
> Is something like this better?

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

Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Sean Nyekjaer]

On 16/09/2019 13.55, Lorenzo Bianconi wrote:
>>
>>
>> On 16/09/2019 11.16, Lorenzo Bianconi wrote:
>>>> +	if (hw->event_en_mask & BIT(chan->channel2))
>>>> +		goto out;
>>> Why do we need this?
>>>
>>
>> Guess we need to check if 0 < hw->enable_event before disabling the
>> sensor...
>>>> +
>>>>    	/* do not enable events if they are already enabled */
>>>>    	if (state && hw->enable_event)
>>>> -		return 0;
>>>> +		goto out;
>>
>> 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;
>>          u8 _enable_event;
> 
> please use enable_event instead of _enable_event
> 
>>          int err = 0;
>>
>>          if (type != IIO_EV_TYPE_THRESH)
>>                  return -EINVAL;
>>
>>          if (state) {
>>                  _enable_event = hw->enable_event | BIT(chan->channel2);
>>
>>                  /* do not enable events if they are already enabled */
>>                  if (0 < hw->enable_event)
>>                          goto out;
> 
> we do not need this since there is the check:
> if (hw->enable_event == enable_event)
> 	return 0;

I actually think this is needed as if a channel is enabled and another 
channel is enabled, then 'state' will be 1 and '0 < hw->enable_event' is 
true. Then we don't want to do the event_setup again.

> 
>>          } else {
>>                  _enable_event = hw->enable_event & ~BIT(chan->channel2);
>>
>>                  /* only turn off sensor if no events is enabled */
>>
>>
>>
>>
>>                  if (0 != _enable_event)
>>                          goto out;
> 
> we do not need this as well
Like wise here, if we don't have this and to channels is enabled, if you 
deactivate one of the channels then enable_event indicates that a 
channel is active but the sensor is disabled.
Guess that is not what we want :-)

> 
>>          }
>>
>>          /* stop here if no changes have been made */
>>          if (hw->enable_event == _enable_event)
>>                  return 0;
>>
>>          err = st_lsm6dsx_event_setup(hw, state);
>>          if (err < 0)
>>                  return err;
>>

Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver

[Lorenzo Bianconi]

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

On Sep 16, Sean Nyekjaer wrote:
> 
> 
> On 16/09/2019 13.55, Lorenzo Bianconi wrote:
> > > 
> > > 
> > > On 16/09/2019 11.16, Lorenzo Bianconi wrote:
> > > > > +	if (hw->event_en_mask & BIT(chan->channel2))
> > > > > +		goto out;
> > > > Why do we need this?
> > > > 
> > > 
> > > Guess we need to check if 0 < hw->enable_event before disabling the
> > > sensor...
> > > > > +
> > > > >    	/* do not enable events if they are already enabled */
> > > > >    	if (state && hw->enable_event)
> > > > > -		return 0;
> > > > > +		goto out;
> > > 
> > > 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;
> > >          u8 _enable_event;
> > 
> > please use enable_event instead of _enable_event
> > 
> > >          int err = 0;
> > > 
> > >          if (type != IIO_EV_TYPE_THRESH)
> > >                  return -EINVAL;
> > > 
> > >          if (state) {
> > >                  _enable_event = hw->enable_event | BIT(chan->channel2);
> > > 
> > >                  /* do not enable events if they are already enabled */
> > >                  if (0 < hw->enable_event)
> > >                          goto out;
> > 
> > we do not need this since there is the check:
> > if (hw->enable_event == enable_event)
> > 	return 0;
> 
> I actually think this is needed as if a channel is enabled and another
> channel is enabled, then 'state' will be 1 and '0 < hw->enable_event' is
> true. Then we don't want to do the event_setup again.

got what you mean here, right..just please do:

if (hw->enable_event)
	goto out;

> 
> > 
> > >          } else {
> > >                  _enable_event = hw->enable_event & ~BIT(chan->channel2);
> > > 
> > >                  /* only turn off sensor if no events is enabled */
> > > 
> > > 
> > > 
> > > 
> > >                  if (0 != _enable_event)
> > >                          goto out;

if (enable_event)
	goto out;

> > 
> > we do not need this as well
> Like wise here, if we don't have this and to channels is enabled, if you
> deactivate one of the channels then enable_event indicates that a channel is
> active but the sensor is disabled.
> Guess that is not what we want :-)
> 
> > 
> > >          }
> > > 
> > >          /* stop here if no changes have been made */
> > >          if (hw->enable_event == _enable_event)
> > >                  return 0;
> > > 
> > >          err = st_lsm6dsx_event_setup(hw, state);
> > >          if (err < 0)
> > >                  return err;
> > > 
> 

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