[PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup

[PATCH 0/5] iio: imu: st_lsm6dsx: add event reporting and wakeup

[Sean Nyekjaer]

Hi,

This series is completely reworked from the RFC, I hope I included all comments.
I'm now using the iio event system to report events while running. :-)

Wakeup is controlled via the PM framework, wakeup is only activated if
the event system have been activated before suspending.

/Sean

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       |   3 +
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    |  80 +----
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 302 +++++++++++++++++-
 3 files changed, 301 insertions(+), 84 deletions(-)

-- 
2.22.0

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

[Sean Nyekjaer]

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

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  1 +
 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 80 +----------------
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 87 +++++++++++++++++++
 3 files changed, 90 insertions(+), 78 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index edcd838037cd..a5e373680e9c 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
 int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
 int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
 			     enum st_lsm6dsx_fifo_mode fifo_mode);
+int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
 
 #endif /* ST_LSM6DSX_H */
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 631360b14ca7..a1ed61a64a64 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -25,8 +25,6 @@
  * Licensed under the GPL-2.
  */
 #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>
@@ -37,10 +35,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)
@@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
  *
  * Return: Number of bytes read from the FIFO
  */
-static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
+int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
 {
 	u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
 	u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
@@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, 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 = st_lsm6dsx_read_fifo(hw);
-	mutex_unlock(&hw->fifo_lock);
-
-	return !count ? IRQ_NONE : IRQ_HANDLED;
-}
-
 static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
 {
 	return st_lsm6dsx_update_fifo(iio_dev, true);
@@ -501,59 +476,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++) {
 		buffer = devm_iio_kfifo_allocate(hw->dev);
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index aebbe0ddd8d8..b5d3fa354de7 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -36,6 +36,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>
@@ -55,6 +57,11 @@
 #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR	0x13
 #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK	BIT(5)
 
+#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_ACC_ODR_ADDR		0x10
 #define ST_LSM6DSX_REG_ACC_ODR_MASK		GENMASK(7, 4)
 #define ST_LSM6DSX_REG_ACC_FS_ADDR		0x10
@@ -804,6 +811,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 = st_lsm6dsx_read_fifo(hw);
+	mutex_unlock(&hw->fifo_lock);
+
+	return !count ? IRQ_NONE : IRQ_HANDLED;
+}
+
+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 err;
+}
+
 int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
 		     struct regmap *regmap)
 {
@@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
 		return err;
 
 	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.22.0

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

[Sean Nyekjaer]

Add event channels that controls the creation of motion events.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
 2 files changed, 144 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index a5e373680e9c..966cc6e91c7f 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
 	u8 enable_mask;
 	u8 ts_sip;
 	u8 sip;
+	u8 event_threshold;
+	bool enable_event;
 
 	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 b5d3fa354de7..351c46f01662 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -78,6 +78,17 @@
 #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
 #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
 
+#define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
+#define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
+#define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
+
+#define ST_LSM6DSX_DEFAULT_WAKE_THRESH		0
+#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR		0x5B
+#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK	GENMASK(5, 0)
+
+#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
+#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK	BIT(5)
+
 #define ST_LSM6DSX_ACC_FS_2G_GAIN		IIO_G_TO_M_S_2(61)
 #define ST_LSM6DSX_ACC_FS_4G_GAIN		IIO_G_TO_M_S_2(122)
 #define ST_LSM6DSX_ACC_FS_8G_GAIN		IIO_G_TO_M_S_2(244)
@@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_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)
+};
+
 #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
 {									\
 	.type = chan_type,						\
@@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 		.storagebits = 16,					\
 		.endianness = IIO_LE,					\
 	},								\
+	.event_spec = &st_lsm6dsx_event,				\
+	.num_event_specs = 1,						\
 }
 
 static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
@@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
 				  ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
 }
 
+static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
+{
+	int err = 0;
+
+	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
+				 ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
+				 threshold);
+
+	if (!err)
+		hw->event_threshold = threshold;
+
+	return err;
+}
+
 int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
 {
 	int err;
@@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
 	int err, delay;
 	__le16 data;
 
-	err = st_lsm6dsx_sensor_enable(sensor);
-	if (err < 0)
-		return err;
+	if (!hw->enable_event) {
+		err = st_lsm6dsx_sensor_enable(sensor);
+		if (err < 0)
+			return err;
 
-	delay = 1000000 / sensor->odr;
-	usleep_range(delay, 2 * delay);
+		delay = 1000000 / sensor->odr;
+		usleep_range(delay, 2 * delay);
+	}
 
 	err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
 	if (err < 0)
 		return err;
 
-	st_lsm6dsx_sensor_disable(sensor);
+	if (!hw->enable_event)
+		st_lsm6dsx_sensor_disable(sensor);
 
 	*val = (s16)le16_to_cpu(data);
 
@@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
 	return err;
 }
 
+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);
+
+	*val2 = 0;
+	*val = sensor->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;
+
+	if (!hw->enable_event)
+		return -EBUSY;
+
+	if ((val < 0) || (val > 31))
+		return -EINVAL;
+
+	if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
+		return -EINVAL;
+
+	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;
+
+	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;
+
+	if (state && hw->enable_event)
+		return 0;
+
+	hw->enable_event = state;
+	if (state)
+		st_lsm6dsx_sensor_enable(sensor);
+	else
+		st_lsm6dsx_sensor_disable(sensor);
+
+	return 0;
+}
+
 static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
@@ -632,6 +738,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,
 };
 
@@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 	if (err < 0)
 		return err;
 
+	st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
+
 	return st_lsm6dsx_init_hw_timer(hw);
 }
 
@@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	return iio_dev;
 }
 
+int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
+{
+	int err;
+
+	/* Enable inactivity function - low power ACC, GYRO powered-down */
+	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
+				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
+				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
+				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
+				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
+	if (err < 0)
+		return err;
+
+	/* Enable wakeup interrupt */
+	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
+				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
+				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
+
+	return err;
+}
+
 static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
 {
 	struct st_lsm6dsx_hw *hw = private;
@@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
 		err = st_lsm6dsx_fifo_setup(hw);
 		if (err < 0)
 			return err;
+		err = st_lsm6dsx_event_setup(hw);
+		if (err < 0)
+			return err;
 	}
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
-- 
2.22.0

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

[Sean Nyekjaer]

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

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

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 351c46f01662..59a34894e495 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
 			return err;
 	}
 
+	if (dev->of_node)
+		if (of_property_read_bool(dev->of_node, "wakeup-source"))
+			device_init_wakeup(dev, true);
+
 	return 0;
 }
 EXPORT_SYMBOL(st_lsm6dsx_probe);
@@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
 	int i, err = 0;
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
+		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
+			/* Enable wake from IRQ */
+			enable_irq_wake(hw->irq);
+			continue;
+		}
+
 		sensor = iio_priv(hw->iio_devs[i]);
 		if (!(hw->enable_mask & BIT(sensor->id)))
 			continue;
