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

[PATCH v2 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>
---
Changes in v2:
- change struct order to avoir DMA overflow
- separate wom enable/disable in 2 functions
- delete mutex rework
- Link to v1: https://lore.kernel.org/r/20250220-losd-3-inv-icm42600-add-wom-support-v1-0-9b937f986954@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        |  56 +++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 283 ++++++++++++++++++++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   | 103 +++++++-
 4 files changed, 430 insertions(+), 14 deletions(-)
---
base-commit: d3d6cb27a945c6fc7ddd3e7423c4303b4b6bad36
change-id: 20250220-losd-3-inv-icm42600-add-wom-support-0620fef9db23

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

[PATCH v2 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        |  54 +++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 280 ++++++++++++++++++++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  58 +++++
 4 files changed, 386 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index f893dbe6996506a33eb5d3be47e6765a923665c9..f5adebe4640fc36c5dc9810b515369d4909ba834 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.
@@ -148,9 +156,10 @@ struct inv_icm42600_suspended {
  *  @suspended:		suspended sensors configuration.
  *  @indio_gyro:	gyroscope IIO device.
  *  @indio_accel:	accelerometer IIO device.
- *  @buffer:		data transfer buffer aligned for DMA.
- *  @fifo:		FIFO management structure.
  *  @timestamp:		interrupt timestamps.
+ *  @apex:		APEX features management.
+ *  @fifo:		FIFO management structure.
+ *  @buffer:		data transfer buffer aligned for DMA.
  */
 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);
-	struct inv_icm42600_fifo fifo;
 	struct {
 		int64_t gyro;
 		int64_t accel;
 	} timestamp;
+	struct inv_icm42600_apex apex;
+	struct inv_icm42600_fifo fifo;
+	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
 };
 
 
@@ -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,9 @@ 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_enable_wom(struct inv_icm42600_state *st);
+int inv_icm42600_disable_wom(struct inv_icm42600_state *st);
+
 int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
 			     unsigned int writeval, unsigned int *readval);
 
