Skip to contentSkip to conversation

iio: invensense: fix timestamp glitches when switching frequency

PATCH In review [email protected] 4 participants 6 messages Apr 26, 2024 – May 3, 2024

Subsystems (4)
Stable branch IIO Invensense mpu-6050 IMU driver Invensense ICM-426xx IMU driver
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 iio: invensense: fix timestamp glitches when switching frequency

From: Jean-Baptiste Maneyrol <[email protected]>

When a sensor is running and there is a FIFO frequency change due to another sensor turned on/off, there are glitches on timestamp. Fix that by using only interrupt timestamp when there is the corresponding sensor data in the FIFO.

Delete FIFO period handling and simplify internal functions.

Update integration inside inv_mpu6050 and inv_icm42600 drivers.

Fixes 0ecc363ccea7 ("iio: make invensense timestamp module generic)
CC [email protected]
Signed-off-by Jean-Baptiste Maneyrol <[email protected]>
---
 .../inv_sensors/inv_sensors_timestamp.c       | 24 +++++++++----------
 .../imu/inv_icm42600/inv_icm42600_buffer.c    | 20 +++++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  2 +-
 .../linux/iio/common/inv_sensors_timestamp.h  |  3 +--
 4 files changed, 21 insertions(+), 28 deletions(-)
drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +11 -13
index 3b0f9598a7c7..5f3ba77da740 100644
@@ -70,13 +70,13 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 }
 EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, IIO_INV_SENSORS_TIMESTAMP);
 