@@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
 	int i, err = 0;
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
+		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
+			disable_irq_wake(hw->irq);
+			continue;
+		}
+
 		sensor = iio_priv(hw->iio_devs[i]);
 		if (!(hw->enable_mask & BIT(sensor->id)))
 			continue;
-- 
2.22.0

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

[Sean Nyekjaer]

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

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 59a34894e495..76aec5024d83 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -78,6 +78,12 @@
 #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
 #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
 
+#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR		0x1B
+#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK	BIT(0)
+#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK	BIT(1)
+#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK	BIT(2)
+#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK	BIT(4)
+
 #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
 #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
 #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
@@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(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)
 {
 	struct st_lsm6dsx_hw *hw = private;
-	int count;
+	int count = 0;
+	int data, err;
+
+	if (hw->enable_event) {
+		err = regmap_read(hw->regmap,
+				  ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);
+		if (err < 0)
+			goto try_fifo;
+
+	}
 
-	mutex_lock(&hw->fifo_lock);
-	count = st_lsm6dsx_read_fifo(hw);
-	mutex_unlock(&hw->fifo_lock);
+try_fifo:
+	if (hw->sip > 0) {
+		mutex_lock(&hw->fifo_lock);
+		count = st_lsm6dsx_read_fifo(hw);
+		mutex_unlock(&hw->fifo_lock);
+	}
 
 	return !count ? IRQ_NONE : IRQ_HANDLED;
 }
-- 
2.22.0

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

[Sean Nyekjaer]

Report iio motion events to iio subsystem

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 76aec5024d83..7b66799acf4d 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -34,6 +34,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>
@@ -949,6 +950,39 @@ int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
 
 	return err;
 }
+int 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 & ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_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 & ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_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 & ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_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);
+
+	return 0;
+}
 
 static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
 {
@@ -967,6 +1001,8 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
 		if (err < 0)
 			goto try_fifo;
 
+		if (data && ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK)
+			st_lsm6dsx_report_motion_event(hw, data);
 	}
 
 try_fifo:
-- 
2.22.0

Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events

[Lorenzo Bianconi]

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

> Add event channels that controls the creation of motion events.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
>  2 files changed, 144 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index a5e373680e9c..966cc6e91c7f 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
>  	u8 enable_mask;
>  	u8 ts_sip;
>  	u8 sip;
> +	u8 event_threshold;
> +	bool enable_event;
>  
>  	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 b5d3fa354de7..351c46f01662 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -78,6 +78,17 @@
>  #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
>  #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
>  
> +#define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
> +#define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
> +#define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
> +
> +#define ST_LSM6DSX_DEFAULT_WAKE_THRESH		0
> +#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR		0x5B
> +#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK	GENMASK(5, 0)

Could you please verify this registermap is used for all supported devices?
(e.g. LSM6DS3 or LSM6DS3H)

> +
> +#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
> +#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK	BIT(5)
> +
>  #define ST_LSM6DSX_ACC_FS_2G_GAIN		IIO_G_TO_M_S_2(61)
>  #define ST_LSM6DSX_ACC_FS_4G_GAIN		IIO_G_TO_M_S_2(122)
>  #define ST_LSM6DSX_ACC_FS_8G_GAIN		IIO_G_TO_M_S_2(244)
> @@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_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)
> +};
> +
>  #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
>  {									\
>  	.type = chan_type,						\
> @@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  		.storagebits = 16,					\
>  		.endianness = IIO_LE,					\
>  	},								\
> +	.event_spec = &st_lsm6dsx_event,				\
> +	.num_event_specs = 1,						\
>  }

ST_LSM6DSX_CHANNEL is used even for sensor-hub so I do not think you can use in
this way

>  
>  static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> @@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
>  				  ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
>  }
>  
> +static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
> +{
> +	int err = 0;
> +
> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
> +				 ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
> +				 threshold);
> +
> +	if (!err)
> +		hw->event_threshold = threshold;
> +
> +	return err;

I think we can just move this configuration in st_lsm6dsx_write_event()

> +}
> +
>  int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
>  {
>  	int err;
> @@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
>  	int err, delay;
>  	__le16 data;
>  
> -	err = st_lsm6dsx_sensor_enable(sensor);
> -	if (err < 0)
> -		return err;
> +	if (!hw->enable_event) {
> +		err = st_lsm6dsx_sensor_enable(sensor);
> +		if (err < 0)
> +			return err;
>  
> -	delay = 1000000 / sensor->odr;
> -	usleep_range(delay, 2 * delay);
> +		delay = 1000000 / sensor->odr;
> +		usleep_range(delay, 2 * delay);
> +	}
>  
>  	err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
>  	if (err < 0)
>  		return err;
>  
> -	st_lsm6dsx_sensor_disable(sensor);
> +	if (!hw->enable_event)
> +		st_lsm6dsx_sensor_disable(sensor);
>  
>  	*val = (s16)le16_to_cpu(data);
>  
> @@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
>  	return err;
>  }
>  
> +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);
> +
> +	*val2 = 0;
> +	*val = sensor->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;
> +
> +	if (!hw->enable_event)
> +		return -EBUSY;
> +
> +	if ((val < 0) || (val > 31))

unnecessary brackets

> +		return -EINVAL;
> +
> +	if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
> +		return -EINVAL;
> +
> +	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;
> +
> +	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;
> +
> +	if (state && hw->enable_event)
> +		return 0;
> +
> +	hw->enable_event = state;
> +	if (state)
> +		st_lsm6dsx_sensor_enable(sensor);

please correct me if I am wrong but in this way we break normal operation (e.g.
if we are reading acc data from the FIFO). You need to check enabled_mask.
Moreover we should check event type

> +	else
> +		st_lsm6dsx_sensor_disable(sensor);
> +
> +	return 0;
> +}
> +
>  static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
>  {
>  	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> @@ -632,6 +738,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,
>  };
>  
> @@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  	if (err < 0)
>  		return err;
>  
> +	st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);

we do not need this since default value is already 0

> +
>  	return st_lsm6dsx_init_hw_timer(hw);
>  }
>  
> @@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>  	return iio_dev;
>  }
>  
> +int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
> +{
> +	int err;
> +
> +	/* Enable inactivity function - low power ACC, GYRO powered-down */
> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
> +				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> +				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
> +				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> +				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
> +	if (err < 0)
> +		return err;
> +
> +	/* Enable wakeup interrupt */
> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
> +				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
> +				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
> +
> +	return err;
> +}
> +
>  static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>  {
>  	struct st_lsm6dsx_hw *hw = private;
> @@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>  		err = st_lsm6dsx_fifo_setup(hw);
>  		if (err < 0)
>  			return err;
> +		err = st_lsm6dsx_event_setup(hw);
> +		if (err < 0)
> +			return err;
>  	}
>  
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> -- 
> 2.22.0
> 

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1833 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>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 351c46f01662..59a34894e495 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>  			return err;
>  	}
>  
> +	if (dev->of_node)
> +		if (of_property_read_bool(dev->of_node, "wakeup-source"))
> +			device_init_wakeup(dev, true);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(st_lsm6dsx_probe);
> @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>  	int i, err = 0;
>  
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> +			/* Enable wake from IRQ */
> +			enable_irq_wake(hw->irq);
> +			continue;

