[PATCH 0/4] inv_icm42600 driver enhancements

[PATCH 0/4] inv_icm42600 driver enhancements

[Jean-Baptiste Maneyrol via B4 Relay]

This series add enhancements patches to inv_icm42600 driver.

It includes:
1. Move of MEMS stabilization sleep before FIFO configuration to avoid
   pushing incorrect data.
2. Use a fixed FIFO frame of 16 bits to avoid a bug of dynamic FIFO
   configuration changed triggered by the sleep move.
3. Simplify watermark computation using gcd.
4. Enhance support of high frequency streaming by avoiding read of FIFO
   count for watermark interrupt.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
Jean-Baptiste Maneyrol (4):
      iio: imu: inv_icm42600: sleep before enabling FIFO data
      iio: imu: inv_icm42600: use 2 sensors fixed packet size of 16 bytes
      iio: imu: inv_icm42600: simplify watermark computation by using gcd
      iio: imu: inv_icm42600: do not read FIFO count for watermark it

 drivers/iio/imu/inv_icm42600/inv_icm42600.h        |   4 +-
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c  |  12 ++-
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 109 +++++++++------------
 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c   |  12 ++-
 4 files changed, 62 insertions(+), 75 deletions(-)
---
base-commit: 53c022f51fbe540fffee2121ce9ed3e15266c9a4
change-id: 20260709-inv-icm42600-enhacements-1e0e18e6f868

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

[PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data

[Jean-Baptiste Maneyrol via B4 Relay]

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

When turning accel or gyro on, a sleep is required for letting the
mechanical part stabilize before reporting correct data. Do the sleep
before enabling FIFO data to prevent push of incorrect data. This is
required if the FIFO is already on because a sensor is already running.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 12 ++++++++----
 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c  | 12 ++++++++----
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
index 4b0e3cd8a506..d889c1dceec1 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
@@ -254,15 +254,19 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
 		fifo_en |= INV_ICM42600_SENSOR_ACCEL;
 	}
 
+	/* sleep maximum required time */
+	sleep = max(sleep_accel, sleep_temp);
+	if (sleep) {
+		mutex_unlock(&st->lock);
+		msleep(sleep);
+		mutex_lock(&st->lock);
+	}
+
 	/* update data FIFO write */
 	ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
 
 out_unlock:
 	mutex_unlock(&st->lock);
-	/* sleep maximum required time */
-	sleep = max(sleep_accel, sleep_temp);
-	if (sleep)
-		msleep(sleep);
 	return ret;
 }
 
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
index 253bf571439d..34037b9c1f82 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
@@ -126,15 +126,19 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
 		fifo_en |= INV_ICM42600_SENSOR_GYRO;
 	}
 
+	/* sleep maximum required time */
+	sleep = max(sleep_gyro, sleep_temp);
+	if (sleep) {
+		mutex_unlock(&st->lock);
+		msleep(sleep);
+		mutex_lock(&st->lock);
+	}
+
 	/* update data FIFO write */
 	ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
 
 out_unlock:
 	mutex_unlock(&st->lock);
-	/* sleep maximum required time */
-	sleep = max(sleep_gyro, sleep_temp);
-	if (sleep)
-		msleep(sleep);
 	return ret;
 }
 

-- 
2.55.0

[PATCH 2/4] iio: imu: inv_icm42600: use 2 sensors fixed packet size of 16 bytes

[Jean-Baptiste Maneyrol via B4 Relay]

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

Sometimes dynamic switch between 1 sensor frame to 2 sensors frame is
not working when there are too much frames already in the FIFO. By
moving accel/gyro on sleep before turning FIFO on, we are storing many
FIFO frames before updating the frame format hitting the bug everytime.

Fix that by always using the 2 sensors frame of 16 bytes. Also update
the hwfifo max watermark reported.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600.h        |  4 +--
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 31 +++++++---------------
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
index b55d993f0264..f6c7c84c7e45 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
@@ -356,8 +356,8 @@ struct inv_icm42600_sensor_state {
 		cpu_to_le16((_wm) & GENMASK(11, 0))
 /* FIFO is 2048 bytes, let 12 samples for reading latency */
 #define INV_ICM42600_FIFO_WATERMARK_MAX			(2048 - 12 * 16)
-/* INV_ICM42600_FIFO_WATERMARK_MAX / 8 = 232 */
-#define INV_ICM42600_FIFO_WATERMARK_MAX_SAMPLES		232
+/* INV_ICM42600_FIFO_WATERMARK_MAX / 16 = 116 */
+#define INV_ICM42600_FIFO_WATERMARK_MAX_SAMPLES		116
 
 #define INV_ICM42600_REG_INT_CONFIG1			0x0064
 #define INV_ICM42600_INT_CONFIG1_TPULSE_DURATION	BIT(6)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
index 998d312f7bde..043ae9deee65 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
@@ -131,13 +131,13 @@ int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
 		INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
 		INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
 
-	val = 0;
-	if (fifo_en & INV_ICM42600_SENSOR_GYRO)
-		val |= INV_ICM42600_FIFO_CONFIG1_GYRO_EN;
-	if (fifo_en & INV_ICM42600_SENSOR_ACCEL)
-		val |= INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
-	if (fifo_en & INV_ICM42600_SENSOR_TEMP)
-		val |= INV_ICM42600_FIFO_CONFIG1_TEMP_EN;
+	/* always enable/disable all bits for allowing dynamic transitions */
+	if (fifo_en)
+		val = INV_ICM42600_FIFO_CONFIG1_TEMP_EN |
+		      INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
+		      INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
+	else
+		val = 0;
 
 	ret = regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1, mask, val);
 	if (ret)
@@ -149,19 +149,6 @@ int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
 	return 0;
 }
 