-static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period, uint32_t mult)
+static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period)
 {
 	uint32_t period_min, period_max;
 
 	/* check that period is acceptable */
-	period_min = ts->min_period * mult;
-	period_max = ts->max_period * mult;
+	period_min = ts->min_period * ts->mult;
+	period_max = ts->max_period * ts->mult;
 	if (period > period_min && period < period_max)
 		return true;
 	else

@@ -84,15 +84,15 @@ static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t perio
 }
 
 static bool inv_update_chip_period(struct inv_sensors_timestamp *ts,
-				    uint32_t mult, uint32_t period)
+				   uint32_t period)
 {
 	uint32_t new_chip_period;
 
-	if (!inv_validate_period(ts, period, mult))
+	if (!inv_validate_period(ts, period))
 		return false;
 
 	/* update chip internal period estimation */
-	new_chip_period = period / mult;
+	new_chip_period = period / ts->mult;
 	inv_update_acc(&ts->chip_period, new_chip_period);
 	ts->period = ts->mult * ts->chip_period.val;
 

@@ -120,16 +120,14 @@ static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts)
 }
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				      uint32_t fifo_period, size_t fifo_nb,
-				      size_t sensor_nb, int64_t timestamp)
+				     size_t sample_nb, int64_t timestamp)
 {
 	struct inv_sensors_timestamp_interval *it;
 	int64_t delta, interval;
-	const uint32_t fifo_mult = fifo_period / ts->chip.clock_period;
 	uint32_t period;
 	bool valid = false;
 
-	if (fifo_nb == 0)
+	if (sample_nb == 0)
 		return;
 
 	/* update interrupt timestamp and compute chip and sensor periods */

@@ -139,14 +137,14 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
 	delta = it->up - it->lo;
 	if (it->lo != 0) {
 		/* compute period: delta time divided by number of samples */
-		period = div_s64(delta, fifo_nb);
-		valid = inv_update_chip_period(ts, fifo_mult, period);
+		period = div_s64(delta, sample_nb);
+		valid = inv_update_chip_period(ts, period);
 	}
 
 	/* no previous data, compute theoritical value from interrupt */
 	if (ts->timestamp == 0) {
 		/* elapsed time: sensor period * sensor samples number */
-		interval = (int64_t)ts->period * (int64_t)sensor_nb;
+		interval = (int64_t)ts->period * (int64_t)sample_nb;
 		ts->timestamp = it->up - interval;
 		return;
 	}
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +8 -12
index b52f328fd26c..9cde9a9337ad 100644
@@ -509,20 +509,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
 		return 0;
 
 	/* handle gyroscope timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_gyro);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.gyro, st->timestamp.gyro);
 	if (st->fifo.nb.gyro > 0) {
+		ts = iio_priv(st->indio_gyro);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+						st->timestamp.gyro);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;
 	}
 
 	/* handle accelerometer timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_accel);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.accel, st->timestamp.accel);
 	if (st->fifo.nb.accel > 0) {
+		ts = iio_priv(st->indio_accel);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+						st->timestamp.accel);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;

@@ -550,9 +550,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.gyro > 0) {
 		ts = iio_priv(st->indio_gyro);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.gyro,
-						gyro_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;

@@ -560,9 +558,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.accel > 0) {
 		ts = iio_priv(st->indio_accel);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.accel,
-						accel_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +1 -1
index 86465226f7e1..0dc0f22a5582 100644
@@ -100,7 +100,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto end_session;
 	/* Each FIFO data contains all sensors, so same number for FIFO and sensor data */
 	fifo_period = NSEC_PER_SEC / INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	inv_sensors_timestamp_interrupt(&st->timestamp, fifo_period, nb, nb, pf->timestamp);
+	inv_sensors_timestamp_interrupt(&st->timestamp, nb, pf->timestamp);
 	inv_sensors_timestamp_apply_odr(&st->timestamp, fifo_period, nb, 0);
 
 	/* clear internal data buffer for avoiding kernel data leak */
include/linux/iio/common/inv_sensors_timestamp.h +1 -2
index a47d304d1ba7..8d506f1e9df2 100644
@@ -71,8 +71,7 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 				     uint32_t period, bool fifo);
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				     uint32_t fifo_period, size_t fifo_nb,
-				     size_t sensor_nb, int64_t timestamp);
+				     size_t sample_nb, int64_t timestamp);
 
 static inline int64_t inv_sensors_timestamp_pop(struct inv_sensors_timestamp *ts)
 {

--
2.34.1

maintainer
#1 lore raw
Quoted 7 lines · On Fri, 26 Apr 2024 09:48:35 +0000 [email protected] wrote:

From: Jean-Baptiste Maneyrol <[email protected]>

When a sensor is running and there is a FIFO frequency change due to another sensor turned on/off, there are glitches on timestamp. Fix that by using only interrupt timestamp when there is the corresponding sensor data in the FIFO.

Quoted 7 lines

Delete FIFO period handling and simplify internal functions.

Update integration inside inv_mpu6050 and inv_icm42600 drivers.

Fixes 0ecc363ccea7 ("iio: make invensense timestamp module generic)
CC [email protected]
Signed-off-by Jean-Baptiste Maneyrol <[email protected]>

Whilst I don't fully follow the logic here, the new code is simpler and seems reasonable. Getting my head around this will probably take longer than it's worth :(

Hence applied to the fixes-togreg branch of iio.git.

Jonathan

Quoted 167 lines
---
 .../inv_sensors/inv_sensors_timestamp.c       | 24 +++++++++----------
 .../imu/inv_icm42600/inv_icm42600_buffer.c    | 20 +++++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  2 +-
 .../linux/iio/common/inv_sensors_timestamp.h  |  3 +--
 4 files changed, 21 insertions(+), 28 deletions(-)
drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +11 -13
index 3b0f9598a7c7..5f3ba77da740 100644
@@ -70,13 +70,13 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 }
 EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, IIO_INV_SENSORS_TIMESTAMP);
 
-static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period, uint32_t mult)
+static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period)
 {
 	uint32_t period_min, period_max;
 
 	/* check that period is acceptable */
-	period_min = ts->min_period * mult;
-	period_max = ts->max_period * mult;
+	period_min = ts->min_period * ts->mult;
+	period_max = ts->max_period * ts->mult;
 	if (period > period_min && period < period_max)
 		return true;
 	else

@@ -84,15 +84,15 @@ static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t perio
 }
 
 static bool inv_update_chip_period(struct inv_sensors_timestamp *ts,
-				    uint32_t mult, uint32_t period)
+				   uint32_t period)
 {
 	uint32_t new_chip_period;
 
-	if (!inv_validate_period(ts, period, mult))
+	if (!inv_validate_period(ts, period))
 		return false;
 
 	/* update chip internal period estimation */
-	new_chip_period = period / mult;
+	new_chip_period = period / ts->mult;
 	inv_update_acc(&ts->chip_period, new_chip_period);
 	ts->period = ts->mult * ts->chip_period.val;
 

@@ -120,16 +120,14 @@ static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts)
 }
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				      uint32_t fifo_period, size_t fifo_nb,
-				      size_t sensor_nb, int64_t timestamp)
+				     size_t sample_nb, int64_t timestamp)
 {
 	struct inv_sensors_timestamp_interval *it;
 	int64_t delta, interval;
-	const uint32_t fifo_mult = fifo_period / ts->chip.clock_period;
 	uint32_t period;
 	bool valid = false;
 
-	if (fifo_nb == 0)
+	if (sample_nb == 0)
 		return;
 
 	/* update interrupt timestamp and compute chip and sensor periods */

@@ -139,14 +137,14 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
 	delta = it->up - it->lo;
 	if (it->lo != 0) {
 		/* compute period: delta time divided by number of samples */
-		period = div_s64(delta, fifo_nb);
-		valid = inv_update_chip_period(ts, fifo_mult, period);
+		period = div_s64(delta, sample_nb);
+		valid = inv_update_chip_period(ts, period);
 	}
 
 	/* no previous data, compute theoritical value from interrupt */
 	if (ts->timestamp == 0) {
 		/* elapsed time: sensor period * sensor samples number */
-		interval = (int64_t)ts->period * (int64_t)sensor_nb;
+		interval = (int64_t)ts->period * (int64_t)sample_nb;
 		ts->timestamp = it->up - interval;
 		return;
 	}
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +8 -12
index b52f328fd26c..9cde9a9337ad 100644
@@ -509,20 +509,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
 		return 0;
 
 	/* handle gyroscope timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_gyro);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.gyro, st->timestamp.gyro);
 	if (st->fifo.nb.gyro > 0) {
+		ts = iio_priv(st->indio_gyro);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+						st->timestamp.gyro);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;
 	}
 
 	/* handle accelerometer timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_accel);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.accel, st->timestamp.accel);
 	if (st->fifo.nb.accel > 0) {
+		ts = iio_priv(st->indio_accel);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+						st->timestamp.accel);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;

@@ -550,9 +550,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.gyro > 0) {
 		ts = iio_priv(st->indio_gyro);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.gyro,
-						gyro_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;

@@ -560,9 +558,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.accel > 0) {
 		ts = iio_priv(st->indio_accel);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.accel,
-						accel_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +1 -1
index 86465226f7e1..0dc0f22a5582 100644
@@ -100,7 +100,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto end_session;
 	/* Each FIFO data contains all sensors, so same number for FIFO and sensor data */
 	fifo_period = NSEC_PER_SEC / INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	inv_sensors_timestamp_interrupt(&st->timestamp, fifo_period, nb, nb, pf->timestamp);
+	inv_sensors_timestamp_interrupt(&st->timestamp, nb, pf->timestamp);
 	inv_sensors_timestamp_apply_odr(&st->timestamp, fifo_period, nb, 0);
 
 	/* clear internal data buffer for avoiding kernel data leak */
include/linux/iio/common/inv_sensors_timestamp.h +1 -3
index a47d304d1ba7..8d506f1e9df2 100644
@@ -71,8 +71,7 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 				     uint32_t period, bool fifo);
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				     uint32_t fifo_period, size_t fifo_nb,
-				     size_t sensor_nb, int64_t timestamp);
+				     size_t sample_nb, int64_t timestamp);
 
 static inline int64_t inv_sensors_timestamp_pop(struct inv_sensors_timestamp *ts)
 {
--

2.34.1

maintainer
#2 lore raw
Quoted 18 lines · On Sun, 28 Apr 2024 14:13:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 7 lines · On Fri, 26 Apr 2024 09:48:35 +0000 [email protected] wrote:

From: Jean-Baptiste Maneyrol <[email protected]>

When a sensor is running and there is a FIFO frequency change due to another sensor turned on/off, there are glitches on timestamp. Fix that by using only interrupt timestamp when there is the corresponding sensor data in the FIFO.

Quoted 7 lines

Delete FIFO period handling and simplify internal functions.

Update integration inside inv_mpu6050 and inv_icm42600 drivers.

Fixes 0ecc363ccea7 ("iio: make invensense timestamp module generic)
CC [email protected]
Signed-off-by Jean-Baptiste Maneyrol <[email protected]>
Quoted 5 lines

Whilst I don't fully follow the logic here, the new code is simpler and seems reasonable. Getting my head around this will probably take longer than it's worth :(

Hence applied to the fixes-togreg branch of iio.git.

This made a bit of a mess wrt to some new part additions that went in via the togreg tree.

Given timing I'm going to pull the fixes on top of that tree so this will need a manual backport. Please take a look at iio.git togreg to check I didn't mess anything up.

Jonathan

Quoted 172 lines

Jonathan

Quoted 167 lines
---
 .../inv_sensors/inv_sensors_timestamp.c       | 24 +++++++++----------
 .../imu/inv_icm42600/inv_icm42600_buffer.c    | 20 +++++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  2 +-
 .../linux/iio/common/inv_sensors_timestamp.h  |  3 +--
 4 files changed, 21 insertions(+), 28 deletions(-)
drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +11 -13
index 3b0f9598a7c7..5f3ba77da740 100644
@@ -70,13 +70,13 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 }
 EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, IIO_INV_SENSORS_TIMESTAMP);
 
-static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period, uint32_t mult)
+static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period)
 {
 	uint32_t period_min, period_max;
 
 	/* check that period is acceptable */
-	period_min = ts->min_period * mult;
-	period_max = ts->max_period * mult;
+	period_min = ts->min_period * ts->mult;
+	period_max = ts->max_period * ts->mult;
 	if (period > period_min && period < period_max)
 		return true;
 	else

@@ -84,15 +84,15 @@ static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t perio
 }
 
 static bool inv_update_chip_period(struct inv_sensors_timestamp *ts,
-				    uint32_t mult, uint32_t period)
+				   uint32_t period)
 {
 	uint32_t new_chip_period;
 
-	if (!inv_validate_period(ts, period, mult))
+	if (!inv_validate_period(ts, period))
 		return false;
 
 	/* update chip internal period estimation */
-	new_chip_period = period / mult;
+	new_chip_period = period / ts->mult;
 	inv_update_acc(&ts->chip_period, new_chip_period);
 	ts->period = ts->mult * ts->chip_period.val;
 

@@ -120,16 +120,14 @@ static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts)
 }
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				      uint32_t fifo_period, size_t fifo_nb,
-				      size_t sensor_nb, int64_t timestamp)
+				     size_t sample_nb, int64_t timestamp)
 {
 	struct inv_sensors_timestamp_interval *it;
 	int64_t delta, interval;
-	const uint32_t fifo_mult = fifo_period / ts->chip.clock_period;
 	uint32_t period;
 	bool valid = false;
 
-	if (fifo_nb == 0)
+	if (sample_nb == 0)
 		return;
 
 	/* update interrupt timestamp and compute chip and sensor periods */

@@ -139,14 +137,14 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
 	delta = it->up - it->lo;
 	if (it->lo != 0) {
 		/* compute period: delta time divided by number of samples */
-		period = div_s64(delta, fifo_nb);
-		valid = inv_update_chip_period(ts, fifo_mult, period);
+		period = div_s64(delta, sample_nb);
+		valid = inv_update_chip_period(ts, period);
 	}
 
 	/* no previous data, compute theoritical value from interrupt */
 	if (ts->timestamp == 0) {
 		/* elapsed time: sensor period * sensor samples number */
-		interval = (int64_t)ts->period * (int64_t)sensor_nb;
+		interval = (int64_t)ts->period * (int64_t)sample_nb;
 		ts->timestamp = it->up - interval;
 		return;
 	}
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +8 -12
index b52f328fd26c..9cde9a9337ad 100644
@@ -509,20 +509,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
 		return 0;
 
 	/* handle gyroscope timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_gyro);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.gyro, st->timestamp.gyro);
 	if (st->fifo.nb.gyro > 0) {
+		ts = iio_priv(st->indio_gyro);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+						st->timestamp.gyro);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;
 	}
 
 	/* handle accelerometer timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_accel);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.accel, st->timestamp.accel);
 	if (st->fifo.nb.accel > 0) {
+		ts = iio_priv(st->indio_accel);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+						st->timestamp.accel);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;

@@ -550,9 +550,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.gyro > 0) {
 		ts = iio_priv(st->indio_gyro);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.gyro,
-						gyro_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;

@@ -560,9 +558,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.accel > 0) {
 		ts = iio_priv(st->indio_accel);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.accel,
-						accel_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +1 -1
index 86465226f7e1..0dc0f22a5582 100644
@@ -100,7 +100,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto end_session;
 	/* Each FIFO data contains all sensors, so same number for FIFO and sensor data */
 	fifo_period = NSEC_PER_SEC / INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	inv_sensors_timestamp_interrupt(&st->timestamp, fifo_period, nb, nb, pf->timestamp);
+	inv_sensors_timestamp_interrupt(&st->timestamp, nb, pf->timestamp);
 	inv_sensors_timestamp_apply_odr(&st->timestamp, fifo_period, nb, 0);
 
 	/* clear internal data buffer for avoiding kernel data leak */
include/linux/iio/common/inv_sensors_timestamp.h +1 -3
index a47d304d1ba7..8d506f1e9df2 100644
@@ -71,8 +71,7 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 				     uint32_t period, bool fifo);
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				     uint32_t fifo_period, size_t fifo_nb,
-				     size_t sensor_nb, int64_t timestamp);
+				     size_t sample_nb, int64_t timestamp);
 
 static inline int64_t inv_sensors_timestamp_pop(struct inv_sensors_timestamp *ts)
 {
--

2.34.1

maintainer
#3 lore raw

Hello Jonathan,

beware that your porting of the patch "iio: invensense: fix timestamp glitches..." is not correct inside togreg and testing branches.

Inside file drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c, you must keep the sensor_state structures declaration and use them to get the iio_priv structure.

You need to keep the following declarations in inv_icm42600_buffer fifo_parse and hwfifo_flush functions:
struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);

