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

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

[Sean Nyekjaer]

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

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---

Changes since v1:
 * none

 .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 78 +----------------
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 87 +++++++++++++++++++
 2 files changed, 88 insertions(+), 77 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
index 38194f4d2b7e..2b938d87ae34 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
@@ -29,8 +29,6 @@
  * Denis Ciocca <denis.ciocca@st.com>
  */
 #include <linux/module.h>
-#include <linux/interrupt.h>
-#include <linux/irq.h>
 #include <linux/iio/kfifo_buf.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/buffer.h>
@@ -41,10 +39,6 @@
 
 #include "st_lsm6dsx.h"
 
-#define ST_LSM6DSX_REG_HLACTIVE_ADDR		0x12
-#define ST_LSM6DSX_REG_HLACTIVE_MASK		BIT(5)
-#define ST_LSM6DSX_REG_PP_OD_ADDR		0x12
-#define ST_LSM6DSX_REG_PP_OD_MASK		BIT(4)
 #define ST_LSM6DSX_REG_FIFO_MODE_ADDR		0x0a
 #define ST_LSM6DSX_FIFO_MODE_MASK		GENMASK(2, 0)
 #define ST_LSM6DSX_FIFO_ODR_MASK		GENMASK(6, 3)
@@ -654,25 +648,6 @@ 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 = hw->settings->fifo_ops.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);
@@ -690,59 +665,8 @@ static const struct iio_buffer_setup_ops st_lsm6dsx_buffer_ops = {
 
 int st_lsm6dsx_fifo_setup(struct st_lsm6dsx_hw *hw)
 {
-	struct device_node *np = hw->dev->of_node;
-	struct st_sensors_platform_data *pdata;
 	struct iio_buffer *buffer;
-	unsigned long irq_type;
-	bool irq_active_low;
-	int i, err;
-
-	irq_type = irqd_get_trigger_type(irq_get_irq_data(hw->irq));
-
-	switch (irq_type) {
-	case IRQF_TRIGGER_HIGH:
-	case IRQF_TRIGGER_RISING:
-		irq_active_low = false;
-		break;
-	case IRQF_TRIGGER_LOW:
-	case IRQF_TRIGGER_FALLING:
-		irq_active_low = true;
-		break;
-	default:
-		dev_info(hw->dev, "mode %lx unsupported\n", irq_type);
-		return -EINVAL;
-	}
-
-	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_HLACTIVE_ADDR,
-				 ST_LSM6DSX_REG_HLACTIVE_MASK,
-				 FIELD_PREP(ST_LSM6DSX_REG_HLACTIVE_MASK,
-					    irq_active_low));
-	if (err < 0)
-		return err;
-
-	pdata = (struct st_sensors_platform_data *)hw->dev->platform_data;
-	if ((np && of_property_read_bool(np, "drive-open-drain")) ||
-	    (pdata && pdata->open_drain)) {
-		err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_PP_OD_ADDR,
-					 ST_LSM6DSX_REG_PP_OD_MASK,
-					 FIELD_PREP(ST_LSM6DSX_REG_PP_OD_MASK,
-						    1));
-		if (err < 0)
-			return err;
-
-		irq_type |= IRQF_SHARED;
-	}
-
-	err = devm_request_threaded_irq(hw->dev, hw->irq,
-					st_lsm6dsx_handler_irq,
-					st_lsm6dsx_handler_thread,
-					irq_type | IRQF_ONESHOT,
-					"lsm6dsx", hw);
-	if (err) {
-		dev_err(hw->dev, "failed to request trigger irq %d\n",
-			hw->irq);
-		return err;
-	}
+	int i;
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
 		if (!hw->iio_devs[i])
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index d8c4417cf4eb..e67341802fc6 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -41,6 +41,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>
@@ -61,6 +63,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_OUT_X_L_ADDR		0x28
 #define ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR		0x2a
 #define ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR		0x2c