-static size_t inv_icm42600_get_packet_size(unsigned int fifo_en)
-{
-	size_t packet_size;
-
-	if ((fifo_en & INV_ICM42600_SENSOR_GYRO) &&
-	    (fifo_en & INV_ICM42600_SENSOR_ACCEL))
-		packet_size = INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
-	else
-		packet_size = INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;
-
-	return packet_size;
-}
-
 static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,
 					     size_t packet_size)
 {
@@ -209,7 +196,7 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
 	__le16 raw_wm;
 	int ret;
 
-	packet_size = inv_icm42600_get_packet_size(st->fifo.en);
+	packet_size = INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
 
 	/* compute sensors latency, depending on sensor watermark and odr */
 	wm_gyro = inv_icm42600_wm_truncate(st->fifo.watermark.gyro, packet_size);
@@ -495,7 +482,7 @@ int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
 	/* compute maximum FIFO read size (watermark for max = 0 interrupt case) */
 	if (max == 0)
 		max = st->fifo.watermark.value;
-	max_count = max * inv_icm42600_get_packet_size(st->fifo.en);
+	max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
 
 	/* read FIFO count value */
 	raw_fifo_count = (__be16 *)st->buffer;

-- 
2.55.0

[PATCH 3/4] iio: imu: inv_icm42600: simplify watermark computation by using gcd

[Jean-Baptiste Maneyrol via B4 Relay]

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

The watermark computation was in fact resulting in computing the gcd
of the latencies when both sensors are on. Gcd is required because
of the IIO buffer watermark. We need to use a divider of IIO buffer
watermark, otherwise we will overflow the requested watermark.

Move to use gcd and update documentation accordingly.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 47 ++++++++++------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
index 043ae9deee65..1428f18408ce 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
@@ -5,6 +5,7 @@
 
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gcd.h>
 #include <linux/kernel.h>
 #include <linux/minmax.h>
 #include <linux/mutex.h>
@@ -172,15 +173,14 @@ static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,
  *
  * FIFO watermark threshold is computed based on the required watermark values
  * set for gyro and accel sensors. Since watermark is all about acceptable data
- * latency, use the smallest setting between the 2. It means choosing the
- * smallest latency but this is not as simple as choosing the smallest watermark
- * value. Latency depends on watermark and ODR. It requires several steps:
- * 1) compute gyro and accel latencies and choose the smallest value.
- * 2) adapt the chosen latency so that it is a multiple of both gyro and accel
- *    ones. Otherwise it is possible that you don't meet a requirement. (for
- *    example with gyro @100Hz wm 4 and accel @100Hz with wm 6, choosing the
- *    value of 4 will not meet accel latency requirement because 6 is not a
- *    multiple of 4. You need to use the value 2.)
+ * latency, we should need to use the smallest latency value. But it is not as
+ * simple as choosing the smallest watermark value. Latency depends on watermark
+ * and ODR and IIO buffer watermark adds another requirement. The required steps:
+ * 1) compute gyro and accel periods and latencies
+ * 2) Use the smallest period and the gcd of the latencies. Gcd is required
+ *    because of the IIO buffer watermark that will prevent send of data if not
+ *    crossed. Thus accel and gyro watermarks must be a multiple of the watermark
+ *    value. Computing the gcd gives us the biggest value that meets this criteria.
  * 3) Since all periods are multiple of each others, watermark is computed by
  *    dividing this computed latency by the smallest period, which corresponds
  *    to the FIFO frequency. Beware that this is only true because we are not
@@ -190,7 +190,7 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
 {
 	size_t packet_size, wm_size;
 	unsigned int wm_gyro, wm_accel, watermark;
-	u32 period_gyro, period_accel;
+	u32 period_gyro, period_accel, period;
 	u32 latency_gyro, latency_accel, latency;
 	bool restore;
 	__le16 raw_wm;
@@ -218,22 +218,17 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
 		watermark = wm_gyro;
 		st->fifo.watermark.eff_gyro = wm_gyro;
 	} else {
-		/* compute the smallest latency that is a multiple of both */
-		if (latency_gyro <= latency_accel)
-			latency = latency_gyro - (latency_accel % latency_gyro);
-		else
-			latency = latency_accel - (latency_gyro % latency_accel);
-		/* all this works because periods are multiple of each others */
-		watermark = latency / min(period_gyro, period_accel);
-		if (watermark < 1)
-			watermark = 1;
-		/* update effective watermark */
-		st->fifo.watermark.eff_gyro = latency / period_gyro;
-		if (st->fifo.watermark.eff_gyro < 1)
-			st->fifo.watermark.eff_gyro = 1;
-		st->fifo.watermark.eff_accel = latency / period_accel;
-		if (st->fifo.watermark.eff_accel < 1)
-			st->fifo.watermark.eff_accel = 1;
+		/*
+		 * In case of both accel and gyro enabled, we need to use the
+		 * shortest period and the gcd of the latencies. Gcd is required
+		 * because of the IIO buffer watermark that will prevent data
+		 * sending if we are not crossing the watermark level.
+		 */
+		period = min(period_gyro, period_accel);
+		latency = gcd(latency_gyro, latency_accel);
+		watermark = max(latency / period, 1);
+		st->fifo.watermark.eff_gyro = max(latency / period_gyro, 1);
+		st->fifo.watermark.eff_accel = max(latency / period_accel, 1);
 	}
 
 	/* compute watermark value in bytes */

-- 
2.55.0

[PATCH 4/4] iio: imu: inv_icm42600: do not read FIFO count for watermark it

[Jean-Baptiste Maneyrol via B4 Relay]

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

Optimize data reading for high frequencies by not reading FIFO
count in case of watermark interrupt. We already know there is
watermark samples in the FIFO and we need to not read more than
watermark samples for timestamping mechanism. Let's just read FIFO
data directly without reading FIFO count in this case.

Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
---
 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 33 ++++++++++------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
index 1428f18408ce..1410096f6e6d 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
@@ -474,24 +474,21 @@ int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
 	st->fifo.nb.accel = 0;
 	st->fifo.nb.total = 0;
 
