Received-SPF: pass (google.com: domain of linux-kernel+bounces-181839-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ec8174c5-723d-43ad-beaf-0930d1b2c19e@gmail.com>
Date: Fri, 17 May 2024 11:00:36 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 8/8] drivers: iio: imu: Add support for adis1657x
 family
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, conor+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, robh@kernel.org, nuno.sa@analog.com
References: <20240508131310.880479-1-ramona.bolboaca13@gmail.com>
 <20240508131310.880479-9-ramona.bolboaca13@gmail.com>
 <20240511145447.68de0f59@jic23-huawei>
Content-Language: en-US
From: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
In-Reply-To: <20240511145447.68de0f59@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

>> + * 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.

Ramona G.