This is breaking buffering mode

> +		}
> +
>  		sensor = iio_priv(hw->iio_devs[i]);
>  		if (!(hw->enable_mask & BIT(sensor->id)))
>  			continue;
> @@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
>  	int i, err = 0;
>  
>  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> +			disable_irq_wake(hw->irq);

same here

> +			continue;
> +		}
> +
>  		sensor = iio_priv(hw->iio_devs[i]);
>  		if (!(hw->enable_mask & BIT(sensor->id)))
>  			continue;
> -- 
> 2.22.0
> 

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

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

[Lorenzo Bianconi]

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

> 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 | 30 +++++++++++++++-----
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 59a34894e495..76aec5024d83 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -78,6 +78,12 @@
>  #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
>  #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
>  
> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR		0x1B
> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK	BIT(0)
> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK	BIT(1)
> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK	BIT(2)
> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK	BIT(4)
> +
>  #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
>  #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
>  #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
> @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(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 guess this will break shared interrupt, isn't it?

>  }
>  
>  static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>  {
>  	struct st_lsm6dsx_hw *hw = private;
> -	int count;
> +	int count = 0;
> +	int data, err;
> +
> +	if (hw->enable_event) {
> +		err = regmap_read(hw->regmap,
> +				  ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);
> +		if (err < 0)
> +			goto try_fifo;

You can simplify this just doing something like:

		if (err < 0 && !hw->sip)
			return IRQ_NONE;

> +
> +	}
>  
> -	mutex_lock(&hw->fifo_lock);
> -	count = st_lsm6dsx_read_fifo(hw);
> -	mutex_unlock(&hw->fifo_lock);
> +try_fifo:
> +	if (hw->sip > 0) {
> +		mutex_lock(&hw->fifo_lock);
> +		count = st_lsm6dsx_read_fifo(hw);
> +		mutex_unlock(&hw->fifo_lock);
> +	}
>  
>  	return !count ? IRQ_NONE : IRQ_HANDLED;
>  }
> -- 
> 2.22.0
> 

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

Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core

[Lorenzo Bianconi]

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

> This prepares the interrupt to be used for other stuff than
> fifo reading -> event readings.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  1 +
>  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 80 +----------------
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 87 +++++++++++++++++++
>  3 files changed, 90 insertions(+), 78 deletions(-)
> 

I can't see why we need this patch

Regards,
Lorenzo

> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index edcd838037cd..a5e373680e9c 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
>  int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
>  int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
>  			     enum st_lsm6dsx_fifo_mode fifo_mode);
> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
>  
>  #endif /* ST_LSM6DSX_H */
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 631360b14ca7..a1ed61a64a64 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> @@ -25,8 +25,6 @@
>   * Licensed under the GPL-2.
>   */
>  #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>
> @@ -37,10 +35,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)
> @@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
>   *
>   * Return: Number of bytes read from the FIFO
>   */
> -static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>  {
>  	u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
>  	u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
> @@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, 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 = st_lsm6dsx_read_fifo(hw);
> -	mutex_unlock(&hw->fifo_lock);
> -
> -	return !count ? IRQ_NONE : IRQ_HANDLED;
> -}
> -
>  static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
>  {
>  	return st_lsm6dsx_update_fifo(iio_dev, true);
> @@ -501,59 +476,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++) {
>  		buffer = devm_iio_kfifo_allocate(hw->dev);
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index aebbe0ddd8d8..b5d3fa354de7 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -36,6 +36,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>
> @@ -55,6 +57,11 @@
>  #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR	0x13
>  #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK	BIT(5)
>  
> +#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_ACC_ODR_ADDR		0x10
>  #define ST_LSM6DSX_REG_ACC_ODR_MASK		GENMASK(7, 4)
>  #define ST_LSM6DSX_REG_ACC_FS_ADDR		0x10
> @@ -804,6 +811,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 = st_lsm6dsx_read_fifo(hw);
> +	mutex_unlock(&hw->fifo_lock);
> +
> +	return !count ? IRQ_NONE : IRQ_HANDLED;
> +}
> +
> +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 err;
> +}
> +
>  int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>  		     struct regmap *regmap)
>  {
> @@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>  		return err;
>  
>  	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.22.0
> 

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

Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core

[Sean Nyekjær]

On 18 Jun 2019, at 17.57, Lorenzo Bianconi <lorenzo@kernel.org> wrote:

>> This prepares the interrupt to be used for other stuff than
>> fifo reading -> event readings.
>> 
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  1 +
>> .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 80 +----------------
>> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 87 +++++++++++++++++++
>> 3 files changed, 90 insertions(+), 78 deletions(-)
>> 
> 
> I can't see why we need this patch
> 
> Regards,
> Lorenzo

The interrupt handling isn’t only for fifo reading, so I think it’s correct to move it out of the buffer handling file.

/Sean
> 
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index edcd838037cd..a5e373680e9c 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
>> int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
>> int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
>>                 enum st_lsm6dsx_fifo_mode fifo_mode);
>> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
>> 
>> #endif /* ST_LSM6DSX_H */
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
>> index 631360b14ca7..a1ed61a64a64 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
>> @@ -25,8 +25,6 @@
>>  * Licensed under the GPL-2.
>>  */
>> #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>
>> @@ -37,10 +35,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)
>> @@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
>>  *
>>  * Return: Number of bytes read from the FIFO
>>  */
>> -static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
>> {
>>    u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
>>    u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
>> @@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, 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 = st_lsm6dsx_read_fifo(hw);
>> -    mutex_unlock(&hw->fifo_lock);
>> -
>> -    return !count ? IRQ_NONE : IRQ_HANDLED;
>> -}
>> -
>> static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
>> {
>>    return st_lsm6dsx_update_fifo(iio_dev, true);
>> @@ -501,59 +476,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++) {
>>        buffer = devm_iio_kfifo_allocate(hw->dev);
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index aebbe0ddd8d8..b5d3fa354de7 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -36,6 +36,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>
>> @@ -55,6 +57,11 @@
>> #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR    0x13
>> #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK    BIT(5)
>> 
>> +#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_ACC_ODR_ADDR        0x10
>> #define ST_LSM6DSX_REG_ACC_ODR_MASK        GENMASK(7, 4)
>> #define ST_LSM6DSX_REG_ACC_FS_ADDR        0x10
>> @@ -804,6 +811,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 = st_lsm6dsx_read_fifo(hw);
>> +    mutex_unlock(&hw->fifo_lock);
>> +
>> +    return !count ? IRQ_NONE : IRQ_HANDLED;
>> +}
>> +
>> +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 err;
>> +}
>> +
>> int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>             struct regmap *regmap)
>> {
>> @@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>        return err;
>> 
>>    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.22.0
>> 

Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events

[Sean Nyekjaer]

On 18/06/2019 17.49, Lorenzo Bianconi wrote:
>> Add event channels that controls the creation of motion events.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
>>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
>>   2 files changed, 144 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index a5e373680e9c..966cc6e91c7f 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
>>   	u8 enable_mask;
>>   	u8 ts_sip;
>>   	u8 sip;
>> +	u8 event_threshold;
>> +	bool enable_event;
>>   
>>   	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 b5d3fa354de7..351c46f01662 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -78,6 +78,17 @@
>>   #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
>>   #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
>>   
>> +#define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
>> +#define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
>> +#define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
>> +
>> +#define ST_LSM6DSX_DEFAULT_WAKE_THRESH		0
>> +#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR		0x5B
>> +#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK	GENMASK(5, 0)
> 
> Could you please verify this registermap is used for all supported devices?
> (e.g. LSM6DS3 or LSM6DS3H)

They have the same registers and the bits meaning the same.
But I can't test on those devices...

> 
>> +
>> +#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
>> +#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK	BIT(5)
>> +
>>   #define ST_LSM6DSX_ACC_FS_2G_GAIN		IIO_G_TO_M_S_2(61)
>>   #define ST_LSM6DSX_ACC_FS_4G_GAIN		IIO_G_TO_M_S_2(122)
>>   #define ST_LSM6DSX_ACC_FS_8G_GAIN		IIO_G_TO_M_S_2(244)
>> @@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_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)
>> +};
>> +
>>   #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
>>   {									\
>>   	.type = chan_type,						\
>> @@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>>   		.storagebits = 16,					\
>>   		.endianness = IIO_LE,					\
>>   	},								\
>> +	.event_spec = &st_lsm6dsx_event,				\
>> +	.num_event_specs = 1,						\
>>   }
> 
> ST_LSM6DSX_CHANNEL is used even for sensor-hub so I do not think you can use in
> this way