-	/* compute maximum FIFO read size (watermark for max = 0 interrupt case) */
-	if (max == 0)
-		max = st->fifo.watermark.value;
-	max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
-
-	/* read FIFO count value */
-	raw_fifo_count = (__be16 *)st->buffer;
-	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
-			       raw_fifo_count, sizeof(*raw_fifo_count));
-	if (ret)
-		return ret;
-	st->fifo.count = be16_to_cpup(raw_fifo_count);
-
-	/* check and clamp FIFO count value */
-	if (st->fifo.count == 0)
-		return 0;
-	if (st->fifo.count > max_count)
-		st->fifo.count = max_count;
+	/* read watermark samples for interrupt case (max = 0) or read FIFO count */
+	if (max == 0) {
+		st->fifo.count = st->fifo.watermark.value *
+				 INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
+	} else {
+		raw_fifo_count = (__be16 *)st->buffer;
+		ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
+					raw_fifo_count, sizeof(*raw_fifo_count));
+		if (ret)
+			return ret;
+		max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
+		st->fifo.count = min(be16_to_cpup(raw_fifo_count), max_count);
+		if (st->fifo.count == 0)
+			return 0;
+	}
 
 	/* read all FIFO data in internal buffer */
 	ret = regmap_noinc_read(st->map, INV_ICM42600_REG_FIFO_DATA,

-- 
2.55.0

Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data

[Jonathan Cameron]

On Thu, 20 Aug 2026 21:02:37 +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>
> 
> When turning accel or gyro on, a sleep is required for letting the
> mechanical part stabilize before reporting correct data. Do the sleep
> before enabling FIFO data to prevent push of incorrect data. This is
> required if the FIFO is already on because a sensor is already running.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

I would like a note in here on why the lock dance is
useful. What sort of actions will be waiting on that lock and
make sense in the interval?

Other than that looks fine to me and does indeed seem logical to 
do this before the fifo starts

Jonathan


> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 12 ++++++++----
>  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c  | 12 ++++++++----
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 4b0e3cd8a506..d889c1dceec1 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -254,15 +254,19 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
>  		fifo_en |= INV_ICM42600_SENSOR_ACCEL;
>  	}
>  
> +	/* sleep maximum required time */
> +	sleep = max(sleep_accel, sleep_temp);
> +	if (sleep) {
> +		mutex_unlock(&st->lock);
> +		msleep(sleep);
> +		mutex_lock(&st->lock);
> +	}
> +
>  	/* update data FIFO write */
>  	ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
>  
>  out_unlock:
>  	mutex_unlock(&st->lock);
> -	/* sleep maximum required time */
> -	sleep = max(sleep_accel, sleep_temp);
> -	if (sleep)
> -		msleep(sleep);
>  	return ret;
>  }
>  
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index 253bf571439d..34037b9c1f82 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -126,15 +126,19 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
>  		fifo_en |= INV_ICM42600_SENSOR_GYRO;
>  	}
>  
> +	/* sleep maximum required time */
> +	sleep = max(sleep_gyro, sleep_temp);
> +	if (sleep) {
> +		mutex_unlock(&st->lock);
> +		msleep(sleep);
> +		mutex_lock(&st->lock);
> +	}
> +
>  	/* update data FIFO write */
>  	ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
>  
>  out_unlock:
>  	mutex_unlock(&st->lock);
> -	/* sleep maximum required time */
> -	sleep = max(sleep_gyro, sleep_temp);
> -	if (sleep)
> -		msleep(sleep);
>  	return ret;
>  }
>  
> 

Re: [PATCH 2/4] iio: imu: inv_icm42600: use 2 sensors fixed packet size of 16 bytes

[Jonathan Cameron]

On Thu, 20 Aug 2026 21:02:38 +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>
> 
> Sometimes dynamic switch between 1 sensor frame to 2 sensors frame is
> not working when there are too much frames already in the FIFO. By
> moving accel/gyro on sleep before turning FIFO on, we are storing many
> FIFO frames before updating the frame format hitting the bug everytime.
> 
> Fix that by always using the 2 sensors frame of 16 bytes. Also update
> the hwfifo max watermark reported.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>

So from that description this sometimes happens without the reorder?
If so is it appropriate to give this a Fixes tag?

> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> index 998d312f7bde..043ae9deee65 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -131,13 +131,13 @@ int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
>  		INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
>  		INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
>  
> -	val = 0;
> -	if (fifo_en & INV_ICM42600_SENSOR_GYRO)
> -		val |= INV_ICM42600_FIFO_CONFIG1_GYRO_EN;
> -	if (fifo_en & INV_ICM42600_SENSOR_ACCEL)
> -		val |= INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
> -	if (fifo_en & INV_ICM42600_SENSOR_TEMP)
> -		val |= INV_ICM42600_FIFO_CONFIG1_TEMP_EN;
> +	/* always enable/disable all bits for allowing dynamic transitions */

This needs a little more.  It makes sense in the context of
this patch but will seem rather odd if we look at it sometime in
the distant future.  Dynamic transitions of what?

> +	if (fifo_en)
> +		val = INV_ICM42600_FIFO_CONFIG1_TEMP_EN |
> +		      INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
> +		      INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
> +	else
> +		val = 0;
>  
>  	ret = regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1, mask, val);
>  	if (ret)
> @@ -149,19 +149,6 @@ int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
>  	return 0;
>  }

Re: [PATCH 3/4] iio: imu: inv_icm42600: simplify watermark computation by using gcd

[Jonathan Cameron]

On Thu, 20 Aug 2026 21:02:39 +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>
> 
> The watermark computation was in fact resulting in computing the gcd

Maybe GCD if we are referring to it by acronym rather than talking
about the function that implements it. 

> of the latencies when both sensors are on. Gcd is required because
> of the IIO buffer watermark. We need to use a divider of IIO buffer
> watermark, otherwise we will overflow the requested watermark.
> 
> Move to use gcd and update documentation accordingly.

gcd()
as this is about the function I think.

> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 47 ++++++++++------------
>  1 file changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> index 043ae9deee65..1428f18408ce 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/gcd.h>
>  #include <linux/kernel.h>
>  #include <linux/minmax.h>
>  #include <linux/mutex.h>
> @@ -172,15 +173,14 @@ static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,
>   *
>   * FIFO watermark threshold is computed based on the required watermark values
>   * set for gyro and accel sensors. Since watermark is all about acceptable data
> - * latency, use the smallest setting between the 2. It means choosing the
> - * smallest latency but this is not as simple as choosing the smallest watermark
> - * value. Latency depends on watermark and ODR. It requires several steps:
> - * 1) compute gyro and accel latencies and choose the smallest value.
> - * 2) adapt the chosen latency so that it is a multiple of both gyro and accel
> - *    ones. Otherwise it is possible that you don't meet a requirement. (for
> - *    example with gyro @100Hz wm 4 and accel @100Hz with wm 6, choosing the
> - *    value of 4 will not meet accel latency requirement because 6 is not a
> - *    multiple of 4. You need to use the value 2.)
> + * latency, we should need to use the smallest latency value. But it is not as
> + * simple as choosing the smallest watermark value. Latency depends on watermark
> + * and ODR and IIO buffer watermark adds another requirement. The required steps:
> + * 1) compute gyro and accel periods and latencies
> + * 2) Use the smallest period and the gcd of the latencies. Gcd is required

