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

[PATCH v4 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 v4:
- Avoid mix of gotos and scoped_guard()
- Invert conditionals for better code readability
- Switch to use devm_device_init_wakeup()
- Several code readabilities improvements
- Link to v3: https://lore.kernel.org/r/20250418-losd-3-inv-icm42600-add-wom-support-v3-0-7a180af02bfe@tdk.com

Changes in v3:
- Rewrites following code review
- Link to v2: https://lore.kernel.org/r/20250415-losd-3-inv-icm42600-add-wom-support-v2-0-de94dfb92b7e@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  | 292 ++++++++++++++++++++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  97 ++++++-
 4 files changed, 433 insertions(+), 14 deletions(-)
---
base-commit: 4c6073fec2fee4827fa0dd8a4ab4e6f7bbc05ee6
change-id: 20250220-losd-3-inv-icm42600-add-wom-support-0620fef9db23

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

[PATCH v4 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  | 289 ++++++++++++++++++++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  58 +++++
 4 files changed, 395 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..413a15493bcb880dc00b20da3b3168d5addd32a9 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 {
+		uint64_t value;
+		bool enable;
+	} 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 (Advanced Pedometer and Event detection) 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..9a2089527a9426b70eb796d4e9c234d8804c508b 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -10,9 +10,12 @@
 #include <linux/regmap.h>
 #include <linux/delay.h>
 #include <linux/math64.h>
+#include <linux/minmax.h>
+#include <linux/units.h>
 
 #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 +50,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 +95,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 +174,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 +193,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 +320,160 @@ 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² */
+	const unsigned int convert = (9807U * (MICRO / MILLI)) / 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 * MICRO + (uint64_t)accel_uhz;
+	value = div64_u64(roc * MICRO, freq_uhz * (uint64_t)convert);
+
+	/* limit value to 8 bits and prevent 0 */
+	return clamp(value, 1, 255);
+}
+
+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² */
+	const unsigned int convert = (9807U * (MICRO / MILLI)) / 256U;
+	uint64_t value;
+	uint64_t freq_uhz;
+
+	value = threshold * convert;
+	freq_uhz = (uint64_t)accel_hz * MICRO + (uint64_t)accel_uhz;
+
+	/* compute the differential by multiplying by the frequency */
+	return div_u64(value * freq_uhz, MICRO);
+}
+
+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)
+			break;
+		st->apex.on++;
+		st->apex.wom.enable = true;
+	}
+	if (ret)
+		goto error_suspend;
+
+	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) {
+		/*
+		 * Consider that turning off WoM is always working to avoid
+		 * blocking the chip in on mode and prevent going back to sleep.
+		 * If there is an error, the chip will anyway go back to sleep
+		 * and the feature will not work anymore.
+		 */
+		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 +644,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 +1003,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);
+
+	/* handle only WoM (roc rising) event */
+	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+
+	return st->apex.wom.enable ? 1 : 0;
+}
+
+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 only WoM (roc rising) event */
+	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
+		return -EINVAL;
+
+	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);
+}
+
+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;
+
+	/* handle only WoM (roc rising) event value */
+	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
+		return -EINVAL;
+
+	guard(mutex)(&st->lock);
+
+	/* return value in micro */
+	*val = div_u64_rem(st->apex.wom.value, MICRO, &rem);
+	*val2 = rem;
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+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 only WoM (roc rising) event value */
+	if (type != IIO_EV_TYPE_ROC || dir != IIO_EV_DIR_RISING)
+		return -EINVAL;
+
+	if (val < 0 || val2 < 0)
+		return -EINVAL;
+
+	value = (uint64_t)val * MICRO + (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;
+}
+
 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 +1107,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

[PATCH v4 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 the chip on
for waking up the system with an 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  | 53 +++++++++++++++++------
 3 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index 413a15493bcb880dc00b20da3b3168d5addd32a9..67f5c1a611988776d0bdf4d43a46f2fda43e3e44 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 9a2089527a9426b70eb796d4e9c234d8804c508b..ddc04fdf1d4ed3afaa8c0c9385ed9739cec39a4b 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -1171,6 +1171,9 @@ struct iio_dev *inv_icm42600_accel_init(struct inv_icm42600_state *st)
 	if (ret)
 		return ERR_PTR(ret);
 
+	/* accel events are wakeup capable */
+	devm_device_init_wakeup(&indio_dev->dev);
+
 	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..0809561b8119d1ff5cf4b36ff571f9c8dc4050a0 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,32 @@ 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);
