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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----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.

Ramona G.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux