[PATCH 0/2] Add support for WoM (Wake-on-Motion) feature

[PATCH 0/2] Add support for WoM (Wake-on-Motion) feature

[Jean-Baptiste Maneyrol via B4 Relay]

Similar to feature present in older chip, it compares the magnitude of
the last 2 accel samples against a threshold and returns an interrupt
even if the value is higher.

WoM maps best to accel x|y|z ROC event. This series add system wakeup
functionality if WoM is on and wakeup is enabled when system suspends.

This series also prepare the driver for supporting further APEX
features like pedometer, tilt, ... It introduces an apex structure that
will hold all APEX settings and track the enable state.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
Jean-Baptiste Maneyrol (2):
      iio: imu: inv_icm42600: add WoM support
      iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

 drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  49 +++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 267 ++++++++++++++++++++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   | 125 ++++++++--
 4 files changed, 416 insertions(+), 27 deletions(-)
---
base-commit: c0f115a8d97599623294c8e9ec28530e19c1e85b
change-id: 20250220-losd-3-inv-icm42600-add-wom-support-0620fef9db23

Best regards,
-- 
Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

[PATCH 1/2] iio: imu: inv_icm42600: add WoM support

[Jean-Baptiste Maneyrol via B4 Relay]

From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

Add WoM as accel roc rising x|y|z event.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  47 +++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 264 ++++++++++++++++++++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  56 ++++-
 4 files changed, 363 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 18787a43477b89db12caee597ab040af5c8f52d5..8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -135,6 +135,14 @@ struct inv_icm42600_suspended {
 	bool temp;
 };
 
+struct inv_icm42600_apex {
+	unsigned int on;
+	struct {
+		bool enable;
+		uint64_t value;
+	} wom;
+};
+
 /**
  *  struct inv_icm42600_state - driver state variables
  *  @lock:		lock for serializing multiple registers access.
@@ -151,6 +159,7 @@ struct inv_icm42600_suspended {
  *  @buffer:		data transfer buffer aligned for DMA.
  *  @fifo:		FIFO management structure.
  *  @timestamp:		interrupt timestamps.
+ *  @apex:		APEX features management.
  */
 struct inv_icm42600_state {
 	struct mutex lock;
@@ -164,12 +173,13 @@ struct inv_icm42600_state {
 	struct inv_icm42600_suspended suspended;
 	struct iio_dev *indio_gyro;
 	struct iio_dev *indio_accel;
-	uint8_t buffer[2] __aligned(IIO_DMA_MINALIGN);
+	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
 	struct inv_icm42600_fifo fifo;
 	struct {
 		int64_t gyro;
 		int64_t accel;
 	} timestamp;
+	struct inv_icm42600_apex apex;
 };
 
 
@@ -253,6 +263,18 @@ struct inv_icm42600_sensor_state {
 #define INV_ICM42600_REG_FIFO_COUNT			0x002E
 #define INV_ICM42600_REG_FIFO_DATA			0x0030
 
+#define INV_ICM42600_REG_INT_STATUS2			0x0037
+#define INV_ICM42600_INT_STATUS2_SMD_INT		BIT(3)
+#define INV_ICM42600_INT_STATUS2_WOM_INT		GENMASK(2, 0)
+
+#define INV_ICM42600_REG_INT_STATUS3			0x0038
+#define INV_ICM42600_INT_STATUS3_STEP_DET_INT		BIT(5)
+#define INV_ICM42600_INT_STATUS3_STEP_CNT_OVF_INT	BIT(4)
+#define INV_ICM42600_INT_STATUS3_TILT_DET_INT		BIT(3)
+#define INV_ICM42600_INT_STATUS3_WAKE_INT		BIT(2)
+#define INV_ICM42600_INT_STATUS3_SLEEP_INT		BIT(1)
+#define INV_ICM42600_INT_STATUS3_TAP_DET_INT		BIT(0)
+
 #define INV_ICM42600_REG_SIGNAL_PATH_RESET		0x004B
 #define INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN	BIT(6)
 #define INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET	BIT(5)
@@ -309,6 +331,14 @@ struct inv_icm42600_sensor_state {
 #define INV_ICM42600_TMST_CONFIG_TMST_FSYNC_EN		BIT(1)
 #define INV_ICM42600_TMST_CONFIG_TMST_EN		BIT(0)
 
+#define INV_ICM42600_REG_SMD_CONFIG			0x0057
+#define INV_ICM42600_SMD_CONFIG_WOM_INT_MODE		BIT(3)
+#define INV_ICM42600_SMD_CONFIG_WOM_MODE		BIT(2)
+#define INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF		0x00
+#define INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM		0x01
+#define INV_ICM42600_SMD_CONFIG_SMD_MODE_SHORT		0x02
+#define INV_ICM42600_SMD_CONFIG_SMD_MODE_LONG		0x03
+
 #define INV_ICM42600_REG_FIFO_CONFIG1			0x005F
 #define INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD	BIT(6)
 #define INV_ICM42600_FIFO_CONFIG1_WM_GT_TH		BIT(5)
@@ -338,6 +368,11 @@ struct inv_icm42600_sensor_state {
 #define INV_ICM42600_INT_SOURCE0_FIFO_FULL_INT1_EN	BIT(1)
 #define INV_ICM42600_INT_SOURCE0_UI_AGC_RDY_INT1_EN	BIT(0)
 
+#define INV_ICM42600_REG_INT_SOURCE1			0x0066
+#define INV_ICM42600_INT_SOURCE1_I3C_ERROR_INT1_EN	BIT(6)
+#define INV_ICM42600_INT_SOURCE1_SMD_INT1_EN		BIT(3)
+#define INV_ICM42600_INT_SOURCE1_WOM_INT1_EN		GENMASK(2, 0)
+
 #define INV_ICM42600_REG_WHOAMI				0x0075
 #define INV_ICM42600_WHOAMI_ICM42600			0x40
 #define INV_ICM42600_WHOAMI_ICM42602			0x41
@@ -373,6 +408,10 @@ struct inv_icm42600_sensor_state {
 #define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN		BIT(0)
 
 /* User bank 4 (MSB 0x40) */
+#define INV_ICM42600_REG_ACCEL_WOM_X_THR		0x404A
+#define INV_ICM42600_REG_ACCEL_WOM_Y_THR		0x404B
+#define INV_ICM42600_REG_ACCEL_WOM_Z_THR		0x404C
+
 #define INV_ICM42600_REG_INT_SOURCE8			0x404F
 #define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN		BIT(5)
 #define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN		BIT(4)
@@ -423,6 +462,8 @@ int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
 int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
 			       unsigned int *sleep_ms);
 
+int inv_icm42600_set_wom(struct inv_icm42600_state *st, bool enable);
+
 int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
 			     unsigned int writeval, unsigned int *readval);
 
@@ -437,4 +478,8 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st);
 
 int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev);
 
+void inv_icm42600_accel_handle_events(struct iio_dev *indio_dev,
+				      unsigned int status2, unsigned int status3,
+				      int64_t timestamp);
+
 #endif
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 388520ec60b5c5d21b16717978ebf330e38aa1fe..8ce2276b3edc61cc1ea26810198dd0057054ec48 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -13,6 +13,7 @@
 
 #include <linux/iio/buffer.h>
 #include <linux/iio/common/inv_sensors_timestamp.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/kfifo_buf.h>
 
@@ -47,6 +48,16 @@
 		.ext_info = _ext_info,					\
 	}
 