GCD here as well.

> + *    because of the IIO buffer watermark that will prevent send of data if not
> + *    crossed. Thus accel and gyro watermarks must be a multiple of the watermark
> + *    value. Computing the gcd gives us the biggest value that meets this criteria.
>   * 3) Since all periods are multiple of each others, watermark is computed by
>   *    dividing this computed latency by the smallest period, which corresponds
>   *    to the FIFO frequency. Beware that this is only true because we are not
> @@ -190,7 +190,7 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>  {
>  	size_t packet_size, wm_size;
>  	unsigned int wm_gyro, wm_accel, watermark;
> -	u32 period_gyro, period_accel;
> +	u32 period_gyro, period_accel, period;
>  	u32 latency_gyro, latency_accel, latency;
>  	bool restore;
>  	__le16 raw_wm;
> @@ -218,22 +218,17 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>  		watermark = wm_gyro;
>  		st->fifo.watermark.eff_gyro = wm_gyro;
>  	} else {
> -		/* compute the smallest latency that is a multiple of both */
> -		if (latency_gyro <= latency_accel)
> -			latency = latency_gyro - (latency_accel % latency_gyro);
> -		else
> -			latency = latency_accel - (latency_gyro % latency_accel);
> -		/* all this works because periods are multiple of each others */
> -		watermark = latency / min(period_gyro, period_accel);
> -		if (watermark < 1)
> -			watermark = 1;
> -		/* update effective watermark */
> -		st->fifo.watermark.eff_gyro = latency / period_gyro;
> -		if (st->fifo.watermark.eff_gyro < 1)
> -			st->fifo.watermark.eff_gyro = 1;
> -		st->fifo.watermark.eff_accel = latency / period_accel;
> -		if (st->fifo.watermark.eff_accel < 1)
> -			st->fifo.watermark.eff_accel = 1;
> +		/*
> +		 * In case of both accel and gyro enabled, we need to use the
> +		 * shortest period and the gcd of the latencies. Gcd is required
> +		 * because of the IIO buffer watermark that will prevent data
> +		 * sending if we are not crossing the watermark level.
> +		 */
> +		period = min(period_gyro, period_accel);
> +		latency = gcd(latency_gyro, latency_accel);
> +		watermark = max(latency / period, 1);
> +		st->fifo.watermark.eff_gyro = max(latency / period_gyro, 1);
> +		st->fifo.watermark.eff_accel = max(latency / period_accel, 1);
>  	}
>  
>  	/* compute watermark value in bytes */
> 

Re: [PATCH 4/4] iio: imu: inv_icm42600: do not read FIFO count for watermark it

[Jonathan Cameron]

On Thu, 20 Aug 2026 21:02:40 +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>
> 
> Optimize data reading for high frequencies by not reading FIFO
> count in case of watermark interrupt. We already know there is
> watermark samples in the FIFO and we need to not read more than
> watermark samples for timestamping mechanism. Let's just read FIFO
> data directly without reading FIFO count in this case.

Just to check

Level interrupt?  Or one that will definitely get triggered again?

The thinking on reading as much as possible is that for some devices
we need to be very sure we drained them past the threshold or we
don't get a fresh interrupt.  So if the threshold is set low (say 1)
it could easily reach 2 before we get to handling the interrupt.
Races are nasty anyway with those devices anyway but reading as
much as possible helped.

Do you have data on it being worth skipping the read? If we are
at high frequencies I'd assume it is more useful to read more than
the minimum just to reduce how often we read the fifo at all.

Jonathan