And you need to replace
ts = iio_priv(st->indio_gyro) by ts = &gyro_st->ts; ts = iio_priv(st->indio_accel) by ts = &accel_st->ts;

Correct diff should be something like:

@@ -512,20 +510,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)

                return 0;
 
        /* handle gyroscope timestamp and FIFO data parsing */
-       ts = &gyro_st->ts;
-       inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-                                       st->fifo.nb.gyro, st->timestamp.gyro);
        if (st->fifo.nb.gyro > 0) {
+               ts = &gyro_st->ts;
+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+                                               st->timestamp.gyro);
                ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
                if (ret)
                        return ret;
        }
 
        /* handle accelerometer timestamp and FIFO data parsing */
-       ts = &accel_st->ts;
-       inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-                                       st->fifo.nb.accel, st->timestamp.accel);
        if (st->fifo.nb.accel > 0) {
+               ts = iio_priv(st->indio_accel);
+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+                                               st->timestamp.accel);
                ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
                if (ret)
                        return ret;

@@ -554,20 +550,16 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,

                return 0;
 
        if (st->fifo.nb.gyro > 0) {

- ts = &gyro_st->ts;
- inv_sensors_timestamp_interrupt(ts, st->fifo.period,
- st->fifo.nb.total, st->fifo.nb.gyro,
- gyro_ts);
+ ts = &gyro_st->ts;

+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
                ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
                if (ret)
                        return ret;
        }
 
        if (st->fifo.nb.accel > 0) {

- ts = &accel_st->ts;
- inv_sensors_timestamp_interrupt(ts, st->fifo.period,
- st->fifo.nb.total, st->fifo.nb.accel,
- accel_ts);
+ ts = &accel_st->ts;

+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
                ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
                if (ret)
                        return ret;

Thanks for fixing this,
JB

________________________________________
From: Jonathan Cameron <[email protected]>
Sent: Wednesday, May 1, 2024 09:37
To: INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Jean-Baptiste Maneyrol <[email protected]> Subject: Re: [PATCH] iio: invensense: fix timestamp glitches when switching frequency

This Message Is From an External Sender
This message came from outside your organization.

Quoted 18 lines · On Sun, 28 Apr 2024 14:13:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 7 lines · On Fri, 26 Apr 2024 09:48:35 +0000 [email protected] wrote:

From: Jean-Baptiste Maneyrol <[email protected]>

When a sensor is running and there is a FIFO frequency change due to another sensor turned on/off, there are glitches on timestamp. Fix that by using only interrupt timestamp when there is the corresponding sensor data in the FIFO.

Quoted 7 lines

Delete FIFO period handling and simplify internal functions.

Update integration inside inv_mpu6050 and inv_icm42600 drivers.

Fixes 0ecc363ccea7 ("iio: make invensense timestamp module generic)
CC [email protected]
Signed-off-by Jean-Baptiste Maneyrol <[email protected]>
Quoted 5 lines

Whilst I don't fully follow the logic here, the new code is simpler and seems reasonable. Getting my head around this will probably take longer than it's worth :(

Hence applied to the fixes-togreg branch of iio.git.

This made a bit of a mess wrt to some new part additions that went in via the togreg tree.

Given timing I'm going to pull the fixes on top of that tree so this will need a manual backport. Please take a look at iio.git togreg to check I didn't mess anything up.

Jonathan

Quoted 172 lines

Jonathan

Quoted 167 lines
---
 .../inv_sensors/inv_sensors_timestamp.c       | 24 +++++++++----------
 .../imu/inv_icm42600/inv_icm42600_buffer.c    | 20 +++++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  2 +-
 .../linux/iio/common/inv_sensors_timestamp.h  |  3 +--
 4 files changed, 21 insertions(+), 28 deletions(-)
drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +11 -13
index 3b0f9598a7c7..5f3ba77da740 100644
@@ -70,13 +70,13 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 }
 EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, IIO_INV_SENSORS_TIMESTAMP);
 
-static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period, uint32_t mult)
+static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period)
 {
 	uint32_t period_min, period_max;
 
 	/* check that period is acceptable */
-	period_min = ts->min_period * mult;
-	period_max = ts->max_period * mult;
+	period_min = ts->min_period * ts->mult;
+	period_max = ts->max_period * ts->mult;
 	if (period > period_min && period < period_max)
 		return true;
 	else

@@ -84,15 +84,15 @@ static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t perio
 }
 
 static bool inv_update_chip_period(struct inv_sensors_timestamp *ts,
-				    uint32_t mult, uint32_t period)
+				   uint32_t period)
 {
 	uint32_t new_chip_period;
 
-	if (!inv_validate_period(ts, period, mult))
+	if (!inv_validate_period(ts, period))
 		return false;
 
 	/* update chip internal period estimation */
-	new_chip_period = period / mult;
+	new_chip_period = period / ts->mult;
 	inv_update_acc(&ts->chip_period, new_chip_period);
 	ts->period = ts->mult * ts->chip_period.val;
 

@@ -120,16 +120,14 @@ static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts)
 }
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				      uint32_t fifo_period, size_t fifo_nb,
-				      size_t sensor_nb, int64_t timestamp)
+				     size_t sample_nb, int64_t timestamp)
 {
 	struct inv_sensors_timestamp_interval *it;
 	int64_t delta, interval;
-	const uint32_t fifo_mult = fifo_period / ts->chip.clock_period;
 	uint32_t period;
 	bool valid = false;
 
-	if (fifo_nb == 0)
+	if (sample_nb == 0)
 		return;
 
 	/* update interrupt timestamp and compute chip and sensor periods */

@@ -139,14 +137,14 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
 	delta = it->up - it->lo;
 	if (it->lo != 0) {
 		/* compute period: delta time divided by number of samples */
-		period = div_s64(delta, fifo_nb);
-		valid = inv_update_chip_period(ts, fifo_mult, period);
+		period = div_s64(delta, sample_nb);
+		valid = inv_update_chip_period(ts, period);
 	}
 
 	/* no previous data, compute theoritical value from interrupt */
 	if (ts->timestamp == 0) {
 		/* elapsed time: sensor period * sensor samples number */
-		interval = (int64_t)ts->period * (int64_t)sensor_nb;
+		interval = (int64_t)ts->period * (int64_t)sample_nb;
 		ts->timestamp = it->up - interval;
 		return;
 	}
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +8 -12
index b52f328fd26c..9cde9a9337ad 100644
@@ -509,20 +509,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
 		return 0;
 
 	/* handle gyroscope timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_gyro);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.gyro, st->timestamp.gyro);
 	if (st->fifo.nb.gyro > 0) {
+		ts = iio_priv(st->indio_gyro);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+						st->timestamp.gyro);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;
 	}
 
 	/* handle accelerometer timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_accel);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.accel, st->timestamp.accel);
 	if (st->fifo.nb.accel > 0) {
+		ts = iio_priv(st->indio_accel);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+						st->timestamp.accel);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;

@@ -550,9 +550,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.gyro > 0) {
 		ts = iio_priv(st->indio_gyro);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.gyro,
-						gyro_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;

@@ -560,9 +558,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.accel > 0) {
 		ts = iio_priv(st->indio_accel);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.accel,
-						accel_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +1 -1
index 86465226f7e1..0dc0f22a5582 100644
@@ -100,7 +100,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto end_session;
 	/* Each FIFO data contains all sensors, so same number for FIFO and sensor data */
 	fifo_period = NSEC_PER_SEC / INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	inv_sensors_timestamp_interrupt(&st->timestamp, fifo_period, nb, nb, pf->timestamp);
+	inv_sensors_timestamp_interrupt(&st->timestamp, nb, pf->timestamp);
 	inv_sensors_timestamp_apply_odr(&st->timestamp, fifo_period, nb, 0);
 
 	/* clear internal data buffer for avoiding kernel data leak */
include/linux/iio/common/inv_sensors_timestamp.h +1 -3
index a47d304d1ba7..8d506f1e9df2 100644
@@ -71,8 +71,7 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 				     uint32_t period, bool fifo);
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				     uint32_t fifo_period, size_t fifo_nb,
-				     size_t sensor_nb, int64_t timestamp);
+				     size_t sample_nb, int64_t timestamp);
 
 static inline int64_t inv_sensors_timestamp_pop(struct inv_sensors_timestamp *ts)
 {
--

2.34.1

maintainer
#4 lore raw

Hello Jonathan,

I see that this fix has now been pulled.

Do I need to create a new fixes patch that fix this patch porting? inv_icm42600 is currently broken now inside the current IIO tree.

Thanks for your help.

Best regards,
JB

________________________________________
From: Jean-Baptiste Maneyrol <[email protected]>
Sent: Thursday, May 2, 2024 11:15
To: Jonathan Cameron <[email protected]>; INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]> Subject: Re: [PATCH] iio: invensense: fix timestamp glitches when switching frequency