+#define INV_ICM42600_ACCEL_EVENT_CHAN(_modifier, _events, _events_nb)	\
+	{								\
+		.type = IIO_ACCEL,					\
+		.modified = 1,						\
+		.channel2 = _modifier,					\
+		.event_spec = _events,					\
+		.num_event_specs = _events_nb,				\
+		.scan_index = -1,					\
+	}
+
 enum inv_icm42600_accel_scan {
 	INV_ICM42600_ACCEL_SCAN_X,
 	INV_ICM42600_ACCEL_SCAN_Y,
@@ -82,14 +93,15 @@ static int inv_icm42600_accel_power_mode_set(struct iio_dev *indio_dev,
 	if (idx >= ARRAY_SIZE(inv_icm42600_accel_power_mode_values))
 		return -EINVAL;
 
-	if (iio_buffer_enabled(indio_dev))
-		return -EBUSY;
-
 	power_mode = inv_icm42600_accel_power_mode_values[idx];
 	filter = inv_icm42600_accel_filter_values[idx];
 
 	guard(mutex)(&st->lock);
 
+	/* cannot change if accel sensor is on */
+	if (st->conf.accel.mode != INV_ICM42600_SENSOR_MODE_OFF)
+		return -EBUSY;
+
 	/* prevent change if power mode is not supported by the ODR */
 	switch (power_mode) {
 	case INV_ICM42600_SENSOR_MODE_LOW_NOISE:
@@ -160,6 +172,16 @@ static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
 	{},
 };
 
+/* WoM event: rising ROC */
+static const struct iio_event_spec inv_icm42600_wom_events[] = {
+	{
+		.type = IIO_EV_TYPE_ROC,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE) |
+				 BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
 static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
 	INV_ICM42600_ACCEL_CHAN(IIO_MOD_X, INV_ICM42600_ACCEL_SCAN_X,
 				inv_icm42600_accel_ext_infos),
@@ -169,6 +191,8 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
 				inv_icm42600_accel_ext_infos),
 	INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
 	IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
+	INV_ICM42600_ACCEL_EVENT_CHAN(IIO_MOD_X_OR_Y_OR_Z, inv_icm42600_wom_events,
+				      ARRAY_SIZE(inv_icm42600_wom_events)),
 };
 
 /*
@@ -294,6 +318,140 @@ static int inv_icm42600_accel_read_sensor(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
+							  int accel_hz, int accel_uhz)
+{
+	/* 1000/256mg per LSB converted in m/s² in micro (1000000) */
+	const unsigned int convert = (1000U * 9807U) / 256U;
+	uint64_t value;
+	uint64_t freq_uhz;
+
+	/* return 0 only if roc is 0 */
+	if (roc == 0)
+		return 0;
+
+	freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;
+	value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);
+
+	/* limit value to 8 bits and prevent 0 */
+	return min(255, max(1, value));
+}
+
+static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
+						      int accel_hz, int accel_uhz)
+{
+	/* 1000/256mg per LSB converted in m/s² in micro (1000000) */
+	const unsigned int convert = (1000U * 9807U) / 256U;
+	uint64_t value;
+	uint64_t freq_uhz;
+
+	value = threshold * convert;
+	freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;
+
+	/* compute the differential by multiplying by the frequency */
+	return div_u64(value * freq_uhz, 1000000U);
+}
+
+static int inv_icm42600_accel_set_wom_threshold(struct inv_icm42600_state *st,
+						uint64_t value,
+						int accel_hz, int accel_uhz)
+{
+	unsigned int threshold;
+	int ret;
+
+	/* convert roc to wom threshold and convert back to handle clipping */
+	threshold = inv_icm42600_accel_convert_roc_to_wom(value, accel_hz, accel_uhz);
+	value = inv_icm42600_accel_convert_wom_to_roc(threshold, accel_hz, accel_uhz);
+
+	dev_dbg(regmap_get_device(st->map), "wom_threshold: 0x%x\n", threshold);
+
+	/* set accel WoM threshold for the 3 axes */
+	st->buffer[0] = threshold;
+	st->buffer[1] = threshold;
+	st->buffer[2] = threshold;
+	ret = regmap_bulk_write(st->map, INV_ICM42600_REG_ACCEL_WOM_X_THR, st->buffer, 3);
+	if (ret)
+		return ret;
+
+	st->apex.wom.value = value;
+
+	return 0;
+}
+
+static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev, bool enable)
+{
+	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+	struct inv_icm42600_sensor_state *accel_st = iio_priv(indio_dev);
+	struct device *pdev = regmap_get_device(st->map);
+	struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
+	unsigned int sleep_ms = 0;
+	int ret;
+
+	if (enable) {
+		ret = pm_runtime_resume_and_get(pdev);
+		if (ret)
+			return ret;
+		scoped_guard(mutex, &st->lock) {
+			/* turn on accel sensor */
+			conf.mode = accel_st->power_mode;
+			conf.filter = accel_st->filter;
+			ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
+			if (ret)
+				goto error_suspend;
+		}
+		if (sleep_ms)
+			msleep(sleep_ms);
+		scoped_guard(mutex, &st->lock) {
+			ret = inv_icm42600_set_wom(st, true);
+			if (ret)
+				goto error_suspend;
+			st->apex.on++;
+			st->apex.wom.enable = true;
+		}
+	} else {
+		scoped_guard(mutex, &st->lock) {
+			st->apex.wom.enable = false;
+			st->apex.on--;
+			ret = inv_icm42600_set_wom(st, false);
+			if (ret)
+				goto error_suspend;
+			/* turn off accel sensor if not used */
+			if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
+				conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
+				ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
+				if (ret)
+					goto error_suspend;
+			}
+		}
+		if (sleep_ms)
+			msleep(sleep_ms);
+		pm_runtime_mark_last_busy(pdev);
+		pm_runtime_put_autosuspend(pdev);
+	}
+
+	return 0;
+
+error_suspend:
+	pm_runtime_mark_last_busy(pdev);
+	pm_runtime_put_autosuspend(pdev);
+	return ret;
+}
+
+void inv_icm42600_accel_handle_events(struct iio_dev *indio_dev,
+				      unsigned int status2, unsigned int status3,
+				      int64_t timestamp)
+{
+	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+	uint64_t ev_code;
+
+	/* handle WoM event */
+	if (st->apex.wom.enable && (status2 & INV_ICM42600_INT_STATUS2_WOM_INT)) {
+		ev_code = IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X_OR_Y_OR_Z,
+					     IIO_EV_TYPE_ROC, IIO_EV_DIR_RISING);
+		iio_push_event(indio_dev, ev_code, timestamp);
+	}
+}
+
 /* IIO format int + nano */
 static const int inv_icm42600_accel_scale[] = {
 	/* +/- 16G => 0.004788403 m/s-2 */
@@ -464,6 +622,10 @@ static int inv_icm42600_accel_write_odr(struct iio_dev *indio_dev,
 		goto out_unlock;
 
 	ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
+	if (ret)
+		goto out_unlock;
+	/* update wom threshold since roc is dependent on sampling frequency */
+	ret = inv_icm42600_accel_set_wom_threshold(st, st->apex.wom.value, val, val2);
 	if (ret)
 		goto out_unlock;
 	inv_icm42600_buffer_update_fifo_period(st);
@@ -822,6 +984,98 @@ static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int inv_icm42600_accel_read_event_config(struct iio_dev *indio_dev,
+						const struct iio_chan_spec *chan,
+						enum iio_event_type type,
+						enum iio_event_direction dir)
+{
+	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+
+	guard(mutex)(&st->lock);
+
+	/* handle WoM (roc rising) event */
+	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING)
+		return st->apex.wom.enable ? 1 : 0;
+
+	return -EINVAL;
+}
+
+static int inv_icm42600_accel_write_event_config(struct iio_dev *indio_dev,
+						 const struct iio_chan_spec *chan,
+						 enum iio_event_type type,
+						 enum iio_event_direction dir,
+						 bool state)
+{
+	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+
+	/* handle WoM (roc rising) event */
+	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
+		scoped_guard(mutex, &st->lock) {
+			if (st->apex.wom.enable == state)
+				return 0;
+		}
+		return inv_icm42600_accel_enable_wom(indio_dev, state);
+	}
+
+	return -EINVAL;
+}
+
+static int inv_icm42600_accel_read_event_value(struct iio_dev *indio_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+	u32 rem;
+
+	guard(mutex)(&st->lock);
+
+	/* handle WoM (roc rising) event value */
+	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
+		/* return value in micro */
+		*val = div_u64_rem(st->apex.wom.value, 1000000U, &rem);
+		*val2 = rem;
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+
+	return -EINVAL;
+}
+
+static int inv_icm42600_accel_write_event_value(struct iio_dev *indio_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
+	struct device *dev = regmap_get_device(st->map);
+	uint64_t value;
+	unsigned int accel_hz, accel_uhz;
+	int ret;
+
+	/* handle WoM (roc rising) event value */
+	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
+		if (val < 0 || val2 < 0)
+			return -EINVAL;
+		value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
+		pm_runtime_get_sync(dev);
+		scoped_guard(mutex, &st->lock) {
+			ret = inv_icm42600_accel_read_odr(st, &accel_hz, &accel_uhz);
+			if (ret >= 0)
+				ret = inv_icm42600_accel_set_wom_threshold(st, value,
+									   accel_hz, accel_uhz);
+		}
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+		return ret;
+	}
+
+	return -EINVAL;
+}
+
 static const struct iio_info inv_icm42600_accel_info = {
 	.read_raw = inv_icm42600_accel_read_raw,
 	.read_avail = inv_icm42600_accel_read_avail,
@@ -831,6 +1085,10 @@ static const struct iio_info inv_icm42600_accel_info = {
 	.update_scan_mode = inv_icm42600_accel_update_scan_mode,
 	.hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,
 	.hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,
+	.read_event_config = inv_icm42600_accel_read_event_config,
+	.write_event_config = inv_icm42600_accel_write_event_config,
+	.read_event_value = inv_icm42600_accel_read_event_value,
+	.write_event_value = inv_icm42600_accel_write_event_value,
 };
 
 struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
index aae7c56481a3fa4351e921fb98ce61d31d1d7d6a..094d739d7d862f6916ff31f09af227c80a3bdce5 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
@@ -422,7 +422,7 @@ static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)
 	conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
 	if (sensor == INV_ICM42600_SENSOR_GYRO)
 		ret = inv_icm42600_set_gyro_conf(st, &conf, &sleep_sensor);
-	else
+	else if (!st->apex.on)
 		ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_sensor);
 	if (ret)
 		goto out_unlock;
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index ef9875d3b79db116f9fb4f6d881a7979292c1792..c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -404,6 +404,35 @@ int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
 					  sleep_ms);
 }
 
+int inv_icm42600_set_wom(struct inv_icm42600_state *st, bool enable)
+{
+	unsigned int val;
+	int ret;
+
+	if (enable) {
+		/* enable WoM hardware */
+		val = INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM |
+		      INV_ICM42600_SMD_CONFIG_WOM_MODE;
+		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG, val);
+		if (ret)
+			return ret;
+		/* enable WoM interrupt */
+		ret = regmap_set_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
+				      INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
+	} else {
+		/* disable WoM interrupt */
+		ret = regmap_clear_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
+					INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
+		if (ret)
+			return ret;
+		/* disable WoM hardware */
+		val = INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF;
+		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG, val);
+	}
+
+	return ret;
+}
+
 int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
 			     unsigned int writeval, unsigned int *readval)
 {
@@ -543,11 +572,22 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
 {
 	struct inv_icm42600_state *st = _data;
 	struct device *dev = regmap_get_device(st->map);
-	unsigned int status;
+	unsigned int status, status2, status3;
 	int ret;
 
 	mutex_lock(&st->lock);
 
+	if (st->apex.on) {
+		/* read INT_STATUS2 and INT_STATUS3 in 1 operation */
+		ret = regmap_bulk_read(st->map, INV_ICM42600_REG_INT_STATUS2, st->buffer, 2);
+		if (ret)
+			goto out_unlock;
+		status2 = st->buffer[0];
+		status3 = st->buffer[1];
+		inv_icm42600_accel_handle_events(st->indio_accel, status2, status3,
+						 st->timestamp.accel);
+	}
+
 	ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);
 	if (ret)
 		goto out_unlock;
@@ -809,6 +849,13 @@ static int inv_icm42600_suspend(struct device *dev)
 			goto out_unlock;
 	}
 
+	/* disable APEX features */
+	if (st->apex.wom.enable) {
+		ret = inv_icm42600_set_wom(st, false);
+		if (ret)
+			goto out_unlock;
+	}
+
 	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
 					 INV_ICM42600_SENSOR_MODE_OFF, false,
 					 NULL);
@@ -850,6 +897,13 @@ static int inv_icm42600_resume(struct device *dev)
 	if (ret)
 		goto out_unlock;
 
