Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v8 0/5] enable motion events for st_lsm6dsx
@ 2019-09-13  9:07 Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-13  9:07 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v8 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core
  2019-09-13  9:07 [PATCH v8 0/5] enable motion events for st_lsm6dsx Sean Nyekjaer
@ 2019-09-13  9:07 ` Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 2/5] iio: imu: st_lsm6dsx: add motion events Sean Nyekjaer
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-13  9:07 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti,
	martin, Lorenzo Bianconi

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v8 2/5] iio: imu: st_lsm6dsx: add motion events
  2019-09-13  9:07 [PATCH v8 0/5] enable motion events for st_lsm6dsx Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
@ 2019-09-13  9:07 ` Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-13  9:07 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
  2019-09-13  9:07 [PATCH v8 0/5] enable motion events for st_lsm6dsx Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 2/5] iio: imu: st_lsm6dsx: add motion events Sean Nyekjaer
@ 2019-09-13  9:07 ` Sean Nyekjaer
  2019-09-15 13:42   ` Lorenzo Bianconi
  2019-09-13  9:07 ` [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
  2019-09-13  9:07 ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Sean Nyekjaer
  4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-13  9:07 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
  2019-09-13  9:07 [PATCH v8 0/5] enable motion events for st_lsm6dsx Sean Nyekjaer
                   ` (2 preceding siblings ...)
  2019-09-13  9:07 ` [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
@ 2019-09-13  9:07 ` Sean Nyekjaer
  2019-09-15 12:33   ` Jonathan Cameron
  2019-09-13  9:07 ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Sean Nyekjaer
  4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-13  9:07 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-13  9:07 [PATCH v8 0/5] enable motion events for st_lsm6dsx Sean Nyekjaer
                   ` (3 preceding siblings ...)
  2019-09-13  9:07 ` [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
@ 2019-09-13  9:07 ` Sean Nyekjaer
  2019-09-15 12:30   ` Jonathan Cameron
  4 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-13  9:07 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-13  9:07 ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Sean Nyekjaer
@ 2019-09-15 12:30   ` Jonathan Cameron
  2019-09-15 13:05     ` Lorenzo Bianconi
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-09-15 12:30 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: linux-iio, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
  2019-09-13  9:07 ` [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
@ 2019-09-15 12:33   ` Jonathan Cameron
  2019-09-15 12:48     ` Lorenzo Bianconi
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-09-15 12:33 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: linux-iio, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin

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)


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
  2019-09-15 12:33   ` Jonathan Cameron
@ 2019-09-15 12:48     ` Lorenzo Bianconi
  2019-09-15 13:00       ` Jonathan Cameron
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-15 12:48 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
  2019-09-15 12:48     ` Lorenzo Bianconi
@ 2019-09-15 13:00       ` Jonathan Cameron
  2019-09-15 13:07         ` Lorenzo Bianconi
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-09-15 13:00 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-15 12:30   ` Jonathan Cameron
@ 2019-09-15 13:05     ` Lorenzo Bianconi
  2019-09-15 13:18       ` Jonathan Cameron
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-15 13:05 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread
  2019-09-15 13:00       ` Jonathan Cameron
@ 2019-09-15 13:07         ` Lorenzo Bianconi
  0 siblings, 0 replies; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-15 13:07 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-15 13:05     ` Lorenzo Bianconi
