Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7 families

[Jonathan Cameron]

On Fri, 03 May 2024 08:09:29 +0200
Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > >     
> > > > > > -----Original Message-----
> > > > > > From: Nuno Sá <noname.nuno@xxxxxxxxx>
> > > > > > Sent: Monday, April 29, 2024 10:59 AM
> > > > > > To: Jonathan Cameron <jic23@xxxxxxxxxx>; Ramona Gradinariu
> > > > > > <ramona.bolboaca13@xxxxxxxxx>
> > > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; linux-
> > > > > > doc@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; corbet@xxxxxxx;
> > > > > > conor+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> > > > > > robh@xxxxxxxxxx;
> > > > > > Gradinariu, Ramona <Ramona.Gradinariu@xxxxxxxxxx>; Sa, Nuno
> > > > > > <Nuno.Sa@xxxxxxxxxx>
> > > > > > 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@xxxxxxxxx> 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@xxxxxxxxxx>      
> > > > > > > 
> > > > > > > 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@xxxxxxxxxx>     
> > > > > > > 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.

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.

Jonathan

> 
> - Nuno Sá
> >   
> 
>