+	if (wakeup) {
+		/* keep accel on and setup irq for wakeup */
+		accel_conf = st->conf.accel.mode;
+		enable_irq_wake(st->irq);
+		disable_irq(st->irq);
+	} else {
+		/* 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;
 	}
 
 	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 +908,24 @@ 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);
+	/* restore irq state or vddio if cut off */
+	if (wakeup) {
+		enable_irq(st->irq);
+		disable_irq_wake(st->irq);
+	} else {
+		ret = inv_icm42600_enable_regulator_vddio(st);
+		if (ret)
+			goto out_unlock;
+	}
 
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
@@ -911,8 +938,8 @@ static int inv_icm42600_resume(struct device *dev)
 	if (ret)
 		goto out_unlock;
 
-	/* restore APEX features */
-	if (st->apex.wom.enable) {
+	/* restore APEX features if disabled */
+	if (!wakeup && st->apex.wom.enable) {
 		ret = inv_icm42600_enable_wom(st);
 		if (ret)
 			goto out_unlock;

-- 
2.49.0

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
> 
> Add WoM as accel roc rising x|y|z event.

...

> +	if (sleep_ms)
> +		msleep(sleep_ms);

I still wonder if we can get rid of the conditional here.
Would the

	fsleep(sleep_ms * USEC_PER_MSEC)

actually work as expected?

Ditto for other case(s) like this.

...

Overall, looking to this patch again, I think it would be better to prepend it
by replacing *int*_t types by the respective uXX ones. Because in this patch
we add dozens of new ones which increases an unneeded churn in the future.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 09:34:27AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> When Wake-on-Motion is on, enable system wakeup and keep the chip on
> for waking up the system with an interrupt.

...

> +	/* accel events are wakeup capable */
> +	devm_device_init_wakeup(&indio_dev->dev);

No checking for return code? Why is it okay? This needs a really good comment,
and even better a (info / debug) message when it fails if it's not a fatal
error.

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

>
>________________________________________
>From: Andy Shevchenko <andriy.shevchenko@intel.com>
>Sent: Friday, June 13, 2025 10:31
>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 v4 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 Fri, Jun 13, 2025 at 09:34:27AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
>> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>> 
>> When Wake-on-Motion is on, enable system wakeup and keep the chip on
>> for waking up the system with an interrupt.
>
>...
>
>> +	/* accel events are wakeup capable */
>> +	devm_device_init_wakeup(&indio_dev->dev);
>
>No checking for return code? Why is it okay? This needs a really good comment,
>and even better a (info / debug) message when it fails if it's not a fatal
>error.

Sorry, just forgot. I will add it in v5.

>
>-- 
>With Best Regards,
>Andy Shevchenko
>
>
>

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

[Jean-Baptiste Maneyrol]

>
>________________________________________
>From: Andy Shevchenko <andriy.shevchenko@intel.com>
>Sent: Friday, June 13, 2025 10:29
>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 v4 1/2] iio: imu: inv_icm42600: add WoM support
> 
>This Message Is From an External Sender
>This message came from outside your organization.
> 
>On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
>> 
>> Add WoM as accel roc rising x|y|z event.
>
>...
>
>> +	if (sleep_ms)
>> +		msleep(sleep_ms);
>
>I still wonder if we can get rid of the conditional here.
>Would the
>
>	fsleep(sleep_ms * USEC_PER_MSEC)
>
>actually work as expected?
>
>Ditto for other case(s) like this.

fsleep(0) would call udelay(0) which is architecture dependent. It seems like
it may delay for a very little while, but I'm not able to check that.

>
>...
>
>Overall, looking to this patch again, I think it would be better to prepend it
>by replacing *int*_t types by the respective uXX ones. Because in this patch
>we add dozens of new ones which increases an unneeded churn in the future.
>
In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
to keep *int*_t types. If it need to be changed, we can change afterward the
whole driver types with a replace tool and send it in a separate patch.

Jonathan,
what is your statement on this point?

>-- 
>With Best Regards,
>Andy Shevchenko
>

Thanks,
JB

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
> >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >Sent: Friday, June 13, 2025 10:29
> >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

...

> >> +	if (sleep_ms)
> >> +		msleep(sleep_ms);
> >
> >I still wonder if we can get rid of the conditional here.
> >Would the
> >
> >	fsleep(sleep_ms * USEC_PER_MSEC)
> >
> >actually work as expected?
> >
> >Ditto for other case(s) like this.
> 
> fsleep(0) would call udelay(0) which is architecture dependent. It seems like
> it may delay for a very little while, but I'm not able to check that.

Hmm... This is unfortunate. Somebody needs at least make it clear in the
documentation if not yet done.

...

> >Overall, looking to this patch again, I think it would be better to prepend it
> >by replacing *int*_t types by the respective uXX ones. Because in this patch
> >we add dozens of new ones which increases an unneeded churn in the future.
> >
> In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> to keep *int*_t types. If it need to be changed, we can change afterward the
> whole driver types with a replace tool and send it in a separate patch.

It will be never ending story, sorry. We need someone to solve this tech debt.
And since this patch adds more than 3 new users of it, I think it's a candidate
to embrace the burden.

> Jonathan,
> what is your statement on this point?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >Sent: Friday, June 13, 2025 10:29
> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

...

> > >Overall, looking to this patch again, I think it would be better to prepend it
> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
> > >we add dozens of new ones which increases an unneeded churn in the future.
> > >
> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> > to keep *int*_t types. If it need to be changed, we can change afterward the
> > whole driver types with a replace tool and send it in a separate patch.
> 
> It will be never ending story, sorry. We need someone to solve this tech debt.
> And since this patch adds more than 3 new users of it, I think it's a candidate
> to embrace the burden.

For your convenience I can mock-up a change...

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

>________________________________________
>From: Andy Shevchenko <andriy.shevchenko@intel.com>
>Sent: Friday, June 13, 2025 14:54
>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 v4 1/2] iio: imu: inv_icm42600: add WoM support
> 
>This Message Is From an External Sender
>This message came from outside your organization.
> 
>On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:
>> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
>> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
>> > >Sent: Friday, June 13, 2025 10:29
>> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
>
>...
>
>> > >Overall, looking to this patch again, I think it would be better to prepend it
>> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
>> > >we add dozens of new ones which increases an unneeded churn in the future.
>> > >
>> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
>> > to keep *int*_t types. If it need to be changed, we can change afterward the
>> > whole driver types with a replace tool and send it in a separate patch.
>> 
>> It will be never ending story, sorry. We need someone to solve this tech debt.
>> And since this patch adds more than 3 new users of it, I think it's a candidate
>> to embrace the burden.
>
>For your convenience I can mock-up a change...

It looks like there's something I don't understand in the kernel Documentation about
types then.
Quoting Documentation/process/coding-style.rst, section 5.d:
---
New types which are identical to standard C99 types, in certain exceptional circumstances.

Although it would only take a short amount of time for the eyes and brain to become accustomed
to the standard types like uint32_t, some people object to their use anyway.

Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
identical to standard types are permitted -- although they are not mandatory in new code
of your own.

When editing existing code which already uses one or the other set of types, you should
conform to the existing choices in that code.
---

My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
But you need to conform afterward to the existing choice. That's why this driver was
done initially with *int*_t types, and that patches are conforming to this choice.

By looking at all Linux drivers, there are plenty of them using *int*_t, even
inside iio:
adc/xilinx-xadc.h:	uint16_t threshold[16];
adc/xilinx-xadc.h:	uint16_t temp_hysteresis;
adc/xilinx-xadc.h:	uint16_t *data;
adc/xilinx-xadc.h:	int (*read)(struct xadc *xadc, unsigned int reg, uint16_t *val);
adc/xilinx-xadc.h:	int (*write)(struct xadc *xadc, unsigned int reg, uint16_t val);
adc/xilinx-xadc.h:	uint16_t *val)
adc/xilinx-xadc.h:	uint16_t val)
adc/xilinx-xadc.h:	uint16_t *val)
adc/xilinx-xadc.h:	uint16_t val)
adc/xilinx-xadc-events.c:	uint16_t cfg, old_cfg;
adc/xilinx-xadc-core.c:	uint16_t val)
adc/xilinx-xadc-core.c:	uint16_t *val)
adc/xilinx-xadc-core.c:	uint16_t *val)
adc/xilinx-xadc-core.c:	uint16_t val)
adc/xilinx-xadc-core.c:	uint16_t mask, uint16_t val)
adc/xilinx-xadc-core.c:	uint16_t tmp;
adc/xilinx-xadc-core.c:	uint16_t mask, uint16_t val)
adc/xilinx-xadc-core.c:	uint16_t val;
adc/xilinx-xadc-core.c:	uint16_t val16;
adc/xilinx-xadc-core.c:	uint16_t val16;
chemical/scd4x.c:static int scd4x_write(struct scd4x_state *state, enum scd4x_cmd cmd, uint16_t arg)
chemical/scd4x.c:				uint16_t arg, void *response, int response_sz)
chemical/scd4x.c:static int scd4x_read_meas(struct scd4x_state *state, uint16_t *meas)
chemical/scd4x.c:		uint16_t val;
chemical/scd4x.c:static int scd4x_read_poll(struct scd4x_state *state, uint16_t *buf)
chemical/scd4x.c:	uint16_t buf[3];
chemical/scd4x.c:	uint16_t value;
chemical/scd4x.c:	uint16_t val, arg;
chemical/scd4x.c:		uint16_t data[3];
dac/ad7303.c:	uint16_t config;
dac/ti-dac7612.c:	uint16_t cache[2];
dac/ad5766.c:	uint16_t val;
dac/ad5766.c:	uint16_t val;
dac/ad5766.c:	uint16_t val;
dac/ad5449.c:	uint16_t dac_cache[AD5449_MAX_CHANNELS];
dac/ad8460.c:	uint16_t sym;
gyro/adis16136.c:	uint16_t lot1, lot2, lot3, serial;
gyro/adis16136.c:	uint16_t flash_count;
gyro/adis16136.c:	uint16_t t;
gyro/adis16136.c:	uint16_t val16;
gyro/adis16136.c:	uint16_t prod_id;
humidity/ens210.c:	uint16_t part_id;
imu/adis16400.c:	uint16_t prod_id;
imu/adis16400.c:	uint16_t flash_count;
imu/adis16400.c:	uint16_t t;
imu/adis16400.c:	uint16_t t;
imu/adis16400.c:	uint16_t val16;
imu/adis16400.c:	uint16_t prod_id, smp_prd;
imu/adis16400.c:	int16_t val16;
imu/adis16460.c:	uint16_t t;
...