@@ -1069,6 +1076,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,
 		     struct regmap *regmap)
 {
@@ -1117,6 +1201,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 	}
 
 	if (hw->irq > 0) {
+		err = st_lsm6dsx_irq_setup(hw);
+		if (err < 0)
+			return err;
 		err = st_lsm6dsx_fifo_setup(hw);
 		if (err < 0)
 			return err;
-- 
2.22.0

[PATCH v2 2/6] iio: imu: st_lsm6dsx: save drdy_pin in device struct

[Sean Nyekjaer]

This prepares the use of the drdy_pin for selecting the correct
event interrupt register.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---

Changes since v1:
 * new commit

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index af379a5429ed..738bed4a9752 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -297,6 +297,7 @@ struct st_lsm6dsx_hw {
 	u8 enable_mask;
 	u8 ts_sip;
 	u8 sip;
+	int drdy_pin;
 
 	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 e67341802fc6..2c11addf568b 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -871,17 +871,17 @@ static int st_lsm6dsx_of_get_drdy_pin(struct st_lsm6dsx_hw *hw, int *drdy_pin)
 
 static int st_lsm6dsx_get_drdy_reg(struct st_lsm6dsx_hw *hw, u8 *drdy_reg)
 {
-	int err = 0, drdy_pin;
+	int err = 0;
 
-	if (st_lsm6dsx_of_get_drdy_pin(hw, &drdy_pin) < 0) {
+	if (st_lsm6dsx_of_get_drdy_pin(hw, &hw->drdy_pin) < 0) {
 		struct st_sensors_platform_data *pdata;
 		struct device *dev = hw->dev;
 
 		pdata = (struct st_sensors_platform_data *)dev->platform_data;
-		drdy_pin = pdata ? pdata->drdy_int_pin : 1;
+		hw->drdy_pin = pdata ? pdata->drdy_int_pin : 1;
 	}
 
-	switch (drdy_pin) {
+	switch (hw->drdy_pin) {
 	case 1:
 		*drdy_reg = ST_LSM6DSX_REG_INT1_ADDR;
 		break;
-- 
2.22.0

[PATCH v2 3/6] 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>
---

Changes since v1:
 * added handling of LSM6
 * added CHANNEL info with events for ACC
 * removed st_lsm6dsx_set_event_threshold function
 * added check of event type to event channels

Issues:
 * This currently breaks buffered reads, as the interrupt stays high.
   This happens when MD1_CFG INT1_WU (wakeup event routes to INT1) is
   enabled.
   The datasheet doesn't seem to decribe whats happening and I can't
   find a status register to read somehing useful.
   Maybe it's impossible to share the buffered reads interrupt with
   the wakeup interrupt?

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  30 ++++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 +++++++++++++++++--
 2 files changed, 182 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index 738bed4a9752..fef08b7cf2a0 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -12,6 +12,7 @@
 #define ST_LSM6DSX_H
 
 #include <linux/device.h>
+#include <linux/iio/iio.h>
 
 #define ST_LSM6DS3_DEV_NAME	"lsm6ds3"
 #define ST_LSM6DS3H_DEV_NAME	"lsm6ds3h"
@@ -50,6 +51,26 @@ enum st_lsm6dsx_hw_id {
 					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
 #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
 
+#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
+{									\
+	.type = chan_type,						\
+	.address = addr,						\
+	.modified = 1,							\
+	.channel2 = mod,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
+			      BIT(IIO_CHAN_INFO_SCALE),			\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.scan_index = scan_idx,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+	.event_spec = &st_lsm6dsx_event,				\
+	.num_event_specs = 1,						\
+}
+
 #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
 {									\
 	.type = chan_type,						\
@@ -297,6 +318,8 @@ struct st_lsm6dsx_hw {
 	u8 enable_mask;
 	u8 ts_sip;
 	u8 sip;
+	u8 event_threshold;
+	bool enable_event;
 	int drdy_pin;
 
 	u8 *buff;
@@ -306,6 +329,13 @@ struct st_lsm6dsx_hw {
 	const struct st_lsm6dsx_settings *settings;
 };
 
+static const struct iio_event_spec st_lsm6dsx_event = {
+	.type = IIO_EV_TYPE_THRESH,
+	.dir = IIO_EV_DIR_EITHER,
+	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			 BIT(IIO_EV_INFO_ENABLE)
+};
+
 static const unsigned long st_lsm6dsx_available_scan_masks[] = {0x7, 0x0};
 extern const struct dev_pm_ops st_lsm6dsx_pm_ops;
 
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 2c11addf568b..6decb0846f1a 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -76,6 +76,16 @@
 #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_WAKE_UP_ADDR		0x5B
+#define ST_LSM6DSX_REG_WAKE_UP_THRES_MASK	GENMASK(5, 0)
+
+#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
+#define ST_LSM6DSX_REG_MD2_CFG_ADDR		0x5F
+#define ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK	BIT(5)
+
 static const struct st_lsm6dsx_odr_table_entry st_lsm6dsx_odr_table[] = {
 	[ST_LSM6DSX_ID_ACC] = {
 		.reg = {
@@ -470,12 +480,12 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 };
 
 static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
-	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
-			   IIO_MOD_X, 0),
-	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
-			   IIO_MOD_Y, 1),
-	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
-			   IIO_MOD_Z, 2),
+	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
+			       IIO_MOD_X, 0),
+	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
+			       IIO_MOD_Y, 1),
+	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
+			       IIO_MOD_Z, 2),
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
@@ -679,18 +689,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
 	int err, delay;
 	__le16 data;
 
-	err = st_lsm6dsx_sensor_set_enable(sensor, true);
-	if (err < 0)
-		return err;
+	if (!hw->enable_event) {
+		err = st_lsm6dsx_sensor_set_enable(sensor, true);
+		if (err < 0)
+			return err;
 
-	delay = 1000000 / sensor->odr;
-	usleep_range(delay, 2 * delay);
+		delay = 1000000 / sensor->odr;
+		usleep_range(delay, 2 * delay);
+	}
 
 	err = st_lsm6dsx_read_locked(hw, addr, &data, sizeof(data));
 	if (err < 0)
 		return err;
 
-	st_lsm6dsx_sensor_set_enable(sensor, false);
+	if (!hw->enable_event)
+		st_lsm6dsx_sensor_set_enable(sensor, false);
 
 	*val = (s16)le16_to_cpu(data);
 
@@ -763,6 +776,94 @@ 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);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	*val2 = 0;
+	*val = hw->event_threshold;
+
+	return IIO_VAL_INT;
+}
+
+static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int val, int val2)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+	int err;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	if (!hw->enable_event)
+		return -EBUSY;
+
+	if (val < 0 || val > 31)
+		return -EINVAL;
+
+	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_ADDR,
+				 ST_LSM6DSX_REG_WAKE_UP_THRES_MASK,
+				 val);
+	if (err)
+		return -EINVAL;
+
+	hw->event_threshold = val;
+
+	return 0;
+}
+
+static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
+					  const struct iio_chan_spec *chan,
+					  enum iio_event_type type,
+					  enum iio_event_direction dir)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	return hw->enable_event;
+}
+
+static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
+					   const struct iio_chan_spec *chan,
+					   enum iio_event_type type,
+					   enum iio_event_direction dir,
+					   int state)
+{
+	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
+	struct st_lsm6dsx_hw *hw = sensor->hw;
+
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	if (state && hw->enable_event)
+		return 0;
+
+	hw->enable_event = state;
+	if (state)
+		st_lsm6dsx_sensor_set_enable(sensor, true);
+	else
+		st_lsm6dsx_sensor_set_enable(sensor, false);
+
+	return 0;
+}
+
 int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
 {
 	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
@@ -839,6 +940,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,
 };
 
@@ -1076,6 +1181,38 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
 	return iio_dev;
 }
 
+int st_lsm6dsx_event_setup(int id, struct st_lsm6dsx_hw *hw)
+{
+	int err;
+	unsigned int md_reg;
+
+	if (id == ST_ISM330DLC_ID) {
+		/* Enable basic interrupts for ISM330 */
+		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_INT_EN_MASK);
+		if (err < 0)
+			return err;
+	}
+
+	switch (hw->drdy_pin) {
+	case 1:
+		md_reg = ST_LSM6DSX_REG_MD1_CFG_ADDR;
+		break;
+	case 2:
+		md_reg = ST_LSM6DSX_REG_MD2_CFG_ADDR;
+		break;
+	default:
+		return -EINVAL;
+	}
+	/* Enable wakeup interrupt */
+	err = regmap_update_bits(hw->regmap, md_reg,
+				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK,
+				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK);
+
+	return err;
+}
+
 static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
 {
 	struct st_lsm6dsx_hw *hw = private;
@@ -1207,6 +1344,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 		err = st_lsm6dsx_fifo_setup(hw);
 		if (err < 0)
 			return err;
+		err = st_lsm6dsx_event_setup(hw_id, hw);
+		if (err < 0)
+			return err;
 	}
 
 	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
