Message-ID: <928971b3b62a04144e1661ef6585264668efc447.camel@gmail.com>
Date: Fri, 03 May 2024 11:07:23 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: jic23@...nel.org, Ramona.Gradinariu@...log.com,
 ramona.bolboaca13@...il.com,  linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org,  linux-doc@...r.kernel.org,
 devicetree@...r.kernel.org, corbet@....net,  conor+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, robh@...nel.org,  Nuno.Sa@...log.com
Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

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@...il.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@...il.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@...log.com> wrote:
> > > > >     
> > > > > > > -----Original Message-----
> > > > > > > From: Nuno Sá <noname.nuno@...il.com>
> > > > > > > Sent: Monday, April 29, 2024 10:59 AM
> > > > > > > To: Jonathan Cameron <jic23@...nel.org>; Ramona Gradinariu
> > > > > > > <ramona.bolboaca13@...il.com>
> > > > > > > Cc: linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org;
> > > > > > > linux-
> > > > > > > doc@...r.kernel.org; devicetree@...r.kernel.org; corbet@....net;
> > > > > > > conor+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> > > > > > > robh@...nel.org;
> > > > > > > Gradinariu, Ramona <Ramona.Gradinariu@...log.com>; Sa, Nuno
> > > > > > > <Nuno.Sa@...log.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@...il.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@...log.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@...log.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á

Powered by blists - more mailing lists