@@ -437,4 +479,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 e6cd9dcb0687d19554e63a69dc60f065c58d70ee..94515166c1fcdf6cc7ba4843334f51cb14696eba 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,153 @@ 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)
+{
+	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;
+
+	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_enable_wom(st);
+		if (ret)
+			goto error_suspend;
+		st->apex.on++;
+		st->apex.wom.enable = true;
+	}
+
+	return 0;
+
+error_suspend:
+	pm_runtime_mark_last_busy(pdev);
+	pm_runtime_put_autosuspend(pdev);
+	return ret;
+}
+
+static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
+{
+	struct inv_icm42600_state *st = iio_device_get_drvdata(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;
+
+	scoped_guard(mutex, &st->lock) {
+		st->apex.wom.enable = false;
+		st->apex.on--;
+		ret = inv_icm42600_disable_wom(st);
+		if (ret)
+			break;
+		/* 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)
+				break;
+		}
+	}
+
+	if (sleep_ms)
+		msleep(sleep_ms);
+	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 +635,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);
@@ -819,6 +994,101 @@ 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;
+		}
+		if (state)
+			return inv_icm42600_accel_enable_wom(indio_dev);
+		else
+			return inv_icm42600_accel_disable_wom(indio_dev);
+	}
+
+	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,
@@ -828,6 +1098,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 63d46619ebfaa1372171129fca96381ef4606b2e..9a2150ab9e874f17d7fda731cb131c3f688a75a3 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -404,6 +404,37 @@ int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
 					  sleep_ms);
 }
 
+int inv_icm42600_enable_wom(struct inv_icm42600_state *st)
+{
+	int ret;
+
+	/* enable WoM hardware */
+	ret = regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG,
+			   INV_ICM42600_SMD_CONFIG_SMD_MODE_WOM |
+			   INV_ICM42600_SMD_CONFIG_WOM_MODE);
+	if (ret)
+		return ret;
+
+	/* enable WoM interrupt */
+	return regmap_set_bits(st->map, INV_ICM42600_REG_INT_SOURCE1,
+			      INV_ICM42600_INT_SOURCE1_WOM_INT1_EN);
+}
+
+int inv_icm42600_disable_wom(struct inv_icm42600_state *st)
+{
+	int ret;
+
+	/* 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 */
+	return regmap_write(st->map, INV_ICM42600_REG_SMD_CONFIG,
+			    INV_ICM42600_SMD_CONFIG_SMD_MODE_OFF);
+}
+
 int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
 			     unsigned int writeval, unsigned int *readval)
 {
@@ -548,6 +579,19 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
 
 	mutex_lock(&st->lock);
 
+	if (st->apex.on) {
+		unsigned int status2, status3;
+
+		/* 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;
@@ -819,6 +863,13 @@ static int inv_icm42600_suspend(struct device *dev)
 			goto out_unlock;
 	}
 
+	/* disable APEX features */
+	if (st->apex.wom.enable) {
+		ret = inv_icm42600_disable_wom(st);
+		if (ret)
+			goto out_unlock;
+	}
+
 	ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
 					 INV_ICM42600_SENSOR_MODE_OFF, false,
 					 NULL);
@@ -860,6 +911,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_enable_wom(st);
+		if (ret)
+			goto out_unlock;
+	}
+
 	/* restore FIFO data streaming */
 	if (st->fifo.on) {
 		inv_sensors_timestamp_reset(&gyro_st->ts);

-- 
2.49.0

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

[Andy Shevchenko]

On Tue, Apr 15, 2025 at 5:47 PM 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.

...

> +struct inv_icm42600_apex {
> +       unsigned int on;
> +       struct {
> +               bool enable;
> +               uint64_t value;

Why not kernel types? It seems in many places in the driver the
*intXX_t are used instead of simple uXX/sXX.

> +       } wom;

Have you run `pahole`? To me it seems swapping the members in the
outer struct will save some memory at run time.

> +};

...

>         struct {
>                 int64_t gyro;
>                 int64_t accel;

s64 ?

>         } timestamp;

...

> +       uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);

u8 ?

...

> +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,

u64

> +                                                         int accel_hz, int accel_uhz)
> +{
> +       /* 1000/256mg per LSB converted in m/s² in micro (1000000) */

That 1000000 is redundant, just properly spell the units.

> +       const unsigned int convert = (1000U * 9807U) / 256U;
> +       uint64_t value;
> +       uint64_t freq_uhz;

u64

> +       /* return 0 only if roc is 0 */
> +       if (roc == 0)
> +               return 0;
> +
> +       freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;

u64

MICRO?

> +       value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);

MICRO

> +       /* limit value to 8 bits and prevent 0 */
> +       return min(255, max(1, value));

Reinvention of the clamp() ?

> +}

...

> +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
> +                                                     int accel_hz, int accel_uhz)

Similar comments as per above.

...

> +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev)
> +{
> +       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;
> +
> +       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)

Do you need this check?