-- 
2.22.0

Re: [PATCH v2 3/6] iio: imu: st_lsm6dsx: add motion events

[Lorenzo Bianconi]

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

> Add event channels that controls the creation of motion events.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> 
> Changes since v1:
>  * added handling of LSM6
>  * added CHANNEL info with events for ACC
>  * removed st_lsm6dsx_set_event_threshold function
>  * added check of event type to event channels
> 
> Issues:
>  * This currently breaks buffered reads, as the interrupt stays high.
>    This happens when MD1_CFG INT1_WU (wakeup event routes to INT1) is
>    enabled.
>    The datasheet doesn't seem to decribe whats happening and I can't
>    find a status register to read somehing useful.
>    Maybe it's impossible to share the buffered reads interrupt with
>    the wakeup interrupt?

Could you explain this issue a bit more? adding st folks...

> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  30 ++++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 +++++++++++++++++--
>  2 files changed, 182 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index 738bed4a9752..fef08b7cf2a0 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -12,6 +12,7 @@
>  #define ST_LSM6DSX_H
>  
>  #include <linux/device.h>
> +#include <linux/iio/iio.h>
>  
>  #define ST_LSM6DS3_DEV_NAME	"lsm6ds3"
>  #define ST_LSM6DS3H_DEV_NAME	"lsm6ds3h"
> @@ -50,6 +51,26 @@ enum st_lsm6dsx_hw_id {
>  					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
>  #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
>  
> +#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
> +{									\
> +	.type = chan_type,						\
> +	.address = addr,						\
> +	.modified = 1,							\
> +	.channel2 = mod,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE),			\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +	.scan_index = scan_idx,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = 16,						\
> +		.storagebits = 16,					\
> +		.endianness = IIO_LE,					\
> +	},								\
> +	.event_spec = &st_lsm6dsx_event,				\
> +	.num_event_specs = 1,						\
> +}

I would prefer to extend existing macros

> +
>  #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
>  {									\
>  	.type = chan_type,						\
> @@ -297,6 +318,8 @@ struct st_lsm6dsx_hw {
>  	u8 enable_mask;
>  	u8 ts_sip;
>  	u8 sip;
> +	u8 event_threshold;
> +	bool enable_event;
>  	int drdy_pin;
>  
>  	u8 *buff;
> @@ -306,6 +329,13 @@ struct st_lsm6dsx_hw {
>  	const struct st_lsm6dsx_settings *settings;
>  };
>  
> +static const struct iio_event_spec st_lsm6dsx_event = {
> +	.type = IIO_EV_TYPE_THRESH,
> +	.dir = IIO_EV_DIR_EITHER,
> +	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +			 BIT(IIO_EV_INFO_ENABLE)
> +};
> +
>  static const unsigned long st_lsm6dsx_available_scan_masks[] = {0x7, 0x0};
>  extern const struct dev_pm_ops st_lsm6dsx_pm_ops;
>  
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 2c11addf568b..6decb0846f1a 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -76,6 +76,16 @@
>  #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)

I am pretty sure this is not true at least for lsm6ds3/lsm6ds3h

> +
> +#define ST_LSM6DSX_REG_WAKE_UP_ADDR		0x5B
> +#define ST_LSM6DSX_REG_WAKE_UP_THRES_MASK	GENMASK(5, 0)
> +
> +#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
> +#define ST_LSM6DSX_REG_MD2_CFG_ADDR		0x5F
> +#define ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK	BIT(5)
> +
>  static const struct st_lsm6dsx_odr_table_entry st_lsm6dsx_odr_table[] = {
>  	[ST_LSM6DSX_ID_ACC] = {
>  		.reg = {
> @@ -470,12 +480,12 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  };
>  
>  static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
> -			   IIO_MOD_X, 0),
> -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
> -			   IIO_MOD_Y, 1),
> -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
> -			   IIO_MOD_Z, 2),
> +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
> +			       IIO_MOD_X, 0),
> +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
> +			       IIO_MOD_Y, 1),
> +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
> +			       IIO_MOD_Z, 2),
>  	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
> @@ -679,18 +689,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
>  	int err, delay;
>  	__le16 data;
>  
> -	err = st_lsm6dsx_sensor_set_enable(sensor, true);
> -	if (err < 0)
> -		return err;
> +	if (!hw->enable_event) {
> +		err = st_lsm6dsx_sensor_set_enable(sensor, true);
> +		if (err < 0)
> +			return err;
>  
> -	delay = 1000000 / sensor->odr;
> -	usleep_range(delay, 2 * delay);
> +		delay = 1000000 / sensor->odr;
> +		usleep_range(delay, 2 * delay);
> +	}
>  
>  	err = st_lsm6dsx_read_locked(hw, addr, &data, sizeof(data));
>  	if (err < 0)
>  		return err;
>  
> -	st_lsm6dsx_sensor_set_enable(sensor, false);
> +	if (!hw->enable_event)
> +		st_lsm6dsx_sensor_set_enable(sensor, false);
>  
>  	*val = (s16)le16_to_cpu(data);
>  
> @@ -763,6 +776,94 @@ 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);
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	*val2 = 0;
> +	*val = hw->event_threshold;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
> +				    const struct iio_chan_spec *chan,
> +				    enum iio_event_type type,
> +				    enum iio_event_direction dir,
> +				    enum iio_event_info info,
> +				    int val, int val2)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +	int err;
> +
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	if (!hw->enable_event)
> +		return -EBUSY;

I guess it is ok to configure the threshold first, no?