> 
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> ---
>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 33 ++++++++++------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> index 1428f18408ce..1410096f6e6d 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -474,24 +474,21 @@ int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
>  	st->fifo.nb.accel = 0;
>  	st->fifo.nb.total = 0;
>  
> -	/* compute maximum FIFO read size (watermark for max = 0 interrupt case) */
> -	if (max == 0)
> -		max = st->fifo.watermark.value;
> -	max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> -
> -	/* read FIFO count value */
> -	raw_fifo_count = (__be16 *)st->buffer;
> -	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
> -			       raw_fifo_count, sizeof(*raw_fifo_count));
> -	if (ret)
> -		return ret;
> -	st->fifo.count = be16_to_cpup(raw_fifo_count);
> -
> -	/* check and clamp FIFO count value */
> -	if (st->fifo.count == 0)
> -		return 0;
> -	if (st->fifo.count > max_count)
> -		st->fifo.count = max_count;
> +	/* read watermark samples for interrupt case (max = 0) or read FIFO count */
> +	if (max == 0) {
> +		st->fifo.count = st->fifo.watermark.value *
> +				 INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> +	} else {
> +		raw_fifo_count = (__be16 *)st->buffer;
> +		ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
> +					raw_fifo_count, sizeof(*raw_fifo_count));
> +		if (ret)
> +			return ret;
> +		max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> +		st->fifo.count = min(be16_to_cpup(raw_fifo_count), max_count);
> +		if (st->fifo.count == 0)
> +			return 0;
> +	}
>  
>  	/* read all FIFO data in internal buffer */
>  	ret = regmap_noinc_read(st->map, INV_ICM42600_REG_FIFO_DATA,
> 

Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data

[Andy Shevchenko]

On Thu, Aug 20, 2026 at 09:02:37PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

> When turning accel or gyro on, a sleep is required for letting the
> mechanical part stabilize before reporting correct data. Do the sleep
> before enabling FIFO data to prevent push of incorrect data. This is
> required if the FIFO is already on because a sensor is already running.

...

> +	/* sleep maximum required time */
> +	sleep = max(sleep_accel, sleep_temp);
> +	if (sleep) {
> +		mutex_unlock(&st->lock);
> +		msleep(sleep);
> +		mutex_lock(&st->lock);
> +	}

Doesn't sounds correct. Here it breaks the atomicity. Is it a problem?
(Imagine a few CPUs doing something with this driver, is any other thread may
 intervene the flow here?)

...

> +	/* sleep maximum required time */
> +	sleep = max(sleep_gyro, sleep_temp);
> +	if (sleep) {
> +		mutex_unlock(&st->lock);
> +		msleep(sleep);
> +		mutex_lock(&st->lock);
> +	}

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data

[Jean-Baptiste Maneyrol]

>
>
>________________________________________
>From: Jonathan Cameron <jic23@kernel.org>
>Sent: Friday, August 21, 2026 02:06
>To: Jean-Baptiste Maneyrol via B4 Relay
>Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data
>
>On Thu, 20 Aug 2026 21: 02: 37 +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> > > When turning accel or
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Thu, 20 Aug 2026 21:02:37 +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>
>>
>> When turning accel or gyro on, a sleep is required for letting the
>> mechanical part stabilize before reporting correct data. Do the sleep
>> before enabling FIFO data to prevent push of incorrect data. This is
>> required if the FIFO is already on because a sensor is already running.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>
>I would like a note in here on why the lock dance is
>useful. What sort of actions will be waiting on that lock and
>make sense in the interval?
>
>Other than that looks fine to me and does indeed seem logical to
>do this before the fifo starts
>
>Jonathan

Hello Jonathan,

the lock dance is required to prevent blocking the other sensor. We don't want
to prevent data coming from accelerometer when enabling gyroscope for example.
This is even more important that at high rates the FIFO can easily overflow
when sleeping, resulting in data loss.

I will add some more explanations in the commit message for v2.

Thanks,
JB

>
>
>> ---
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c | 12 ++++++++----
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c  | 12 ++++++++----
>>  2 files changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> index 4b0e3cd8a506..d889c1dceec1 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
>> @@ -254,15 +254,19 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
>>               fifo_en |= INV_ICM42600_SENSOR_ACCEL;
>>       }
>>
>> +     /* sleep maximum required time */
>> +     sleep = max(sleep_accel, sleep_temp);
>> +     if (sleep) {
>> +             mutex_unlock(&st->lock);
>> +             msleep(sleep);
>> +             mutex_lock(&st->lock);
>> +     }
>> +
>>       /* update data FIFO write */
>>       ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
>>
>>  out_unlock:
>>       mutex_unlock(&st->lock);
>> -     /* sleep maximum required time */
>> -     sleep = max(sleep_accel, sleep_temp);
>> -     if (sleep)
>> -             msleep(sleep);
>>       return ret;
>>  }
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
>> index 253bf571439d..34037b9c1f82 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
>> @@ -126,15 +126,19 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
>>               fifo_en |= INV_ICM42600_SENSOR_GYRO;
>>       }
>>
>> +     /* sleep maximum required time */
>> +     sleep = max(sleep_gyro, sleep_temp);
>> +     if (sleep) {
>> +             mutex_unlock(&st->lock);
>> +             msleep(sleep);
>> +             mutex_lock(&st->lock);
>> +     }
>> +
>>       /* update data FIFO write */
>>       ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
>>
>>  out_unlock:
>>       mutex_unlock(&st->lock);
>> -     /* sleep maximum required time */
>> -     sleep = max(sleep_gyro, sleep_temp);
>> -     if (sleep)
>> -             msleep(sleep);
>>       return ret;
>>  }
>>
>>
>
>

Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data

[Jean-Baptiste Maneyrol]

>
>
>________________________________________
>From: Andy Shevchenko <andriy.shevchenko@intel.com>
>Sent: Friday, August 21, 2026 13:15
>To: Jean-Baptiste Maneyrol
>Cc: Jonathan Cameron; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data
>
>On Thu, Aug 20, 2026 at 09: 02: 37PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote: > When turning accel or gyro on, a sleep is required for letting the > mechanical part stabilize before reporting correct data. Do the sleep > before
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Thu, Aug 20, 2026 at 09:02:37PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:
>
>> When turning accel or gyro on, a sleep is required for letting the
>> mechanical part stabilize before reporting correct data. Do the sleep
>> before enabling FIFO data to prevent push of incorrect data. This is
>> required if the FIFO is already on because a sensor is already running.
>
>...
>
>> +     /* sleep maximum required time */
>> +     sleep = max(sleep_accel, sleep_temp);
>> +     if (sleep) {
>> +             mutex_unlock(&st->lock);
>> +             msleep(sleep);
>> +             mutex_lock(&st->lock);
>> +     }
>
>Doesn't sounds correct. Here it breaks the atomicity. Is it a problem?
>(Imagine a few CPUs doing something with this driver, is any other thread may
> intervene the flow here?)

Hello Andy,

there is no risk here because we are inside the buffer enable function, and
there is an internal IIO mutex here preventing multiple calls. The driver mutex
is used to serialize access to the chip, mainly because we have 2 independent
sensors inside, accelerometer and gyroscope. We need to unlock it here to prevent
stopping data from the other sensor if it is enabled. For example, when we enable
gyroscope while accelerometer is running, we don't want to block the chip and
prevent sending of accelerometer data during 30ms. This is even more important
because the FIFO can easily overflow at high rates, and there would be data
loss.

Hope I'm clear enough in my explanations.

Thanks,
JB

>
>...
>
>> +     /* sleep maximum required time */
>> +     sleep = max(sleep_gyro, sleep_temp);
>> +     if (sleep) {
>> +             mutex_unlock(&st->lock);
>> +             msleep(sleep);
>> +             mutex_lock(&st->lock);
>> +     }
>
>Ditto.
>
>--
>With Best Regards,
>Andy Shevchenko
>
>

Re: [PATCH 2/4] iio: imu: inv_icm42600: use 2 sensors fixed packet size of 16 bytes

[Jean-Baptiste Maneyrol]