Then, why it is mandatory to change this driver to use uXX instead?

>
>-- 
>With Best Regards,
>Andy Shevchenko
>

Thanks,
JB

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 01:43:58PM +0000, Jean-Baptiste Maneyrol wrote:
> >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >Sent: Friday, June 13, 2025 14:54
> >On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:
> >> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
> >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >> > >Sent: Friday, June 13, 2025 10:29
> >> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

...

> >> > >Overall, looking to this patch again, I think it would be better to prepend it
> >> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
> >> > >we add dozens of new ones which increases an unneeded churn in the future.
> >> > >
> >> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> >> > to keep *int*_t types. If it need to be changed, we can change afterward the
> >> > whole driver types with a replace tool and send it in a separate patch.
> >> 
> >> It will be never ending story, sorry. We need someone to solve this tech debt.
> >> And since this patch adds more than 3 new users of it, I think it's a candidate
> >> to embrace the burden.
> >
> >For your convenience I can mock-up a change...
> 
> It looks like there's something I don't understand in the kernel Documentation about
> types then.
> Quoting Documentation/process/coding-style.rst, section 5.d:
> ---
> New types which are identical to standard C99 types, in certain exceptional circumstances.
> 
> Although it would only take a short amount of time for the eyes and brain to become accustomed
> to the standard types like uint32_t, some people object to their use anyway.
> 
> Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
> identical to standard types are permitted -- although they are not mandatory in new code
> of your own.
> 
> When editing existing code which already uses one or the other set of types, you should
> conform to the existing choices in that code.
> ---
> 
> My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
> But you need to conform afterward to the existing choice. That's why this driver was
> done initially with *int*_t types, and that patches are conforming to this choice.