> +
> +	if (val < 0 || val > 31)
> +		return -EINVAL;
> +
> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_ADDR,
> +				 ST_LSM6DSX_REG_WAKE_UP_THRES_MASK,
> +				 val);
> +	if (err)
> +		return -EINVAL;
> +
> +	hw->event_threshold = val;
> +
> +	return 0;
> +}
> +
> +static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	return hw->enable_event;
> +}
> +
> +static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
> +					   const struct iio_chan_spec *chan,
> +					   enum iio_event_type type,
> +					   enum iio_event_direction dir,
> +					   int state)
> +{
> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> +	struct st_lsm6dsx_hw *hw = sensor->hw;
> +
> +	if (type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	if (state && hw->enable_event)
> +		return 0;
> +
> +	hw->enable_event = state;
> +	if (state)
> +		st_lsm6dsx_sensor_set_enable(sensor, true);
> +	else
> +		st_lsm6dsx_sensor_set_enable(sensor, false);

st_lsm6dsx_sensor_set_enable can fails. Why not do

	err = st_lsm6dsx_sensor_set_enable(sensor, state);
	if (err < 0)
		return err;

	hw->enable_event = state;;
	return 0;

> +
> +	return 0;
> +}
> +
>  int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
>  {
>  	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> @@ -839,6 +940,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,
>  };
>  
> @@ -1076,6 +1181,38 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>  	return iio_dev;
>  }
>  
> +int st_lsm6dsx_event_setup(int id, struct st_lsm6dsx_hw *hw)
> +{
> +	int err;
> +	unsigned int md_reg;
> +
> +	if (id == ST_ISM330DLC_ID) {
> +		/* Enable basic interrupts for ISM330 */
> +		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_INT_EN_MASK);

please put device differences in st_lsm6dsx_sensor_settings[]

> +		if (err < 0)
> +			return err;
> +	}
> +
> +	switch (hw->drdy_pin) {

drdy_pin it is only used here right? If so we do not need it just enable this
configuration by default. I would prefer to maintain the code simple

> +	case 1:
> +		md_reg = ST_LSM6DSX_REG_MD1_CFG_ADDR;
> +		break;
> +	case 2:
> +		md_reg = ST_LSM6DSX_REG_MD2_CFG_ADDR;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	/* Enable wakeup interrupt */
> +	err = regmap_update_bits(hw->regmap, md_reg,
> +				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK,
> +				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK);
> +
> +	return err;
> +}
> +
>  static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>  {
>  	struct st_lsm6dsx_hw *hw = private;
> @@ -1207,6 +1344,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
>  		err = st_lsm6dsx_fifo_setup(hw);
>  		if (err < 0)
>  			return err;

newline here please

> +		err = st_lsm6dsx_event_setup(hw_id, 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 v2 3/6] iio: imu: st_lsm6dsx: add motion events

[Jonathan Cameron]

On Tue, 16 Jul 2019 10:29:27 +0200
Lorenzo Bianconi <lorenzo@kernel.org> wrote:

> > Add event channels that controls the creation of motion events.
> > 
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> > 
> > Changes since v1:
> >  * added handling of LSM6
> >  * added CHANNEL info with events for ACC
> >  * removed st_lsm6dsx_set_event_threshold function
> >  * added check of event type to event channels
> > 
> > Issues:
> >  * This currently breaks buffered reads, as the interrupt stays high.
> >    This happens when MD1_CFG INT1_WU (wakeup event routes to INT1) is
> >    enabled.
> >    The datasheet doesn't seem to decribe whats happening and I can't
> >    find a status register to read somehing useful.
> >    Maybe it's impossible to share the buffered reads interrupt with
> >    the wakeup interrupt?  
> 
> Could you explain this issue a bit more? adding st folks...

This isn't totally unheard of for other hardware. If it turns out to be
a hardware restriction then the software approach is to return -EBUSY
if anyone tries to enable an event whilst in buffered mode, and -EBUSY
if anyone tries to enable buffered mode with an event enabled.

It's rather ugly though definitely good to see if there is a proper
solution!

Thanks,

Jonathan

> 
> > 
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  30 ++++
> >  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 +++++++++++++++++--
> >  2 files changed, 182 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > index 738bed4a9752..fef08b7cf2a0 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> > @@ -12,6 +12,7 @@
> >  #define ST_LSM6DSX_H
> >  
> >  #include <linux/device.h>
> > +#include <linux/iio/iio.h>
> >  
> >  #define ST_LSM6DS3_DEV_NAME	"lsm6ds3"
> >  #define ST_LSM6DS3H_DEV_NAME	"lsm6ds3h"
> > @@ -50,6 +51,26 @@ enum st_lsm6dsx_hw_id {
> >  					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
> >  #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
> >  
> > +#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
> > +{									\
> > +	.type = chan_type,						\
> > +	.address = addr,						\
> > +	.modified = 1,							\
> > +	.channel2 = mod,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> > +			      BIT(IIO_CHAN_INFO_SCALE),			\
> > +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> > +	.scan_index = scan_idx,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.realbits = 16,						\
> > +		.storagebits = 16,					\
> > +		.endianness = IIO_LE,					\
> > +	},								\
> > +	.event_spec = &st_lsm6dsx_event,				\
> > +	.num_event_specs = 1,						\
> > +}  
> 
> I would prefer to extend existing macros
> 
> > +
> >  #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
> >  {									\
> >  	.type = chan_type,						\
> > @@ -297,6 +318,8 @@ struct st_lsm6dsx_hw {
> >  	u8 enable_mask;
> >  	u8 ts_sip;
> >  	u8 sip;
> > +	u8 event_threshold;
> > +	bool enable_event;
> >  	int drdy_pin;
> >  
> >  	u8 *buff;
> > @@ -306,6 +329,13 @@ struct st_lsm6dsx_hw {
> >  	const struct st_lsm6dsx_settings *settings;
> >  };
> >  
> > +static const struct iio_event_spec st_lsm6dsx_event = {
> > +	.type = IIO_EV_TYPE_THRESH,
> > +	.dir = IIO_EV_DIR_EITHER,
> > +	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +			 BIT(IIO_EV_INFO_ENABLE)
> > +};
> > +
> >  static const unsigned long st_lsm6dsx_available_scan_masks[] = {0x7, 0x0};
> >  extern const struct dev_pm_ops st_lsm6dsx_pm_ops;
> >  
> > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > index 2c11addf568b..6decb0846f1a 100644
> > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > @@ -76,6 +76,16 @@
> >  #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)  
> 
> I am pretty sure this is not true at least for lsm6ds3/lsm6ds3h
> 
> > +
> > +#define ST_LSM6DSX_REG_WAKE_UP_ADDR		0x5B
> > +#define ST_LSM6DSX_REG_WAKE_UP_THRES_MASK	GENMASK(5, 0)
> > +
> > +#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
> > +#define ST_LSM6DSX_REG_MD2_CFG_ADDR		0x5F
> > +#define ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK	BIT(5)
> > +
> >  static const struct st_lsm6dsx_odr_table_entry st_lsm6dsx_odr_table[] = {
> >  	[ST_LSM6DSX_ID_ACC] = {
> >  		.reg = {
> > @@ -470,12 +480,12 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
> >  };
> >  
> >  static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
> > -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
> > -			   IIO_MOD_X, 0),
> > -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
> > -			   IIO_MOD_Y, 1),
> > -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
> > -			   IIO_MOD_Z, 2),
> > +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
> > +			       IIO_MOD_X, 0),
> > +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
> > +			       IIO_MOD_Y, 1),
> > +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
> > +			       IIO_MOD_Z, 2),
> >  	IIO_CHAN_SOFT_TIMESTAMP(3),
> >  };
> >  
> > @@ -679,18 +689,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
> >  	int err, delay;
> >  	__le16 data;
> >  
> > -	err = st_lsm6dsx_sensor_set_enable(sensor, true);
> > -	if (err < 0)
> > -		return err;
> > +	if (!hw->enable_event) {
> > +		err = st_lsm6dsx_sensor_set_enable(sensor, true);
> > +		if (err < 0)
> > +			return err;
> >  
> > -	delay = 1000000 / sensor->odr;
> > -	usleep_range(delay, 2 * delay);
> > +		delay = 1000000 / sensor->odr;
> > +		usleep_range(delay, 2 * delay);
> > +	}
> >  
> >  	err = st_lsm6dsx_read_locked(hw, addr, &data, sizeof(data));
> >  	if (err < 0)
> >  		return err;
> >  
> > -	st_lsm6dsx_sensor_set_enable(sensor, false);
> > +	if (!hw->enable_event)
> > +		st_lsm6dsx_sensor_set_enable(sensor, false);
> >  
> >  	*val = (s16)le16_to_cpu(data);
> >  
> > @@ -763,6 +776,94 @@ 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);
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +
> > +	if (type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	*val2 = 0;
> > +	*val = hw->event_threshold;
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
> > +				    const struct iio_chan_spec *chan,
> > +				    enum iio_event_type type,
> > +				    enum iio_event_direction dir,
> > +				    enum iio_event_info info,
> > +				    int val, int val2)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +	int err;
> > +
> > +	if (type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	if (!hw->enable_event)
> > +		return -EBUSY;  
> 
> I guess it is ok to configure the threshold first, no?
> 
> > +
> > +	if (val < 0 || val > 31)
> > +		return -EINVAL;
> > +
> > +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_ADDR,
> > +				 ST_LSM6DSX_REG_WAKE_UP_THRES_MASK,
> > +				 val);
> > +	if (err)
> > +		return -EINVAL;
> > +
> > +	hw->event_threshold = val;
> > +
> > +	return 0;
> > +}
> > +
> > +static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
> > +					  const struct iio_chan_spec *chan,
> > +					  enum iio_event_type type,
> > +					  enum iio_event_direction dir)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +
> > +	if (type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	return hw->enable_event;
> > +}
> > +
> > +static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
> > +					   const struct iio_chan_spec *chan,
> > +					   enum iio_event_type type,
> > +					   enum iio_event_direction dir,
> > +					   int state)
> > +{
> > +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > +	struct st_lsm6dsx_hw *hw = sensor->hw;
> > +
> > +	if (type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	if (state && hw->enable_event)
> > +		return 0;
> > +
> > +	hw->enable_event = state;
> > +	if (state)
> > +		st_lsm6dsx_sensor_set_enable(sensor, true);
> > +	else
> > +		st_lsm6dsx_sensor_set_enable(sensor, false);  
> 
> st_lsm6dsx_sensor_set_enable can fails. Why not do
> 
> 	err = st_lsm6dsx_sensor_set_enable(sensor, state);
> 	if (err < 0)
> 		return err;
> 
> 	hw->enable_event = state;;
> 	return 0;
> 
> > +
> > +	return 0;
> > +}
> > +
> >  int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
> >  {
> >  	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
> > @@ -839,6 +940,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,
> >  };
> >  
> > @@ -1076,6 +1181,38 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
> >  	return iio_dev;
> >  }
> >  
> > +int st_lsm6dsx_event_setup(int id, struct st_lsm6dsx_hw *hw)
> > +{
> > +	int err;
> > +	unsigned int md_reg;
> > +
> > +	if (id == ST_ISM330DLC_ID) {
> > +		/* Enable basic interrupts for ISM330 */
> > +		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_INT_EN_MASK);  
> 
> please put device differences in st_lsm6dsx_sensor_settings[]
> 
> > +		if (err < 0)
> > +			return err;
> > +	}
> > +
> > +	switch (hw->drdy_pin) {  
> 
> drdy_pin it is only used here right? If so we do not need it just enable this
> configuration by default. I would prefer to maintain the code simple
> 
> > +	case 1:
> > +		md_reg = ST_LSM6DSX_REG_MD1_CFG_ADDR;
> > +		break;
> > +	case 2:
> > +		md_reg = ST_LSM6DSX_REG_MD2_CFG_ADDR;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	/* Enable wakeup interrupt */
> > +	err = regmap_update_bits(hw->regmap, md_reg,
> > +				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK,
> > +				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK);
> > +
> > +	return err;
> > +}
> > +
> >  static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
> >  {
> >  	struct st_lsm6dsx_hw *hw = private;
> > @@ -1207,6 +1344,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
> >  		err = st_lsm6dsx_fifo_setup(hw);
> >  		if (err < 0)
> >  			return err;  
> 
> newline here please
> 
> > +		err = st_lsm6dsx_event_setup(hw_id, hw);
> > +		if (err < 0)
> > +			return err;
> >  	}
> >  
> >  	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
> > -- 
> > 2.22.0
> >   

