Re: [PATCH v2 8/8] drivers: iio: imu: Add support for adis1657x family

[Jonathan Cameron]

On Fri, 17 May 2024 11:00:36 +0300
Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx> wrote:

> >> + * device will send the data popped with the (n-1)th consecutive burst request.
> >> + * In order to read the data which was popped previously, without popping the FIFO,
> >> + * the 0x00 0x00 burst request has to be sent.
> >> + * If after a 0x68 0x00 FIFO pop burst request, there is any other device access
> >> + * different from a 0x68 0x00 or a 0x00 0x00 burst request, the FIFO data popped
> >> + * previously will be lost.
> >> + */
> >> +static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p)
> >>  {
> >>  	struct iio_poll_func *pf = p;
> >>  	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct adis16475 *st = iio_priv(indio_dev);
> >> +	struct adis *adis = &st->adis;
> >> +	int ret;
> >> +	u16 fifo_cnt, i;
> >>
> >> -	adis16475_push_single_sample(pf);
> >> +	adis_dev_lock(&st->adis);
> >> +
> >> +	ret = __adis_read_reg_16(adis, ADIS16575_REG_FIFO_CNT, &fifo_cnt);
> >> +	if (ret)
> >> +		goto unlock;
> >> +
> >> +	/*
> >> +	 * If no sample is available, nothing can be read. This can happen if
> >> +	 * a the used trigger has a higher frequency than the selected sample rate.
> >> +	 */
> >> +	if (!fifo_cnt)
> >> +		goto unlock;
> >> +
> >> +	/*
> >> +	 * First burst request - FIFO pop: popped data will be returned in the
> >> +	 * next burst request.
> >> +	 */
> >> +	ret = adis16575_custom_burst_read(pf, adis->data->burst_reg_cmd);
> >> +	if (ret)
> >> +		goto unlock;
> >> +
> >> +	for (i = 0; i < fifo_cnt - 1; i++) {
> >> +		ret = adis16475_push_single_sample(pf);
> >> +		if (ret)
> >> +			goto unlock;
> >> +	}
> >> +  
> > My paranoid instincts for potential race conditions kick in.
> > Is this one of those annoying devices where the fifo interrupt will only occur
> > again if we successfully read enough data to get below the threshold?
> > Snag with no public datasheet is I can't just look it up!
> > If it's a level interrupt this won't be a problem.
> >
> > If so the race is the following.
> > 1. Interrupt happens, we read the number of entries in fifo.
> > 2. We read out the fifo, but for some reason our read is slow... (contention on
> >    bus, CPU overheating, who knows).  The data fills up at roughly the
> >    same rate as we are reading.
> > 3. We try to carry on but in reality the fifo contents never dropped below
> >    the watermark, so not more interrupts ever occur.
> >
> > Solution normally is to put this read sequence in a while (fifo_cnt)
> > and reread that after you've done the burst read.  If there is more data
> > go around again.  That way we drive for definitely having gotten to zero
> > at some stage - and hence whatever the threshold is set to a new interrupt
> > will occur.  
> 
> Hello Jonathan,
> 
> Indeed the watermark interrupt is a level interrupt. However adis lib does not 
> allow for level interrupts, so I had to create a new patch in v3 to handle it.
> Until now I tested the watermark interrupt as and edge interrupt and I did not
> see any issues, but indeed if the FIFO is not read fast enough the watermark pin 
> will stay high (or low depending on the configured polarity), so the correct 
> implementation is to use level interrupts for FIFO watermark interrupts.
That is the easy way certainly!

Sounds good to me.

Jonathan

> 
> Ramona G.
> 
>