I see, will need to come up with a solution to avoid the event channels 
on the gyro.

> 
>>   
>>   static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
>> @@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
>>   				  ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
>>   }
>>   
>> +static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
>> +{
>> +	int err = 0;
>> +
>> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
>> +				 ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
>> +				 threshold);
>> +
>> +	if (!err)
>> +		hw->event_threshold = threshold;
>> +
>> +	return err;
> 
> I think we can just move this configuration in st_lsm6dsx_write_event()

We could, should I then call st_lsm6dsx_write_event() from the probe 
function?

> 
>> +}
>> +
>>   int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
>>   {
>>   	int err;
>> @@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
>>   	int err, delay;
>>   	__le16 data;
>>   
>> -	err = st_lsm6dsx_sensor_enable(sensor);
>> -	if (err < 0)
>> -		return err;
>> +	if (!hw->enable_event) {
>> +		err = st_lsm6dsx_sensor_enable(sensor);
>> +		if (err < 0)
>> +			return err;
>>   
>> -	delay = 1000000 / sensor->odr;
>> -	usleep_range(delay, 2 * delay);
>> +		delay = 1000000 / sensor->odr;
>> +		usleep_range(delay, 2 * delay);
>> +	}
>>   
>>   	err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
>>   	if (err < 0)
>>   		return err;
>>   
>> -	st_lsm6dsx_sensor_disable(sensor);
>> +	if (!hw->enable_event)
>> +		st_lsm6dsx_sensor_disable(sensor);
>>   
>>   	*val = (s16)le16_to_cpu(data);
>>   
>> @@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
>>   	return err;
>>   }
>>   
>> +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);
>> +
>> +	*val2 = 0;
>> +	*val = sensor->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;
>> +
>> +	if (!hw->enable_event)
>> +		return -EBUSY;
>> +
>> +	if ((val < 0) || (val > 31))
> 
> unnecessary brackets

Will remove them :-)

> 
>> +		return -EINVAL;
>> +
>> +	if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
>> +		return -EINVAL;
>> +
>> +	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;
>> +
>> +	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;
>> +
>> +	if (state && hw->enable_event)
>> +		return 0;
>> +
>> +	hw->enable_event = state;
>> +	if (state)
>> +		st_lsm6dsx_sensor_enable(sensor);
> 
> please correct me if I am wrong but in this way we break normal operation (e.g.
> if we are reading acc data from the FIFO). You need to check enabled_mask.

Will change to the enabled_mask

> Moreover we should check event type

Do we, we only have one type?

> 
>> +	else
>> +		st_lsm6dsx_sensor_disable(sensor);
>> +
>> +	return 0;
>> +}
>> +
>>   static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
>>   {
>>   	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> @@ -632,6 +738,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,
>>   };
>>   
>> @@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>>   	if (err < 0)
>>   		return err;
>>   
>> +	st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
> 
> we do not need this since default value is already 0

I actually think we do need it, if we come up from a reboot we don't 
know the value stored in the register.

> 
>> +
>>   	return st_lsm6dsx_init_hw_timer(hw);
>>   }
>>   
>> @@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>>   	return iio_dev;
>>   }
>>   
>> +int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
>> +{
>> +	int err;
>> +
>> +	/* Enable inactivity function - low power ACC, GYRO powered-down */
>> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
>> +				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
>> +				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
>> +				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
>> +				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	/* Enable wakeup interrupt */
>> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
>> +				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
>> +				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
>> +
>> +	return err;
>> +}
>> +
>>   static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>>   {
>>   	struct st_lsm6dsx_hw *hw = private;
>> @@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>   		err = st_lsm6dsx_fifo_setup(hw);
>>   		if (err < 0)
>>   			return err;
>> +		err = st_lsm6dsx_event_setup(hw);
>> +		if (err < 0)
>> +			return err;
>>   	}
>>   
>>   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> -- 
>> 2.22.0
>>

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

[Sean Nyekjaer]

On 18/06/2019 17.51, Lorenzo Bianconi wrote:
>> 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>
>> ---
>>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 351c46f01662..59a34894e495 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>   			return err;
>>   	}
>>   
>> +	if (dev->of_node)
>> +		if (of_property_read_bool(dev->of_node, "wakeup-source"))
>> +			device_init_wakeup(dev, true);
>> +
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL(st_lsm6dsx_probe);
>> @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>>   	int i, err = 0;
>>   
>>   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
>> +			/* Enable wake from IRQ */
>> +			enable_irq_wake(hw->irq);
>> +			continue;
> 
> This is breaking buffering mode