>
>
>________________________________________
>From: Jonathan Cameron <jic23@kernel.org>
>Sent: Friday, August 21, 2026 02:09
>To: Jean-Baptiste Maneyrol via B4 Relay
>Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH 2/4] iio: imu: inv_icm42600: use 2 sensors fixed packet size of 16 bytes
>
>On Thu, 20 Aug 2026 21: 02: 38 +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> > > Sometimes dynamic switch
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Thu, 20 Aug 2026 21:02:38 +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>
>>
>> Sometimes dynamic switch between 1 sensor frame to 2 sensors frame is
>> not working when there are too much frames already in the FIFO. By
>> moving accel/gyro on sleep before turning FIFO on, we are storing many
>> FIFO frames before updating the frame format hitting the bug everytime.
>>
>> Fix that by always using the 2 sensors frame of 16 bytes. Also update
>> the hwfifo max watermark reported.
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>
>So from that description this sometimes happens without the reorder?
>If so is it appropriate to give this a Fixes tag?

Hello Jonathan,

it may be possible, but I never encountered it before. That's why I didn't
make a fixes tag.

>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> index 998d312f7bde..043ae9deee65 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> @@ -131,13 +131,13 @@ int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
>>               INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
>>               INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
>>
>> -     val = 0;
>> -     if (fifo_en & INV_ICM42600_SENSOR_GYRO)
>> -             val |= INV_ICM42600_FIFO_CONFIG1_GYRO_EN;
>> -     if (fifo_en & INV_ICM42600_SENSOR_ACCEL)
>> -             val |= INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
>> -     if (fifo_en & INV_ICM42600_SENSOR_TEMP)
>> -             val |= INV_ICM42600_FIFO_CONFIG1_TEMP_EN;
>> +     /* always enable/disable all bits for allowing dynamic transitions */
>
>This needs a little more.  It makes sense in the context of
>this patch but will seem rather odd if we look at it sometime in
>the distant future.  Dynamic transitions of what?

It is dynamic transition of the FIFO frame format from 8 bits to 16 bits. I
will reword that in V2.

>
>> +     if (fifo_en)
>> +             val = INV_ICM42600_FIFO_CONFIG1_TEMP_EN |
>> +                   INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
>> +                   INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
>> +     else
>> +             val = 0;
>>
>>       ret = regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1, mask, val);
>>       if (ret)
>> @@ -149,19 +149,6 @@ int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
>>       return 0;
>>  }
>
>

Thanks,
JB

Re: [PATCH 3/4] iio: imu: inv_icm42600: simplify watermark computation by using gcd

[Jean-Baptiste Maneyrol]

>
>
>
>________________________________________
>From: Jonathan Cameron <jic23@kernel.org>
>Sent: Friday, August 21, 2026 02:14
>To: Jean-Baptiste Maneyrol via B4 Relay
>Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH 3/4] iio: imu: inv_icm42600: simplify watermark computation by using gcd
>
>On Thu, 20 Aug 2026 21: 02: 39 +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> > > The watermark computation
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Thu, 20 Aug 2026 21:02:39 +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>
>>
>> The watermark computation was in fact resulting in computing the gcd
>
>Maybe GCD if we are referring to it by acronym rather than talking
>about the function that implements it.

Hello Jonathan,

no problem, will fix that in V2.

Thanks,
JB

>
>> of the latencies when both sensors are on. Gcd is required because
>> of the IIO buffer watermark. We need to use a divider of IIO buffer
>> watermark, otherwise we will overflow the requested watermark.
>>
>> Move to use gcd and update documentation accordingly.
>
>gcd()
>as this is about the function I think.
>
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>> ---
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 47 ++++++++++------------
>>  1 file changed, 21 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> index 043ae9deee65..1428f18408ce 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> @@ -5,6 +5,7 @@
>>
>>  #include <linux/delay.h>
>>  #include <linux/device.h>
>> +#include <linux/gcd.h>
>>  #include <linux/kernel.h>
>>  #include <linux/minmax.h>
>>  #include <linux/mutex.h>
>> @@ -172,15 +173,14 @@ static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,
>>   *
>>   * FIFO watermark threshold is computed based on the required watermark values
>>   * set for gyro and accel sensors. Since watermark is all about acceptable data
>> - * latency, use the smallest setting between the 2. It means choosing the
>> - * smallest latency but this is not as simple as choosing the smallest watermark
>> - * value. Latency depends on watermark and ODR. It requires several steps:
>> - * 1) compute gyro and accel latencies and choose the smallest value.
>> - * 2) adapt the chosen latency so that it is a multiple of both gyro and accel
>> - *    ones. Otherwise it is possible that you don't meet a requirement. (for
>> - *    example with gyro @100Hz wm 4 and accel @100Hz with wm 6, choosing the
>> - *    value of 4 will not meet accel latency requirement because 6 is not a
>> - *    multiple of 4. You need to use the value 2.)
>> + * latency, we should need to use the smallest latency value. But it is not as
>> + * simple as choosing the smallest watermark value. Latency depends on watermark
>> + * and ODR and IIO buffer watermark adds another requirement. The required steps:
>> + * 1) compute gyro and accel periods and latencies
>> + * 2) Use the smallest period and the gcd of the latencies. Gcd is required
>
>GCD here as well.
>
>> + *    because of the IIO buffer watermark that will prevent send of data if not
>> + *    crossed. Thus accel and gyro watermarks must be a multiple of the watermark
>> + *    value. Computing the gcd gives us the biggest value that meets this criteria.
>>   * 3) Since all periods are multiple of each others, watermark is computed by
>>   *    dividing this computed latency by the smallest period, which corresponds
>>   *    to the FIFO frequency. Beware that this is only true because we are not
>> @@ -190,7 +190,7 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>>  {
>>       size_t packet_size, wm_size;
>>       unsigned int wm_gyro, wm_accel, watermark;
>> -     u32 period_gyro, period_accel;
>> +     u32 period_gyro, period_accel, period;
>>       u32 latency_gyro, latency_accel, latency;
>>       bool restore;
>>       __le16 raw_wm;
>> @@ -218,22 +218,17 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>>               watermark = wm_gyro;
>>               st->fifo.watermark.eff_gyro = wm_gyro;
>>       } else {
>> -             /* compute the smallest latency that is a multiple of both */
>> -             if (latency_gyro <= latency_accel)
>> -                     latency = latency_gyro - (latency_accel % latency_gyro);
>> -             else
>> -                     latency = latency_accel - (latency_gyro % latency_accel);
>> -             /* all this works because periods are multiple of each others */
>> -             watermark = latency / min(period_gyro, period_accel);
>> -             if (watermark < 1)
>> -                     watermark = 1;
>> -             /* update effective watermark */
>> -             st->fifo.watermark.eff_gyro = latency / period_gyro;
>> -             if (st->fifo.watermark.eff_gyro < 1)
>> -                     st->fifo.watermark.eff_gyro = 1;
>> -             st->fifo.watermark.eff_accel = latency / period_accel;
>> -             if (st->fifo.watermark.eff_accel < 1)
>> -                     st->fifo.watermark.eff_accel = 1;
>> +             /*
>> +              * In case of both accel and gyro enabled, we need to use the
>> +              * shortest period and the gcd of the latencies. Gcd is required
>> +              * because of the IIO buffer watermark that will prevent data
>> +              * sending if we are not crossing the watermark level.
>> +              */
>> +             period = min(period_gyro, period_accel);
>> +             latency = gcd(latency_gyro, latency_accel);
>> +             watermark = max(latency / period, 1);
>> +             st->fifo.watermark.eff_gyro = max(latency / period_gyro, 1);
>> +             st->fifo.watermark.eff_accel = max(latency / period_accel, 1);
>>       }
>>
>>       /* compute watermark value in bytes */
>>
>