This part of the documentation has a lot of room for different interpretations.
One [1] may consider this as uXX superior, another, like you, that it's okay
to use.  In any case Greg KH prefers uXX over uintXX_t. And he is also in
the chain of maintainers here. Feel free to amend the Documentation. But
be sure all stakeholders will see your proposal (like Greg KH and other
key maintainers).

> By looking at all Linux drivers, there are plenty of them using *int*_t, even
> inside iio:

$ git grep -l 'u\?int[0-9][0-9]\?_t' -- drivers/iio/ | wc -l
59

$ git ls-files drivers/iio*.c | wc -l
640

Less than 10%.

> Then, why it is mandatory to change this driver to use uXX instead?

TO be consistent. With the above wording in the documentation I may argue that
entire subsystem should be consistent and at least in IIO we have tons of patch
series that are against the whole subsystem to do one style change or another
(look at the recent memset() vs. {} for initialisation).

[1] https://lore.kernel.org/all/20250409180953.398686-1-matchstick@neverthere.org/

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 05:41:47PM +0300, Andy Shevchenko wrote:
> On Fri, Jun 13, 2025 at 01:43:58PM +0000, Jean-Baptiste Maneyrol wrote:
> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >Sent: Friday, June 13, 2025 14:54
> > >On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:
> > >> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >> > >Sent: Friday, June 13, 2025 10:29
> > >> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

...

> > >> > >Overall, looking to this patch again, I think it would be better to prepend it
> > >> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
> > >> > >we add dozens of new ones which increases an unneeded churn in the future.
> > >> > >
> > >> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> > >> > to keep *int*_t types. If it need to be changed, we can change afterward the
> > >> > whole driver types with a replace tool and send it in a separate patch.
> > >> 
> > >> It will be never ending story, sorry. We need someone to solve this tech debt.
> > >> And since this patch adds more than 3 new users of it, I think it's a candidate
> > >> to embrace the burden.
> > >
> > >For your convenience I can mock-up a change...
> > 
> > It looks like there's something I don't understand in the kernel Documentation about
> > types then.
> > Quoting Documentation/process/coding-style.rst, section 5.d:
> > ---
> > New types which are identical to standard C99 types, in certain exceptional circumstances.
> > 
> > Although it would only take a short amount of time for the eyes and brain to become accustomed
> > to the standard types like uint32_t, some people object to their use anyway.
> > 
> > Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
> > identical to standard types are permitted -- although they are not mandatory in new code
> > of your own.
> > 
> > When editing existing code which already uses one or the other set of types, you should
> > conform to the existing choices in that code.
> > ---
> > 
> > My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
> > But you need to conform afterward to the existing choice. That's why this driver was
> > done initially with *int*_t types, and that patches are conforming to this choice.
> 
> This part of the documentation has a lot of room for different interpretations.
> One [1] may consider this as uXX superior, another, like you, that it's okay
> to use.  In any case Greg KH prefers uXX over uintXX_t. And he is also in
> the chain of maintainers here. Feel free to amend the Documentation. But
> be sure all stakeholders will see your proposal (like Greg KH and other
> key maintainers).
> 
> > By looking at all Linux drivers, there are plenty of them using *int*_t, even
> > inside iio:
> 
> $ git grep -l 'u\?int[0-9][0-9]\?_t' -- drivers/iio/ | wc -l
> 59
> 
> $ git ls-files drivers/iio*.c | wc -l
> 640
> 
> Less than 10%.
> 
> > Then, why it is mandatory to change this driver to use uXX instead?
> 
> TO be consistent. With the above wording in the documentation I may argue that
> entire subsystem should be consistent and at least in IIO we have tons of patch
> series that are against the whole subsystem to do one style change or another
> (look at the recent memset() vs. {} for initialisation).
> 
> [1] https://lore.kernel.org/all/20250409180953.398686-1-matchstick@neverthere.org/

Oh, this [2] is golden!
YUou may found support for your arguments and for mine in that thread, but the
bottom line is: what do maintainers of IIO prefer? (Taking into account that it
goes via Greg KH)


[2]: https://lore.kernel.org/all/20210423230609.13519-1-alx.manpages@gmail.com/

-- 
With Best Regards,
Andy Shevchenko

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

[Jean-Baptiste Maneyrol]