Hmm, what is missing? :-)
We need the accelerometer to continue to run in suspend, but skip 
writing to the internal fifo.

> 
>> +		}
>> +
>>   		sensor = iio_priv(hw->iio_devs[i]);
>>   		if (!(hw->enable_mask & BIT(sensor->id)))
>>   			continue;
>> @@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
>>   	int i, err = 0;
>>   
>>   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
>> +			disable_irq_wake(hw->irq);
> 
> same here
> 
>> +			continue;
>> +		}
>> +
>>   		sensor = iio_priv(hw->iio_devs[i]);
>>   		if (!(hw->enable_mask & BIT(sensor->id)))
>>   			continue;
>> -- 
>> 2.22.0
>>

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

[Sean Nyekjaer]

On 18/06/2019 17.55, Lorenzo Bianconi 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 | 30 +++++++++++++++-----
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 59a34894e495..76aec5024d83 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -78,6 +78,12 @@
>>   #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
>>   #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
>>   
>> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR		0x1B
>> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK	BIT(0)
>> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK	BIT(1)
>> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK	BIT(2)
>> +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK	BIT(4)
>> +
>>   #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
>>   #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
>>   #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
>> @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(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 guess this will break shared interrupt, isn't it?

When you write shared interrupt you mean: the drdy-int-pin stuff?
I need to add so we can use all this functionality with the INT2 as well...

> 
>>   }
>>   
>>   static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>>   {
>>   	struct st_lsm6dsx_hw *hw = private;
>> -	int count;
>> +	int count = 0;
>> +	int data, err;
>> +
>> +	if (hw->enable_event) {
>> +		err = regmap_read(hw->regmap,
>> +				  ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);
>> +		if (err < 0)
>> +			goto try_fifo;
> 
> You can simplify this just doing something like:
> 
> 		if (err < 0 && !hw->sip)
> 			return IRQ_NONE;
>
Will apply :-)

Thanks for the review Lorenzo...

-- 
Best regards,
Sean Nyekjær
Embedded Linux Consultant

+45 42427326
sean@geanix.com

Geanix ApS
https://geanix.com
DK39600706

Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events

[Lorenzo Bianconi]

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

On Jun 18, Sean Nyekjaer wrote:
> 
> 
> On 18/06/2019 17.49, Lorenzo Bianconi wrote:
> > > Add event channels that controls the creation of motion events.
> > > 
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > > ---
> > >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |   2 +
> > >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 148 ++++++++++++++++++-
> > >   2 files changed, 144 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > index a5e373680e9c..966cc6e91c7f 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > > @@ -155,6 +155,8 @@ struct st_lsm6dsx_hw {
> > >   	u8 enable_mask;
> > >   	u8 ts_sip;
> > >   	u8 sip;
> > > +	u8 event_threshold;
> > > +	bool enable_event;
> > >   	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 b5d3fa354de7..351c46f01662 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -78,6 +78,17 @@
> > >   #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
> > >   #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
> > > +#define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
> > > +#define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
> > > +#define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
> > > +
> > > +#define ST_LSM6DSX_DEFAULT_WAKE_THRESH		0
> > > +#define ST_LSM6DSX_REG_WAKE_UP_THS_ADDR		0x5B
> > > +#define ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK	GENMASK(5, 0)
> > 
> > Could you please verify this registermap is used for all supported devices?
> > (e.g. LSM6DS3 or LSM6DS3H)
> 
> They have the same registers and the bits meaning the same.
> But I can't test on those devices...

I do think so since BIT(7) of 0x58 TAP_CFG in LSM6DS3/LSM6DS3H is used to
enable hw timestamp...moreover I think there are other differences

> 
> > 
> > > +
> > > +#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
> > > +#define ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK	BIT(5)
> > > +
> > >   #define ST_LSM6DSX_ACC_FS_2G_GAIN		IIO_G_TO_M_S_2(61)
> > >   #define ST_LSM6DSX_ACC_FS_4G_GAIN		IIO_G_TO_M_S_2(122)
> > >   #define ST_LSM6DSX_ACC_FS_8G_GAIN		IIO_G_TO_M_S_2(244)
> > > @@ -303,6 +314,13 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_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)
> > > +};
> > > +
> > >   #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
> > >   {									\
> > >   	.type = chan_type,						\
> > > @@ -319,6 +337,8 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> > >   		.storagebits = 16,					\
> > >   		.endianness = IIO_LE,					\
> > >   	},								\
> > > +	.event_spec = &st_lsm6dsx_event,				\
> > > +	.num_event_specs = 1,						\
> > >   }
> > 
> > ST_LSM6DSX_CHANNEL is used even for sensor-hub so I do not think you can use in
> > this way
> 
> I see, will need to come up with a solution to avoid the event channels on
> the gyro.
> 
> > 
> > >   static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> > > @@ -435,6 +455,20 @@ static int st_lsm6dsx_set_odr(struct st_lsm6dsx_sensor *sensor, u16 odr)
> > >   				  ST_LSM6DSX_SHIFT_VAL(val, reg->mask));
> > >   }
> > > +static int st_lsm6dsx_set_event_threshold(struct st_lsm6dsx_hw *hw, u8 threshold)
> > > +{
> > > +	int err = 0;
> > > +
> > > +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_THS_ADDR,
> > > +				 ST_LSM6DSX_REG_WAKE_UP_THS_THRES_MASK,
> > > +				 threshold);
> > > +
> > > +	if (!err)
> > > +		hw->event_threshold = threshold;
> > > +
> > > +	return err;
> > 
> > I think we can just move this configuration in st_lsm6dsx_write_event()
> 
> We could, should I then call st_lsm6dsx_write_event() from the probe
> function?

nope, just run regmap_update_bits() in st_lsm6dsx_write_event (You do not need
to call st_lsm6dsx_set_event_threshold during the probe since the default value
is already 0)

