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

[Sabau, Radu bogdan]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, October 17, 2026 4:41 AM

...

> > 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@xxxxxxxxxx>
> Various comments inline.  Biggest one is I don't follow why we
> need to stop the fifo (which is a ring buffer given overwrite can't
> happen in a fifo!)
> 
> Jonathan
> 
> >  /* Self-test registers and masks */
> >  #define ADIS16607_REG_SELF_TEST_DATA(x)	((x) + 0x23)
> >  #define ADIS16607_SNSR_SELF_TEST_MASK	BIT(6)

...

> > +
> > +/*
> > + * Trigger handler - reads samples from the device FIFO. Triggered by the
> > + * DEV_NOK interrupt, which fires on any DIAG_STAT flag, not just
> > + * FIFO_THRESH_MET - the DIAG_STAT read below both checks which
> flag(s)
> > + * raised the interrupt and clears the latched status, and must happen
> > + * before any FIFO pop, or we'd pop for nothing on an unrelated flag.
> > + */
> > +static irqreturn_t adis16607_trigger_handler(int irq, void *p)
> 
> A trigger handler that is associated with a fifo is usually a bad
> idea.  We often only use fifos if there is no trigger associated
> because the meaning of one trigger ever scan is lost so they
> become just a pointless intermediate layer.  Not sure if we can
> do that here.

Fair concern. I was looking at adis16475.c and noticed it already
does the same thing for its FIFO variants (adis1657x family) -
has_fifo = true, with a trigger handler that pops a variable sample
count per IRQ instead of exactly one. adis_trigger.c backs this up
too: it already special-cases FIFO devices with a level-triggered,
threaded IRQ instead of the edge-triggered one used for non-FIFO
devices, specifically because the watermark condition needs clearing
over SPI before the line deasserts.

So I thought that since this pattern is already established in the
shared library for exactly this kind of device, it made sense to
follow it here, too. 

> 
> > +{
> > +	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;
> > +	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;
> > +
> > +	/*
> > +	 * Disable the FIFO before draining: FIFO_THR_MET means it's at the
> > +	 * configured threshold, and leaving it enabled while we pop would
> > +	 * let incoming samples overwrite ones not yet read. No new samples
> It will overwrite without overflowing?  That's odd if true. If it's the
> overflow case that was true before you started popping so no reason to
> care about it more in this case.  We generally don't disable fifos for
> this reason. The description in the datasheet suggests this is a ring buffer
> rather than a fifo as a fifo never overwrites of you'd not be getting
> the 'first in' but such is the world of datasheets.
> 

I re-read the datasheet and I agree. I got it all wrong in the first place, as there
was a passage in the FIFO section saying new entries overwrite old ones, but
that is only if the capacity is insufficient. So it's exactly the overflow-only
overwrite you'd expect from a normal ring buffer - not something that happens
just because a pop is in progress. 

> Does it at least guarantee no value tearing so worst that happens is we
> get skips of old data if we read to slow?

As per datasheet, "The memory is implemented as 1024 words using a dual-port
architecture so that new data writes and host FIFO reads proceed without
contention."

So no, no tearing - reads and writes are architected not to interfere with each other,
which is also exactly what makes draining without disabling safe.

> 
> Overall other than the overwrite not being controllable, this sounds
> like most other fifos.  We get an overflow if we don't read fast enough.
> In that case and only that case we should flush (though I'm not sure
> we do it even then when we have overwriting as only option).
> 

On point with the flush, too. The flush is completely optional and makes no sense
to exist here anyway. Will be removing this.

> In short, I'm not seeing the special bit of this implementation that makes
> data loss inevitable. Perhaps some spec pointers and quotes would convince
> me?
> 

To close the loop: there isn't one. Once FIFO_THR_MET's clear-on-read
behaviour and FIFO_DATA's pointer-advancing pop are accounted for,
this is a standard watermark-triggered ring buffer like any other -
nothing here actually needed the disable step. Dropping it in v4,
along with the flush-and-re-enable that followed it.