+	/* restore APEX features */
+	if (st->apex.wom.enable) {
+		ret = inv_icm42600_set_wom(st, true);
+		if (ret)
+			goto out_unlock;
+	}
+
 	/* restore FIFO data streaming */
 	if (st->fifo.on) {
 		inv_sensors_timestamp_reset(&gyro_st->ts);

-- 
2.48.1

[PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jean-Baptiste Maneyrol via B4 Relay]

From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

When Wake-on-Motion is on, enable system wakeup and keep chip on for
waking up system with interrupt.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
 3 files changed, 63 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -151,6 +151,7 @@ struct inv_icm42600_apex {
  *  @map:		regmap pointer.
  *  @vdd_supply:	VDD voltage regulator for the chip.
  *  @vddio_supply:	I/O voltage regulator for the chip.
+ *  @irq:		chip irq.
  *  @orientation:	sensor chip orientation relative to main hardware.
  *  @conf:		chip sensors configurations.
  *  @suspended:		suspended sensors configuration.
@@ -168,6 +169,7 @@ struct inv_icm42600_state {
 	struct regmap *map;
 	struct regulator *vdd_supply;
 	struct regulator *vddio_supply;
+	int irq;
 	struct iio_mount_matrix orientation;
 	struct inv_icm42600_conf conf;
 	struct inv_icm42600_suspended suspended;
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
 	if (ret)
 		return ERR_PTR(ret);
 
+	/* accel events are wakeup capable */
+	device_set_wakeup_capable(&indio_dev->dev, true);
+
 	return indio_dev;
 }
 
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
 	mutex_init(&st->lock);
 	st->chip = chip;
 	st->map = regmap;
+	st->irq = irq;
 
 	ret = iio_read_mount_matrix(dev, &st->orientation);
 	if (ret) {
@@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
 static int inv_icm42600_suspend(struct device *dev)
 {
 	struct inv_icm42600_state *st = dev_get_drvdata(dev);
+	struct device *accel_dev;
+	bool wakeup;
+	int accel_conf;
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
 	st->suspended.gyro = st->conf.gyro.mode;
 	st->suspended.accel = st->conf.accel.mode;
 	st->suspended.temp = st->conf.temp_en;
-	if (pm_runtime_suspended(dev)) {
-		ret = 0;
-		goto out_unlock;
-	}
+	if (pm_runtime_suspended(dev))
+		return 0;
 
 	/* disable FIFO data streaming */
 	if (st->fifo.on) {
 		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
 				   INV_ICM42600_FIFO_CONFIG_BYPASS);
 		if (ret)
-			goto out_unlock;
+			return ret;
 	}
 
-	/* disable APEX features */
-	if (st->apex.wom.enable) {
-		ret = inv_icm42600_set_wom(st, false);
-		if (ret)
-			goto out_unlock;
+	/* keep chip on and wake-up capable if APEX and wakeup on */
+	accel_dev = &st->indio_accel->dev;
+	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
+
+	if (!wakeup) {
+		/* disable APEX features and accel if wakeup disabled */
+		if (st->apex.wom.enable) {
+			ret = inv_icm42600_set_wom(st, false);
+			if (ret)
+				return ret;
+		}
+		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
+	} else {
+		/* keep accel on and setup irq for wakeup */
+		accel_conf = st->conf.accel.mode;
+		enable_irq_wake(st->irq);
+		disable_irq(st->irq);
 	}
 
 	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
-					 INV_ICM42600_SENSOR_MODE_OFF, false,
-					 NULL);
+					 accel_conf, false, NULL);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	regulator_disable(st->vddio_supply);
+	/* disable vddio regulator if chip is sleeping */
+	if (!wakeup)
+		regulator_disable(st->vddio_supply);
 
-out_unlock:
-	mutex_unlock(&st->lock);
-	return ret;
+	return 0;
 }
 
 /*
@@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
 	struct inv_icm42600_state *st = dev_get_drvdata(dev);
 	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
 	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
+	struct device *accel_dev;
+	bool wakeup;
 	int ret;
 
-	mutex_lock(&st->lock);
+	guard(mutex)(&st->lock);
 
-	ret = inv_icm42600_enable_regulator_vddio(st);
-	if (ret)
-		goto out_unlock;
+	/* check wakeup capability */
+	accel_dev = &st->indio_accel->dev;
+	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
+
+	/* restore vddio if cut off or irq state */
+	if (!wakeup) {
+		ret = inv_icm42600_enable_regulator_vddio(st);
+		if (ret)
+			return ret;
+	} else {
+		enable_irq(st->irq);
+		disable_irq_wake(st->irq);
+	}
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
@@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
 					 st->suspended.accel,
 					 st->suspended.temp, NULL);
 	if (ret)
-		goto out_unlock;
+		return ret;
 
-	/* restore APEX features */
-	if (st->apex.wom.enable) {
-		ret = inv_icm42600_set_wom(st, true);
-		if (ret)
-			goto out_unlock;
+	/* restore APEX features if disabled */
+	if (!wakeup) {
+		if (st->apex.wom.enable) {
+			ret = inv_icm42600_set_wom(st, true);
+			if (ret)
+				return ret;
+		}
 	}
 
 	/* restore FIFO data streaming */
@@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
 		inv_sensors_timestamp_reset(&accel_st->ts);
 		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
 				   INV_ICM42600_FIFO_CONFIG_STREAM);
+		if (ret)
+			return ret;
 	}
 
-out_unlock:
-	mutex_unlock(&st->lock);
-	return ret;
+	return 0;
 }
 
 /* Runtime suspend will turn off sensors that are enabled by iio devices. */

-- 
2.48.1

Re: [PATCH 1/2] iio: imu: inv_icm42600: add WoM support

[Jonathan Cameron]

On Thu, 20 Feb 2025 21:52:06 +0100
Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> Add WoM as accel roc rising x|y|z event.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Some comments on unrelated bug (that this duplicates) inline.

Jonathan

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  47 +++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 264 ++++++++++++++++++++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  56 ++++-
>  4 files changed, 363 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 18787a43477b89db12caee597ab040af5c8f52d5..8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -135,6 +135,14 @@ struct inv_icm42600_suspended {
>  	bool temp;
>  };
>  
> +struct inv_icm42600_apex {
> +	unsigned int on;
> +	struct {
> +		bool enable;
> +		uint64_t value;
> +	} wom;
> +};
> +
>  /**
>   *  struct inv_icm42600_state - driver state variables
>   *  @lock:		lock for serializing multiple registers access.
> @@ -151,6 +159,7 @@ struct inv_icm42600_suspended {
>   *  @buffer:		data transfer buffer aligned for DMA.
>   *  @fifo:		FIFO management structure.
>   *  @timestamp:		interrupt timestamps.
> + *  @apex:		APEX features management.
>   */
>  struct inv_icm42600_state {
>  	struct mutex lock;
> @@ -164,12 +173,13 @@ struct inv_icm42600_state {
>  	struct inv_icm42600_suspended suspended;
>  	struct iio_dev *indio_gyro;
>  	struct iio_dev *indio_accel;
> -	uint8_t buffer[2] __aligned(IIO_DMA_MINALIGN);
> +	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>  	struct inv_icm42600_fifo fifo;
This being after buffer also looks like a problem (see below)
This needs fixing.  Just  move it before buffer in a separate patch
and all should be fine.
>  	struct {
>  		int64_t gyro;
>  		int64_t accel;
>  	} timestamp;
Maybe this as well.
> +	struct inv_icm42600_apex apex;

That seems unwise. It's in the region that DMA transactions may
scribble all over.  Move this before buffer or you'll get some
weird and wonderful bug reports sometime in the future!

>  };
>  
>  
> @@ -253,6 +263,18 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_REG_FIFO_COUNT			0x002E
>  #define INV_ICM42600_REG_FIFO_DATA			0x0030
>  
> +#define INV_ICM42600_REG_INT_STATUS2			0x0037
> +#define INV_ICM42600_INT_STATUS2_SMD_INT		BIT(3)
> +#define INV_ICM42600_INT_STATUS2_WOM_INT		GENMASK(2, 0)
> +
> +#define INV_ICM42600_REG_INT_STATUS3			0x0038
> +#define INV_ICM42600_INT_STATUS3_STEP_DET_INT		BIT(5)
> +#define INV_ICM42600_INT_STATUS3_STEP_CNT_OVF_INT	BIT(4)
> +#define INV_ICM42600_INT_STATUS3_TILT_DET_INT		BIT(3)
> +#define INV_ICM42600_INT_STATUS3_WAKE_INT		BIT(2)
> +#define INV_ICM42600_INT_STATUS3_SLEEP_INT		BIT(1)
> +#define INV_ICM42600_INT_STATUS3_TAP_DET_INT		BIT(0)
> +
>  #define INV_ICM42600_REG_SIGNAL_PATH_RESET		0x004B
>  #define INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN	BIT(6)
>  #define INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET	BIT(5)
> @@ -309,6 +331,14 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_TMST_CONFIG_TMST_FSYNC_EN		BIT(1)
>  #define INV_ICM42600_TMST_CONFIG_TMST_EN		BIT(0)
>  
> +#define INV_ICM42600_REG_SMD_CONFIG			0x0057
> +#define INV_ICM42600_SMD_CONFIG_WOM_INT_MODE		BIT(3)
> +#define INV_ICM42600_SMD_CONFIG_WOM_MODE		BIT(2)
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF		0x00
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM		0x01
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_SHORT		0x02
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_LONG		0x03
> +
>  #define INV_ICM42600_REG_FIFO_CONFIG1			0x005F
>  #define INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD	BIT(6)
>  #define INV_ICM42600_FIFO_CONFIG1_WM_GT_TH		BIT(5)
> @@ -338,6 +368,11 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_INT_SOURCE0_FIFO_FULL_INT1_EN	BIT(1)
>  #define INV_ICM42600_INT_SOURCE0_UI_AGC_RDY_INT1_EN	BIT(0)
>  
> +#define INV_ICM42600_REG_INT_SOURCE1			0x0066
> +#define INV_ICM42600_INT_SOURCE1_I3C_ERROR_INT1_EN	BIT(6)
> +#define INV_ICM42600_INT_SOURCE1_SMD_INT1_EN		BIT(3)
> +#define INV_ICM42600_INT_SOURCE1_WOM_INT1_EN		GENMASK(2, 0)
> +
>  #define INV_ICM42600_REG_WHOAMI				0x0075
>  #define INV_ICM42600_WHOAMI_ICM42600			0x40
>  #define INV_ICM42600_WHOAMI_ICM42602			0x41
> @@ -373,6 +408,10 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN		BIT(0)
>  
>  /* User bank 4 (MSB 0x40) */
> +#define INV_ICM42600_REG_ACCEL_WOM_X_THR		0x404A
> +#define INV_ICM42600_REG_ACCEL_WOM_Y_THR		0x404B
> +#define INV_ICM42600_REG_ACCEL_WOM_Z_THR		0x404C
> +
>  #define INV_ICM42600_REG_INT_SOURCE8			0x404F
>  #define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN		BIT(5)
>  #define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN		BIT(4)
> @@ -423,6 +462,8 @@ int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
>  int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
>  			       unsigned int *sleep_ms);
>  
> +int inv_icm42600_set_wom(struct inv_icm42600_state *st, bool enable);
> +
>  int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
>  			     unsigned int writeval, unsigned int *readval);
>  
> @@ -437,4 +478,8 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st);
>  
>  int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev);
>  
> +void inv_icm42600_accel_handle_events(struct iio_dev *indio_dev,
> +				      unsigned int status2, unsigned int status3,
> +				      int64_t timestamp);
> +
>  #endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 388520ec60b5c5d21b16717978ebf330e38aa1fe..8ce2276b3edc61cc1ea26810198dd0057054ec48 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c


