Received-SPF: pass (google.com: domain of linux-kernel+bounces-186012-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <c7996393-a2d0-425e-a37a-0f6bf1ac01b9@gmail.com>
Date: Wed, 22 May 2024 12:51:39 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 9/9] 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: <20240517074750.87376-1-ramona.bolboaca13@gmail.com>
 <20240517074750.87376-10-ramona.bolboaca13@gmail.com>
 <20240519195707.71163f84@jic23-huawei>
Content-Language: en-US
From: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
In-Reply-To: <20240519195707.71163f84@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 5/19/24 21:57, Jonathan Cameron wrote:

> On Fri, 17 May 2024 10:47:50 +0300
> Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
>
>> Add support for ADIS1657X family devices in already exiting ADIS16475
>> driver.
>>
>> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
> Whilst it's not necessarily vital to support, it I'm curious about
> what happens to the hardware timestamp? I thought we had one driver
> still doing hardware timestamps directly to the buffer, but I can't
> find it so I guess we now deal with alignment in the few devices with
> this support.  The st_lsm6dsx has this sort of combining of local
> and fifo timestamps for example.
>
> As it stands I think you push the same timestamp for all scans read
> from the fifo on a particular watermark interrupt?  That isn't
> ideal given we should definitely be able to do better than that.

Currently the timestamp is added when the FIFO is read, which does not 
equal the sample time.

ADI IMU devices do not actually offer a hardware timestamp. The 
functionality is as follows:
- for internal sync / output sync / direct external sync the burst 
data returns a data counter (which increments with each sample);

- for scaled external sync the burst data returns the 'timestamp',
which contains the time associated with the last sample in each data
update relative to the most recent edge of the external clock signal.
For example, when the value in the UP_SCALE register represents a scale
factor of 20, DEC_RATE = 0, and the external SYNC rate = 100 Hz, the
following time stamp sequence results: 0LSB, 10LSB, 21LSB, 31LSB, 
41LSB, 51LSB, 61LSB, 72LSB, …, 194LSB for the 20th sample, which 
translates to 0us, 490us, …, 9510 us which is the time from the 
previous sync edge.

We can make assumptions, and try to better estimate the timestamp,
based on the sampling frequency. 
We can assume that the last sample in the FIFO was sampled when the
watermark interrupt took place, and then, based on the sample frequency
we could decrease the timestamp with the according period for each
sample.
However, I am not sure this assumption is always valid, it depends
on when the IRQ is actually handled.

I can remove the timestamp for devices which use FIFO seeing how it
does not represent the actual sample time.

>> +static const struct iio_buffer_setup_ops adis16475_buffer_ops = {
>> +	.postenable = adis16475_buffer_postenable,
>> +	.postdisable = adis16475_buffer_postdisable,
>> +};
>> +
>> +static int adis16475_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>> +{
>> +	struct adis16475 *st  = iio_priv(indio_dev);
>> +	int ret;
>> +	u16 wm_lvl;
>> +
>> +	adis_dev_lock(&st->adis);
> As a follow up perhaps consider defining magic to use guard() for these as there are
> enough users that will be simplified to make it worth the effort.	

I will do this in another patch series if that is alright.

>
>> +
>> +	val = min_t(unsigned int, val, ADIS16575_MAX_FIFO_WM);
>> +
>> +	wm_lvl = ADIS16575_WM_LVL(val - 1);
>> +	ret = __adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16575_WM_LVL_MASK, wm_lvl);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	st->fifo_watermark = val;
>> +
>> +unlock:
>> +	adis_dev_unlock(&st->adis);
>> +	return ret;
>> +}
>> +