On Wed, 22 May 2024 12:51:39 +0300
Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx> wrote:
> On 5/19/24 21:57, Jonathan Cameron wrote:
>
> > On Fri, 17 May 2024 10:47:50 +0300
> > Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx> wrote:
> >
> >> Add support for ADIS1657X family devices in already exiting ADIS16475
> >> driver.
> >>
> >> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@xxxxxxxxx>
> > 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;
> >> +}
> >> +
>