This Message Is From an External Sender
This message came from outside your organization.

Hello Jonathan,

beware that your porting of the patch "iio: invensense: fix timestamp glitches..." is not correct inside togreg and testing branches.

Inside file drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c, you must keep the sensor_state structures declaration and use them to get the iio_priv structure.

You need to keep the following declarations in inv_icm42600_buffer fifo_parse and hwfifo_flush functions:
struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro);
struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);

And you need to replace
ts = iio_priv(st->indio_gyro) by ts = &gyro_st->ts; ts = iio_priv(st->indio_accel) by ts = &accel_st->ts;

Correct diff should be something like:

@@ -512,20 +510,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)

                return 0;
 
        /* handle gyroscope timestamp and FIFO data parsing */
-       ts = &gyro_st->ts;
-       inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-                                       st->fifo.nb.gyro, st->timestamp.gyro);
        if (st->fifo.nb.gyro > 0) {
+               ts = &gyro_st->ts;
+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+                                               st->timestamp.gyro);
                ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
                if (ret)
                        return ret;
        }
 
        /* handle accelerometer timestamp and FIFO data parsing */
-       ts = &accel_st->ts;
-       inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-                                       st->fifo.nb.accel, st->timestamp.accel);
        if (st->fifo.nb.accel > 0) {
+               ts = iio_priv(st->indio_accel);
+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+                                               st->timestamp.accel);
                ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
                if (ret)
                        return ret;