Re: [PATCH v2 3/6] iio: imu: st_lsm6dsx: add motion events

[Sean Nyekjaer]

Back from vacation :-)

On 16/07/2019 10.29, Lorenzo Bianconi wrote:
>> Add event channels that controls the creation of motion events.
>>
>> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>> ---
>>
>> Changes since v1:
>>   * added handling of LSM6
>>   * added CHANNEL info with events for ACC
>>   * removed st_lsm6dsx_set_event_threshold function
>>   * added check of event type to event channels
>>
>> Issues:
>>   * This currently breaks buffered reads, as the interrupt stays high.
>>     This happens when MD1_CFG INT1_WU (wakeup event routes to INT1) is
>>     enabled.
>>     The datasheet doesn't seem to decribe whats happening and I can't
>>     find a status register to read somehing useful.
>>     Maybe it's impossible to share the buffered reads interrupt with
>>     the wakeup interrupt?
> 
> Could you explain this issue a bit more? adding st folks...
> 
I can try, there is not much to it...
When buffered reads is enabled, and I enable wake-up at the same time, 
and then drop or shake the acc to create an event.
The irq pin stays high, expected behavior would be it drops again when 
the buffer is read and the event is finished signaling.

>>
>>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  30 ++++
>>   drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 164 +++++++++++++++++--
>>   2 files changed, 182 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> index 738bed4a9752..fef08b7cf2a0 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
>> @@ -12,6 +12,7 @@
>>   #define ST_LSM6DSX_H
>>   
>>   #include <linux/device.h>
>> +#include <linux/iio/iio.h>
>>   
>>   #define ST_LSM6DS3_DEV_NAME	"lsm6ds3"
>>   #define ST_LSM6DS3H_DEV_NAME	"lsm6ds3h"
>> @@ -50,6 +51,26 @@ enum st_lsm6dsx_hw_id {
>>   					 * ST_LSM6DSX_TAGGED_SAMPLE_SIZE)
>>   #define ST_LSM6DSX_SHIFT_VAL(val, mask)	(((val) << __ffs(mask)) & (mask))
>>   
>> +#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
>> +{									\
>> +	.type = chan_type,						\
>> +	.address = addr,						\
>> +	.modified = 1,							\
>> +	.channel2 = mod,						\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>> +			      BIT(IIO_CHAN_INFO_SCALE),			\
>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
>> +	.scan_index = scan_idx,						\
>> +	.scan_type = {							\
>> +		.sign = 's',						\
>> +		.realbits = 16,						\
>> +		.storagebits = 16,					\
>> +		.endianness = IIO_LE,					\
>> +	},								\
>> +	.event_spec = &st_lsm6dsx_event,				\
>> +	.num_event_specs = 1,						\
>> +}
> 
> I would prefer to extend existing macros
> 
Please explain how...

