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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 02 Nov 2026 11:25:42 +0200
Radu Sabau via B4 Relay <devnull+radu.sabau.analog.com@kernel.org> wrote:

> From: Radu Sabau <radu.sabau@analog.com>
> 
> Add buffered capture via the device's hardware FIFO - the only
> capture mode this driver supports. Popping the FIFO uses a dedicated
> command and payload format (FIFO_DATA_CMD, 52 bytes, no checksum),
> handled directly rather than through adislib's generic burst-read
> helpers, which assume one fixed-shape read per trigger rather than a
> variable-count drain loop.
> 
> The watermark-met condition is signaled via DEV_NOK on the device's
> one interrupt pin, which stays asserted (level-triggered) until
> DIAG_STAT is read and cleared - matching adislib's has_fifo contract.
> DEV_NOK fires on any DIAG_STAT flag, not just FIFO_THRESH_MET, so the
> trigger handler checks that specific bit before popping anything.
> DR_GPIO_MASK stays enabled unconditionally: DR pulses drive the FIFO
> controller's internal sample enqueue, independent of pin routing.
> 
> Watermark is configurable via the standard hwfifo_set_watermark
> callback and the usual hwfifo_* sysfs attributes.
> 
> Signed-off-by: Radu Sabau <radu.sabau@analog.com>
https://sashiko.dev/#/patchset/20261102-adis16607-v5-0-4f98b0d271ac%40analog.com
Has some feedback on this and later patches.

If you want to increase chance of me pickup up a patch I'd
suggest watching for any feedback from sashiko and
commenting on whether and why it is wrong before I get
to catching up on reviews.  If it is repeatedly saying stuff
you disagree with add a comment under the --- for the relevant patch.
If you have looked at it and plan to make changes but haven't
yet, please save reviewers time by replying to the thread to say
that.  If you've just had a busy week and not gotten back to this
then fair enough!

Following what sashiko came up with, I'm not sure how the adis
fifo handling works today.

https://elixir.bootlin.com/linux/v7.3.1/source/drivers/iio/imu/adis_trigger.c#L90


	if (adis->data->has_fifo)
		ret = devm_request_threaded_irq(&adis->spi->dev, adis->spi->irq,
						NULL,
						&iio_trigger_generic_data_rdy_poll,
						adis->irq_flag | IRQF_ONESHOT,
						indio_dev->name,
						adis->trig);

uses a thread of:
irqreturn_t iio_trigger_generic_data_rdy_poll(int irq, void *private)
{
	iio_trigger_poll(private);
	return IRQ_HANDLED;
}

Which is calling iio_trigger_poll() not valid from a thread which
should be using iio_trigger_poll_nested()

If it were using nested form then the sashiko thing about interrupt
storm would be fine as handle_nested_irq() calls in the same thread
so it would wait for the fifo code to run before returning and IRQF_ONESHOT
would be sufficient.

Sashiko's final point about ideal being no trigger for a device that only
does fifo based buffer capture is correct as well.



Jonathan



> ---
>  drivers/iio/imu/adis16607.c | 392 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 388 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16607.c b/drivers/iio/imu/adis16607.c
> index 5e2c999e68ac..bd7752f7a1ec 100644
> --- a/drivers/iio/imu/adis16607.c
> +++ b/drivers/iio/imu/adis16607.c





> @@ -817,6 +1180,8 @@ static int adis16607_probe(struct spi_device *spi)
>  	indio_dev->info = &adis16607_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	st->fifo_watermark = ADIS16607_MAX_FIFO_WM;
> +
>  	st->adis.ops = &adis16607_ops;
>  
>  	ret = devm_regulator_get_enable(dev, "vdd");
> @@ -855,6 +1220,25 @@ static int adis16607_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * DEV_NOK stays asserted until DIAG_STAT is read, so the interrupt
> +	 * must be level-triggered, active high.

I'm missing something in this comment.  Why does DEV_NOK staying asserted
have anything to do with level triggered?

> +	 */
> +	irq_type = irq_get_trigger_type(spi->irq);
> +	if (irq_type != IRQ_TYPE_LEVEL_HIGH)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid interrupt type 0x%x specified\n",
> +				     irq_type);
> +
> +	st->adis.irq_flag = IRQF_TRIGGER_HIGH;
> +
> +	ret = devm_adis_setup_buffer_and_trigger_with_attrs(&st->adis, indio_dev,
> +							    adis16607_trigger_handler,
> +							    &adis16607_buffer_ops,
> +							    adis16607_fifo_attributes);
> +	if (ret)
> +		return ret;
> +
>  	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret)
>  		return ret;
>