> +
> +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev, bool enable)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42600_sensor_state *accel_st = iio_priv(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
> +	struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> +	unsigned int sleep_ms = 0;
> +	int ret;
> +
> +	if (enable) {

Not a lot of shared logic. Maybe split into enable and siable functions.

> +		ret = pm_runtime_resume_and_get(pdev);
> +		if (ret)
> +			return ret;
> +		scoped_guard(mutex, &st->lock) {
> +			/* turn on accel sensor */
> +			conf.mode = accel_st->power_mode;
> +			conf.filter = accel_st->filter;
> +			ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> +			if (ret)
> +				goto error_suspend;
> +		}
> +		if (sleep_ms)
> +			msleep(sleep_ms);
> +		scoped_guard(mutex, &st->lock) {
> +			ret = inv_icm42600_set_wom(st, true);
> +			if (ret)
> +				goto error_suspend;
> +			st->apex.on++;
> +			st->apex.wom.enable = true;
> +		}
	return 0;

> +	} else {
> +		scoped_guard(mutex, &st->lock) {
> +			st->apex.wom.enable = false;
> +			st->apex.on--;
> +			ret = inv_icm42600_set_wom(st, false);
> +			if (ret)
> +				goto error_suspend;
> +			/* turn off accel sensor if not used */
> +			if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
> +				conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
> +				ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> +				if (ret)
> +					goto error_suspend;
> +			}
> +		}
> +		if (sleep_ms)
> +			msleep(sleep_ms);

With return above, you could share this with the error handling.

> +		pm_runtime_mark_last_busy(pdev);
> +		pm_runtime_put_autosuspend(pdev);
> +	}
> +
> +	return 0;
> +
> +error_suspend:
> +	pm_runtime_mark_last_busy(pdev);
> +	pm_runtime_put_autosuspend(pdev);
> +	return ret;
> +}

> +static int inv_icm42600_accel_read_event_value(struct iio_dev *indio_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	u32 rem;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* handle WoM (roc rising) event value */
> +	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {

Similar to below. Consider flipping logic.

> +		/* return value in micro */
> +		*val = div_u64_rem(st->apex.wom.value, 1000000U, &rem);
> +		*val2 = rem;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int inv_icm42600_accel_write_event_value(struct iio_dev *indio_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	uint64_t value;
> +	unsigned int accel_hz, accel_uhz;
> +	int ret;
> +
> +	/* handle WoM (roc rising) event value */
> +	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {

If you don't plan to add other event types soon I'd be tempted to flip
the logic of this to save in indent and exit early in the error case.

> +		if (val < 0 || val2 < 0)
> +			return -EINVAL;
> +		value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
> +		pm_runtime_get_sync(dev);
> +		scoped_guard(mutex, &st->lock) {
> +			ret = inv_icm42600_accel_read_odr(st, &accel_hz, &accel_uhz);
> +			if (ret >= 0)
> +				ret = inv_icm42600_accel_set_wom_threshold(st, value,
> +									   accel_hz, accel_uhz);
> +		}
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +		return ret;
> +	}
> +
> +	return -EINVAL;
> +}

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index ef9875d3b79db116f9fb4f6d881a7979292c1792..c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -404,6 +404,35 @@ int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
>  					  sleep_ms);
>  }
>  
> +int inv_icm42600_set_wom(struct inv_icm42600_state *st, bool enable)
> +{
> +	unsigned int val;
> +	int ret;

Given the set and disable code paths have no shared code, maybe split into
two functions?

> +
> +	if (enable) {
> +		/* enable WoM hardware */
> +		val = INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM |
> +		      INV_ICM42600_SMD_CONFIG_WOM_MODE;
> +		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG, val);
		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG,
				   INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM |
				   INV_ICM42600_SMD_CONFIG_WOM_MODE);
Seems not to loose any readabilty and avoids need for local variable.


> +		if (ret)
> +			return ret;
> +		/* enable WoM interrupt */
> +		ret = regmap_set_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
> +				      INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
return regmap_write()
> +	} else {
> +		/* disable WoM interrupt */
> +		ret = regmap_clear_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
> +					INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
> +		if (ret)
> +			return ret;
> +		/* disable WoM hardware */
> +		val = INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF;
> +		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG, val);
return regmap_write()
and no need for val as I don't think that adds any readability advantages here.