>> +
>>   #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
>>   {									\
>>   	.type = chan_type,						\
>> @@ -297,6 +318,8 @@ struct st_lsm6dsx_hw {
>>   	u8 enable_mask;
>>   	u8 ts_sip;
>>   	u8 sip;
>> +	u8 event_threshold;
>> +	bool enable_event;
>>   	int drdy_pin;
>>   
>>   	u8 *buff;
>> @@ -306,6 +329,13 @@ struct st_lsm6dsx_hw {
>>   	const struct st_lsm6dsx_settings *settings;
>>   };
>>   
>> +static const struct iio_event_spec st_lsm6dsx_event = {
>> +	.type = IIO_EV_TYPE_THRESH,
>> +	.dir = IIO_EV_DIR_EITHER,
>> +	.mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> +			 BIT(IIO_EV_INFO_ENABLE)
>> +};
>> +
>>   static const unsigned long st_lsm6dsx_available_scan_masks[] = {0x7, 0x0};
>>   extern const struct dev_pm_ops st_lsm6dsx_pm_ops;
>>   
>> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> index 2c11addf568b..6decb0846f1a 100644
>> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
>> @@ -76,6 +76,16 @@
>>   #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)
> 
> I am pretty sure this is not true at least for lsm6ds3/lsm6ds3h
> 
Will check

>> +
>> +#define ST_LSM6DSX_REG_WAKE_UP_ADDR		0x5B
>> +#define ST_LSM6DSX_REG_WAKE_UP_THRES_MASK	GENMASK(5, 0)
>> +
>> +#define ST_LSM6DSX_REG_MD1_CFG_ADDR		0x5E
>> +#define ST_LSM6DSX_REG_MD2_CFG_ADDR		0x5F
>> +#define ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK	BIT(5)
>> +
>>   static const struct st_lsm6dsx_odr_table_entry st_lsm6dsx_odr_table[] = {
>>   	[ST_LSM6DSX_ID_ACC] = {
>>   		.reg = {
>> @@ -470,12 +480,12 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>>   };
>>   
>>   static const struct iio_chan_spec st_lsm6dsx_acc_channels[] = {
>> -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
>> -			   IIO_MOD_X, 0),
>> -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
>> -			   IIO_MOD_Y, 1),
>> -	ST_LSM6DSX_CHANNEL(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
>> -			   IIO_MOD_Z, 2),
>> +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_X_L_ADDR,
>> +			       IIO_MOD_X, 0),
>> +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Y_L_ADDR,
>> +			       IIO_MOD_Y, 1),
>> +	ST_LSM6DSX_CHANNEL_ACC(IIO_ACCEL, ST_LSM6DSX_REG_ACC_OUT_Z_L_ADDR,
>> +			       IIO_MOD_Z, 2),
>>   	IIO_CHAN_SOFT_TIMESTAMP(3),
>>   };
>>   
>> @@ -679,18 +689,21 @@ static int st_lsm6dsx_read_oneshot(struct st_lsm6dsx_sensor *sensor,
>>   	int err, delay;
>>   	__le16 data;
>>   
>> -	err = st_lsm6dsx_sensor_set_enable(sensor, true);
>> -	if (err < 0)
>> -		return err;
>> +	if (!hw->enable_event) {
>> +		err = st_lsm6dsx_sensor_set_enable(sensor, true);
>> +		if (err < 0)
>> +			return err;
>>   
>> -	delay = 1000000 / sensor->odr;
>> -	usleep_range(delay, 2 * delay);
>> +		delay = 1000000 / sensor->odr;
>> +		usleep_range(delay, 2 * delay);
>> +	}
>>   
>>   	err = st_lsm6dsx_read_locked(hw, addr, &data, sizeof(data));
>>   	if (err < 0)
>>   		return err;
>>   
>> -	st_lsm6dsx_sensor_set_enable(sensor, false);
>> +	if (!hw->enable_event)
>> +		st_lsm6dsx_sensor_set_enable(sensor, false);
>>   
>>   	*val = (s16)le16_to_cpu(data);
>>   
>> @@ -763,6 +776,94 @@ 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);
>> +	struct st_lsm6dsx_hw *hw = sensor->hw;
>> +
>> +	if (type != IIO_EV_TYPE_THRESH)
>> +		return -EINVAL;
>> +
>> +	*val2 = 0;
>> +	*val = hw->event_threshold;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static int st_lsm6dsx_write_event(struct iio_dev *iio_dev,
>> +				    const struct iio_chan_spec *chan,
>> +				    enum iio_event_type type,
>> +				    enum iio_event_direction dir,
>> +				    enum iio_event_info info,
>> +				    int val, int val2)
>> +{
>> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> +	struct st_lsm6dsx_hw *hw = sensor->hw;
>> +	int err;
>> +
>> +	if (type != IIO_EV_TYPE_THRESH)
>> +		return -EINVAL;
>> +
>> +	if (!hw->enable_event)
>> +		return -EBUSY;
> 
> I guess it is ok to configure the threshold first, no?
> 
Will test and allow the threshold to be configured first.

>> +
>> +	if (val < 0 || val > 31)
>> +		return -EINVAL;
>> +
>> +	err = regmap_update_bits(hw->regmap, ST_LSM6DSX_REG_WAKE_UP_ADDR,
>> +				 ST_LSM6DSX_REG_WAKE_UP_THRES_MASK,
>> +				 val);
>> +	if (err)
>> +		return -EINVAL;
>> +
>> +	hw->event_threshold = val;
>> +
>> +	return 0;
>> +}
>> +
>> +static int st_lsm6dsx_read_event_config(struct iio_dev *iio_dev,
>> +					  const struct iio_chan_spec *chan,
>> +					  enum iio_event_type type,
>> +					  enum iio_event_direction dir)
>> +{
>> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> +	struct st_lsm6dsx_hw *hw = sensor->hw;
>> +
>> +	if (type != IIO_EV_TYPE_THRESH)
>> +		return -EINVAL;
>> +
>> +	return hw->enable_event;
>> +}
>> +
>> +static int st_lsm6dsx_write_event_config(struct iio_dev *iio_dev,
>> +					   const struct iio_chan_spec *chan,
>> +					   enum iio_event_type type,
>> +					   enum iio_event_direction dir,
>> +					   int state)
>> +{
>> +	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> +	struct st_lsm6dsx_hw *hw = sensor->hw;
>> +
>> +	if (type != IIO_EV_TYPE_THRESH)
>> +		return -EINVAL;
>> +
>> +	if (state && hw->enable_event)
>> +		return 0;
>> +
>> +	hw->enable_event = state;
>> +	if (state)
>> +		st_lsm6dsx_sensor_set_enable(sensor, true);
>> +	else
>> +		st_lsm6dsx_sensor_set_enable(sensor, false);
> 
> st_lsm6dsx_sensor_set_enable can fails. Why not do
> 
> 	err = st_lsm6dsx_sensor_set_enable(sensor, state);
> 	if (err < 0)
> 		return err;
> 
> 	hw->enable_event = state;;
> 	return 0;
> 
Will change to this approach :-)

