Re: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x family

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 22 May 2024 12:51:39 +0300
Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:

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

Thanks for info.  I was hopeful for a real clock, but we rarely get
one (and it introduces clock drift issues anyway so they are quite
tricky to handle :(

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

Assume the one that corresponds to the watermark, not the last one as we
may take a while to get to processing it.

> 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.
I would do that for now as this si tricky to do well.

There are examples in tree of how to get something reasonable. 
Take a look at common/inv_sensors/inv_sensors_timestap.c
for one example

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

Sure.

> 
> >  
> >> +
> >> +	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;
> >> +}
> >> +  
>