On Sun, 2024-05-19 at 19:57 +0100, 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. > > > + > > +static const struct iio_dev_attr *adis16475_fifo_attributes[] = { > > + &iio_dev_attr_hwfifo_watermark_min.dev_attr.attr, > > + &iio_dev_attr_hwfifo_watermark_max.dev_attr.attr, > > + &iio_dev_attr_hwfifo_watermark.dev_attr.attr, > > + &iio_dev_attr_hwfifo_enabled.dev_attr.attr, > The autobuilder caught this one. Drop the dev_attr.attr. > > > + NULL > > +}; > > + > > > + > > +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. > Already on my queue but if Ramona wants to step in, good. I already have plenty to do :) - Nuno Sá >