@@ -554,20 +550,16 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,

                return 0;
 
        if (st->fifo.nb.gyro > 0) {

- ts = &gyro_st->ts;
- inv_sensors_timestamp_interrupt(ts, st->fifo.period,
- st->fifo.nb.total, st->fifo.nb.gyro,
- gyro_ts);
+ ts = &gyro_st->ts;

+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
                ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
                if (ret)
                        return ret;
        }
 
        if (st->fifo.nb.accel > 0) {

- ts = &accel_st->ts;
- inv_sensors_timestamp_interrupt(ts, st->fifo.period,
- st->fifo.nb.total, st->fifo.nb.accel,
- accel_ts);
+ ts = &accel_st->ts;

+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
                ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
                if (ret)
                        return ret;

Thanks for fixing this,
JB

________________________________________
From: Jonathan Cameron <[email protected]>
Sent: Wednesday, May 1, 2024 09:37
To: INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Jean-Baptiste Maneyrol <[email protected]> Subject: Re: [PATCH] iio: invensense: fix timestamp glitches when switching frequency

This Message Is From an External Sender
This message came from outside your organization.

Quoted 18 lines · On Sun, 28 Apr 2024 14:13:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 7 lines · On Fri, 26 Apr 2024 09:48:35 +0000 [email protected] wrote:

From: Jean-Baptiste Maneyrol <[email protected]>

When a sensor is running and there is a FIFO frequency change due to another sensor turned on/off, there are glitches on timestamp. Fix that by using only interrupt timestamp when there is the corresponding sensor data in the FIFO.

Quoted 7 lines

Delete FIFO period handling and simplify internal functions.

Update integration inside inv_mpu6050 and inv_icm42600 drivers.

Fixes 0ecc363ccea7 ("iio: make invensense timestamp module generic)
CC [email protected]
Signed-off-by Jean-Baptiste Maneyrol <[email protected]>
Quoted 5 lines

Whilst I don't fully follow the logic here, the new code is simpler and seems reasonable. Getting my head around this will probably take longer than it's worth :(

Hence applied to the fixes-togreg branch of iio.git.

This made a bit of a mess wrt to some new part additions that went in via the togreg tree.

Given timing I'm going to pull the fixes on top of that tree so this will need a manual backport. Please take a look at iio.git togreg to check I didn't mess anything up.

Jonathan

Quoted 172 lines

Jonathan

Quoted 167 lines
---
 .../inv_sensors/inv_sensors_timestamp.c       | 24 +++++++++----------
 .../imu/inv_icm42600/inv_icm42600_buffer.c    | 20 +++++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  2 +-
 .../linux/iio/common/inv_sensors_timestamp.h  |  3 +--
 4 files changed, 21 insertions(+), 28 deletions(-)
drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +11 -13
index 3b0f9598a7c7..5f3ba77da740 100644
@@ -70,13 +70,13 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 }
 EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, IIO_INV_SENSORS_TIMESTAMP);
 
-static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period, uint32_t mult)
+static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t period)
 {
 	uint32_t period_min, period_max;
 
 	/* check that period is acceptable */
-	period_min = ts->min_period * mult;
-	period_max = ts->max_period * mult;
+	period_min = ts->min_period * ts->mult;
+	period_max = ts->max_period * ts->mult;
 	if (period > period_min && period < period_max)
 		return true;
 	else

@@ -84,15 +84,15 @@ static bool inv_validate_period(struct inv_sensors_timestamp *ts, uint32_t perio
 }
 
 static bool inv_update_chip_period(struct inv_sensors_timestamp *ts,
-				    uint32_t mult, uint32_t period)
+				   uint32_t period)
 {
 	uint32_t new_chip_period;
 
-	if (!inv_validate_period(ts, period, mult))
+	if (!inv_validate_period(ts, period))
 		return false;
 
 	/* update chip internal period estimation */
-	new_chip_period = period / mult;
+	new_chip_period = period / ts->mult;
 	inv_update_acc(&ts->chip_period, new_chip_period);
 	ts->period = ts->mult * ts->chip_period.val;
 

@@ -120,16 +120,14 @@ static void inv_align_timestamp_it(struct inv_sensors_timestamp *ts)
 }
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				      uint32_t fifo_period, size_t fifo_nb,
-				      size_t sensor_nb, int64_t timestamp)
+				     size_t sample_nb, int64_t timestamp)
 {
 	struct inv_sensors_timestamp_interval *it;
 	int64_t delta, interval;
-	const uint32_t fifo_mult = fifo_period / ts->chip.clock_period;
 	uint32_t period;
 	bool valid = false;
 
-	if (fifo_nb == 0)
+	if (sample_nb == 0)
 		return;
 
 	/* update interrupt timestamp and compute chip and sensor periods */

@@ -139,14 +137,14 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
 	delta = it->up - it->lo;
 	if (it->lo != 0) {
 		/* compute period: delta time divided by number of samples */
-		period = div_s64(delta, fifo_nb);
-		valid = inv_update_chip_period(ts, fifo_mult, period);
+		period = div_s64(delta, sample_nb);
+		valid = inv_update_chip_period(ts, period);
 	}
 
 	/* no previous data, compute theoritical value from interrupt */
 	if (ts->timestamp == 0) {
 		/* elapsed time: sensor period * sensor samples number */
-		interval = (int64_t)ts->period * (int64_t)sensor_nb;
+		interval = (int64_t)ts->period * (int64_t)sample_nb;
 		ts->timestamp = it->up - interval;
 		return;
 	}
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +8 -12
index b52f328fd26c..9cde9a9337ad 100644
@@ -509,20 +509,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
 		return 0;
 
 	/* handle gyroscope timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_gyro);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.gyro, st->timestamp.gyro);
 	if (st->fifo.nb.gyro > 0) {
+		ts = iio_priv(st->indio_gyro);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+						st->timestamp.gyro);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;
 	}
 
 	/* handle accelerometer timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_accel);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.accel, st->timestamp.accel);
 	if (st->fifo.nb.accel > 0) {
+		ts = iio_priv(st->indio_accel);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+						st->timestamp.accel);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;

@@ -550,9 +550,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.gyro > 0) {
 		ts = iio_priv(st->indio_gyro);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.gyro,
-						gyro_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;

@@ -560,9 +558,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
 
 	if (st->fifo.nb.accel > 0) {
 		ts = iio_priv(st->indio_accel);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.accel,
-						accel_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +1 -1
index 86465226f7e1..0dc0f22a5582 100644
@@ -100,7 +100,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto end_session;
 	/* Each FIFO data contains all sensors, so same number for FIFO and sensor data */
 	fifo_period = NSEC_PER_SEC / INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	inv_sensors_timestamp_interrupt(&st->timestamp, fifo_period, nb, nb, pf->timestamp);
+	inv_sensors_timestamp_interrupt(&st->timestamp, nb, pf->timestamp);
 	inv_sensors_timestamp_apply_odr(&st->timestamp, fifo_period, nb, 0);
 
 	/* clear internal data buffer for avoiding kernel data leak */
include/linux/iio/common/inv_sensors_timestamp.h +1 -3
index a47d304d1ba7..8d506f1e9df2 100644
@@ -71,8 +71,7 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 				     uint32_t period, bool fifo);
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				     uint32_t fifo_period, size_t fifo_nb,
-				     size_t sensor_nb, int64_t timestamp);
+				     size_t sample_nb, int64_t timestamp);
 
 static inline int64_t inv_sensors_timestamp_pop(struct inv_sensors_timestamp *ts)
 {
--

2.34.1

#5 lore raw

Please send a fixup patch or a replacement for the original fix.

Given I messed up the pull request anyway I can apply it on top, or squash with relevant fix before sending a revised pull request.

Jonathan

-----Original Message-----
From: Jean-Baptiste Maneyrol <[email protected]>
Sent: 03 May 2024 08:04
To: Jonathan Cameron <[email protected]>; INV Git Commit <[email protected]>
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] iio: invensense: fix timestamp glitches when switching frequency