> +	}
> +
> +	return ret;
> +}
> +
>  int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
>  			     unsigned int writeval, unsigned int *readval)
>  {
> @@ -543,11 +572,22 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
>  {
>  	struct inv_icm42600_state *st = _data;
>  	struct device *dev = regmap_get_device(st->map);
> -	unsigned int status;
> +	unsigned int status, status2, status3;
>  	int ret;
>  
>  	mutex_lock(&st->lock);
>  
> +	if (st->apex.on) {

I'd drag the declaration of additional local variables in here.

> +		/* read INT_STATUS2 and INT_STATUS3 in 1 operation */
> +		ret = regmap_bulk_read(st->map, INV_ICM42600_REG_INT_STATUS2, st->buffer, 2);
> +		if (ret)
> +			goto out_unlock;
> +		status2 = st->buffer[0];
> +		status3 = st->buffer[1];
> +		inv_icm42600_accel_handle_events(st->indio_accel, status2, status3,
> +						 st->timestamp.accel);
> +	}
> +

Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jonathan Cameron]

On Thu, 20 Feb 2025 21:52:07 +0100
Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> When Wake-on-Motion is on, enable system wakeup and keep chip on for
> waking up system with interrupt.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Hi Jean-Baptiste,

A few comments inline.

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
>  3 files changed, 63 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
>   *  @map:		regmap pointer.
>   *  @vdd_supply:	VDD voltage regulator for the chip.
>   *  @vddio_supply:	I/O voltage regulator for the chip.
> + *  @irq:		chip irq.

Maybe say a little on what the variable is used for. It's not otherwise
obvious why we need it.  Also, does this chip really only have one irq line?
Looks like there are two. So the drivers should be fixed to cope with the
only one wired being irq2  unless I've found the wrong datasheet which is
certainly possible.


>   *  @orientation:	sensor chip orientation relative to main hardware.
>   *  @conf:		chip sensors configurations.
>   *  @suspended:		suspended sensors configuration.
> @@ -168,6 +169,7 @@ struct inv_icm42600_state {
>  	struct regmap *map;
>  	struct regulator *vdd_supply;
>  	struct regulator *vddio_supply;
> +	int irq;
>  	struct iio_mount_matrix orientation;
>  	struct inv_icm42600_conf conf;
>  	struct inv_icm42600_suspended suspended;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	/* accel events are wakeup capable */
> +	device_set_wakeup_capable(&indio_dev->dev, true);
> +
>  	return indio_dev;
>  }
>  
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
>  	mutex_init(&st->lock);
>  	st->chip = chip;
>  	st->map = regmap;
> +	st->irq = irq;
>  
>  	ret = iio_read_mount_matrix(dev, &st->orientation);
>  	if (ret) {
> @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
>  static int inv_icm42600_suspend(struct device *dev)
>  {
>  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	struct device *accel_dev;
> +	bool wakeup;
> +	int accel_conf;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

As below. Pull these guard changes out as a precursor patch. That will make
it easier to spot the real changes here.

>  
>  	st->suspended.gyro = st->conf.gyro.mode;
>  	st->suspended.accel = st->conf.accel.mode;
>  	st->suspended.temp = st->conf.temp_en;
> -	if (pm_runtime_suspended(dev)) {
> -		ret = 0;
> -		goto out_unlock;
> -	}
> +	if (pm_runtime_suspended(dev))
> +		return 0;
>  
>  	/* disable FIFO data streaming */
>  	if (st->fifo.on) {
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  	}
>  
> -	/* disable APEX features */
> -	if (st->apex.wom.enable) {
> -		ret = inv_icm42600_set_wom(st, false);
> -		if (ret)
> -			goto out_unlock;
> +	/* keep chip on and wake-up capable if APEX and wakeup on */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +	if (!wakeup) {
> +		/* disable APEX features and accel if wakeup disabled */
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42600_set_wom(st, false);
> +			if (ret)
> +				return ret;
> +		}
> +		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> +	} else {
> +		/* keep accel on and setup irq for wakeup */
> +		accel_conf = st->conf.accel.mode;
> +		enable_irq_wake(st->irq);
> +		disable_irq(st->irq);
>  	}
>  
>  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> -					 INV_ICM42600_SENSOR_MODE_OFF, false,
> -					 NULL);
> +					 accel_conf, false, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
> -	regulator_disable(st->vddio_supply);
> +	/* disable vddio regulator if chip is sleeping */
> +	if (!wakeup)
> +		regulator_disable(st->vddio_supply);
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
>  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
>  	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
>  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> +	struct device *accel_dev;
> +	bool wakeup;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

Good change.  But separate patch as not related to most of what is going on here.


>  
> -	ret = inv_icm42600_enable_regulator_vddio(st);
> -	if (ret)
> -		goto out_unlock;
> +	/* check wakeup capability */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +	/* restore vddio if cut off or irq state */
> +	if (!wakeup) {
> +		ret = inv_icm42600_enable_regulator_vddio(st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		enable_irq(st->irq);
> +		disable_irq_wake(st->irq);
> +	}
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_active(dev);
> @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
>  					 st->suspended.accel,
>  					 st->suspended.temp, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
> -	/* restore APEX features */
> -	if (st->apex.wom.enable) {
> -		ret = inv_icm42600_set_wom(st, true);
> -		if (ret)
> -			goto out_unlock;
> +	/* restore APEX features if disabled */
> +	if (!wakeup) {
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42600_set_wom(st, true);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	/* restore FIFO data streaming */
> @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
>  		inv_sensors_timestamp_reset(&accel_st->ts);
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_STREAM);
> +		if (ret)
> +			return ret;
>  	}
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> 

Re: [PATCH 1/2] iio: imu: inv_icm42600: add WoM support

[Jean-Baptiste Maneyrol]

Hello Jonathan,

first I want to apologize for not being able to reply in-line properly, really sorry about that.

No problem for all your requests, except for flipping the logic of read/write event since we are planning to add very soon support for other events like steps and tilt.

I will send a V2 with all the changes.

Thanks,
JB

________________________________________
From: Jonathan Cameron <jic23@kernel.org>
Sent: Saturday, February 22, 2025 17:14
To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>
Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] iio: imu: inv_icm42600: add WoM support
 
This Message Is From an External Sender
This message came from outside your organization.
 
On Thu, 20 Feb 2025 21:52:06 +0100
Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> Add WoM as accel roc rising x|y|z event.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Some comments on unrelated bug (that this duplicates) inline.

Jonathan

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  47 +++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 264 ++++++++++++++++++++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  56 ++++-
>  4 files changed, 363 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 18787a43477b89db12caee597ab040af5c8f52d5..8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -135,6 +135,14 @@ struct inv_icm42600_suspended {
>  	bool temp;
>  };
>  
> +struct inv_icm42600_apex {
> +	unsigned int on;
> +	struct {
> +		bool enable;
> +		uint64_t value;
> +	} wom;
> +};
> +
>  /**
>   *  struct inv_icm42600_state - driver state variables
>   *  @lock:		lock for serializing multiple registers access.
> @@ -151,6 +159,7 @@ struct inv_icm42600_suspended {
>   *  @buffer:		data transfer buffer aligned for DMA.
>   *  @fifo:		FIFO management structure.
>   *  @timestamp:		interrupt timestamps.
> + *  @apex:		APEX features management.
>   */
>  struct inv_icm42600_state {
>  	struct mutex lock;
> @@ -164,12 +173,13 @@ struct inv_icm42600_state {
>  	struct inv_icm42600_suspended suspended;
>  	struct iio_dev *indio_gyro;
>  	struct iio_dev *indio_accel;
> -	uint8_t buffer[2] __aligned(IIO_DMA_MINALIGN);
> +	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>  	struct inv_icm42600_fifo fifo;
This being after buffer also looks like a problem (see below)
This needs fixing.  Just  move it before buffer in a separate patch
and all should be fine.
>  	struct {
>  		int64_t gyro;
>  		int64_t accel;
>  	} timestamp;
Maybe this as well.
> +	struct inv_icm42600_apex apex;

That seems unwise. It's in the region that DMA transactions may
scribble all over.  Move this before buffer or you'll get some
weird and wonderful bug reports sometime in the future!

>  };
>  
>  
> @@ -253,6 +263,18 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_REG_FIFO_COUNT			0x002E
>  #define INV_ICM42600_REG_FIFO_DATA			0x0030
>  
> +#define INV_ICM42600_REG_INT_STATUS2			0x0037
> +#define INV_ICM42600_INT_STATUS2_SMD_INT		BIT(3)
> +#define INV_ICM42600_INT_STATUS2_WOM_INT		GENMASK(2, 0)
> +
> +#define INV_ICM42600_REG_INT_STATUS3			0x0038
> +#define INV_ICM42600_INT_STATUS3_STEP_DET_INT		BIT(5)
> +#define INV_ICM42600_INT_STATUS3_STEP_CNT_OVF_INT	BIT(4)
> +#define INV_ICM42600_INT_STATUS3_TILT_DET_INT		BIT(3)
> +#define INV_ICM42600_INT_STATUS3_WAKE_INT		BIT(2)
> +#define INV_ICM42600_INT_STATUS3_SLEEP_INT		BIT(1)
> +#define INV_ICM42600_INT_STATUS3_TAP_DET_INT		BIT(0)
> +
>  #define INV_ICM42600_REG_SIGNAL_PATH_RESET		0x004B
>  #define INV_ICM42600_SIGNAL_PATH_RESET_DMP_INIT_EN	BIT(6)
>  #define INV_ICM42600_SIGNAL_PATH_RESET_DMP_MEM_RESET	BIT(5)
> @@ -309,6 +331,14 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_TMST_CONFIG_TMST_FSYNC_EN		BIT(1)
>  #define INV_ICM42600_TMST_CONFIG_TMST_EN		BIT(0)
>  
> +#define INV_ICM42600_REG_SMD_CONFIG			0x0057
> +#define INV_ICM42600_SMD_CONFIG_WOM_INT_MODE		BIT(3)
> +#define INV_ICM42600_SMD_CONFIG_WOM_MODE		BIT(2)
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF		0x00
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM		0x01
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_SHORT		0x02
> +#define INV_ICM42600_SMD_CONFIG_SMD_MODE_LONG		0x03
> +
>  #define INV_ICM42600_REG_FIFO_CONFIG1			0x005F
>  #define INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD	BIT(6)
>  #define INV_ICM42600_FIFO_CONFIG1_WM_GT_TH		BIT(5)
> @@ -338,6 +368,11 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_INT_SOURCE0_FIFO_FULL_INT1_EN	BIT(1)
>  #define INV_ICM42600_INT_SOURCE0_UI_AGC_RDY_INT1_EN	BIT(0)
>  
> +#define INV_ICM42600_REG_INT_SOURCE1			0x0066
> +#define INV_ICM42600_INT_SOURCE1_I3C_ERROR_INT1_EN	BIT(6)
> +#define INV_ICM42600_INT_SOURCE1_SMD_INT1_EN		BIT(3)
> +#define INV_ICM42600_INT_SOURCE1_WOM_INT1_EN		GENMASK(2, 0)
> +
>  #define INV_ICM42600_REG_WHOAMI				0x0075
>  #define INV_ICM42600_WHOAMI_ICM42600			0x40
>  #define INV_ICM42600_WHOAMI_ICM42602			0x41
> @@ -373,6 +408,10 @@ struct inv_icm42600_sensor_state {
>  #define INV_ICM42600_INTF_CONFIG6_I3C_SDR_EN		BIT(0)
>  
>  /* User bank 4 (MSB 0x40) */
> +#define INV_ICM42600_REG_ACCEL_WOM_X_THR		0x404A
> +#define INV_ICM42600_REG_ACCEL_WOM_Y_THR		0x404B
> +#define INV_ICM42600_REG_ACCEL_WOM_Z_THR		0x404C
> +
>  #define INV_ICM42600_REG_INT_SOURCE8			0x404F
>  #define INV_ICM42600_INT_SOURCE8_FSYNC_IBI_EN		BIT(5)
>  #define INV_ICM42600_INT_SOURCE8_PLL_RDY_IBI_EN		BIT(4)
> @@ -423,6 +462,8 @@ int inv_icm42600_set_gyro_conf(struct inv_icm42600_state *st,
>  int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
>  			       unsigned int *sleep_ms);
>  
> +int inv_icm42600_set_wom(struct inv_icm42600_state *st, bool enable);
> +
>  int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
>  			     unsigned int writeval, unsigned int *readval);
>  
> @@ -437,4 +478,8 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st);
>  
>  int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev);
>  
> +void inv_icm42600_accel_handle_events(struct iio_dev *indio_dev,
> +				      unsigned int status2, unsigned int status3,
> +				      int64_t timestamp);
> +
>  #endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 388520ec60b5c5d21b16717978ebf330e38aa1fe..8ce2276b3edc61cc1ea26810198dd0057054ec48 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c


> +
> +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev, bool enable)
> +{
> +	struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct inv_icm42600_sensor_state *accel_st = iio_priv(indio_dev);
> +	struct device *pdev = regmap_get_device(st->map);
> +	struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> +	unsigned int sleep_ms = 0;
> +	int ret;
> +
> +	if (enable) {

Not a lot of shared logic. Maybe split into enable and siable functions.

> +		ret = pm_runtime_resume_and_get(pdev);
> +		if (ret)
> +			return ret;
> +		scoped_guard(mutex, &st->lock) {
> +			/* turn on accel sensor */
> +			conf.mode = accel_st->power_mode;
> +			conf.filter = accel_st->filter;
> +			ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> +			if (ret)
> +				goto error_suspend;
> +		}
> +		if (sleep_ms)
> +			msleep(sleep_ms);
> +		scoped_guard(mutex, &st->lock) {
> +			ret = inv_icm42600_set_wom(st, true);
> +			if (ret)
> +				goto error_suspend;
> +			st->apex.on++;
> +			st->apex.wom.enable = true;
> +		}
	return 0;

> +	} else {
> +		scoped_guard(mutex, &st->lock) {
> +			st->apex.wom.enable = false;
> +			st->apex.on--;
> +			ret = inv_icm42600_set_wom(st, false);
> +			if (ret)
> +				goto error_suspend;
> +			/* turn off accel sensor if not used */
> +			if (!st->apex.on && !iio_buffer_enabled(indio_dev)) {
> +				conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
> +				ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_ms);
> +				if (ret)
> +					goto error_suspend;
> +			}
> +		}
> +		if (sleep_ms)
> +			msleep(sleep_ms);

With return above, you could share this with the error handling.

> +		pm_runtime_mark_last_busy(pdev);
> +		pm_runtime_put_autosuspend(pdev);
> +	}
> +
> +	return 0;
> +
> +error_suspend:
> +	pm_runtime_mark_last_busy(pdev);
> +	pm_runtime_put_autosuspend(pdev);
> +	return ret;
> +}

> +static int inv_icm42600_accel_read_event_value(struct iio_dev *indio_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	u32 rem;
> +
> +	guard(mutex)(&st->lock);
> +
> +	/* handle WoM (roc rising) event value */
> +	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {

Similar to below. Consider flipping logic.

> +		/* return value in micro */
> +		*val = div_u64_rem(st->apex.wom.value, 1000000U, &rem);
> +		*val2 = rem;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int inv_icm42600_accel_write_event_value(struct iio_dev *indio_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 inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +	struct device *dev = regmap_get_device(st->map);
> +	uint64_t value;
> +	unsigned int accel_hz, accel_uhz;
> +	int ret;
> +
> +	/* handle WoM (roc rising) event value */
> +	if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {

If you don't plan to add other event types soon I'd be tempted to flip
the logic of this to save in indent and exit early in the error case.

> +		if (val < 0 || val2 < 0)
> +			return -EINVAL;
> +		value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
> +		pm_runtime_get_sync(dev);
> +		scoped_guard(mutex, &st->lock) {
> +			ret = inv_icm42600_accel_read_odr(st, &accel_hz, &accel_uhz);
> +			if (ret >= 0)
> +				ret = inv_icm42600_accel_set_wom_threshold(st, value,
> +									   accel_hz, accel_uhz);
> +		}
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +		return ret;
> +	}
> +
> +	return -EINVAL;
> +}

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index ef9875d3b79db116f9fb4f6d881a7979292c1792..c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -404,6 +404,35 @@ int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
>  					  sleep_ms);
>  }
>  
> +int inv_icm42600_set_wom(struct inv_icm42600_state *st, bool enable)
> +{
> +	unsigned int val;
> +	int ret;

Given the set and disable code paths have no shared code, maybe split into
two functions?

> +
> +	if (enable) {
> +		/* enable WoM hardware */
> +		val = INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM |
> +		      INV_ICM42600_SMD_CONFIG_WOM_MODE;
> +		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG, val);
		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG,
				   INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM |
				   INV_ICM42600_SMD_CONFIG_WOM_MODE);
