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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 19 Oct 2026 08:48:59 +0000
"Sabau, Radu bogdan" <radu.sabau@analog.com> wrote:

> > -----Original Message-----
> > From: Jonathan Cameron <jic23@kernel.org>
> > 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@analog.com>  
> > 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. 

The adis library is old + the fifo was retrofitted much
later - I can't remember if we discussed doing it differently
at the time.  We might want to consider making no trigger correspond
to the fifo case just to be more inline with other drivers (whilst
allowing what works today by keeping that path there as well!)

Anyhow, as long as we have the validate callbacks to prevent the trigger
being used by another consumer then it is safe enough to still
have the trigger control being set.

Thanks,

Jonathan