On Fri, 26 Apr 2024 09:48:35 +0000
inv.git-commit@xxxxxxx wrote:
> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
>
> 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: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@xxxxxxx>
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
> ---
> .../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(-)
>
> diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
> index 3b0f9598a7c7..5f3ba77da740 100644
> --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
> +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
> @@ -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;
> }
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> index b52f328fd26c..9cde9a9337ad 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -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;
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> index 86465226f7e1..0dc0f22a5582 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c
> @@ -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 */
> diff --git a/include/linux/iio/common/inv_sensors_timestamp.h b/include/linux/iio/common/inv_sensors_timestamp.h
> index a47d304d1ba7..8d506f1e9df2 100644
> --- a/include/linux/iio/common/inv_sensors_timestamp.h
> +++ b/include/linux/iio/common/inv_sensors_timestamp.h
> @@ -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
>