>> +
>> +	return 0;
>> +}
>> +
>>   int st_lsm6dsx_set_watermark(struct iio_dev *iio_dev, unsigned int val)
>>   {
>>   	struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
>> @@ -839,6 +940,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,
>>   };
>>   
>> @@ -1076,6 +1181,38 @@ static struct iio_dev *st_lsm6dsx_alloc_iiodev(struct st_lsm6dsx_hw *hw,
>>   	return iio_dev;
>>   }
>>   
>> +int st_lsm6dsx_event_setup(int id, struct st_lsm6dsx_hw *hw)
>> +{
>> +	int err;
>> +	unsigned int md_reg;
>> +
>> +	if (id == ST_ISM330DLC_ID) {
>> +		/* Enable basic interrupts for ISM330 */
>> +		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_INT_EN_MASK);
> 
> please put device differences in st_lsm6dsx_sensor_settings[]
> 
Will do.

>> +		if (err < 0)
>> +			return err;
>> +	}
>> +
>> +	switch (hw->drdy_pin) {
> 
> drdy_pin it is only used here right? If so we do not need it just enable this
> configuration by default. I would prefer to maintain the code simple
> 
>> +	case 1:
>> +		md_reg = ST_LSM6DSX_REG_MD1_CFG_ADDR;
>> +		break;
>> +	case 2:
>> +		md_reg = ST_LSM6DSX_REG_MD2_CFG_ADDR;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	/* Enable wakeup interrupt */
>> +	err = regmap_update_bits(hw->regmap, md_reg,
>> +				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK,
>> +				 ST_LSM6DSX_REG_MD_CFG_INT_WU_MASK);
>> +
>> +	return err;
>> +}
>> +
>>   static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
>>   {
>>   	struct st_lsm6dsx_hw *hw = private;
>> @@ -1207,6 +1344,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
>>   		err = st_lsm6dsx_fifo_setup(hw);
>>   		if (err < 0)
>>   			return err;
> 
> newline here please
> 
>> +		err = st_lsm6dsx_event_setup(hw_id, hw);
>> +		if (err < 0)
>> +			return err;
>>   	}
>>   
>>   	for (i = 0; i < ST_LSM6DSX_ID_MAX; i++) {
>> -- 
>> 2.22.0
>>

Re: [PATCH v2 3/6] iio: imu: st_lsm6dsx: add motion events

[Sean Nyekjaer]

On 16/07/2019 10.29, Lorenzo Bianconi wrote:

>> +#define ST_LSM6DSX_CHANNEL_ACC(chan_type, addr, mod, scan_idx)		\
>> +{									\
>> +	.type = chan_type,						\
>> +	.address = addr,						\
>> +	.modified = 1,							\
>> +	.channel2 = mod,						\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
>> +			      BIT(IIO_CHAN_INFO_SCALE),			\
>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
>> +	.scan_index = scan_idx,						\
>> +	.scan_type = {							\
>> +		.sign = 's',						\
>> +		.realbits = 16,						\
>> +		.storagebits = 16,					\
>> +		.endianness = IIO_LE,					\
>> +	},								\
>> +	.event_spec = &st_lsm6dsx_event,				\
>> +	.num_event_specs = 1,						\
>> +}
> 
> I would prefer to extend existing macros
> 
>> +
>>   #define ST_LSM6DSX_CHANNEL(chan_type, addr, mod, scan_idx)		\
>>   {									\
>>   	.type = chan_type,						\
>> @@ -297,6 +318,8 @@ struct st_lsm6dsx_hw {
>>   	u8 enable_mask;
>>   	u8 ts_sip;
>>   	u8 sip;
>> +	u8 event_threshold;
>> +	bool enable_event;
>>   	int drdy_pin;
>>   
>>   	u8 *buff;
>> @@ -306,6 +329,13 @@ struct st_lsm6dsx_hw {
>>   	const struct st_lsm6dsx_settings *settings;
>>   };
>>   

How would I do that when we have the `event_spec` for the accelerometer?

/Sean

[PATCH v2 4/6] iio: imu: st_lsm6dsx: add wakeup-source option

[Sean Nyekjaer]

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

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

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---

Changes since v1:
 * none, as the call to st_lsm6dsx_flush_fifo will put the fifo in
   bypass mode.

 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 6decb0846f1a..fc450eeb9870 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1358,6 +1358,10 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
 			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);
@@ -1372,6 +1376,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
 		if (!hw->iio_devs[i])
 			continue;
 
+		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;
@@ -1404,6 +1414,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
 		if (!hw->iio_devs[i])
 			continue;
 
+		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {
+			disable_irq_wake(hw->irq);
+			continue;
+		}
+
 		sensor = iio_priv(hw->iio_devs[i]);
 		if (!(hw->suspend_mask & BIT(sensor->id)))
 			continue;
-- 
2.22.0

Re: [PATCH v2 4/6] iio: imu: st_lsm6dsx: add wakeup-source option

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1154 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.
> 

[...]

>  EXPORT_SYMBOL(st_lsm6dsx_probe);
> @@ -1372,6 +1376,12 @@ static int __maybe_unused st_lsm6dsx_suspend(struct device *dev)
>  		if (!hw->iio_devs[i])
>  			continue;
>  
> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {

unnecessary brackets

> +			/* Enable wake from IRQ */
> +			enable_irq_wake(hw->irq);
> +			continue;
> +		}

I think we need to move this after enable_mask check, dont' we?

> +
>  		sensor = iio_priv(hw->iio_devs[i]);
>  		if (!(hw->enable_mask & BIT(sensor->id)))
>  			continue;
> @@ -1404,6 +1414,11 @@ static int __maybe_unused st_lsm6dsx_resume(struct device *dev)
>  		if (!hw->iio_devs[i])
>  			continue;
>  
> +		if (device_may_wakeup(dev) && (i == ST_LSM6DSX_ID_ACC)) {

unnecessary brackets

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

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

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

[Sean Nyekjaer]

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

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---

Changes since v1:
 * None

 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 +++++++-------
 1 file changed, 7 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 fc450eeb9870..0503abab6efc 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1215,19 +1215,19 @@ int st_lsm6dsx_event_setup(int id, 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;
 
-	mutex_lock(&hw->fifo_lock);
-	count = st_lsm6dsx_read_fifo(hw);
-	mutex_unlock(&hw->fifo_lock);
+	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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1530 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>
> ---
> 
> Changes since v1:
>  * None
> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 +++++++-------
>  1 file changed, 7 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 fc450eeb9870..0503abab6efc 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -1215,19 +1215,19 @@ int st_lsm6dsx_event_setup(int id, 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;
>  
> -	mutex_lock(&hw->fifo_lock);
> -	count = st_lsm6dsx_read_fifo(hw);
> -	mutex_unlock(&hw->fifo_lock);
> +	if (hw->sip > 0) {

I think we do not need this if() statement since st_lsm6dsx_read_fifo will
return 0 in case

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

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

[Sean Nyekjaer]

Report iio motion events to iio subsystem

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---

Changes since v1:
 * none

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

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 0503abab6efc..acc653d5e00e 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -39,6 +39,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>
@@ -76,6 +77,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(3)
+
 #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
 #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
 
@@ -1212,6 +1219,39 @@ int st_lsm6dsx_event_setup(int id, 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)
 {
@@ -1222,7 +1262,19 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
 {
 	struct st_lsm6dsx_hw *hw = private;
 	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;
+
+		if (data & ST_LSM6DSX_REG_WAKE_UP_SRC_WU_MASK)
+			st_lsm6dsx_report_motion_event(hw, data);
+	}
 
+try_fifo:
 	if (hw->sip > 0) {
 		mutex_lock(&hw->fifo_lock);
 		count = st_lsm6dsx_read_fifo(hw);
-- 
2.22.0

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

[Lorenzo Bianconi]

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

> Report iio motion events to iio subsystem
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> 
> Changes since v1:
>  * none
> 
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 52 ++++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 0503abab6efc..acc653d5e00e 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -39,6 +39,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>
> @@ -76,6 +77,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(3)
> +
>  #define ST_LSM6DSX_REG_TAP_CFG_ADDR		0x58
>  #define ST_LSM6DSX_REG_TAP_CFG_INT_EN_MASK	BIT(7)
>  
> @@ -1212,6 +1219,39 @@ int st_lsm6dsx_event_setup(int id, 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)
>  {
> @@ -1222,7 +1262,19 @@ static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
>  {
>  	struct st_lsm6dsx_hw *hw = private;
>  	int count = 0;
> +	int data, err;
> +
> +	if (hw->enable_event) {
> +		err = regmap_read(hw->regmap,
> +				  ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR, &data);

I think we can move the ST_LSM6DSX_REG_WAKE_UP_SRC_ADDR read in
st_lsm6dsx_report_motion_event (and rename it) and avoid goto try_fifo

> +		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:
>  	if (hw->sip > 0) {
>  		mutex_lock(&hw->fifo_lock);
>  		count = st_lsm6dsx_read_fifo(hw);
> -- 
> 2.22.0
> 

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

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

[Lorenzo Bianconi]

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

> This prepares the interrupt to be used for other stuff than
> fifo reading + event readings.
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> 
> Changes since v1:
>  * none
> 
>  .../iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c    | 78 +----------------
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c  | 87 +++++++++++++++++++
>  2 files changed, 88 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> index 38194f4d2b7e..2b938d87ae34 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_buffer.c

[...]

>  int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
>  		     struct regmap *regmap)
>  {
> @@ -1117,6 +1201,9 @@ int st_lsm6dsx_probe(struct device *dev, int irq, int hw_id,
>  	}
>  
>  	if (hw->irq > 0) {
> +		err = st_lsm6dsx_irq_setup(hw);
> +		if (err < 0)
> +			return err;

Could you please put a 'newline' here?

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