Re: [PATCH] iio: imu: inv_icm42600: stabilized timestamp in interrupt

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 29 May 2024 15:47:17 +0000
inv.git-commit@tdk.com wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
> 
> Use IRQ_ONESHOT flag to ensure the timestamp is not updated in the
IRQF_ONESHOT

I fixed that up whilst applying.
> hard handler during the thread handler. And compute and use the
> effective watermark value that correspond to this first timestamp.
> 
> This way we can ensure the timestamp is always corresponding to the
> value used by the timestamping mechanism. Otherwise, it is possible
> that between FIFO count read and FIFO processing the timestamp is
> overwritten in the hard handler.
> 
> Fixes: ec74ae9fd37c ("iio: imu: inv_icm42600: add accurate timestamping")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@tdk.com>
Applied to the fixes-togreg branch of iio.git

Jonathan

> ---
>  .../imu/inv_icm42600/inv_icm42600_buffer.c    | 19 +++++++++++++++++--
>  .../imu/inv_icm42600/inv_icm42600_buffer.h    |  2 ++
>  .../iio/imu/inv_icm42600/inv_icm42600_core.c  |  1 +
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> index 63b85ec88c13..a8cf74c84c3c 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -222,10 +222,15 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>  	latency_accel = period_accel * wm_accel;
>  
>  	/* 0 value for watermark means that the sensor is turned off */
> +	if (wm_gyro == 0 && wm_accel == 0)
> +		return 0;
> +
>  	if (latency_gyro == 0) {
>  		watermark = wm_accel;
> +		st->fifo.watermark.eff_accel = wm_accel;
>  	} else if (latency_accel == 0) {
>  		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)
> @@ -241,6 +246,13 @@ int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
>  		watermark = latency / period;
>  		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;
>  	}
>  
>  	/* compute watermark value in bytes */
> @@ -514,7 +526,7 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
>  	/* handle gyroscope timestamp and FIFO data parsing */
>  	if (st->fifo.nb.gyro > 0) {
>  		ts = &gyro_st->ts;
> -		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.gyro,
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.watermark.eff_gyro,
>  						st->timestamp.gyro);
>  		ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
>  		if (ret)
> @@ -524,7 +536,7 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
>  	/* handle accelerometer timestamp and FIFO data parsing */
>  	if (st->fifo.nb.accel > 0) {
>  		ts = &accel_st->ts;
> -		inv_sensors_timestamp_interrupt(ts, st->fifo.nb.accel,
> +		inv_sensors_timestamp_interrupt(ts, st->fifo.watermark.eff_accel,
>  						st->timestamp.accel);
>  		ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
>  		if (ret)
> @@ -577,6 +589,9 @@ int inv_icm42600_buffer_init(struct inv_icm42600_state *st)
>  	unsigned int val;
>  	int ret;
>  
> +	st->fifo.watermark.eff_gyro = 1;
> +	st->fifo.watermark.eff_accel = 1;
> +
>  	/*
>  	 * Default FIFO configuration (bits 7 to 5)
>  	 * - use invalid value
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h
> index 8b85ee333bf8..f6c85daf42b0 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h
> @@ -32,6 +32,8 @@ struct inv_icm42600_fifo {
>  	struct {
>  		unsigned int gyro;
>  		unsigned int accel;
> +		unsigned int eff_gyro;
> +		unsigned int eff_accel;
>  	} watermark;
>  	size_t count;
>  	struct {
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 96116a68ab29..62fdae530334 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -537,6 +537,7 @@ static int inv_icm42600_irq_init(struct inv_icm42600_state *st, int irq,
>  	if (ret)
>  		return ret;
>  
> +	irq_type |= IRQF_ONESHOT;
>  	return devm_request_threaded_irq(dev, irq, inv_icm42600_irq_timestamp,
>  					 inv_icm42600_irq_handler, irq_type,
>  					 "inv_icm42600", st);