>
>
>________________________________________
>From: Andy Shevchenko <andriy.shevchenko@intel.com>
>Sent: Friday, June 13, 2025 17:01
>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 v4 1/2] iio: imu: inv_icm42600: add WoM support
> 
>This Message Is From an External Sender
>This message came from outside your organization.
> 
>On Fri, Jun 13, 2025 at 05:41:47PM +0300, Andy Shevchenko wrote:
>> On Fri, Jun 13, 2025 at 01:43:58PM +0000, Jean-Baptiste Maneyrol wrote:
>> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
>> > >Sent: Friday, June 13, 2025 14:54
>> > >On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:
>> > >> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
>> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
>> > >> > >Sent: Friday, June 13, 2025 10:29
>> > >> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
>
>...
>
>> > >> > >Overall, looking to this patch again, I think it would be better to prepend it
>> > >> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
>> > >> > >we add dozens of new ones which increases an unneeded churn in the future.
>> > >> > >
>> > >> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
>> > >> > to keep *int*_t types. If it need to be changed, we can change afterward the
>> > >> > whole driver types with a replace tool and send it in a separate patch.
>> > >> 
>> > >> It will be never ending story, sorry. We need someone to solve this tech debt.
>> > >> And since this patch adds more than 3 new users of it, I think it's a candidate
>> > >> to embrace the burden.
>> > >
>> > >For your convenience I can mock-up a change...
>> > 
>> > It looks like there's something I don't understand in the kernel Documentation about
>> > types then.
>> > Quoting Documentation/process/coding-style.rst, section 5.d:
>> > ---
>> > New types which are identical to standard C99 types, in certain exceptional circumstances.
>> > 
>> > Although it would only take a short amount of time for the eyes and brain to become accustomed
>> > to the standard types like uint32_t, some people object to their use anyway.
>> > 
>> > Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
>> > identical to standard types are permitted -- although they are not mandatory in new code
>> > of your own.
>> > 
>> > When editing existing code which already uses one or the other set of types, you should
>> > conform to the existing choices in that code.
>> > ---
>> > 
>> > My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
>> > But you need to conform afterward to the existing choice. That's why this driver was
>> > done initially with *int*_t types, and that patches are conforming to this choice.
>> 
>> This part of the documentation has a lot of room for different interpretations.
>> One [1] may consider this as uXX superior, another, like you, that it's okay
>> to use.  In any case Greg KH prefers uXX over uintXX_t. And he is also in
>> the chain of maintainers here. Feel free to amend the Documentation. But
>> be sure all stakeholders will see your proposal (like Greg KH and other
>> key maintainers).
>> 
>> > By looking at all Linux drivers, there are plenty of them using *int*_t, even
>> > inside iio:
>> 
>> $ git grep -l 'u\?int[0-9][0-9]\?_t' -- drivers/iio/ | wc -l
>> 59
>> 
>> $ git ls-files drivers/iio*.c | wc -l
>> 640
>> 
>> Less than 10%.
>> 
>> > Then, why it is mandatory to change this driver to use uXX instead?
>> 
>> TO be consistent. With the above wording in the documentation I may argue that
>> entire subsystem should be consistent and at least in IIO we have tons of patch
>> series that are against the whole subsystem to do one style change or another
>> (look at the recent memset() vs. {} for initialisation).
>> 
>> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20250409180953.398686-1-matchstick@neverthere.org/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiGbYQqsw$[lore[.]kernel[.]org]
>
>Oh, this [2] is golden!
>YUou may found support for your arguments and for mine in that thread, but the
>bottom line is: what do maintainers of IIO prefer? (Taking into account that it
>goes via Greg KH)
>
>
>[2]: https://urldefense.com/v3/__https://lore.kernel.org/all/20210423230609.13519-1-alx.manpages@gmail.com/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiuFc54eI$[lore[.]kernel[.]org]
>

If this is required, I can do it. I would just want to know if this is mandatory
since we already have a couple of drivers merged using standard types and
other drivers planned to be merged.

Can I do it in the same series or should it be in a separate patch before this
series?


>-- 
>With Best Regards,
>Andy Shevchenko
>
>

Thanks,
JB

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

[Andy Shevchenko]

On Fri, Jun 13, 2025 at 8:14 PM Jean-Baptiste Maneyrol
<Jean-Baptiste.Maneyrol@tdk.com> wrote:
 >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >Sent: Friday, June 13, 2025 17:01
> >On Fri, Jun 13, 2025 at 05:41:47PM +0300, Andy Shevchenko wrote:
> >> On Fri, Jun 13, 2025 at 01:43:58PM +0000, Jean-Baptiste Maneyrol wrote:
> >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >> > >Sent: Friday, June 13, 2025 14:54
> >> > >On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:
> >> > >> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:
> >> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >> > >> > >Sent: Friday, June 13, 2025 10:29
> >> > >> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

...