Hello Jonathan,

I see that this fix has now been pulled.

Do I need to create a new fixes patch that fix this patch porting? inv_icm42600 is currently broken now inside the current IIO tree.

Thanks for your help.

Best regards,
JB

________________________________________
From: Jean-Baptiste Maneyrol <[email protected]>
Sent: Thursday, May 2, 2024 11:15
To: Jonathan Cameron <[email protected]>; INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]> Subject: Re: [PATCH] iio: invensense: fix timestamp glitches when switching frequency

This Message Is From an External Sender
This message came from outside your organization.

Hello Jonathan,

beware that your porting of the patch "iio: invensense: fix timestamp glitches..." is not correct inside togreg and testing branches.

Inside file drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c, you must keep the sensor_state structures declaration and use them to get the iio_priv structure.

You need to keep the following declarations in inv_icm42600_buffer fifo_parse and hwfifo_flush functions:
struct inv_icm42600_sensor_state *gyro_st = iio_priv(st->indio_gyro); struct inv_icm42600_sensor_state *accel_st = iio_priv(st->indio_accel);

And you need to replace
ts = iio_priv(st->indio_gyro) by ts = &gyro_st->ts; ts = iio_priv(st->indio_accel) by ts = &accel_st->ts;

Correct diff should be something like:

@@ -512,20 +510,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)

                return 0;
 
        /* handle gyroscope timestamp and FIFO data parsing */
-       ts = &gyro_st->ts;
-       inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-                                       st->fifo.nb.gyro, st->timestamp.gyro);
        if (st->fifo.nb.gyro > 0) {
+               ts = &gyro_st->ts;
+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+                                               st->timestamp.gyro);
                ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
                if (ret)
                        return ret;
        }
 
        /* handle accelerometer timestamp and FIFO data parsing */
-       ts = &accel_st->ts;
-       inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-                                       st->fifo.nb.accel, st->timestamp.accel);
        if (st->fifo.nb.accel > 0) {
+               ts = iio_priv(st->indio_accel);
+               inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+                                               st->timestamp.accel);
                ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
                if (ret)
                        return ret;

@@ -554,20 +550,16 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,

                return 0;
 
        if (st->fifo.nb.gyro > 0) {

- ts = &gyro_st->ts;
- inv_sensors_timestamp_interrupt(ts, st->fifo.period,
- st->fifo.nb.total, st->fifo.nb.gyro,
- gyro_ts);
+ ts = &gyro_st->ts;
+ inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,

+ gyro_ts);
                ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
                if (ret)
                        return ret;
        }
 
        if (st->fifo.nb.accel > 0) {

- ts = &accel_st->ts;
- inv_sensors_timestamp_interrupt(ts, st->fifo.period,
- st->fifo.nb.total, st->fifo.nb.accel,
- accel_ts);
+ ts = &accel_st->ts;
+ inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,

+ accel_ts);
                ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
                if (ret)
                        return ret;

Thanks for fixing this,
JB

________________________________________
From: Jonathan Cameron <[email protected]>
Sent: Wednesday, May 1, 2024 09:37
To: INV Git Commit <[email protected]>
Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Jean-Baptiste Maneyrol <[email protected]> Subject: Re: [PATCH] iio: invensense: fix timestamp glitches when switching frequency

This Message Is From an External Sender
This message came from outside your organization.

Quoted 19 lines · On Sun, 28 Apr 2024 14:13:49 +0100 Jonathan Cameron <[email protected]> wrote:
Quoted 7 lines · On Fri, 26 Apr 2024 09:48:35 +0000 [email protected] wrote:

From: Jean-Baptiste Maneyrol <[email protected]>

When a sensor is running and there is a FIFO frequency change due to another sensor turned on/off, there are glitches on timestamp. Fix that by using only interrupt timestamp when there is the corresponding sensor data in the FIFO.

Quoted 8 lines

Delete FIFO period handling and simplify internal functions.

Update integration inside inv_mpu6050 and inv_icm42600 drivers.