> 
> > 
> > > +}
> > > +
> > >   int st_lsm6dsx_sensor_enable(struct st_lsm6dsx_sensor *sensor)
> > >   {
> > >   	int err;
> > > @@ -472,18 +506,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
> > >   	int err, delay;
> > >   	__le16 data;
> > > -	err = st_lsm6dsx_sensor_enable(sensor);
> > > -	if (err < 0)
> > > -		return err;
> > > +	if (!hw->enable_event) {
> > > +		err = st_lsm6dsx_sensor_enable(sensor);
> > > +		if (err < 0)
> > > +			return err;
> > > -	delay = 1000000 / sensor->odr;
> > > -	usleep_range(delay, 2 * delay);
> > > +		delay = 1000000 / sensor->odr;
> > > +		usleep_range(delay, 2 * delay);
> > > +	}
> > >   	err = regmap_bulk_read(hw->regmap, addr, &data, sizeof(data));
> > >   	if (err < 0)
> > >   		return err;
> > > -	st_lsm6dsx_sensor_disable(sensor);
> > > +	if (!hw->enable_event)
> > > +		st_lsm6dsx_sensor_disable(sensor);
> > >   	*val = (s16)le16_to_cpu(data);
> > > @@ -556,6 +593,75 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
> > >   	return err;
> > >   }
> > > +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);
> > > +
> > > +	*val2 = 0;
> > > +	*val = sensor->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;
> > > +
> > > +	if (!hw->enable_event)
> > > +		return -EBUSY;
> > > +
> > > +	if ((val < 0) || (val > 31))
> > 
> > unnecessary brackets
> 
> Will remove them :-)
> 
> > 
> > > +		return -EINVAL;
> > > +
> > > +	if (st_lsm6dsx_set_event_threshold(sensor->hw, val))
> > > +		return -EINVAL;
> > > +
> > > +	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;
> > > +
> > > +	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;
> > > +
> > > +	if (state && hw->enable_event)
> > > +		return 0;
> > > +
> > > +	hw->enable_event = state;
> > > +	if (state)
> > > +		st_lsm6dsx_sensor_enable(sensor);
> > 
> > please correct me if I am wrong but in this way we break normal operation (e.g.
> > if we are reading acc data from the FIFO). You need to check enabled_mask.
> 
> Will change to the enabled_mask
> 
> > Moreover we should check event type
> 
> Do we, we only have one type?

for the moment...

> 
> > 
> > > +	else
> > > +		st_lsm6dsx_sensor_disable(sensor);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   static int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
> > >   {
> > >   	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > > @@ -632,6 +738,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,
> > >   };
> > > @@ -761,6 +871,8 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > >   	if (err < 0)
> > >   		return err;
> > > +	st_lsm6dsx_set_event_threshold(hw, ST_LSM6DSX_DEFAULT_WAKE_THRESH);
> > 
> > we do not need this since default value is already 0
> 
> I actually think we do need it, if we come up from a reboot we don't know
> the value stored in the register.

Nope, we perform a sw reset during probe since the default value are loaded

> 
> > 
> > > +
> > >   	return st_lsm6dsx_init_hw_timer(hw);
> > >   }
> > > @@ -811,6 +923,27 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> > >   	return iio_dev;
> > >   }
> > > +int st_lsm6dsx_event_setup(struct st_lsm6dsx_hw *hw)
> > > +{
> > > +	int err;
> > > +
> > > +	/* Enable inactivity function - low power ACC, GYRO powered-down */
> > > +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_TAP_CFG_ADDR,
> > > +				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> > > +				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK,
> > > +				 ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK |
> > > +				 ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	/* Enable wakeup interrupt */
> > > +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_MD1_CFG_ADDR,
> > > +				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK,
> > > +				 ST_LSM6DSX_REG_MD1_CFG_INT1_WU_MASK);
> > > +
> > > +	return err;
> > > +}
> > > +
> > >   static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
> > >   {
> > >   	struct st_lsm6dsx_hw *hw = private;
> > > @@ -932,6 +1065,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > >   		err = st_lsm6dsx_fifo_setup(hw);
> > >   		if (err < 0)
> > >   			return err;
> > > +		err = st_lsm6dsx_event_setup(hw);
> > > +		if (err < 0)
> > > +			return err;
> > >   	}
> > >   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > -- 
> > > 2.22.0
> > > 

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

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

[Lorenzo Bianconi]

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

On Jun 18, Sean Nyekjaer wrote:
> 
> 
> On 18/06/2019 17.55, Lorenzo Bianconi 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 | 30 +++++++++++++++-----
> > >   1 file changed, 23 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > index 59a34894e495..76aec5024d83 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -78,6 +78,12 @@
> > >   #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
> > >   #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
> > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR		0x1B
> > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK	BIT(0)
> > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK	BIT(1)
> > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK	BIT(2)
> > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK	BIT(4)
> > > +
> > >   #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
> > >   #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
> > >   #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
> > > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(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 guess this will break shared interrupt, isn't it?
> 
> When you write shared interrupt you mean: the drdy-int-pin stuff?
> I need to add so we can use all this functionality with the INT2 as well...

nope, shared IRQ line with other devices (when you set drive-open-drain dts
property)

> 
> > 
> > >   }
> > >   static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > >   {
> > >   	struct st_lsm6dsx_hw *hw = private;
> > > -	int count;
> > > +	int count = 0;
> > > +	int data, err;
> > > +
> > > +	if (hw->enable_event) {
> > > +		err = regmap_read(hw->regmap,
> > > +				  ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);
> > > +		if (err < 0)
> > > +			goto try_fifo;
> > 
> > You can simplify this just doing something like:
> > 
> > 		if (err < 0 && !hw->sip)
> > 			return IRQ_NONE;
> > 
> Will apply :-)
> 
> Thanks for the review Lorenzo...

Thanks for working on this :)

Regards,
Lorenzo

> 
> -- 
> Best regards,
> Sean Nyekjær
> Embedded Linux Consultant
> 
> +45 42427326
> sean@geanix.com
> 
> Geanix ApS
> https://geanix.com
> DK39600706

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

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

[Lorenzo Bianconi]

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

On Jun 18, Sean Nyekjaer wrote:
> 
> 
> On 18/06/2019 17.51, Lorenzo Bianconi wrote:
> > > 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>
> > > ---
> > >   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
> > >   1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > index 351c46f01662..59a34894e495 100644
> > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> > >   			return err;
> > >   	}
> > > +	if (dev->of_node)
> > > +		if (of_property_read_bool(dev->of_node, "wakeup-source"))
> > > +			device_init_wakeup(dev, true);
> > > +
> > >   	return 0;
> > >   }
> > >   EXPORT_SYMBOL(st_lsm6dsx_probe);
> > > @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
> > >   	int i, err = 0;
> > >   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> > > +			/* Enable wake from IRQ */
> > > +			enable_irq_wake(hw->irq);
> > > +			continue;
> > 
> > This is breaking buffering mode
> 
> Hmm, what is missing? :-)
> We need the accelerometer to continue to run in suspend, but skip writing to
> the internal fifo.

I think we should keep the accel enabled, but we need to put the FIFO in bypas
mode

> 
> > 
> > > +		}
> > > +
> > >   		sensor = iio_priv(hw->iio_devs[i]);
> > >   		if (!(hw->enable_mask & BIT(sensor->id)))
> > >   			continue;
> > > @@ -1112,6 +1122,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
> > >   	int i, err = 0;
> > >   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > > +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
> > > +			disable_irq_wake(hw->irq);
> > 
> > same here
> > 
> > > +			continue;
> > > +		}
> > > +
> > >   		sensor = iio_priv(hw->iio_devs[i]);
> > >   		if (!(hw->enable_mask & BIT(sensor->id)))
> > >   			continue;
> > > -- 
> > > 2.22.0
> > > 

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

Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core