> >> > >> > >Overall, looking to this patch again, I think it would be better to prepend it
> >> > >> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
> >> > >> > >we add dozens of new ones which increases an unneeded churn in the future.
> >> > >> > >
> >> > >> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> >> > >> > to keep *int*_t types. If it need to be changed, we can change afterward the
> >> > >> > whole driver types with a replace tool and send it in a separate patch.
> >> > >>
> >> > >> It will be never ending story, sorry. We need someone to solve this tech debt.
> >> > >> And since this patch adds more than 3 new users of it, I think it's a candidate
> >> > >> to embrace the burden.
> >> > >
> >> > >For your convenience I can mock-up a change...
> >> >
> >> > It looks like there's something I don't understand in the kernel Documentation about
> >> > types then.
> >> > Quoting Documentation/process/coding-style.rst, section 5.d:
> >> > ---
> >> > New types which are identical to standard C99 types, in certain exceptional circumstances.
> >> >
> >> > Although it would only take a short amount of time for the eyes and brain to become accustomed
> >> > to the standard types like uint32_t, some people object to their use anyway.
> >> >
> >> > Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
> >> > identical to standard types are permitted -- although they are not mandatory in new code
> >> > of your own.
> >> >
> >> > When editing existing code which already uses one or the other set of types, you should
> >> > conform to the existing choices in that code.
> >> > ---
> >> >
> >> > My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
> >> > But you need to conform afterward to the existing choice. That's why this driver was
> >> > done initially with *int*_t types, and that patches are conforming to this choice.
> >>
> >> This part of the documentation has a lot of room for different interpretations.
> >> One [1] may consider this as uXX superior, another, like you, that it's okay
> >> to use.  In any case Greg KH prefers uXX over uintXX_t. And he is also in
> >> the chain of maintainers here. Feel free to amend the Documentation. But
> >> be sure all stakeholders will see your proposal (like Greg KH and other
> >> key maintainers).
> >>
> >> > By looking at all Linux drivers, there are plenty of them using *int*_t, even
> >> > inside iio:
> >>
> >> $ git grep -l 'u\?int[0-9][0-9]\?_t' -- drivers/iio/ | wc -l
> >> 59
> >>
> >> $ git ls-files drivers/iio*.c | wc -l
> >> 640
> >>
> >> Less than 10%.
> >>
> >> > Then, why it is mandatory to change this driver to use uXX instead?
> >>
> >> TO be consistent. With the above wording in the documentation I may argue that
> >> entire subsystem should be consistent and at least in IIO we have tons of patch
> >> series that are against the whole subsystem to do one style change or another
> >> (look at the recent memset() vs. {} for initialisation).
> >>
> >> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20250409180953.398686-1-matchstick@neverthere.org/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiGbYQqsw$[lore[.]kernel[.]org]
> >
> >Oh, this [2] is golden!
> >You may found support for your arguments and for mine in that thread, but the
> >bottom line is: what do maintainers of IIO prefer? (Taking into account that it
> >goes via Greg KH)
> >
> >[2]: https://urldefense.com/v3/__https://lore.kernel.org/all/20210423230609.13519-1-alx.manpages@gmail.com/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiuFc54eI$[lore[.]kernel[.]org]
>
> If this is required, I can do it. I would just want to know if this is mandatory
> since we already have a couple of drivers merged using standard types and
> other drivers planned to be merged.

Let's wait for others to speak up. Especially maintainers.

> Can I do it in the same series or should it be in a separate patch before this
> series?

