From: "Nuno Sá" <noname.nuno@gmail.com>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: jic23@kernel.org, Ramona.Gradinariu@analog.com,
ramona.bolboaca13@gmail.com, 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.org,
Nuno.Sa@analog.com
Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families
Date: Fri, 03 May 2024 11:07:23 +0200 [thread overview]
Message-ID: <928971b3b62a04144e1661ef6585264668efc447.camel@gmail.com> (raw)
In-Reply-To: <20240503094113.00001879@Huawei.com>
On Fri, 2024-05-03 at 09:42 +0100, Jonathan Cameron wrote:
> On Fri, 03 May 2024 08:09:29 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> Resend as the to / cc entries were garbled. No idea why!
>
> > On Thu, 2024-05-02 at 20:14 +0100, Jonathan Cameron wrote:
> > > On Thu, 02 May 2024 13:31:55 +0200
> > > Nuno Sá <noname.nuno@gmail.com> wrote:
> > >
> > > > 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:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Nuno Sá <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.org;
> > > > > > > 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
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > On Sun, 2024-04-28 at 16:25 +0100, Jonathan Cameron wrote:
> > > > > > > > On Tue, 23 Apr 2024 11:42:09 +0300
> > > > > > > > Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:
> > > > > > > >
> > > > > > > > > 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
> > > > > > > > > provide a
> > > > > > > > > simple interface for data collection and configuration
> > > > > > > > > control.
> > > > > > > > >
> > > > > > > > > These devices are similar to the ones already supported in the
> > > > > > > > > driver,
> > > > > > > > > with changes in the scales, timings and the max spi speed in
> > > > > > > > > burst
> > > > > > > > > mode.
> > > > > > > > > Also, they support delta angle and delta velocity readings in
> > > > > > > > > burst
> > > > > > > > > mode, for which support was added in the trigger handler.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > > > > >
> > > > > > > > What is Nuno's relationship to this patch? 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?
> > > > > > > >
> > > > > > > > > Signed-off-by: Ramona Gradinariu
> > > > > > > > > <ramona.gradinariu@analog.com>
> > > > > > > > A few comments inline. Biggest one is I'd like a clear
> > > > > > > > statement of
> > > > > > > > why you
> > > > > > > > can't do a burst of one type, then a burst of other. My guess
> > > > > > > > is
> > > > > > > > that the
> > > > > > > > transition is very time consuming? If so, that is fine, but you
> > > > > > > > should be
> > > > > > > > able
> > > > > > > > to let available_scan_masks handle the disjoint channel
> > > > > > > > sets.
> > > > > > >
> > > > > > > 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
> > > > > > > configure
> > > > > > > the chip to either give you the "normal vs "delta" readings. Re-
> > > > > > > configuring 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
> > > > > > > burst32.
> > > > > > > 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...
> > > > > > >
> > > > > > > Now, why we don't use available_scan_masks is something I can't
> > > > > > > really
> > > > > > > remember
> > > > > > > but this implementation goes in line with what we have in the
> > > > > > > adis16475
> > > > > > > driver.
> > > > > > >
> > > > > > > - Nuno Sá
> > > > > > >
> > > > > >
> > > > > > Thank you Nuno for all the additional explanations.
> > > > > > Regarding the use of available_scan_masks, the idea is to have any
> > > > > > possible
> > > > > > combination for accel, gyro, temp and timestamp channels or delta
> > > > > > angle,
> > > > > > delta
> > > > > > velocity, temp and timestamp channels. There are a lot of
> > > > > > combinations
> > > > > > for
> > > > > > this and it does not seem like a good idea to write them all
> > > > > > manually.
> > > > > > That is
> > > > > > why adis16480_update_scan_mode is used for checking the correct
> > > > > > channels
> > > > > > selection.
> > > > >
> > > > > If you are using bursts, the data is getting read anyway - which is
> > > > > the
> > > > > main
> > > > > cost here. The real question becomes what are you actually saving by
> > > > > supporting 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
> > > > > them 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
> > > > > core
> > > > > 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
> > > > > demuxing
> > > > > for you - that is enabled by providing available_scan_masks. At
> > > > > buffer
> > > > > 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.
> > > > >
> > > > > 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).
> > > > >
> > > > > 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
> > > > > measurements
> > > > > 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.
> > > > >
> > > >
> > > > Maybe my younger me was smarter but reading again the validation of the
> > > > scan
> > > > mask
> > > > code (when available_scan_masks is available), I'm not sure why we're
> > > > not
> > > > using them.
> > > > I think that having one mask with delta values + temperature and another
> > > > one
> > > > 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
> > > > missing
> > > > 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
> > > > get away
> > > > without picking the channels and place them correctly in the buffer
> > > > passed
> > > > to IIO?
> > >
> > > It runs whenever the enabled mask requested (the channels that are
> > > enabled) is
> > > different from the active_scan_mask. It only sends channels in one
> > > direction if there is only one user but it only sends the ones enabled by
> > > that
> > > consumer.
> > > It's called unconditionally from iio_enable_buffers()
> > >
> > > That iterates over all enabled buffers (often there is only 1)
> > >
> > > and then checks if the active scan mask is the same as the one for this
> > > buffer.
> > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L1006
> > >
> > > The setup for whether to find a superset from available_scan_masks is here
> > > https://elixir.bootlin.com/linux/v6.9-rc6/source/drivers/iio/industrialio-buffer.c#L928
> > >
> > > Key is that if it happens to match, then we don't actually do anything in
> > > the
> > > demux.
> > > It just gets passed straight on through. That does the fast path you
> > > mention
> > > below.
> >
> > Ahh got it... May failure was not realizing that iio_scan_mask_match()
> > returns
> > the available masks so I was assuming that the bitmap_equal() check would
> > only
> > differ when multiple buffers are enabled.
> >
> > Oh well, I think then this should work... I'm not sure it will be more
> > performant for the case where we don't enable all the channels because the
> > demux
> > is a linked list which is far from being a performance friend (maybe we can
> > do
> > some trials with something like xarray...). Still, for this to work the
> > buffer
> > we pass into IIO has to be bigger than it needs to be (so bigger memset())
> > because, AFAIU, we will have to pass on all the scan elements and, as I
> > said,
> > the burst data will either have delta or normal measurements (not both). I
> > guess
> > the code will still look simpler so if there's no visible difference in
> > performance I'm fine with the change. I guess Ramona can give it a try to
> > see
> > how it looks like.
>
> Would be interesting to look at the performance of that code, but the
> real question is what channels do users typically enabled. If they enabled
> a full set (and I suspect most do) then that code doesn't nothing at all.
>
The only channel that makes me doubt is the temperature one but yeah, I would
also expect most users just enable them all...
> Original thinking was that the non common case didn't really matter much.
> If it is worth optimizing I'd suggest a double pass (or cheat - see later)
> 1st pass works out number of elements, 2nd just saves them in a nice
> linear array. It's typically small however, so maybe just allocate a linear
> array big enough for the worst case.
>
Yeah, linear array should also be fine and likely simpler. Maybe if I'm bored at
some point I'll run some experiments :)
- Nuno Sá
next prev parent reply other threads:[~2024-05-03 9:03 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-23 8:42 [PATCH 0/5] Add support for adis16545/47 Ramona Gradinariu
2024-04-23 8:42 ` [PATCH 1/5] iio: adis16480: make the burst_max_speed configurable Ramona Gradinariu
2024-04-23 8:42 ` [PATCH 2/5] iio: imu: adis16480.c: Add delta angle and delta velocity channels Ramona Gradinariu
2024-04-28 15:04 ` Jonathan Cameron
2024-04-28 15:07 ` Jonathan Cameron
2024-04-23 8:42 ` [PATCH 3/5] dt-bindings: iio: imu: Add docs for ADIS16545/47 Ramona Gradinariu
2024-04-23 9:45 ` Krzysztof Kozlowski
2024-04-23 8:42 ` [PATCH 4/5] iio: adis16480: add support for adis16545/7 families Ramona Gradinariu
2024-04-28 15:25 ` Jonathan Cameron
2024-04-29 7:58 ` Nuno Sá
2024-04-29 13:17 ` Gradinariu, Ramona
2024-04-29 19:40 ` Jonathan Cameron
2024-05-02 11:31 ` Nuno Sá
2024-05-02 19:14 ` Jonathan Cameron
2024-05-03 6:09 ` Nuno Sá
2024-05-03 8:42 ` Jonathan Cameron
2024-05-03 9:07 ` Nuno Sá [this message]
2024-05-22 12:01 ` Gradinariu, Ramona
2024-04-29 8:01 ` Nuno Sá
2024-04-23 8:42 ` [PATCH 5/5] docs: iio: add documentation for adis16480 driver Ramona Gradinariu
2024-04-28 15:33 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=928971b3b62a04144e1661ef6585264668efc447.camel@gmail.com \
--to=noname.nuno@gmail.com \
--cc=Jonathan.Cameron@Huawei.com \
--cc=Nuno.Sa@analog.com \
--cc=Ramona.Gradinariu@analog.com \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ramona.bolboaca13@gmail.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).