Received-SPF: pass (google.com: domain of linux-kernel+bounces-166401-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <0e13f8b643bb7afcc7c4f0d62741cf9fda66c1e0.camel@gmail.com>
Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>, "Gradinariu, Ramona"
	 <Ramona.Gradinariu@analog.com>
Cc: Ramona Gradinariu <ramona.bolboaca13@gmail.com>, 
	"linux-kernel@vger.kernel.org"
	 <linux-kernel@vger.kernel.org>, "linux-iio@vger.kernel.org"
	 <linux-iio@vger.kernel.org>, "linux-doc@vger.kernel.org"
	 <linux-doc@vger.kernel.org>, "devicetree@vger.kernel.org"
	 <devicetree@vger.kernel.org>, "corbet@lwn.net" <corbet@lwn.net>, 
	"conor+dt@kernel.org"
	 <conor+dt@kernel.org>, "krzysztof.kozlowski+dt@linaro.org"
	 <krzysztof.kozlowski+dt@linaro.org>, "robh@kernel.org" <robh@kernel.org>, 
	"Sa, Nuno" <Nuno.Sa@analog.com>
Date: Thu, 02 May 2024 13:31:55 +0200
In-Reply-To: <20240429204027.3e47074a@jic23-huawei>
References: <20240423084210.191987-1-ramona.gradinariu@analog.com>
	 <20240423084210.191987-5-ramona.gradinariu@analog.com>
	 <20240428162555.3ddf31ea@jic23-huawei>
	 <e62f8df4b06abc371b1e9fe3232cb593e468d54c.camel@gmail.com>
	 <BL1PR03MB5992DEBF82C0DB7BDC5EA0FF971B2@BL1PR03MB5992.namprd03.prod.outlook.com>
	 <20240429204027.3e47074a@jic23-huawei>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

On Mon, 2024-04-29 at 20:40 +0100, Jonathan Cameron wrote:
> On Mon, 29 Apr 2024 13:17:42 +0000
> "Gradinariu, Ramona" <Ramona.Gradinariu@analog.com> wrote:
>=20
> > > -----Original Message-----
> > > From: Nuno S=C3=A1 <noname.nuno@gmail.com>
> > > Sent: Monday, April 29, 2024 10:59 AM
> > > To: Jonathan Cameron <jic23@kernel.org>; Ramona Gradinariu
> > > <ramona.bolboaca13@gmail.com>
> > > Cc: linux-kernel@vger.kernel.org; linux-iio@vger.kernel.org; linux-
> > > doc@vger.kernel.org; devicetree@vger.kernel.org; corbet@lwn.net;
> > > conor+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; robh@kernel.o=
rg;
> > > Gradinariu, Ramona <Ramona.Gradinariu@analog.com>; Sa, Nuno
> > > <Nuno.Sa@analog.com>
> > > Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 =
families
> > >=20
> > > [External]
> > >=20
> > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:=C2=A0=20
> > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > > =C2=A0
> > > > > The ADIS16545 and ADIS16547 are a complete inertial system that
> > > > > includes a triaxis gyroscope and a triaxis accelerometer.
> > > > > The serial peripheral interface (SPI) and register structure prov=
ide a
> > > > > simple interface for data collection and configuration control.
> > > > >=20
> > > > > These devices are similar to the ones already supported in the dr=
iver,
> > > > > with changes in the scales, timings and the max spi speed in burs=
t
> > > > > mode.
> > > > > Also, they support delta angle and delta velocity readings in bur=
st
> > > > > mode, for which support was added in the trigger handler.
> > > > >=20
> > > > > Signed-off-by: Nuno S=C3=A1 <nuno.sa@analog.com>=C2=A0=20
> > > >=20
> > > > What is Nuno's relationship to this patch?=C2=A0 You are author and=
 the sender
> > > > which is fine, but in that case you need to make Nuno's involvement=
 explicit.
> > > > Perhaps a Co-developed-by or similar is appropriate?
> > > > =C2=A0
> > > > > Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>=
=C2=A0=20
> > > > A few comments inline.=C2=A0 Biggest one is I'd like a clear statem=
ent of why you
> > > > can't do a burst of one type, then a burst of other.=C2=A0 My guess=
 is that the
> > > > transition is very time consuming?=C2=A0 If so, that is fine, but y=
ou should be
> > > > able
> > > > to let available_scan_masks handle the disjoint channel sets.=C2=A0=
=20
> > >=20
> > > Yeah, the burst message is a special spi transfer that brings you all=
 of the
> > > channels data at once but for the accel/gyro you need to explicitly c=
onfigure
> > > the chip to either give you the "normal vs "delta" readings. Re-confi=
guring the
> > > chip and then do another burst would destroy performance I think. We =
could
> > > do
> > > the manual readings as we do in adis16475 for chips not supporting bu=
rst32.
> > > But
> > > in the burst32 case those manual readings should be minimal while in =
here we
> > > could have to do 6 of them which could also be very time consuming...
> > >=20
> > > Now, why we don't use available_scan_masks is something I can't reall=
y
> > > remember
> > > but this implementation goes in line with what we have in the adis164=
75
> > > driver.
> > >=20
> > > - Nuno S=C3=A1
> > > =C2=A0=20
> >=20
> > Thank you Nuno for all the additional explanations.
> > Regarding the use of available_scan_masks, the idea is to have any poss=
ible
> > combination for accel, gyro, temp and timestamp channels or delta angle=
, delta=20
> > velocity, temp and=C2=A0 timestamp channels. There are a lot of combina=
tions for=20
> > this and it does not seem like a good idea to write them all manually. =
That is=20
> > why adis16480_update_scan_mode is used for checking the correct channel=
s=20
> > selection.
>=20
> If you are using bursts, the data is getting read anyway - which is the m=
ain
> cost here. The real question becomes what are you actually saving by supp=
orting all
> the combinations of the the two subsets of channels in the pollfunc?
> Currently you have to pick the channels out and repack them, if pushing t=
hem all
> looks to me like a mempcy and a single value being set (unconditionally).

> Then it's a question of what the overhead of the channel demux in the cor=
e is.
> What you pass out of the driver via iio_push_to_buffers*()
> is not what ends up in the buffer if you allow the IIO core to do data de=
muxing
> for you - that is enabled by providing available_scan_masks.=C2=A0 At buf=
fer
> start up the demux code computes a fairly optimal set of copies to repack
> the incoming data to match with what channels the consumer (here probably
> the kfifo on the way to userspace) is expecting.
>=20
> That demux adds a small overhead but it should be small as long
> as the channels wanted aren't pathological (i.e. every other one).
>=20
> Advantage is the driver ends up simpler and in the common case of turn
> on all the channels (why else did you buy a device with those measurement=
s
> if you didn't want them!) the demux is zerocopy so effectively free which
> is not going to be the case for the bitmap walk and element copy in the
> driver.
>=20

Maybe my younger me was smarter but reading again the validation of the sca=
n mask
code (when available_scan_masks is available), I'm not sure why we're not u=
sing them.
I think that having one mask with delta values + temperature and another on=
e with
normal + temperature would be enough for what we want in here. The code in
adis16480_update_scan_mode() could then be simpler I think.

Now, what I'm still not following is the straight memcpy(). I may be missin=
g
something but the demux code only appears to kick in when we have compound =
masks
resulting of multiple buffers being enabled. So I'm not seeing how we can g=
et away
without picking the channels and place them correctly in the buffer passed =
to IIO?
What we could do in the future (for a similar device) is to maybe have a fa=
stpath in
the handler. Something like:

if (bitmap_full(scan_mask, masklength)) {
	memcpy(iio_buff, burst + data_off, size);
	goto push_to_iio;
}

Right now we would always have to do some "manual" work as the temperature =
scan index
does not match the position on the received burst data.

Some devices with the burst32 (which I think do not exist in this driver) w=
ould also
make the plain memcpy() harder.

- Nuno S=C3=A1