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

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Sunday, November 8, 2026 2:52 AM

...

> > Watermark is configurable via the standard hwfifo_set_watermark
> > callback and the usual hwfifo_* sysfs attributes.
> >
> > Signed-off-by: Radu Sabau <radu.sabau@xxxxxxxxxx>
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20261102-
> adis16607-v5-0-
> 4f98b0d271ac*40analog.com__;IyU!!A3Ni8CS0y2Y!8C9P7exAoBKoTmce13D
> t5nL3mUvpzcX-kaoE9HgOFqmFiDBX3IxbPETNVz_T_1Zb3mJk7YAfzi7H9g$
> 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://urldefense.com/v3/__https://elixir.bootlin.com/linux/v7.3.1/source/
> drivers/iio/imu/adis_trigger.c*L90__;Iw!!A3Ni8CS0y2Y!8C9P7exAoBKoTmce1
> 3Dt5nL3mUvpzcX-
> kaoE9HgOFqmFiDBX3IxbPETNVz_T_1Zb3mJk7YCJFlWadw$
> 
> 
> 	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

Sorry, my earlier replies went to the Sashiko report instead of the patch
threads, my bad.

Agreed on both points: iio_trigger_poll() called from the IRQ thread in the
has_fifo path is wrong (iio_trigger_poll_nested() is the right helper), and
a FIFO-only part is better served by a kfifo buffer than by a trigger.

Since has_fifo is shared with the adis16475 FIFO variants, a proper kfifo
conversion belongs in adislib and needs testing on those parts.

I see three ways forward and would like your preference:

1. Fix the dispatch in this series, kfifo conversion in a following one.
This series gets a fix before the driver patches that moves the has_fifo path
to iio_trigger_poll_nested(), together with a small adis16475 change to take
its sample timestamp in the handler, since the nested path no longer runs
the pollfunc top half.  The ADIS16607 driver then keeps using adislib's trigger
path as it is, and this series is complete and working on its own. A later
series then implements the kfifo buffer in adislib, converts the adis16475 FIFO
variants and moves the ADIS16607 to it. As that one needs testing on the
other devices, keeping it separate means both series are functional by themselves.
This one would mean one extra fix patch before the adis16607 ones so a total
of 9 patches for this patch series.

2. kfifo in this driver only. The ADIS16607 gets its own kfifo buffer and interrupt
handling in this series, with no adislib change. A later series adds the kfifo buffer
to adislib, converts the adis16475 and switches the ADIS16607 over to the adislib
code. This one would mean the same number of patches (8)

3. Everything in this series (about 12 patches).

I'm happy with whichever you prefer. I lean towards 2 personally for now, since
1 implies one extra patch which would see its changes removed/changed by a later
series anyway.

You're also right about the comment in probe: DEV_NOK staying asserted doesn't by
itself imply a level interrupt. It is the OR of the DIAG_STAT flags, so a flag raised while
the line is already high would not produce a new edge. I'll reword the comment.

For the other Sashiko findings, I will also have the reply here real quick :
- patch 1's Fixes tag and message - he is right, the fixes tag should be changed
- patch 7's get_freq() locking and sample period handling - he is also, right, I also plan to fix them in v6.

Thanks,
Radu