@ 2019-09-15 13:18       ` Jonathan Cameron
  2019-09-15 13:22         ` Sean Nyekjaer
  2019-09-15 13:41         ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Lorenzo Bianconi
  0 siblings, 2 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-09-15 13:18 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-15 13:18       ` Jonathan Cameron
@ 2019-09-15 13:22         ` Sean Nyekjaer
  2019-09-15 13:35           ` Jonathan Cameron
  2019-09-15 13:41         ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Lorenzo Bianconi
  1 sibling, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-15 13:22 UTC (permalink / raw)
  To: Jonathan Cameron, Lorenzo Bianconi
  Cc: linux-iio, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin



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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-15 13:22         ` Sean Nyekjaer
@ 2019-09-15 13:35           ` Jonathan Cameron
  2019-09-16  9:02             ` [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver Sean Nyekjaer
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-09-15 13:35 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: Lorenzo Bianconi, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt
  2019-09-15 13:18       ` Jonathan Cameron
  2019-09-15 13:22         ` Sean Nyekjaer
@ 2019-09-15 13:41         ` Lorenzo Bianconi
  1 sibling, 0 replies; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-15 13:41 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Sean Nyekjaer, linux-iio, lorenzo.bianconi83, denis.ciocca,
	mario.tesi, armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option
  2019-09-13  9:07 ` [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
@ 2019-09-15 13:42   ` Lorenzo Bianconi
  0 siblings, 0 replies; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-15 13:42 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: linux-iio, jic23, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver
  2019-09-15 13:35           ` Jonathan Cameron
@ 2019-09-16  9:02             ` Sean Nyekjaer
  2019-09-16  9:16               ` Lorenzo Bianconi
  0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-16  9:02 UTC (permalink / raw)
  To: linux-iio, jic23, lorenzo.bianconi83
  Cc: Sean Nyekjaer, denis.ciocca, mario.tesi, armando.visconti, martin

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver
  2019-09-16  9:02             ` [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver Sean Nyekjaer
@ 2019-09-16  9:16               ` Lorenzo Bianconi
  2019-09-16 11:29                 ` Sean Nyekjaer
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-16  9:16 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: linux-iio, jic23, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver
  2019-09-16  9:16               ` Lorenzo Bianconi
@ 2019-09-16 11:29                 ` Sean Nyekjaer
  2019-09-16 11:55                   ` Lorenzo Bianconi
  0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-16 11:29 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: linux-iio, jic23, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin



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?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver
  2019-09-16 11:29                 ` Sean Nyekjaer
@ 2019-09-16 11:55                   ` Lorenzo Bianconi
  2019-09-16 12:09                     ` Sean Nyekjaer
  0 siblings, 1 reply; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-16 11:55 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: linux-iio, jic23, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver
  2019-09-16 11:55                   ` Lorenzo Bianconi
@ 2019-09-16 12:09                     ` Sean Nyekjaer
  2019-09-16 12:22                       ` Lorenzo Bianconi
  0 siblings, 1 reply; 23+ messages in thread
From: Sean Nyekjaer @ 2019-09-16 12:09 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: linux-iio, jic23, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin



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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver
  2019-09-16 12:09                     ` Sean Nyekjaer
@ 2019-09-16 12:22                       ` Lorenzo Bianconi
  0 siblings, 0 replies; 23+ messages in thread
From: Lorenzo Bianconi @ 2019-09-16 12:22 UTC (permalink / raw)
  To: Sean Nyekjaer
  Cc: linux-iio, jic23, lorenzo.bianconi83, denis.ciocca, mario.tesi,
	armando.visconti, martin

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

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, back to index

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-13  9:07 [PATCH v8 0/5] enable motion events for st_lsm6dsx Sean Nyekjaer
2019-09-13  9:07 ` [PATCH v8 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core Sean Nyekjaer
2019-09-13  9:07 ` [PATCH v8 2/5] iio: imu: st_lsm6dsx: add motion events Sean Nyekjaer
2019-09-13  9:07 ` [PATCH v8 3/5] iio: imu: st_lsm6dsx: add wakeup-source option Sean Nyekjaer
2019-09-15 13:42   ` Lorenzo Bianconi
2019-09-13  9:07 ` [PATCH v8 4/5] iio: imu: st_lsm6dsx: always enter interrupt thread Sean Nyekjaer
2019-09-15 12:33   ` Jonathan Cameron
2019-09-15 12:48     ` Lorenzo Bianconi
2019-09-15 13:00       ` Jonathan Cameron
2019-09-15 13:07         ` Lorenzo Bianconi
2019-09-13  9:07 ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Sean Nyekjaer
2019-09-15 12:30   ` Jonathan Cameron
2019-09-15 13:05     ` Lorenzo Bianconi
2019-09-15 13:18       ` Jonathan Cameron
2019-09-15 13:22         ` Sean Nyekjaer
2019-09-15 13:35           ` Jonathan Cameron
2019-09-16  9:02             ` [RFC PATCH] iio: imu: st_lsm6dsx: filter motion events in driver Sean Nyekjaer
2019-09-16  9:16               ` Lorenzo Bianconi
2019-09-16 11:29                 ` Sean Nyekjaer
2019-09-16 11:55                   ` Lorenzo Bianconi
2019-09-16 12:09                     ` Sean Nyekjaer
2019-09-16 12:22                       ` Lorenzo Bianconi
2019-09-15 13:41         ` [PATCH v8 5/5] iio: imu: st_lsm6dsx: add motion report function and call from interrupt Lorenzo Bianconi

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox