Re: [PATCH v2 4/6] iio: imu: adis16607: Add FIFO-based buffered/triggered capture

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iio: imu: adis16607: Data loss due to FIFO flush in trigger handler
- [Medium] iio: imu: adis16607: Incorrect FIFO timestamps projected into the future
--

commit e39eeb314e616a23019e35eaf1a136dc6bdb17f7
Author: Radu Sabau <radu.sabau@analog.com>

iio: imu: adis16607: Add FIFO-based buffered/triggered capture

This commit adds buffered capture support using the hardware FIFO on the
adis16607. It introduces a custom trigger handler that pops samples directly
and adds configuration for the FIFO watermark and sysfs attributes.

> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> index 24eaccaddfa30..90688037cbd31 100644
> --- a/drivers/iio/imu/adis16607.c
> +++ b/drivers/iio/imu/adis16607.c

[ ... ]

> +static irqreturn_t adis16607_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct adis16607 *st = iio_priv(indio_dev);
> +	u8 fifo_sample[ADIS16607_FIFO_DATA_LEN];
> +	u16 diag_stat, word_cnt, sample_cnt, threshold;
> +	s64 timestamp = pf->timestamp;
> +	int ret, i;
> +
> +	mutex_lock(&st->adis.state_lock);
> +
> +	/* Check if FIFO threshold is met */
> +	ret = __adis_read_reg_16(&st->adis, ADIS16607_REG_DIAG_STAT, &diag_stat);
> +	if (ret)
> +		goto unlock;
> +
> +	if (!(diag_stat & ADIS16607_FIFO_THR_MET_MASK))
> +		goto unlock;
> +
> +	/* Temporarily disable FIFO before reading */
> +	ret = __adis_write_reg_16(&st->adis, ADIS16607_REG_USER_FIFO_CFG, 0);

[Severity: Medium]
Will disabling the hardware FIFO here halt the enqueuing of new samples from
the sensor core? If new data is generated while the SPI read is in progress,
it seems those samples could be lost.

> +	if (ret)
> +		goto unlock;
> +
> +	/* Read FIFO word count */
> +	ret = __adis_read_reg_16(&st->adis, ADIS16607_REG_FIFO_WORD_CNT, &word_cnt);
> +	if (ret)
> +		goto reenable_fifo;
> +
> +	/* Calculate number of complete samples */
> +	sample_cnt = word_cnt / ADIS16607_FIFO_WORDS_PER_SAMPLE;
> +	if (!sample_cnt)
> +		goto reenable_fifo;
> +
> +	/* Pop and process each sample */
> +	for (i = 0; i < sample_cnt; i++) {
> +		ret = adis16607_fifo_pop(st, fifo_sample);
> +		if (ret)
> +			goto reenable_fifo;
> +
> +		adis16607_fifo_push_sample(indio_dev, fifo_sample, timestamp);
> +		timestamp += st->sample_period_ns;

[Severity: Medium]
Does this logic project the timestamps of subsequent samples into the future?
Since the interrupt fires when the FIFO reaches the watermark, pf->timestamp
represents the arrival time of the newest sample in the batch. However, the
FIFO outputs the oldest sample first.

By starting at pf->timestamp and adding st->sample_period_ns for each
subsequent sample, the newest sample ends up with a timestamp in the future:
pf->timestamp + (sample_cnt - 1) * st->sample_period_ns.

Could the timestamp be backdated before the loop instead? For example:
timestamp = pf->timestamp - (sample_cnt - 1) * st->sample_period_ns;

> +	}
> +
> +reenable_fifo:
> +	/* Flush FIFO before re-enabling */
> +	ret = __adis_write_reg_16(&st->adis, ADIS16607_REG_USER_FIFO_CFG,
> +				  ADIS16607_FIFO_FLUSH_MASK);

[Severity: Medium]
Does flushing the FIFO here drop partial sample remainders or new samples that
may have accumulated? For continuous buffered capture, dropping data between
reads seems like it would break the continuous data stream.

> +	if (ret)
> +		dev_warn_ratelimited(&st->adis.spi->dev,
> +				     "failed to flush FIFO: %d\n", ret);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-adis16607-v2-0-7c94df80878a@analog.com?part=4