Received-SPF: pass (google.com: domain of linux-kernel+bounces-184539-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <1b93cc45944c7d1491184adafb55e0c99675ff32.camel@gmail.com>
Subject: Re: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x
 family
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>, Ramona Gradinariu
	 <ramona.bolboaca13@gmail.com>
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
Date: Tue, 21 May 2024 09:01:54 +0200
In-Reply-To: <20240519195707.71163f84@jic23-huawei>
References: <20240517074750.87376-1-ramona.bolboaca13@gmail.com>
	 <20240517074750.87376-10-ramona.bolboaca13@gmail.com>
	 <20240519195707.71163f84@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 
Precedence: bulk
MIME-Version: 1.0

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@gmail.com> wrote:
>=20
> > Add support for ADIS1657X family devices in already exiting ADIS16475
> > driver.
> >=20
> > Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
>=20
> 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.=C2=A0 The st_lsm6dsx has this sort of combining of local
> and fifo timestamps for example.
>=20
> As it stands I think you push the same timestamp for all scans read
> from the fifo on a particular watermark interrupt?=C2=A0 That isn't
> ideal given we should definitely be able to do better than that.
>=20
> > +
> > +static const struct iio_dev_attr *adis16475_fifo_attributes[] =3D {
> > +	&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.=C2=A0 Drop the dev_attr.attr.
>=20
> > +	NULL
> > +};
> > +
>=20
> > +
> > +static const struct iio_buffer_setup_ops adis16475_buffer_ops =3D {
> > +	.postenable =3D adis16475_buffer_postenable,
> > +	.postdisable =3D adis16475_buffer_postdisable,
> > +};
> > +
> > +static int adis16475_set_watermark(struct iio_dev *indio_dev, unsigned=
 int
> > val)
> > +{
> > +	struct adis16475 *st=C2=A0 =3D iio_priv(indio_dev);
> > +	int ret;
> > +	u16 wm_lvl;
> > +
> > +	adis_dev_lock(&st->adis);
>=20
> As a follow up perhaps consider defining magic to use guard() for these a=
s
> there are
> enough users that will be simplified to make it worth the effort.=09
>=20

Already on my queue but if Ramona wants to step in, good. I already have pl=
enty
to do :)

- Nuno S=C3=A1
>=20