> +               msleep(sleep_ms);
> +
> +       scoped_guard(mutex, &st->lock) {
> +               ret = inv_icm42600_enable_wom(st);
> +               if (ret)
> +                       goto error_suspend;
> +               st->apex.on++;
> +               st->apex.wom.enable = true;
> +       }
> +
> +       return 0;
> +
> +error_suspend:
> +       pm_runtime_mark_last_busy(pdev);
> +       pm_runtime_put_autosuspend(pdev);
> +       return ret;
> +}
> +
> +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> +{
> +       struct inv_icm42600_state *st = iio_device_get_drvdata(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;
> +
> +       scoped_guard(mutex, &st->lock) {
> +               st->apex.wom.enable = false;
> +               st->apex.on--;
> +               ret = inv_icm42600_disable_wom(st);
> +               if (ret)
> +                       break;
> +               /* 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)
> +                               break;
> +               }
> +       }

> +       if (sleep_ms)

Ditto.

> +               msleep(sleep_ms);


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

Okay, I believe here int64_t is inherited from IIO definitions.

> +{
> +       struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +       uint64_t ev_code;

u64.

> +       /* 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);
> +       }
> +}

...

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

Invert the conditional to reduce indentation of the below. It will
also follow the traditional pattern, i.e. checking for the errors
first.

> +               scoped_guard(mutex, &st->lock) {
> +                       if (st->apex.wom.enable == state)
> +                               return 0;
> +               }
> +               if (state)
> +                       return inv_icm42600_accel_enable_wom(indio_dev);

> +               else

Redundant 'else', but in this case for the sake of symmetry it can be left.

> +                       return inv_icm42600_accel_disable_wom(indio_dev);
> +       }
> +
> +       return -EINVAL;
> +}

...

> +       if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {

Invert the conditional.

> +               /* 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;

u64

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

Invert the conditional.

> +               if (val < 0 || val2 < 0)
> +                       return -EINVAL;

This can be checked even before anything else, but see above. With
that it will automatically get on the same indentation level as
previous one.

> +               value = (uint64_t)val * 1000000ULL + (uint64_t)val2;

MICRO.
ULL is not needed, the first one already ULL.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

Hello Andy,

concerning usage of kernel types, my understanding was that we should conform to existing types usage in a driver. That's why I'm keeping the standard types instead of kernel ones. I could change them in the patch, but it would mix usage of both standard and kernel types.

Another thing not related, by reading the coding style documentation I would think that for new driver we can use whatever we prefer between standard types and kernel types. Is it not the case?

Thanks for your help,
JB

________________________________________
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Sent: Tuesday, April 15, 2025 20:18
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; David Lechner <dlechner@baylibre.com>; Nuno Sá <nuno.sa@analog.com>; Andy Shevchenko <andy@kernel.org>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] iio: imu: inv_icm42600: add WoM support
 
This Message Is From an External Sender
This message came from outside your organization.
 
On Tue, Apr 15, 2025 at 5:47 PM 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.

...

> +struct inv_icm42600_apex {
> +       unsigned int on;
> +       struct {
> +               bool enable;
> +               uint64_t value;

Why not kernel types? It seems in many places in the driver the
*intXX_t are used instead of simple uXX/sXX.

> +       } wom;

Have you run `pahole`? To me it seems swapping the members in the
outer struct will save some memory at run time.

> +};

...

>         struct {
>                 int64_t gyro;
>                 int64_t accel;

s64 ?

>         } timestamp;

...

> +       uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);

u8 ?

...

> +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,

u64

> +                                                         int accel_hz, int accel_uhz)
> +{
> +       /* 1000/256mg per LSB converted in m/s² in micro (1000000) */

That 1000000 is redundant, just properly spell the units.

> +       const unsigned int convert = (1000U * 9807U) / 256U;
> +       uint64_t value;
> +       uint64_t freq_uhz;

u64

> +       /* return 0 only if roc is 0 */
> +       if (roc == 0)
> +               return 0;
> +
> +       freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;

u64

MICRO?

> +       value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);

MICRO

> +       /* limit value to 8 bits and prevent 0 */
> +       return min(255, max(1, value));

Reinvention of the clamp() ?

> +}

...

> +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
> +                                                     int accel_hz, int accel_uhz)

Similar comments as per above.

...

> +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev)
> +{
> +       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;
> +
> +       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)

Do you need this check?