Seems not to loose any readabilty and avoids need for local variable.


> +		if (ret)
> +			return ret;
> +		/* enable WoM interrupt */
> +		ret = regmap_set_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
> +				      INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
return regmap_write()
> +	} else {
> +		/* disable WoM interrupt */
> +		ret = regmap_clear_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
> +					INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
> +		if (ret)
> +			return ret;
> +		/* disable WoM hardware */
> +		val = INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF;
> +		ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG, val);
return regmap_write()
and no need for val as I don't think that adds any readability advantages here.

> +	}
> +
> +	return ret;
> +}
> +
>  int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
>  			     unsigned int writeval, unsigned int *readval)
>  {
> @@ -543,11 +572,22 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
>  {
>  	struct inv_icm42600_state *st = _data;
>  	struct device *dev = regmap_get_device(st->map);
> -	unsigned int status;
> +	unsigned int status, status2, status3;
>  	int ret;
>  
>  	mutex_lock(&st->lock);
>  
> +	if (st->apex.on) {

I'd drag the declaration of additional local variables in here.

> +		/* read INT_STATUS2 and INT_STATUS3 in 1 operation */
> +		ret = regmap_bulk_read(st->map, INV_ICM42600_REG_INT_STATUS2, st->buffer, 2);
> +		if (ret)
> +			goto out_unlock;
> +		status2 = st->buffer[0];
> +		status3 = st->buffer[1];
> +		inv_icm42600_accel_handle_events(st->indio_accel, status2, status3,
> +						 st->timestamp.accel);
> +	}
> +

Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jean-Baptiste Maneyrol]

Hello Jonathan,

still sorry for not being able to reply in-line.

No problem for all changes, except handling the 2 interrupt lines. Currently our driver only supports INT1 and cannot work with INT2, and this is not related to Wake-on-Motion feature. This is something we could add in another series, and I prefer to have a dedicated series for that.

For the mutex rework, I will delete them. This is something we can also do in another dedicated patch not inside this series.

Is that OK for you?

Thanks,
JB

________________________________________
From: Jonathan Cameron <jic23@kernel.org>
Sent: Saturday, February 22, 2025 17:22
To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>
Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
 
This Message Is From an External Sender
This message came from outside your organization.
 
On Thu, 20 Feb 2025 21:52:07 +0100
Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> When Wake-on-Motion is on, enable system wakeup and keep chip on for
> waking up system with interrupt.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Hi Jean-Baptiste,

A few comments inline.

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
>  3 files changed, 63 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
>   *  @map:		regmap pointer.
>   *  @vdd_supply:	VDD voltage regulator for the chip.
>   *  @vddio_supply:	I/O voltage regulator for the chip.
> + *  @irq:		chip irq.

Maybe say a little on what the variable is used for. It's not otherwise
obvious why we need it.  Also, does this chip really only have one irq line?
Looks like there are two. So the drivers should be fixed to cope with the
only one wired being irq2  unless I've found the wrong datasheet which is
certainly possible.


>   *  @orientation:	sensor chip orientation relative to main hardware.
>   *  @conf:		chip sensors configurations.
>   *  @suspended:		suspended sensors configuration.
> @@ -168,6 +169,7 @@ struct inv_icm42600_state {
>  	struct regmap *map;
>  	struct regulator *vdd_supply;
>  	struct regulator *vddio_supply;
> +	int irq;
>  	struct iio_mount_matrix orientation;
>  	struct inv_icm42600_conf conf;
>  	struct inv_icm42600_suspended suspended;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	/* accel events are wakeup capable */
> +	device_set_wakeup_capable(&indio_dev->dev, true);
> +
>  	return indio_dev;
>  }
>  
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
>  	mutex_init(&st->lock);
>  	st->chip = chip;
>  	st->map = regmap;
> +	st->irq = irq;
>  
>  	ret = iio_read_mount_matrix(dev, &st->orientation);
>  	if (ret) {
> @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
>  static int inv_icm42600_suspend(struct device *dev)
>  {
>  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> +	struct device *accel_dev;
> +	bool wakeup;
> +	int accel_conf;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

As below. Pull these guard changes out as a precursor patch. That will make
it easier to spot the real changes here.

>  
>  	st->suspended.gyro = st->conf.gyro.mode;
>  	st->suspended.accel = st->conf.accel.mode;
>  	st->suspended.temp = st->conf.temp_en;
> -	if (pm_runtime_suspended(dev)) {
> -		ret = 0;
> -		goto out_unlock;
> -	}
> +	if (pm_runtime_suspended(dev))
> +		return 0;
>  
>  	/* disable FIFO data streaming */
>  	if (st->fifo.on) {
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
>  		if (ret)
> -			goto out_unlock;
> +			return ret;
>  	}
>  
> -	/* disable APEX features */
> -	if (st->apex.wom.enable) {
> -		ret = inv_icm42600_set_wom(st, false);
> -		if (ret)
> -			goto out_unlock;
> +	/* keep chip on and wake-up capable if APEX and wakeup on */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +	if (!wakeup) {
> +		/* disable APEX features and accel if wakeup disabled */
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42600_set_wom(st, false);
> +			if (ret)
> +				return ret;
> +		}
> +		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> +	} else {
> +		/* keep accel on and setup irq for wakeup */
> +		accel_conf = st->conf.accel.mode;
> +		enable_irq_wake(st->irq);
> +		disable_irq(st->irq);
>  	}
>  
>  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> -					 INV_ICM42600_SENSOR_MODE_OFF, false,
> -					 NULL);
> +					 accel_conf, false, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
> -	regulator_disable(st->vddio_supply);
> +	/* disable vddio regulator if chip is sleeping */
> +	if (!wakeup)
> +		regulator_disable(st->vddio_supply);
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /*
> @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
>  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
>  	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
>  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> +	struct device *accel_dev;
> +	bool wakeup;
>  	int ret;
>  
> -	mutex_lock(&st->lock);
> +	guard(mutex)(&st->lock);

Good change.  But separate patch as not related to most of what is going on here.


>  
> -	ret = inv_icm42600_enable_regulator_vddio(st);
> -	if (ret)
> -		goto out_unlock;
> +	/* check wakeup capability */
> +	accel_dev = &st->indio_accel->dev;
> +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> +
> +	/* restore vddio if cut off or irq state */
> +	if (!wakeup) {
> +		ret = inv_icm42600_enable_regulator_vddio(st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		enable_irq(st->irq);
> +		disable_irq_wake(st->irq);
> +	}
>  
>  	pm_runtime_disable(dev);
>  	pm_runtime_set_active(dev);
> @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
>  					 st->suspended.accel,
>  					 st->suspended.temp, NULL);
>  	if (ret)
> -		goto out_unlock;
> +		return ret;
>  
> -	/* restore APEX features */
> -	if (st->apex.wom.enable) {
> -		ret = inv_icm42600_set_wom(st, true);
> -		if (ret)
> -			goto out_unlock;
> +	/* restore APEX features if disabled */
> +	if (!wakeup) {
> +		if (st->apex.wom.enable) {
> +			ret = inv_icm42600_set_wom(st, true);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	/* restore FIFO data streaming */
> @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
>  		inv_sensors_timestamp_reset(&accel_st->ts);
>  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
>  				   INV_ICM42600_FIFO_CONFIG_STREAM);
> +		if (ret)
> +			return ret;
>  	}
>  
> -out_unlock:
> -	mutex_unlock(&st->lock);
> -	return ret;
> +	return 0;
>  }
>  
>  /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> 

Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jonathan Cameron]

On Tue, 11 Mar 2025 15:31:44 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello Jonathan,
> 
> still sorry for not being able to reply in-line.
> 
> No problem for all changes, except handling the 2 interrupt lines. Currently our driver only supports INT1 and cannot work with INT2, and this is not related to Wake-on-Motion feature. This is something we could add in another series, and I prefer to have a dedicated series for that.

You should check it isn't INT2 that you are getting via spi->irq etc.
Absolutely fine to reject that in the driver but you need to be
sure you have what you think you do and the spi->irq etc don't
provide that surety - they just give you the first one in the
dt-binding.

Jonathan


> 
> For the mutex rework, I will delete them. This is something we can also do in another dedicated patch not inside this series.
> 
> Is that OK for you?
> 
> Thanks,
> JB
> 
> ________________________________________
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, February 22, 2025 17:22
> To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>
> Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
>  
> This Message Is From an External Sender
> This message came from outside your organization.
>  
> On Thu, 20 Feb 2025 21:52:07 +0100
> Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:
> 
> > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> > 
> > When Wake-on-Motion is on, enable system wakeup and keep chip on for
> > waking up system with interrupt.
> > 
> > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>  
> Hi Jean-Baptiste,
> 
> A few comments inline.
> 
> > ---
> >  drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
> >  3 files changed, 63 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
> >   *  @map:		regmap pointer.
> >   *  @vdd_supply:	VDD voltage regulator for the chip.
> >   *  @vddio_supply:	I/O voltage regulator for the chip.
> > + *  @irq:		chip irq.  
> 
> Maybe say a little on what the variable is used for. It's not otherwise
> obvious why we need it.  Also, does this chip really only have one irq line?
> Looks like there are two. So the drivers should be fixed to cope with the
> only one wired being irq2  unless I've found the wrong datasheet which is
> certainly possible.
> 
> 
> >   *  @orientation:	sensor chip orientation relative to main hardware.
> >   *  @conf:		chip sensors configurations.
> >   *  @suspended:		suspended sensors configuration.
> > @@ -168,6 +169,7 @@ struct inv_icm42600_state {
> >  	struct regmap *map;
> >  	struct regulator *vdd_supply;
> >  	struct regulator *vddio_supply;
> > +	int irq;
> >  	struct iio_mount_matrix orientation;
> >  	struct inv_icm42600_conf conf;
> >  	struct inv_icm42600_suspended suspended;
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> >  	if (ret)
> >  		return ERR_PTR(ret);
> >  
> > +	/* accel events are wakeup capable */
> > +	device_set_wakeup_capable(&indio_dev->dev, true);
> > +
> >  	return indio_dev;
> >  }
> >  
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> >  	mutex_init(&st->lock);
> >  	st->chip = chip;
> >  	st->map = regmap;
> > +	st->irq = irq;
> >  
> >  	ret = iio_read_mount_matrix(dev, &st->orientation);
> >  	if (ret) {
> > @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
> >  static int inv_icm42600_suspend(struct device *dev)
> >  {
> >  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > +	struct device *accel_dev;
> > +	bool wakeup;
> > +	int accel_conf;
> >  	int ret;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);  
> 
> As below. Pull these guard changes out as a precursor patch. That will make
> it easier to spot the real changes here.
> 
> >  
> >  	st->suspended.gyro = st->conf.gyro.mode;
> >  	st->suspended.accel = st->conf.accel.mode;
> >  	st->suspended.temp = st->conf.temp_en;
> > -	if (pm_runtime_suspended(dev)) {
> > -		ret = 0;
> > -		goto out_unlock;
> > -	}
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> >  
> >  	/* disable FIFO data streaming */
> >  	if (st->fifo.on) {
> >  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> >  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
> >  		if (ret)
> > -			goto out_unlock;
> > +			return ret;
> >  	}
> >  
> > -	/* disable APEX features */
> > -	if (st->apex.wom.enable) {
> > -		ret = inv_icm42600_set_wom(st, false);
> > -		if (ret)
> > -			goto out_unlock;
> > +	/* keep chip on and wake-up capable if APEX and wakeup on */
> > +	accel_dev = &st->indio_accel->dev;
> > +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > +
> > +	if (!wakeup) {
> > +		/* disable APEX features and accel if wakeup disabled */
> > +		if (st->apex.wom.enable) {
> > +			ret = inv_icm42600_set_wom(st, false);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> > +	} else {
> > +		/* keep accel on and setup irq for wakeup */
> > +		accel_conf = st->conf.accel.mode;
> > +		enable_irq_wake(st->irq);
> > +		disable_irq(st->irq);
> >  	}
> >  
> >  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> > -					 INV_ICM42600_SENSOR_MODE_OFF, false,
> > -					 NULL);
> > +					 accel_conf, false, NULL);
> >  	if (ret)
> > -		goto out_unlock;
> > +		return ret;
> >  
> > -	regulator_disable(st->vddio_supply);
> > +	/* disable vddio regulator if chip is sleeping */
> > +	if (!wakeup)
> > +		regulator_disable(st->vddio_supply);
> >  
> > -out_unlock:
> > -	mutex_unlock(&st->lock);
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
> >  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> >  	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
> >  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> > +	struct device *accel_dev;
> > +	bool wakeup;
> >  	int ret;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);  
> 
> Good change.  But separate patch as not related to most of what is going on here.
> 
> 
> >  
> > -	ret = inv_icm42600_enable_regulator_vddio(st);
> > -	if (ret)
> > -		goto out_unlock;
> > +	/* check wakeup capability */
> > +	accel_dev = &st->indio_accel->dev;
> > +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > +
> > +	/* restore vddio if cut off or irq state */
> > +	if (!wakeup) {
> > +		ret = inv_icm42600_enable_regulator_vddio(st);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		enable_irq(st->irq);
> > +		disable_irq_wake(st->irq);
> > +	}
> >  
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_set_active(dev);
> > @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
> >  					 st->suspended.accel,
> >  					 st->suspended.temp, NULL);
> >  	if (ret)
> > -		goto out_unlock;
> > +		return ret;
> >  
> > -	/* restore APEX features */
> > -	if (st->apex.wom.enable) {
> > -		ret = inv_icm42600_set_wom(st, true);
> > -		if (ret)
> > -			goto out_unlock;
> > +	/* restore APEX features if disabled */
> > +	if (!wakeup) {
> > +		if (st->apex.wom.enable) {
> > +			ret = inv_icm42600_set_wom(st, true);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	}
> >  
> >  	/* restore FIFO data streaming */
> > @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
> >  		inv_sensors_timestamp_reset(&accel_st->ts);
> >  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> >  				   INV_ICM42600_FIFO_CONFIG_STREAM);
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> > -out_unlock:
> > -	mutex_unlock(&st->lock);
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> >   
> 

Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jean-Baptiste Maneyrol]