Fixes 0ecc363ccea7 ("iio: make invensense timestamp module generic)
CC [email protected]
Signed-off-by Jean-Baptiste Maneyrol

<[email protected]>

Quoted 5 lines

Whilst I don't fully follow the logic here, the new code is simpler and seems reasonable. Getting my head around this will probably take longer than it's worth :(

Hence applied to the fixes-togreg branch of iio.git.

This made a bit of a mess wrt to some new part additions that went in via the togreg tree.

Given timing I'm going to pull the fixes on top of that tree so this will need a manual backport. Please take a look at iio.git togreg to check I didn't mess anything up.

Jonathan

Quoted 183 lines

Jonathan

Quoted 178 lines
---
 .../inv_sensors/inv_sensors_timestamp.c       | 24 +++++++++----------
 .../imu/inv_icm42600/inv_icm42600_buffer.c    | 20 +++++++---------
 drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c    |  2 +-
 .../linux/iio/common/inv_sensors_timestamp.h  |  3 +--
 4 files changed, 21 insertions(+), 28 deletions(-)
drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +9 -10
index 3b0f9598a7c7..5f3ba77da740 100644
@@ -70,13 +70,13 @@ int inv_sensors_timestamp_update_odr(struct 
 inv_sensors_timestamp *ts,  }  
 EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, 
 IIO_INV_SENSORS_TIMESTAMP);
 
-static bool inv_validate_period(struct inv_sensors_timestamp *ts, 
 uint32_t period, uint32_t mult)
+static bool inv_validate_period(struct inv_sensors_timestamp *ts, 
+uint32_t period)
 {
 	uint32_t period_min, period_max;
 
 	/* check that period is acceptable */
-	period_min = ts->min_period * mult;
-	period_max = ts->max_period * mult;
+	period_min = ts->min_period * ts->mult;
+	period_max = ts->max_period * ts->mult;
 	if (period > period_min && period < period_max)
 		return true;
 	else

@@ -84,15 +84,15 @@ static bool inv_validate_period(struct 
 inv_sensors_timestamp *ts, uint32_t perio  }
 
 static bool inv_update_chip_period(struct inv_sensors_timestamp *ts,
-				    uint32_t mult, uint32_t period)
+				   uint32_t period)
 {
 	uint32_t new_chip_period;
 
-	if (!inv_validate_period(ts, period, mult))
+	if (!inv_validate_period(ts, period))
 		return false;
 
 	/* update chip internal period estimation */
-	new_chip_period = period / mult;
+	new_chip_period = period / ts->mult;
 	inv_update_acc(&ts->chip_period, new_chip_period);
 	ts->period = ts->mult * ts->chip_period.val;
 

@@ -120,16 +120,14 @@ static void inv_align_timestamp_it(struct 
 inv_sensors_timestamp *ts)  }
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				      uint32_t fifo_period, size_t fifo_nb,
-				      size_t sensor_nb, int64_t timestamp)
+				     size_t sample_nb, int64_t timestamp)
 {
 	struct inv_sensors_timestamp_interval *it;
 	int64_t delta, interval;
-	const uint32_t fifo_mult = fifo_period / ts->chip.clock_period;
 	uint32_t period;
 	bool valid = false;
 
-	if (fifo_nb == 0)
+	if (sample_nb == 0)
 		return;
 
 	/* update interrupt timestamp and compute chip and sensor periods

*/ @@ -139,14 +137,14 @@ void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,

drivers/iio/common/inv_sensors/inv_sensors_timestamp.c +3 -3
 	delta = it->up - it->lo;
 	if (it->lo != 0) {
 		/* compute period: delta time divided by number of samples */
-		period = div_s64(delta, fifo_nb);
-		valid = inv_update_chip_period(ts, fifo_mult, period);
+		period = div_s64(delta, sample_nb);
+		valid = inv_update_chip_period(ts, period);
 	}
 
 	/* no previous data, compute theoritical value from interrupt */
 	if (ts->timestamp == 0) {
 		/* elapsed time: sensor period * sensor samples number */
-		interval = (int64_t)ts->period * (int64_t)sensor_nb;
+		interval = (int64_t)ts->period * (int64_t)sample_nb;
 		ts->timestamp = it->up - interval;
 		return;
 	}
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +7 -9
index b52f328fd26c..9cde9a9337ad 100644
@@ -509,20 +509,20 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
 		return 0;
 
 	/* handle gyroscope timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_gyro);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.gyro, st->timestamp.gyro);
 	if (st->fifo.nb.gyro > 0) {
+		ts = iio_priv(st->indio_gyro);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
+						st->timestamp.gyro);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
 			return ret;
 	}
 
 	/* handle accelerometer timestamp and FIFO data parsing */
-	ts = iio_priv(st->indio_accel);
-	inv_sensors_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
-					st->fifo.nb.accel, st->timestamp.accel);
 	if (st->fifo.nb.accel > 0) {
+		ts = iio_priv(st->indio_accel);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
+						st->timestamp.accel);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
 			return ret;

@@ -550,9 +550,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct 
 inv_icm42600_state *st,
 
 	if (st->fifo.nb.gyro > 0) {
 		ts = iio_priv(st->indio_gyro);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.gyro,
-						gyro_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro, gyro_ts);
 		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
 		if (ret)
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +1 -3
 			return ret;

@@ -560,9 +558,7 @@ int inv_icm42600_buffer_hwfifo_flush(struct 
 inv_icm42600_state *st,
 
 	if (st->fifo.nb.accel > 0) {
 		ts = iio_priv(st->indio_accel);
-		inv_sensors_timestamp_interrupt(ts, st->fifo.period,
-						st->fifo.nb.total, st->fifo.nb.accel,
-						accel_ts);
+		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel, accel_ts);
 		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
 		if (ret)
drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c +0 -0
 			return ret;
drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +2 -1
index 86465226f7e1..0dc0f22a5582 100644
@@ -100,7 +100,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p)
 		goto end_session;
 	/* Each FIFO data contains all sensors, so same number for FIFO and sensor data */
 	fifo_period = NSEC_PER_SEC / INV_MPU6050_DIVIDER_TO_FIFO_RATE(st->chip_config.divider);
-	inv_sensors_timestamp_interrupt(&st->timestamp, fifo_period, nb, nb, pf->timestamp);
+	inv_sensors_timestamp_interrupt(&st->timestamp, nb, 
+pf->timestamp);
 	inv_sensors_timestamp_apply_odr(&st->timestamp, fifo_period, nb, 
 0);
 
 	/* clear internal data buffer for avoiding kernel data leak */
include/linux/iio/common/inv_sensors_timestamp.h +1 -3
index a47d304d1ba7..8d506f1e9df2 100644
@@ -71,8 +71,7 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
 				     uint32_t period, bool fifo);
 
 void inv_sensors_timestamp_interrupt(struct inv_sensors_timestamp *ts,
-				     uint32_t fifo_period, size_t fifo_nb,
-				     size_t sensor_nb, int64_t timestamp);
+				     size_t sample_nb, int64_t timestamp);
 
 static inline int64_t inv_sensors_timestamp_pop(struct 
 inv_sensors_timestamp *ts)  {
--

2.34.1