> +               msleep(sleep_ms);
> +
> +       scoped_guard(mutex, &st->lock) {
> +               ret = inv_icm42600_enable_wom(st);
> +               if (ret)
> +                       goto error_suspend;
> +               st->apex.on++;
> +               st->apex.wom.enable = true;
> +       }
> +
> +       return 0;
> +
> +error_suspend:
> +       pm_runtime_mark_last_busy(pdev);
> +       pm_runtime_put_autosuspend(pdev);
> +       return ret;
> +}
> +
> +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> +{
> +       struct inv_icm42600_state *st = iio_device_get_drvdata(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;
> +
> +       scoped_guard(mutex, &st->lock) {
> +               st->apex.wom.enable = false;
> +               st->apex.on--;
> +               ret = inv_icm42600_disable_wom(st);
> +               if (ret)
> +                       break;
> +               /* 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)
> +                               break;
> +               }
> +       }

> +       if (sleep_ms)

Ditto.

> +               msleep(sleep_ms);


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

Okay, I believe here int64_t is inherited from IIO definitions.

> +{
> +       struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> +       uint64_t ev_code;

u64.

> +       /* 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);
> +       }
> +}

...

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

Invert the conditional to reduce indentation of the below. It will
also follow the traditional pattern, i.e. checking for the errors
first.

> +               scoped_guard(mutex, &st->lock) {
> +                       if (st->apex.wom.enable == state)
> +                               return 0;
> +               }
> +               if (state)
> +                       return inv_icm42600_accel_enable_wom(indio_dev);

> +               else

Redundant 'else', but in this case for the sake of symmetry it can be left.

> +                       return inv_icm42600_accel_disable_wom(indio_dev);
> +       }
> +
> +       return -EINVAL;
> +}

...

> +       if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {

Invert the conditional.

> +               /* 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;

u64

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

Invert the conditional.

> +               if (val < 0 || val2 < 0)
> +                       return -EINVAL;

This can be checked even before anything else, but see above. With
that it will automatically get on the same indentation level as
previous one.

> +               value = (uint64_t)val * 1000000ULL + (uint64_t)val2;

MICRO.
ULL is not needed, the first one already ULL.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 3:41 PM Jean-Baptiste Maneyrol
<Jean-Baptiste.Maneyrol@tdk.com> wrote:
>
> Hello Andy,
>
> concerning usage of kernel types, my understanding was that we should conform to existing types usage in a driver. That's why I'm keeping the standard types instead of kernel ones. I could change them in the patch, but it would mix usage of both standard and kernel types.
>
> Another thing not related, by reading the coding style documentation I would think that for new driver we can use whatever we prefer between standard types and kernel types. Is it not the case?

It's not for the repositories Greg KH maintaining directly or indirectly:
https://lore.kernel.org/all/20170411140919.GC4388@kroah.com/

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

OK, understood.

But for this particular patch, do I need to stay consistent with the existing driver by keeping the standard type or use kernel types and mix with standard types?

Thanks,
JB

________________________________________
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Sent: Wednesday, April 16, 2025 15:06
To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
Cc: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; David Lechner <dlechner@baylibre.com>; Nuno Sá <nuno.sa@analog.com>; Andy Shevchenko <andy@kernel.org>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] iio: imu: inv_icm42600: add WoM support
 
This Message Is From an External Sender
This message came from outside your organization.
 
On Wed, Apr 16, 2025 at 3:41 PM Jean-Baptiste Maneyrol
<Jean-Baptiste.Maneyrol@tdk.com> wrote:
>
> Hello Andy,
>
> concerning usage of kernel types, my understanding was that we should conform to existing types usage in a driver. That's why I'm keeping the standard types instead of kernel ones. I could change them in the patch, but it would mix usage of both standard and kernel types.
>
> Another thing not related, by reading the coding style documentation I would think that for new driver we can use whatever we prefer between standard types and kernel types. Is it not the case?

It's not for the repositories Greg KH maintaining directly or indirectly:
https://urldefense.com/v3/__https://lore.kernel.org/all/20170411140919.GC4388@kroah.com/__;!!FtrhtPsWDhZ6tw!ACor8Q-WyU0CorzLpw0OtWqNk59Wn2Z_s7KKOZuNnglKBzN4jTqb3c6oea83KLER97bFxkEIgSl8_IcKHTxvaRKCUwVCP5wunvo$[lore[.]kernel[.]org]

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Apr 16, 2025 at 02:40:50PM +0000, Jean-Baptiste Maneyrol wrote:
> OK, understood.
> 
> But for this particular patch, do I need to stay consistent with the existing
> driver by keeping the standard type or use kernel types and mix with standard
> types?

Yes, consistency with the existing code is priority, if the change is a fix.
If it's a feature, better to clean up first (i.e. switch the types elsewhere)
and then add a new feature.

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, April 16, 2025 15:06
> To: Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com>
> On Wed, Apr 16, 2025 at 3:41 PM Jean-Baptiste Maneyrol
> <Jean-Baptiste.Maneyrol@tdk.com> wrote:
> >
> > concerning usage of kernel types, my understanding was that we should
> > conform to existing types usage in a driver. That's why I'm keeping the
> > standard types instead of kernel ones. I could change them in the patch,
> > but it would mix usage of both standard and kernel types.
> >
> > Another thing not related, by reading the coding style documentation I
> > would think that for new driver we can use whatever we prefer between
> > standard types and kernel types. Is it not the case?
> 
> It's not for the repositories Greg KH maintaining directly or indirectly:
> https://urldefense.com/v3/__https://lore.kernel.org/all/20170411140919.GC4388@kroah.com/__;!!FtrhtPsWDhZ6tw!ACor8Q-WyU0CorzLpw0OtWqNk59Wn2Z_s7KKOZuNnglKBzN4jTqb3c6oea83KLER97bFxkEIgSl8_IcKHTxvaRKCUwVCP5wunvo$[lore[.]kernel[.]org]

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

> On Tue, Apr 15, 2025 at 5:47 PM 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.
> 
> ...
> 
> > +struct inv_icm42600_apex {
> > +       unsigned int on;
> > +       struct {
> > +               bool enable;
> > +               uint64_t value;
> 
> Why not kernel types? It seems in many places in the driver the
> *intXX_t are used instead of simple uXX/sXX.
> 
> > +       } wom;
> 
> Have you run `pahole`? To me it seems swapping the members in the
> outer struct will save some memory at run time.

You're right, we can inverse order and gain some memory. pahole is confirming.

> 
> > +};
> 
> ...
> 
> >         struct {
> >                 int64_t gyro;
> >                 int64_t accel;
> 
> s64 ?
> 
> >         } timestamp;
> 
> ...
> 
> > +       uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
> 
> u8 ?
> 
> ...
> 
> > +static unsigned int inv_icm42600_accel_convert_roc_to_wom(uint64_t roc,
> 
> u64
> 
> > +                                                         int accel_hz, int accel_uhz)
> > +{
> > +       /* 1000/256mg per LSB converted in m/s² in micro (1000000) */
> 
> That 1000000 is redundant, just properly spell the units.

I will use um/s²

> 
> > +       const unsigned int convert = (1000U * 9807U) / 256U;
> > +       uint64_t value;
> > +       uint64_t freq_uhz;
> 
> u64
> 
> > +       /* return 0 only if roc is 0 */
> > +       if (roc == 0)
> > +               return 0;
> > +
> > +       freq_uhz = (uint64_t)accel_hz * 1000000U + (uint64_t)accel_uhz;
> 
> u64
> 
> MICRO?

Good idea, I didn't know it exists.

> 
> > +       value = div64_u64(roc * 1000000U, freq_uhz * (uint64_t)convert);
> 
> MICRO

No problem, yes.

> 
> > +       /* limit value to 8 bits and prevent 0 */
> > +       return min(255, max(1, value));
> 
> Reinvention of the clamp() ?

It was a copy-paste of an older driver, at the time clamp was not here.
I will replace by clamp, it is much more readable.

> 
> > +}
> 
> ...
> 
> > +static uint64_t inv_icm42600_accel_convert_wom_to_roc(unsigned int threshold,
> > +                                                     int accel_hz, int accel_uhz)
> 
> Similar comments as per above.
> 
> ...
> 
> > +static int inv_icm42600_accel_enable_wom(struct iio_dev *indio_dev)
> > +{
> > +       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;
> > +
> > +       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)
> 
> Do you need this check?

We need this check in the case sleep_ms is 0. It will happen if accel is
already on. msleep(0) was usually doing a sleep before. I don't know if it
is still the case.

> 
> > +               msleep(sleep_ms);
> > +
> > +       scoped_guard(mutex, &st->lock) {
> > +               ret = inv_icm42600_enable_wom(st);
> > +               if (ret)
> > +                       goto error_suspend;
> > +               st->apex.on++;
> > +               st->apex.wom.enable = true;
> > +       }
> > +
> > +       return 0;
> > +
> > +error_suspend:
> > +       pm_runtime_mark_last_busy(pdev);
> > +       pm_runtime_put_autosuspend(pdev);
> > +       return ret;
> > +}
> > +
> > +static int inv_icm42600_accel_disable_wom(struct iio_dev *indio_dev)
> > +{
> > +       struct inv_icm42600_state *st = iio_device_get_drvdata(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;
> > +
> > +       scoped_guard(mutex, &st->lock) {
> > +               st->apex.wom.enable = false;
> > +               st->apex.on--;
> > +               ret = inv_icm42600_disable_wom(st);
> > +               if (ret)
> > +                       break;
> > +               /* 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)
> > +                               break;
> > +               }
> > +       }
> 
> > +       if (sleep_ms)
> 
> Ditto.

Same thing, it is to avoid possible msleep(0).

> 
> > +               msleep(sleep_ms);
> 
> 
> > +       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)
> 
> Okay, I believe here int64_t is inherited from IIO definitions.
> 
> > +{
> > +       struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> > +       uint64_t ev_code;
> 
> u64.
> 
> > +       /* 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);
> > +       }
> > +}
> 
> ...
> 
> > +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) {
> 
> Invert the conditional to reduce indentation of the below. It will
> also follow the traditional pattern, i.e. checking for the errors
> first.

It was done like this because I am planning to add other events like tilt.
But I will reverse it, it can be changed latter.

> 
> > +               scoped_guard(mutex, &st->lock) {
> > +                       if (st->apex.wom.enable == state)
> > +                               return 0;
> > +               }
> > +               if (state)
> > +                       return inv_icm42600_accel_enable_wom(indio_dev);
> 
> > +               else
> 
> Redundant 'else', but in this case for the sake of symmetry it can be left.
> 
> > +                       return inv_icm42600_accel_disable_wom(indio_dev);
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> 
> ...
> 
> > +       if (type == IIO_EV_TYPE_ROC && dir == IIO_EV_DIR_RISING) {
> 
> Invert the conditional.

No problem either.

> 
> > +               /* 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;
> 
> u64
> 
> > +       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) {
> 
> Invert the conditional.

No problem also.

> 
> > +               if (val < 0 || val2 < 0)
> > +                       return -EINVAL;
> 
> This can be checked even before anything else, but see above. With
> that it will automatically get on the same indentation level as
> previous one.

I will merge with previous check with inverted conditional.

> 
> > +               value = (uint64_t)val * 1000000ULL + (uint64_t)val2;
> 
> MICRO.
> ULL is not needed, the first one already ULL.

Will do.

> 
> > +               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;
> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks,
JB

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

[Andy Shevchenko]

On Thu, Apr 17, 2025 at 5:25 PM Jean-Baptiste Maneyrol
<Jean-Baptiste.Maneyrol@tdk.com> wrote:
> > On Tue, Apr 15, 2025 at 5:47 PM Jean-Baptiste Maneyrol via B4 Relay
> > <devnull+jean-baptiste.maneyrol.tdk.com@kernel.org> wrote:

...

> > > +       /* 1000/256mg per LSB converted in m/s² in micro (1000000) */
> >
> > That 1000000 is redundant, just properly spell the units.
>
> I will use um/s²

FWIW, you may even use a Greek MU letter, kernel is UTF-8 compatible :-)
μm/s²

...

> > > +       /* limit value to 8 bits and prevent 0 */
> > > +       return min(255, max(1, value));
> >
> > Reinvention of the clamp() ?
>
> It was a copy-paste of an older driver, at the time clamp was not here.
> I will replace by clamp, it is much more readable.

For the curious, read this https://lwn.net/Articles/983965/.
The min(max()) may have really unexpected side effects :-)

...

> > > +       if (sleep_ms)
> >
> > Do you need this check?
>
> We need this check in the case sleep_ms is 0. It will happen if accel is
> already on. msleep(0) was usually doing a sleep before. I don't know if it
> is still the case.

I'm wondering if it's documented anywhere, do we need to update /
improve a documentation?

> > > +               msleep(sleep_ms);

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 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  | 65 +++++++++++++++++------
 3 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index f5adebe4640fc36c5dc9810b515369d4909ba834..768d773a9129c5d64f0af1959504889ffbc9c131 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, required to enable/disable and set wakeup
  *  @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 94515166c1fcdf6cc7ba4843334f51cb14696eba..2249f551f10f3211dadde2f6aa1dccd9b704f146 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -1162,6 +1162,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 9a2150ab9e874f17d7fda731cb131c3f688a75a3..14b64d14a6241ec60c23b64486a615a84513ae3c 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -765,6 +765,7 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
 	mutex_init(&st->lock);
 	st->chip = chip;
 	st->map = regmap;
+	st->irq = irq;
 
 	ret = iio_read_mount_matrix(dev, &st->orientation);
 	if (ret) {
@@ -843,6 +844,9 @@ 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);
@@ -863,20 +867,33 @@ static int inv_icm42600_suspend(struct device *dev)
 			goto out_unlock;
 	}
 
-	/* disable APEX features */
-	if (st->apex.wom.enable) {
-		ret = inv_icm42600_disable_wom(st);
-		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_disable_wom(st);
+			if (ret)
+				goto out_unlock;
+		}
+		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;
 
-	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);
@@ -892,13 +909,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);
 