Hello Jonathan,

I understand the issue, but using spi->irq for getting interrupt is something we are already doing inside the driver. I agree we need to fix that, but I would prefer fixing it after adding WoM support.

Is that OK for you? Or do I need to fix getting the right interrupt first?

Thanks,
JB

________________________________________
From: Jonathan Cameron <jic23@kernel.org>
Sent: Saturday, March 15, 2025 19:17
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
Cc: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
 
This Message Is From an External Sender
This message came from outside your organization.
 
On Tue, 11 Mar 2025 15:31:44 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello Jonathan,
> 
> still sorry for not being able to reply in-line.
> 
> No problem for all changes, except handling the 2 interrupt lines. Currently our driver only supports INT1 and cannot work with INT2, and this is not related to Wake-on-Motion feature. This is something we could add in another series, and I prefer to have a dedicated series for that.

You should check it isn't INT2 that you are getting via spi->irq etc.
Absolutely fine to reject that in the driver but you need to be
sure you have what you think you do and the spi->irq etc don't
provide that surety - they just give you the first one in the
dt-binding.

Jonathan


> 
> For the mutex rework, I will delete them. This is something we can also do in another dedicated patch not inside this series.
> 
> Is that OK for you?
> 
> Thanks,
> JB
> 
> ________________________________________
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, February 22, 2025 17:22
> To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>
> Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
>  
> This Message Is From an External Sender
> This message came from outside your organization.
>  
> On Thu, 20 Feb 2025 21:52:07 +0100
> Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:
> 
> > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> > 
> > When Wake-on-Motion is on, enable system wakeup and keep chip on for
> > waking up system with interrupt.
> > 
> > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>  
> Hi Jean-Baptiste,
> 
> A few comments inline.
> 
> > ---
> >  drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
> >  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
> >  3 files changed, 63 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
> >   *  @map:		regmap pointer.
> >   *  @vdd_supply:	VDD voltage regulator for the chip.
> >   *  @vddio_supply:	I/O voltage regulator for the chip.
> > + *  @irq:		chip irq.  
> 
> Maybe say a little on what the variable is used for. It's not otherwise
> obvious why we need it.  Also, does this chip really only have one irq line?
> Looks like there are two. So the drivers should be fixed to cope with the
> only one wired being irq2  unless I've found the wrong datasheet which is
> certainly possible.
> 
> 
> >   *  @orientation:	sensor chip orientation relative to main hardware.
> >   *  @conf:		chip sensors configurations.
> >   *  @suspended:		suspended sensors configuration.
> > @@ -168,6 +169,7 @@ struct inv_icm42600_state {
> >  	struct regmap *map;
> >  	struct regulator *vdd_supply;
> >  	struct regulator *vddio_supply;
> > +	int irq;
> >  	struct iio_mount_matrix orientation;
> >  	struct inv_icm42600_conf conf;
> >  	struct inv_icm42600_suspended suspended;
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> >  	if (ret)
> >  		return ERR_PTR(ret);
> >  
> > +	/* accel events are wakeup capable */
> > +	device_set_wakeup_capable(&indio_dev->dev, true);
> > +
> >  	return indio_dev;
> >  }
> >  
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> >  	mutex_init(&st->lock);
> >  	st->chip = chip;
> >  	st->map = regmap;
> > +	st->irq = irq;
> >  
> >  	ret = iio_read_mount_matrix(dev, &st->orientation);
> >  	if (ret) {
> > @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
> >  static int inv_icm42600_suspend(struct device *dev)
> >  {
> >  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > +	struct device *accel_dev;
> > +	bool wakeup;
> > +	int accel_conf;
> >  	int ret;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);  
> 
> As below. Pull these guard changes out as a precursor patch. That will make
> it easier to spot the real changes here.
> 
> >  
> >  	st->suspended.gyro = st->conf.gyro.mode;
> >  	st->suspended.accel = st->conf.accel.mode;
> >  	st->suspended.temp = st->conf.temp_en;
> > -	if (pm_runtime_suspended(dev)) {
> > -		ret = 0;
> > -		goto out_unlock;
> > -	}
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> >  
> >  	/* disable FIFO data streaming */
> >  	if (st->fifo.on) {
> >  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> >  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
> >  		if (ret)
> > -			goto out_unlock;
> > +			return ret;
> >  	}
> >  
> > -	/* disable APEX features */
> > -	if (st->apex.wom.enable) {
> > -		ret = inv_icm42600_set_wom(st, false);
> > -		if (ret)
> > -			goto out_unlock;
> > +	/* keep chip on and wake-up capable if APEX and wakeup on */
> > +	accel_dev = &st->indio_accel->dev;
> > +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > +
> > +	if (!wakeup) {
> > +		/* disable APEX features and accel if wakeup disabled */
> > +		if (st->apex.wom.enable) {
> > +			ret = inv_icm42600_set_wom(st, false);
> > +			if (ret)
> > +				return ret;
> > +		}
> > +		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> > +	} else {
> > +		/* keep accel on and setup irq for wakeup */
> > +		accel_conf = st->conf.accel.mode;
> > +		enable_irq_wake(st->irq);
> > +		disable_irq(st->irq);
> >  	}
> >  
> >  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> > -					 INV_ICM42600_SENSOR_MODE_OFF, false,
> > -					 NULL);
> > +					 accel_conf, false, NULL);
> >  	if (ret)
> > -		goto out_unlock;
> > +		return ret;
> >  
> > -	regulator_disable(st->vddio_supply);
> > +	/* disable vddio regulator if chip is sleeping */
> > +	if (!wakeup)
> > +		regulator_disable(st->vddio_supply);
> >  
> > -out_unlock:
> > -	mutex_unlock(&st->lock);
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
> >  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> >  	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
> >  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> > +	struct device *accel_dev;
> > +	bool wakeup;
> >  	int ret;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);  
> 
> Good change.  But separate patch as not related to most of what is going on here.
> 
> 
> >  
> > -	ret = inv_icm42600_enable_regulator_vddio(st);
> > -	if (ret)
> > -		goto out_unlock;
> > +	/* check wakeup capability */
> > +	accel_dev = &st->indio_accel->dev;
> > +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > +
> > +	/* restore vddio if cut off or irq state */
> > +	if (!wakeup) {
> > +		ret = inv_icm42600_enable_regulator_vddio(st);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		enable_irq(st->irq);
> > +		disable_irq_wake(st->irq);
> > +	}
> >  
> >  	pm_runtime_disable(dev);
> >  	pm_runtime_set_active(dev);
> > @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
> >  					 st->suspended.accel,
> >  					 st->suspended.temp, NULL);
> >  	if (ret)
> > -		goto out_unlock;
> > +		return ret;
> >  
> > -	/* restore APEX features */
> > -	if (st->apex.wom.enable) {
> > -		ret = inv_icm42600_set_wom(st, true);
> > -		if (ret)
> > -			goto out_unlock;
> > +	/* restore APEX features if disabled */
> > +	if (!wakeup) {
> > +		if (st->apex.wom.enable) {
> > +			ret = inv_icm42600_set_wom(st, true);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	}
> >  
> >  	/* restore FIFO data streaming */
> > @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
> >  		inv_sensors_timestamp_reset(&accel_st->ts);
> >  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> >  				   INV_ICM42600_FIFO_CONFIG_STREAM);
> > +		if (ret)
> > +			return ret;
> >  	}
> >  
> > -out_unlock:
> > -	mutex_unlock(&st->lock);
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> >   
> 

Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion

[Jonathan Cameron]

On Wed, 19 Mar 2025 21:06:41 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> Hello Jonathan,
> 
> I understand the issue, but using spi->irq for getting interrupt is something we are already doing inside the driver. I agree we need to fix that, but I would prefer fixing it after adding WoM support.
> 
> Is that OK for you? Or do I need to fix getting the right interrupt first?

It's a pretty trivial fix to add the sanity check,
but I don't mind if you do it as a follow up.

Jonathan


> 
> Thanks,
> JB
> 
> ________________________________________
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, March 15, 2025 19:17
> To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> Cc: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
>  
> This Message Is From an External Sender
> This message came from outside your organization.
>  
> On Tue, 11 Mar 2025 15:31:44 +0000
> Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:
> 
> > Hello Jonathan,
> > 
> > still sorry for not being able to reply in-line.
> > 
> > No problem for all changes, except handling the 2 interrupt lines. Currently our driver only supports INT1 and cannot work with INT2, and this is not related to Wake-on-Motion feature. This is something we could add in another series, and I prefer to have a dedicated series for that.  
> 
> You should check it isn't INT2 that you are getting via spi->irq etc.
> Absolutely fine to reject that in the driver but you need to be
> sure you have what you think you do and the spi->irq etc don't
> provide that surety - they just give you the first one in the
> dt-binding.
> 
> Jonathan
> 
> 
> > 
> > For the mutex rework, I will delete them. This is something we can also do in another dedicated patch not inside this series.
> > 
> > Is that OK for you?
> > 
> > Thanks,
> > JB
> > 
> > ________________________________________
> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Saturday, February 22, 2025 17:22
> > To: Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org>
> > Cc: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>; Lars-Peter Clausen <lars@metafoo.de>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
> > Subject: Re: [PATCH 2/2] iio: imu: inv_icm42600: add wakeup functionality for Wake-on-Motion
> >  
> > This Message Is From an External Sender
> > This message came from outside your organization.
> >  
> > On Thu, 20 Feb 2025 21:52:07 +0100
> > Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:
> >   
> > > From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> > > 
> > > When Wake-on-Motion is on, enable system wakeup and keep chip on for
> > > waking up system with interrupt.
> > > 
> > > Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>    
> > Hi Jean-Baptiste,
> > 
> > A few comments inline.
> >   
> > > ---
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600.h       |  2 +
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c |  3 +
> > >  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c  | 89 +++++++++++++++--------
> > >  3 files changed, 63 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > > index 8dfbeaf1c768d7d25cb58ecf9804446f3cbbd465..baf1dcd714800e84ccd21dc1d1e486849c77a9ae 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> > > @@ -151,6 +151,7 @@ struct inv_icm42600_apex {
> > >   *  @map:		regmap pointer.
> > >   *  @vdd_supply:	VDD voltage regulator for the chip.
> > >   *  @vddio_supply:	I/O voltage regulator for the chip.
> > > + *  @irq:		chip irq.    
> > 
> > Maybe say a little on what the variable is used for. It's not otherwise
> > obvious why we need it.  Also, does this chip really only have one irq line?
> > Looks like there are two. So the drivers should be fixed to cope with the
> > only one wired being irq2  unless I've found the wrong datasheet which is
> > certainly possible.
> > 
> >   
> > >   *  @orientation:	sensor chip orientation relative to main hardware.
> > >   *  @conf:		chip sensors configurations.
> > >   *  @suspended:		suspended sensors configuration.
> > > @@ -168,6 +169,7 @@ struct inv_icm42600_state {
> > >  	struct regmap *map;
> > >  	struct regulator *vdd_supply;
> > >  	struct regulator *vddio_supply;
> > > +	int irq;
> > >  	struct iio_mount_matrix orientation;
> > >  	struct inv_icm42600_conf conf;
> > >  	struct inv_icm42600_suspended suspended;
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > > index 8ce2276b3edc61cc1ea26810198dd0057054ec48..4240e8c576f4d07af5434e9a91dfda532f87ffb9 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> > > @@ -1149,6 +1149,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
> > >  	if (ret)
> > >  		return ERR_PTR(ret);
> > >  
> > > +	/* accel events are wakeup capable */
> > > +	device_set_wakeup_capable(&indio_dev->dev, true);
> > > +
> > >  	return indio_dev;
> > >  }
> > >  
> > > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > > index c0fd2770d66f02d1965fa07f819fd2db9a1d6bd2..f94bda5dc094d6cc85e3facbd480b830bfbaa3f9 100644
> > > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > > @@ -751,6 +751,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> > >  	mutex_init(&st->lock);
> > >  	st->chip = chip;
> > >  	st->map = regmap;
> > > +	st->irq = irq;
> > >  
> > >  	ret = iio_read_mount_matrix(dev, &st->orientation);
> > >  	if (ret) {
> > > @@ -829,44 +830,56 @@ EXPORT_SYMBOL_NS_GPL(inv_icm42600_core_probe, "IIO_ICM42600");
> > >  static int inv_icm42600_suspend(struct device *dev)
> > >  {
> > >  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > > +	struct device *accel_dev;
> > > +	bool wakeup;
> > > +	int accel_conf;
> > >  	int ret;
> > >  
> > > -	mutex_lock(&st->lock);
> > > +	guard(mutex)(&st->lock);    
> > 
> > As below. Pull these guard changes out as a precursor patch. That will make
> > it easier to spot the real changes here.
> >   
> > >  
> > >  	st->suspended.gyro = st->conf.gyro.mode;
> > >  	st->suspended.accel = st->conf.accel.mode;
> > >  	st->suspended.temp = st->conf.temp_en;
> > > -	if (pm_runtime_suspended(dev)) {
> > > -		ret = 0;
> > > -		goto out_unlock;
> > > -	}
> > > +	if (pm_runtime_suspended(dev))
> > > +		return 0;
> > >  
> > >  	/* disable FIFO data streaming */
> > >  	if (st->fifo.on) {
> > >  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> > >  				   INV_ICM42600_FIFO_CONFIG_BYPASS);
> > >  		if (ret)
> > > -			goto out_unlock;
> > > +			return ret;
> > >  	}
> > >  
> > > -	/* disable APEX features */
> > > -	if (st->apex.wom.enable) {
> > > -		ret = inv_icm42600_set_wom(st, false);
> > > -		if (ret)
> > > -			goto out_unlock;
> > > +	/* keep chip on and wake-up capable if APEX and wakeup on */
> > > +	accel_dev = &st->indio_accel->dev;
> > > +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > > +
> > > +	if (!wakeup) {
> > > +		/* disable APEX features and accel if wakeup disabled */
> > > +		if (st->apex.wom.enable) {
> > > +			ret = inv_icm42600_set_wom(st, false);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > > +		accel_conf = INV_ICM42600_SENSOR_MODE_OFF;
> > > +	} else {
> > > +		/* keep accel on and setup irq for wakeup */
> > > +		accel_conf = st->conf.accel.mode;
> > > +		enable_irq_wake(st->irq);
> > > +		disable_irq(st->irq);
> > >  	}
> > >  
> > >  	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> > > -					 INV_ICM42600_SENSOR_MODE_OFF, false,
> > > -					 NULL);
> > > +					 accel_conf, false, NULL);
> > >  	if (ret)
> > > -		goto out_unlock;
> > > +		return ret;
> > >  
> > > -	regulator_disable(st->vddio_supply);
> > > +	/* disable vddio regulator if chip is sleeping */
> > > +	if (!wakeup)
> > > +		regulator_disable(st->vddio_supply);
> > >  
> > > -out_unlock:
> > > -	mutex_unlock(&st->lock);
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -878,13 +891,25 @@ static int inv_icm42600_resume(struct device *dev)
> > >  	struct inv_icm42600_state *st = dev_get_drvdata(dev);
> > >  	struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
> > >  	struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);
> > > +	struct device *accel_dev;
> > > +	bool wakeup;
> > >  	int ret;
> > >  
> > > -	mutex_lock(&st->lock);
> > > +	guard(mutex)(&st->lock);    
> > 
> > Good change.  But separate patch as not related to most of what is going on here.
> > 
> >   
> > >  
> > > -	ret = inv_icm42600_enable_regulator_vddio(st);
> > > -	if (ret)
> > > -		goto out_unlock;
> > > +	/* check wakeup capability */
> > > +	accel_dev = &st->indio_accel->dev;
> > > +	wakeup = (st->apex.on && device_may_wakeup(accel_dev)) ? true : false;
> > > +
> > > +	/* restore vddio if cut off or irq state */
> > > +	if (!wakeup) {
> > > +		ret = inv_icm42600_enable_regulator_vddio(st);
> > > +		if (ret)
> > > +			return ret;
> > > +	} else {
> > > +		enable_irq(st->irq);
> > > +		disable_irq_wake(st->irq);
> > > +	}
> > >  
> > >  	pm_runtime_disable(dev);
> > >  	pm_runtime_set_active(dev);
> > > @@ -895,13 +920,15 @@ static int inv_icm42600_resume(struct device *dev)
> > >  					 st->suspended.accel,
> > >  					 st->suspended.temp, NULL);
> > >  	if (ret)
> > > -		goto out_unlock;
> > > +		return ret;
> > >  
> > > -	/* restore APEX features */
> > > -	if (st->apex.wom.enable) {
> > > -		ret = inv_icm42600_set_wom(st, true);
> > > -		if (ret)
> > > -			goto out_unlock;
> > > +	/* restore APEX features if disabled */
> > > +	if (!wakeup) {
> > > +		if (st->apex.wom.enable) {
> > > +			ret = inv_icm42600_set_wom(st, true);
> > > +			if (ret)
> > > +				return ret;
> > > +		}
> > >  	}
> > >  
> > >  	/* restore FIFO data streaming */
> > > @@ -910,11 +937,11 @@ static int inv_icm42600_resume(struct device *dev)
> > >  		inv_sensors_timestamp_reset(&accel_st->ts);
> > >  		ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> > >  				   INV_ICM42600_FIFO_CONFIG_STREAM);
> > > +		if (ret)
> > > +			return ret;
> > >  	}
> > >  
> > > -out_unlock:
> > > -	mutex_unlock(&st->lock);
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  /* Runtime suspend will turn off sensors that are enabled by iio devices. */
> > >     
> >   
>