[Lorenzo Bianconi]

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

> 
> On 18 Jun 2019, at 17.57, Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> 
> >> This prepares the interrupt to be used for other stuff than
> >> fifo reading -> event readings.
> >> 
> >> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >> ---
> >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h       |  1 +
> >> .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 80 +----------------
> >> drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 87 +++++++++++++++++++
> >> 3 files changed, 90 insertions(+), 78 deletions(-)
> >> 
> > 
> > I can't see why we need this patch
> > 
> > Regards,
> > Lorenzo
> 
> The interrupt handling isn’t only for fifo reading, so I think it’s correct to move it out of the buffer handling file.

Uhm, re-thinking about it I agree, it will be useful even for upcoming I3C
support. Btw I think you are using a prehistoric version of the driver ;)

Regards,
Lorenzo

> 
> /Sean
> > 
> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> >> index edcd838037cd..a5e373680e9c 100644
> >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> >> @@ -175,5 +175,6 @@ int st_lsm6dsx_update_watermark(struct st_lsm6dsx_sensor *sensor,
> >> int st_lsm6dsx_flush_fifo(struct st_lsm6dsx_hw *hw);
> >> int st_lsm6dsx_set_fifo_mode(struct st_lsm6dsx_hw *hw,
> >>                 enum st_lsm6dsx_fifo_mode fifo_mode);
> >> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw);
> >> 
> >> #endif /* ST_LSM6DSX_H */
> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> >> index 631360b14ca7..a1ed61a64a64 100644
> >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> >> @@ -25,8 +25,6 @@
> >>  * Licensed under the GPL-2.
> >>  */
> >> #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>
> >> @@ -37,10 +35,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)
> >> @@ -282,7 +276,7 @@ static inline int st_lsm6dsx_read_block(struct st_lsm6dsx_hw *hw, u8 *data,
> >>  *
> >>  * Return: Number of bytes read from the FIFO
> >>  */
> >> -static int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >> +int st_lsm6dsx_read_fifo(struct st_lsm6dsx_hw *hw)
> >> {
> >>    u16 fifo_len, pattern_len = hw->sip * ST_LSM6DSX_SAMPLE_SIZE;
> >>    u16 fifo_diff_mask = hw->settings->fifo_ops.fifo_diff.mask;
> >> @@ -465,25 +459,6 @@ static int st_lsm6dsx_update_fifo(struct iio_dev *iio_dev, 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 = st_lsm6dsx_read_fifo(hw);
> >> -    mutex_unlock(&hw->fifo_lock);
> >> -
> >> -    return !count ? IRQ_NONE : IRQ_HANDLED;
> >> -}
> >> -
> >> static int st_lsm6dsx_buffer_preenable(struct iio_dev *iio_dev)
> >> {
> >>    return st_lsm6dsx_update_fifo(iio_dev, true);
> >> @@ -501,59 +476,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++) {
> >>        buffer = devm_iio_kfifo_allocate(hw->dev);
> >> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> >> index aebbe0ddd8d8..b5d3fa354de7 100644
> >> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> >> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> >> @@ -36,6 +36,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>
> >> @@ -55,6 +57,11 @@
> >> #define ST_LSM6DSX_REG_INT2_ON_INT1_ADDR    0x13
> >> #define ST_LSM6DSX_REG_INT2_ON_INT1_MASK    BIT(5)
> >> 
> >> +#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_ACC_ODR_ADDR        0x10
> >> #define ST_LSM6DSX_REG_ACC_ODR_MASK        GENMASK(7, 4)
> >> #define ST_LSM6DSX_REG_ACC_FS_ADDR        0x10
> >> @@ -804,6 +811,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 = st_lsm6dsx_read_fifo(hw);
> >> +    mutex_unlock(&hw->fifo_lock);
> >> +
> >> +    return !count ? IRQ_NONE : IRQ_HANDLED;
> >> +}
> >> +
> >> +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 err;
> >> +}
> >> +
> >> int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> >>             struct regmap *regmap)
> >> {
> >> @@ -842,6 +926,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
> >>        return err;
> >> 
> >>    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.22.0
> >> 

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

Re: [PATCH 1/5] iio: imu: st_lsm6dsx: move interrupt thread to core

[Sean Nyekjaer]

On 19/06/2019 07.58, Lorenzo Bianconi wrote:
>>
>>> I can't see why we need this patch
>>>
>>> Regards,
>>> Lorenzo
>>
>> The interrupt handling isn’t only for fifo reading, so I think it’s correct to move it out of the buffer handling file.
> 
> Uhm, re-thinking about it I agree, it will be useful even for upcoming I3C
> support. Btw I think you are using a prehistoric version of the driver ;)
> 
> Regards,
> Lorenzo
> 

Yes I'm on 4.19... will rebase to iio "testing" for V2

/Sean

Re: [PATCH 2/5] iio: imu: st_lsm6dsx: add motion events

[Sean Nyekjaer]

On 18/06/2019 22.21, Lorenzo Bianconi wrote:

>>>
>>> Could you please verify this registermap is used for all supported devices?
>>> (e.g. LSM6DS3 or LSM6DS3H)
>>
>> They have the same registers and the bits meaning the same.
>> But I can't test on those devices...
> 
> I do think so since BIT(7) of 0x58 TAP_CFG in LSM6DS3/LSM6DS3H is used to
> enable hw timestamp...moreover I think there are other differences
> 

The LSM6DS3/LSM6DS3H have the inactivity function enable in
WAKE_UP_THS 0x5B, BIT(6), ISM330 have that in TAP_CFG 0x58.
The rest of the WAKE_UP_THS have the same register layout

WAKE_UP_SRC 0x1B have the same register layout

MD1_CFG 0x5E have the same register layout

/Sean

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

[Jonathan Cameron]