Re: [PATCH 4/4] iio: imu: inv_icm42600: do not read FIFO count for watermark it

[Jean-Baptiste Maneyrol]

>
>
>________________________________________
>From: Jonathan Cameron <jic23@kernel.org>
>Sent: Friday, August 21, 2026 02:28
>To: Jean-Baptiste Maneyrol via B4 Relay
>Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jean-Baptiste Maneyrol
>Subject: Re: [PATCH 4/4] iio: imu: inv_icm42600: do not read FIFO count for watermark it
>
>On Thu, 20 Aug 2026 21: 02: 40 +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> > > Optimize data reading
>ZjQcmQRYFpfptBannerStart
>This Message Is From an External Sender
>This message came from outside your organization.
>
>ZjQcmQRYFpfptBannerEnd
>
>On Thu, 20 Aug 2026 21:02:40 +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>
>>
>> Optimize data reading for high frequencies by not reading FIFO
>> count in case of watermark interrupt. We already know there is
>> watermark samples in the FIFO and we need to not read more than
>> watermark samples for timestamping mechanism. Let's just read FIFO
>> data directly without reading FIFO count in this case.
>
>Just to check
>
>Level interrupt?  Or one that will definitely get triggered again?
>
>The thinking on reading as much as possible is that for some devices
>we need to be very sure we drained them past the threshold or we
>don't get a fresh interrupt.  So if the threshold is set low (say 1)
>it could easily reach 2 before we get to handling the interrupt.
>Races are nasty anyway with those devices anyway but reading as
>much as possible helped.
>
>Do you have data on it being worth skipping the read? If we are
>at high frequencies I'd assume it is more useful to read more than
>the minimum just to reduce how often we read the fifo at all.
>
>Jonathan

Hello Jonathan,

because of our specific timestamp mechanism, we cannot read as much as we can.
It was already the case before this patch; we were limiting FIFO read to watermark.
It is required to preserve the timing between the watermark interrupts that
we are using to compute time elapsed in the chip frame.

Here it is only an optimization to avoid reading the FIFO count register.
Because we know we have at least watermark data since the interrupt occurred,
and we cannot read more otherwise we would break the timing between 2 interrupts.

With this optimization, I'm able for example to stream data at 1kHz using I2C. Otherwise
it is not possible on my test platform.

Hope I'm clear enough here.

Thanks,
JB

>
>
>>
>> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
>> ---
>>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 33 ++++++++++------------
>>  1 file changed, 15 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> index 1428f18408ce..1410096f6e6d 100644
>> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
>> @@ -474,24 +474,21 @@ int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
>>       st->fifo.nb.accel = 0;
>>       st->fifo.nb.total = 0;
>>
>> -     /* compute maximum FIFO read size (watermark for max = 0 interrupt case) */
>> -     if (max == 0)
>> -             max = st->fifo.watermark.value;
>> -     max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
>> -
>> -     /* read FIFO count value */
>> -     raw_fifo_count = (__be16 *)st->buffer;
>> -     ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
>> -                            raw_fifo_count, sizeof(*raw_fifo_count));
>> -     if (ret)
>> -             return ret;
>> -     st->fifo.count = be16_to_cpup(raw_fifo_count);
>> -
>> -     /* check and clamp FIFO count value */
>> -     if (st->fifo.count == 0)
>> -             return 0;
>> -     if (st->fifo.count > max_count)
>> -             st->fifo.count = max_count;
>> +     /* read watermark samples for interrupt case (max = 0) or read FIFO count */
>> +     if (max == 0) {
>> +             st->fifo.count = st->fifo.watermark.value *
>> +                              INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
>> +     } else {
>> +             raw_fifo_count = (__be16 *)st->buffer;
>> +             ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
>> +                                     raw_fifo_count, sizeof(*raw_fifo_count));
>> +             if (ret)
>> +                     return ret;
>> +             max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
>> +             st->fifo.count = min(be16_to_cpup(raw_fifo_count), max_count);
>> +             if (st->fifo.count == 0)
>> +                     return 0;
>> +     }
>>
>>       /* read all FIFO data in internal buffer */
>>       ret = regmap_noinc_read(st->map, INV_ICM42600_REG_FIFO_DATA,
>>
>

Re: [PATCH 1/4] iio: imu: inv_icm42600: sleep before enabling FIFO data

[Andy Shevchenko]

On Fri, Aug 21, 2026 at 12:50:37PM +0000, Jean-Baptiste Maneyrol wrote:
> >From: Andy Shevchenko <andriy.shevchenko@intel.com>
> >Sent: Friday, August 21, 2026 13:15
> >On Thu, Aug 20, 2026 at 09:02:37PM +0200, Jean-Baptiste Maneyrol via B4 Relay wrote:

...

> >> +     /* sleep maximum required time */
> >> +     sleep = max(sleep_accel, sleep_temp);
> >> +     if (sleep) {
> >> +             mutex_unlock(&st->lock);
> >> +             msleep(sleep);
> >> +             mutex_lock(&st->lock);
> >> +     }
> >
> >Doesn't sounds correct. Here it breaks the atomicity. Is it a problem?
> >(Imagine a few CPUs doing something with this driver, is any other thread may
> > intervene the flow here?)
> 
> there is no risk here because we are inside the buffer enable function, and
> there is an internal IIO mutex here preventing multiple calls. The driver mutex
> is used to serialize access to the chip, mainly because we have 2 independent
> sensors inside, accelerometer and gyroscope. We need to unlock it here to prevent
> stopping data from the other sensor if it is enabled. For example, when we enable
> gyroscope while accelerometer is running, we don't want to block the chip and
> prevent sending of accelerometer data during 30ms. This is even more important
> because the FIFO can easily overflow at high rates, and there would be data
> loss.
> 
> Hope I'm clear enough in my explanations.

Can we have some (short) comment in the code as well?

...

> >> +     /* sleep maximum required time */
> >> +     sleep = max(sleep_gyro, sleep_temp);
> >> +     if (sleep) {
> >> +             mutex_unlock(&st->lock);
> >> +             msleep(sleep);
> >> +             mutex_lock(&st->lock);
> >> +     }

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/4] iio: imu: inv_icm42600: do not read FIFO count for watermark it