-	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)
+			goto out_unlock;
+	} else {
+		enable_irq(st->irq);
+		disable_irq_wake(st->irq);
+	}
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
@@ -911,11 +940,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_enable_wom(st);
-		if (ret)
-			goto out_unlock;
+	/* restore APEX features if disabled */
+	if (!wakeup) {
+		if (st->apex.wom.enable) {
+			ret = inv_icm42600_enable_wom(st);
+			if (ret)
+				goto out_unlock;
+		}
 	}
 
 	/* restore FIFO data streaming */
@@ -924,6 +955,8 @@ 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)
+			goto out_unlock;
 	}
 
 out_unlock:

-- 
2.49.0

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

[Andy Shevchenko]

On Tue, Apr 15, 2025 at 5:47 PM 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

keep the chip

> waking up system with interrupt.

up the system
with an interrupt.




> +       /* 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;

Redundant ternary.

> +

This blank line should be rather after the accel_dev = ... line.

> +       if (!wakeup) {

Can we use positive conditionals? Generally they are easier to read.

> +               /* disable APEX features and accel if wakeup disabled */
> +               if (st->apex.wom.enable) {
> +                       ret = inv_icm42600_disable_wom(st);
> +                       if (ret)
> +                               goto out_unlock;
> +               }
> +               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);
>         }

...

> +       /* 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) {

Same comments as per above.

> +               ret = inv_icm42600_enable_regulator_vddio(st);
> +               if (ret)
> +                       goto out_unlock;
> +       } else {
> +               enable_irq(st->irq);
> +               disable_irq_wake(st->irq);
> +       }

...

> +       /* restore APEX features if disabled */
> +       if (!wakeup) {
> +               if (st->apex.wom.enable) {

This is effectively if (foo && bar).

> +                       ret = inv_icm42600_enable_wom(st);
> +                       if (ret)
> +                               goto out_unlock;
> +               }
>         }

...

> @@ -924,6 +955,8 @@ 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)
> +                       goto out_unlock;
>         }

>  out_unlock:

Stray / unneeded change.

-- 
With Best Regards,
Andy Shevchenko