Same series, just a prerequisite patch. Note, I have one locally, I
just need to send it and you can use it, but the reason why I haven't
sent is the same — I want to know the official position of the IIO
subsystem about this.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Sat, 14 Jun 2025 00:02:52 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jun 13, 2025 at 8:14 PM Jean-Baptiste Maneyrol
> <Jean-Baptiste.Maneyrol@tdk.com> wrote:
>  >From: Andy Shevchenko <andriy.shevchenko@intel.com>  
> > >Sent: Friday, June 13, 2025 17:01
> > >On Fri, Jun 13, 2025 at 05:41:47PM +0300, Andy Shevchenko wrote:  
> > >> On Fri, Jun 13, 2025 at 01:43:58PM +0000, Jean-Baptiste Maneyrol wrote:  
> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >> > >Sent: Friday, June 13, 2025 14:54
> > >> > >On Fri, Jun 13, 2025 at 03:53:36PM +0300, Andy Shevchenko wrote:  
> > >> > >> On Fri, Jun 13, 2025 at 12:46:46PM +0000, Jean-Baptiste Maneyrol wrote:  
> > >> > >> > >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > >> > >> > >Sent: Friday, June 13, 2025 10:29
> > >> > >> > >On Fri, Jun 13, 2025 at 09:34:26AM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:  
> 
> ...
> 
> > >> > >> > >Overall, looking to this patch again, I think it would be better to prepend it
> > >> > >> > >by replacing *int*_t types by the respective uXX ones. Because in this patch
> > >> > >> > >we add dozens of new ones which increases an unneeded churn in the future.
> > >> > >> > >  
> > >> > >> > In my opinion, to respect the rule don't mix *int*_t and uXX types, it is better
> > >> > >> > to keep *int*_t types. If it need to be changed, we can change afterward the
> > >> > >> > whole driver types with a replace tool and send it in a separate patch.  
> > >> > >>
> > >> > >> It will be never ending story, sorry. We need someone to solve this tech debt.
> > >> > >> And since this patch adds more than 3 new users of it, I think it's a candidate
> > >> > >> to embrace the burden.  
> > >> > >
> > >> > >For your convenience I can mock-up a change...  
> > >> >
> > >> > It looks like there's something I don't understand in the kernel Documentation about
> > >> > types then.
> > >> > Quoting Documentation/process/coding-style.rst, section 5.d:
> > >> > ---
> > >> > New types which are identical to standard C99 types, in certain exceptional circumstances.
> > >> >
> > >> > Although it would only take a short amount of time for the eyes and brain to become accustomed
> > >> > to the standard types like uint32_t, some people object to their use anyway.
> > >> >
> > >> > Therefore, the Linux-specific u8/u16/u32/u64 types and their signed equivalents which are
> > >> > identical to standard types are permitted -- although they are not mandatory in new code
> > >> > of your own.
> > >> >
> > >> > When editing existing code which already uses one or the other set of types, you should
> > >> > conform to the existing choices in that code.
> > >> > ---
> > >> >
> > >> > My understanding is that uXX are not mandatory for new code. You can use types like *int*_t.
> > >> > But you need to conform afterward to the existing choice. That's why this driver was
> > >> > done initially with *int*_t types, and that patches are conforming to this choice.  
> > >>
> > >> This part of the documentation has a lot of room for different interpretations.
> > >> One [1] may consider this as uXX superior, another, like you, that it's okay
> > >> to use.  In any case Greg KH prefers uXX over uintXX_t. And he is also in
> > >> the chain of maintainers here. Feel free to amend the Documentation. But
> > >> be sure all stakeholders will see your proposal (like Greg KH and other
> > >> key maintainers).
> > >>  
> > >> > By looking at all Linux drivers, there are plenty of them using *int*_t, even
> > >> > inside iio:  
> > >>
> > >> $ git grep -l 'u\?int[0-9][0-9]\?_t' -- drivers/iio/ | wc -l
> > >> 59
> > >>
> > >> $ git ls-files drivers/iio*.c | wc -l
> > >> 640
> > >>
> > >> Less than 10%.
> > >>  
> > >> > Then, why it is mandatory to change this driver to use uXX instead?  
> > >>
> > >> TO be consistent. With the above wording in the documentation I may argue that
> > >> entire subsystem should be consistent and at least in IIO we have tons of patch
> > >> series that are against the whole subsystem to do one style change or another
> > >> (look at the recent memset() vs. {} for initialisation).
> > >>
> > >> [1] https://urldefense.com/v3/__https://lore.kernel.org/all/20250409180953.398686-1-matchstick@neverthere.org/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiGbYQqsw$[lore[.]kernel[.]org]  
> > >
> > >Oh, this [2] is golden!
> > >You may found support for your arguments and for mine in that thread, but the
> > >bottom line is: what do maintainers of IIO prefer? (Taking into account that it
> > >goes via Greg KH)
> > >
> > >[2]: https://urldefense.com/v3/__https://lore.kernel.org/all/20210423230609.13519-1-alx.manpages@gmail.com/__;!!FtrhtPsWDhZ6tw!DVTvkgDsymM7132dB-wjei-s0JxYiivZxtzEHfWjsrn_6toqTXA__hm2nPUh7jmectCXcP9Z3OAh0hMm-WD6eQAHOtdiuFc54eI$[lore[.]kernel[.]org]  
> >
> > If this is required, I can do it. I would just want to know if this is mandatory
> > since we already have a couple of drivers merged using standard types and
> > other drivers planned to be merged.  
> 
> Let's wait for others to speak up. Especially maintainers.
> 
> > Can I do it in the same series or should it be in a separate patch before this
> > series?  
> 
> Same series, just a prerequisite patch. Note, I have one locally, I
> just need to send it and you can use it, but the reason why I haven't
> sent is the same — I want to know the official position of the IIO
> subsystem about this.
> 

I'm generally not keen on taking strong opinions that add a barrier to
code submission / new contributors when there are arguments both ways.
I am getting fussier on style though in my old age - mostly driven by
just how many drivers we now have and the advantages of consistency
of arbitrary decisions.

So I'll state a strong preference for the kernel internal types u8 etc
and would like to see patches converting larger drivers that are
significantly copied for new code over like this one.

Thanks

Jonathan

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

[Jonathan Cameron]

On Fri, 13 Jun 2025 09:34:26 +0200
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>
Hi Jean-Baptiste.

A couple of comments inline.
Ideally pull the movement of the timestamp struct to before the DMA safe
buffers to a precursor patch.   That is a bit subtle to have hiding in here.

The guards thing can be for next time you are doing a cleanup series on this
driver if you prefer.

Jonathan

> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  54 +++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 289 ++++++++++++++++++++-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  58 +++++
>  4 files changed, 395 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..413a15493bcb880dc00b20da3b3168d5addd32a9 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 {
> +		uint64_t value;
> +		bool enable;
> +	} 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 (Advanced Pedometer and Event detection) 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;
This was a bit subtle and had me going for a minute.
The timestamp should never have been at this location in the structure because
it's mid way through various regions with forced alignment.  It isn't actually a bug
I think though (beyond unnecessary padding) because the fifo struct obeyed c spec rule
that anything after it must be aligned to it's largest aligned element which was
IIO_DMA_MINALIGN.

Maybe move this in a precursor patch where you can talk about whether it was a problem
or not?

> +	struct inv_icm42600_apex apex;
> +	struct inv_icm42600_fifo fifo;
> +	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>  };
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index e6cd9dcb0687d19554e63a69dc60f065c58d70ee..9a2089527a9426b70eb796d4e9c234d8804c508b 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c



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

One for another day, but this would definitely benefit from some guard() magic
and there are a few other bits of existing code that would as well.