On Tue, 18 Jun 2019 22:24:14 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> On Jun 18, Sean Nyekjaer wrote:
> > 
> > 
> > On 18/06/2019 17.55, Lorenzo Bianconi 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 | 30 +++++++++++++++-----
> > > >   1 file changed, 23 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > index 59a34894e495..76aec5024d83 100644
> > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > @@ -78,6 +78,12 @@
> > > >   #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR	0x24
> > > >   #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR	0x26
> > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR		0x1B
> > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK	BIT(0)
> > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK	BIT(1)
> > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK	BIT(2)
> > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK	BIT(4)
> > > > +
> > > >   #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
> > > >   #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
> > > >   #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK	GENMASK(6, 5)
> > > > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(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 guess this will break shared interrupt, isn't it?  
> > 
> > When you write shared interrupt you mean: the drdy-int-pin stuff?
> > I need to add so we can use all this functionality with the INT2 as well...  
> 
> nope, shared IRQ line with other devices (when you set drive-open-drain dts
> property)

It's been a while since I looked at this, but...

It shouldn't be broken.  When using shared interrupts, all interrupt handlers
tend to get run, whether or not a given one return IRQ_NONE.

Nothing stops multiple devices setting their interrupt lines at the same
time.

See __handle_irq_event_percpu in kernel/irq/handle.c which is probably
the right code. No return values are taken notice of until all registered
handlers have run.

Jonathan

> 
> >   
> > >   
> > > >   }
> > > >   static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > >   {
> > > >   	struct st_lsm6dsx_hw *hw = private;
> > > > -	int count;
> > > > +	int count = 0;
> > > > +	int data, err;
> > > > +
> > > > +	if (hw->enable_event) {
> > > > +		err = regmap_read(hw->regmap,
> > > > +				  ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);
> > > > +		if (err < 0)
> > > > +			goto try_fifo;  
> > > 
> > > You can simplify this just doing something like:
> > > 
> > > 		if (err < 0 && !hw->sip)
> > > 			return IRQ_NONE;
> > >   
> > Will apply :-)
> > 
> > Thanks for the review Lorenzo...  
> 
> Thanks for working on this :)
> 
> Regards,
> Lorenzo
> 
> > 
> > -- 
> > Best regards,
> > Sean Nyekjær
> > Embedded Linux Consultant
> > 
> > +45 42427326
> > sean@geanix.com
> > 
> > Geanix ApS
> > https://geanix.com
> > DK39600706  

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

[Lorenzo Bianconi]

>
> On Tue, 18 Jun 2019 22:24:14 +0200
> Lorenzo Bianconi <lorenzo@kernel.org> wrote:
>
> > On Jun 18, Sean Nyekjaer wrote:
> > >
> > >
> > > On 18/06/2019 17.55, Lorenzo Bianconi 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 | 30 +++++++++++++++-----
> > > > >   1 file changed, 23 insertions(+), 7 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > index 59a34894e495..76aec5024d83 100644
> > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > @@ -78,6 +78,12 @@
> > > > >   #define ST_LSM6DSX_REG_GYRO_OUT_Y_L_ADDR      0x24
> > > > >   #define ST_LSM6DSX_REG_GYRO_OUT_Z_L_ADDR      0x26
> > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR                0x1B
> > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Z_WU_MASK   BIT(0)
> > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_Y_WU_MASK   BIT(1)
> > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_X_WU_MASK   BIT(2)
> > > > > +#define ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK     BIT(4)
> > > > > +
> > > > >   #define ST_LSM6DSX_REG_TAP_CFG_ADDR           0x58
> > > > >   #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK    BIT(7)
> > > > >   #define ST_LSM6DSX_REG_TAP_CFG_INACT_EN_MASK  GENMASK(6, 5)
> > > > > @@ -946,19 +952,29 @@ int st_lsm6dsx_event_setup(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 guess this will break shared interrupt, isn't it?
> > >
> > > When you write shared interrupt you mean: the drdy-int-pin stuff?
> > > I need to add so we can use all this functionality with the INT2 as well...
> >
> > nope, shared IRQ line with other devices (when you set drive-open-drain dts
> > property)
>
> It's been a while since I looked at this, but...
>
> It shouldn't be broken.  When using shared interrupts, all interrupt handlers
> tend to get run, whether or not a given one return IRQ_NONE.
>
> Nothing stops multiple devices setting their interrupt lines at the same
> time.
>
> See __handle_irq_event_percpu in kernel/irq/handle.c which is probably
> the right code. No return values are taken notice of until all registered
> handlers have run.

Ops, right. I do not know why I was thinking returning IRQ_NONE stops
the iteration over action list.
Thanks for pointing it out :)

Regards,
Lorenzo

>
> Jonathan
>
> >
> > >
> > > >
> > > > >   }
> > > > >   static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > >   {
> > > > >         struct st_lsm6dsx_hw *hw = private;
> > > > > -       int count;
> > > > > +       int count = 0;
> > > > > +       int data, err;
> > > > > +
> > > > > +       if (hw->enable_event) {
> > > > > +               err = regmap_read(hw->regmap,
> > > > > +                                 ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);
> > > > > +               if (err < 0)
> > > > > +                       goto try_fifo;
> > > >
> > > > You can simplify this just doing something like:
> > > >
> > > >           if (err < 0 && !hw->sip)
> > > >                   return IRQ_NONE;
> > > >
> > > Will apply :-)
> > >
> > > Thanks for the review Lorenzo...
> >
> > Thanks for working on this :)
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > --
> > > Best regards,
> > > Sean Nyekjær
> > > Embedded Linux Consultant
> > >
> > > +45 42427326
> > > sean@geanix.com
> > >
> > > Geanix ApS
> > > https://geanix.com
> > > DK39600706
>


-- 
UNIX is Sexy: who | grep -i blonde | talk; cd ~; wine; talk; touch;
unzip; touch; strip; gasp; finger; gasp; mount; fsck; more; yes; gasp;
umount; make clean; sleep

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

[Sean Nyekjaer]

On 18/06/2019 22.27, Lorenzo Bianconi wrote:
> On Jun 18, Sean Nyekjaer wrote:
>>
>>
>> On 18/06/2019 17.51, Lorenzo Bianconi wrote:
>>>> 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>
>>>> ---
>>>>    drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 15 +++++++++++++++
>>>>    1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> index 351c46f01662..59a34894e495 100644
>>>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>>>> @@ -1076,6 +1076,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id, const char *name,
>>>>    			return err;
>>>>    	}
>>>> +	if (dev->of_node)
>>>> +		if (of_property_read_bool(dev->of_node, "wakeup-source"))
>>>> +			device_init_wakeup(dev, true);
>>>> +
>>>>    	return 0;
>>>>    }
>>>>    EXPORT_SYMBOL(st_lsm6dsx_probe);
>>>> @@ -1088,6 +1092,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>>>>    	int i, err = 0;
>>>>    	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>>>> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
>>>> +			/* Enable wake from IRQ */
>>>> +			enable_irq_wake(hw->irq);
>>>> +			continue;
>>>
>>> This is breaking buffering mode
>>
>> Hmm, what is missing? :-)
>> We need the accelerometer to continue to run in suspend, but skip writing to
>> the internal fifo.
> 
> I think we should keep the accel enabled, but we need to put the FIFO in bypas
> mode
> 

I think it's exactly whats happening here, we keep the accel enabled and 
set the FIFO in bypass mode after.
st_lsm6dsx_flush_fifo (which sets the bypass mode) is called after the 
for loop.

/Sean