[Jonathan Cameron]

On Fri, 21 Aug 2026 13:09:49 +0000
Jean-Baptiste Maneyrol <Jean-Baptiste.Maneyrol@tdk.com> wrote:

> >
> >
> >________________________________________
> >From: Jonathan Cameron <jic23@kernel.org>
> >Sent: Friday, August 21, 2026 02:28
> >To: Jean-Baptiste Maneyrol via B4 Relay
> >Cc: Jean-Baptiste Maneyrol; David Lechner; Nuno Sá; Andy Shevchenko; linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org; Jean-Baptiste Maneyrol
> >Subject: Re: [PATCH 4/4] iio: imu: inv_icm42600: do not read FIFO count for watermark it
> >
> >On Thu, 20 Aug 2026 21: 02: 40 +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> > > Optimize data reading
> >ZjQcmQRYFpfptBannerStart
> >This Message Is From an External Sender
> >This message came from outside your organization.
> >
> >ZjQcmQRYFpfptBannerEnd
> >
> >On Thu, 20 Aug 2026 21:02:40 +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>
> >>
> >> Optimize data reading for high frequencies by not reading FIFO
> >> count in case of watermark interrupt. We already know there is
> >> watermark samples in the FIFO and we need to not read more than
> >> watermark samples for timestamping mechanism. Let's just read FIFO
> >> data directly without reading FIFO count in this case.  
> >
> >Just to check
> >
> >Level interrupt?  Or one that will definitely get triggered again?
> >
> >The thinking on reading as much as possible is that for some devices
> >we need to be very sure we drained them past the threshold or we
> >don't get a fresh interrupt.  So if the threshold is set low (say 1)
> >it could easily reach 2 before we get to handling the interrupt.
> >Races are nasty anyway with those devices anyway but reading as
> >much as possible helped.
> >
> >Do you have data on it being worth skipping the read? If we are
> >at high frequencies I'd assume it is more useful to read more than
> >the minimum just to reduce how often we read the fifo at all.
> >
> >Jonathan  
> 
> Hello Jonathan,
> 
> because of our specific timestamp mechanism, we cannot read as much as we can.
> It was already the case before this patch; we were limiting FIFO read to watermark.
> It is required to preserve the timing between the watermark interrupts that
> we are using to compute time elapsed in the chip frame.
> 
> Here it is only an optimization to avoid reading the FIFO count register.
> Because we know we have at least watermark data since the interrupt occurred,
> and we cannot read more otherwise we would break the timing between 2 interrupts.
> 
> With this optimization, I'm able for example to stream data at 1kHz using I2C. Otherwise
> it is not possible on my test platform.

This is good. Capture a little more in the commit message about only reading
this amount anyway and that you know there is always at least this amount.

Thanks for the detailed and really clear responses btw.

Jonathan

> 
> Hope I'm clear enough here.
> 
> Thanks,
> JB
> 
> >
> >  
> >>
> >> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> >> ---
> >>  drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c | 33 ++++++++++------------
> >>  1 file changed, 15 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> >> index 1428f18408ce..1410096f6e6d 100644
> >> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> >> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> >> @@ -474,24 +474,21 @@ int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
> >>       st->fifo.nb.accel = 0;
> >>       st->fifo.nb.total = 0;
> >>
> >> -     /* compute maximum FIFO read size (watermark for max = 0 interrupt case) */
> >> -     if (max == 0)
> >> -             max = st->fifo.watermark.value;
> >> -     max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> >> -
> >> -     /* read FIFO count value */
> >> -     raw_fifo_count = (__be16 *)st->buffer;
> >> -     ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
> >> -                            raw_fifo_count, sizeof(*raw_fifo_count));
> >> -     if (ret)
> >> -             return ret;
> >> -     st->fifo.count = be16_to_cpup(raw_fifo_count);
> >> -
> >> -     /* check and clamp FIFO count value */
> >> -     if (st->fifo.count == 0)
> >> -             return 0;
> >> -     if (st->fifo.count > max_count)
> >> -             st->fifo.count = max_count;
> >> +     /* read watermark samples for interrupt case (max = 0) or read FIFO count */
> >> +     if (max == 0) {
> >> +             st->fifo.count = st->fifo.watermark.value *
> >> +                              INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> >> +     } else {
> >> +             raw_fifo_count = (__be16 *)st->buffer;
> >> +             ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
> >> +                                     raw_fifo_count, sizeof(*raw_fifo_count));
> >> +             if (ret)
> >> +                     return ret;
> >> +             max_count = max * INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> >> +             st->fifo.count = min(be16_to_cpup(raw_fifo_count), max_count);
> >> +             if (st->fifo.count == 0)
> >> +                     return 0;
> >> +     }
> >>
> >>       /* read all FIFO data in internal buffer */
> >>       ret = regmap_noinc_read(st->map, INV_ICM42600_REG_FIFO_DATA,
> >>  
>