> +	}
> +
>  	/* restore FIFO data streaming */
>  	if (st->fifo.on) {
>  		inv_sensors_timestamp_reset(&gyro_st->ts);
> 

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

[Jean-Baptiste Maneyrol]

>
>________________________________________
>From: Jonathan Cameron <jic23@kernel.org>
>Sent: Saturday, June 14, 2025 14:53
>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>; 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 v4 1/2] iio: imu: inv_icm42600: add WoM support
> 
>This Message Is From an External Sender
>This message came from outside your organization.
> 
>On Fri, 13 Jun 2025 09:34:26 +0200
>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>
>Hi Jean-Baptiste.
>
>A couple of comments inline.
>Ideally pull the movement of the timestamp struct to before the DMA safe
>buffers to a precursor patch.   That is a bit subtle to have hiding in here.
>
>The guards thing can be for next time you are doing a cleanup series on this
>driver if you prefer.
>
>Jonathan

Hello Jonathan,

concerning the full driver rewrite asked by Andy to switch to uXX/sXX kernel types,
can I put it inside this series?

Otherwise, should it be in a separate patch and perhaps with a fixed tag so it
can be backported to enable automatic backport of further fix patches?

Or can it be after this series is accepted? I would prefer that.

Thanks for your help here.

>
>> ---
>>  drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  54 +++-
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  | 289 ++++++++++++++++++++-
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c |   2 +-
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_core.c   |  58 +++++
>>  4 files changed, 395 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..413a15493bcb880dc00b20da3b3168d5addd32a9 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 {
>> +		uint64_t value;
>> +		bool enable;
>> +	} 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 (Advanced Pedometer and Event detection) 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;
>This was a bit subtle and had me going for a minute.
>The timestamp should never have been at this location in the structure because
>it's mid way through various regions with forced alignment.  It isn't actually a bug
>I think though (beyond unnecessary padding) because the fifo struct obeyed c spec rule
>that anything after it must be aligned to it's largest aligned element which was
>IIO_DMA_MINALIGN.
>
>Maybe move this in a precursor patch where you can talk about whether it was a problem
>or not?

I can move it in a separate patch at the beginning of the series. This fix was asked
by you to avoid potential hard bugs, but it dates sorry.

>
>> +	struct inv_icm42600_apex apex;
>> +	struct inv_icm42600_fifo fifo;
>> +	uint8_t buffer[3] __aligned(IIO_DMA_MINALIGN);
>>  };
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> index e6cd9dcb0687d19554e63a69dc60f065c58d70ee..9a2089527a9426b70eb796d4e9c234d8804c508b 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>
>
>
>> @@ -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;
>
>One for another day, but this would definitely benefit from some guard() magic
>and there are a few other bits of existing code that would as well.

Same here, it was decided long ago in the first series to not switch to guard()
yet but later.

>
>
>> +	}
>> +
>>  	/* restore FIFO data streaming */
>>  	if (st->fifo.on) {
>>  		inv_sensors_timestamp_reset(&gyro_st->ts);
>> 
>
>

Thanks,
JB

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

[Jonathan Cameron]

On Mon, 16 Jun 2025 07:42:16 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> >
> >________________________________________
> >From: Jonathan Cameron <jic23@kernel.org>
> >Sent: Saturday, June 14, 2025 14:53
> >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>; 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 v4 1/2] iio: imu: inv_icm42600: add WoM support
> > 
> >This Message Is From an External Sender
> >This message came from outside your organization.
> > 
> >On Fri, 13 Jun 2025 09:34:26 +0200
> >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>  
> >Hi Jean-Baptiste.
> >
> >A couple of comments inline.
> >Ideally pull the movement of the timestamp struct to before the DMA safe
> >buffers to a precursor patch.   That is a bit subtle to have hiding in here.
> >
> >The guards thing can be for next time you are doing a cleanup series on this
> >driver if you prefer.
> >
> >Jonathan  
> 
> Hello Jonathan,
> 
> concerning the full driver rewrite asked by Andy to switch to uXX/sXX kernel types,
> can I put it inside this series?

Sure.

> 
> Otherwise, should it be in a separate patch and perhaps with a fixed tag so it
> can be backported to enable automatic backport of further fix patches?

It shouldn't be fixes tagged as that won't be a fix as such.
Backport wise we might need to call it out the first time something is based
on it but the stable maintainers are pretty good at spotting these sorts
of broad mechanical changes that enable a later fix so they'll probably just
pick it up when needed.

> 
> Or can it be after this series is accepted? I would prefer that.

I want the end result with the kernel types, but not that fussed on ordering.
Whilst it may seem churn heavy this stuff is usually reasonably easy to
result when fixes cross such a patch.

I'll catch up with the other thread as I see there is already such a patch
being discussed.
> >>  /**
> >>   *  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 (Advanced Pedometer and Event detection) 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;  
> >This was a bit subtle and had me going for a minute.
> >The timestamp should never have been at this location in the structure because
> >it's mid way through various regions with forced alignment.  It isn't actually a bug
> >I think though (beyond unnecessary padding) because the fifo struct obeyed c spec rule
> >that anything after it must be aligned to it's largest aligned element which was
> >IIO_DMA_MINALIGN.
> >
> >Maybe move this in a precursor patch where you can talk about whether it was a problem
> >or not?  
> 
> I can move it in a separate patch at the beginning of the series. This fix was asked
> by you to avoid potential hard bugs, but it dates sorry.

yeah. I was wrong I think but definitely subtle kind of code that we don't want
if we can avoid it.  A precursor tidying it up with the reasoning would be good
from a reviewability point of view.

Jonathan