Message-ID: <877ca8alnr.fsf@justinweiss.com>
Date: Tue, 15 Oct 2024 18:20:24 -0700
From: Justin Weiss <justin@...tinweiss.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Alex Lanzano <lanzano.alex@...il.com>,  Lars-Peter Clausen
 <lars@...afoo.de>,  linux-iio@...r.kernel.org,
  linux-kernel@...r.kernel.org,  "Derek J . Clark"
 <derekjohn.clark@...il.com>,  Philip Müller
 <philm@...jaro.org>
Subject: Re: [PATCH 3/3] iio: imu: Add scale and sampling frequency to
 BMI270 IMU

Jonathan Cameron <jic23@...nel.org> writes:

> On Sun, 13 Oct 2024 13:55:36 -0700
> Justin Weiss <justin@...tinweiss.com> wrote:
>
>> Jonathan Cameron <jic23@...nel.org> writes:
>> 
>> > On Sat, 12 Oct 2024 19:45:18 -0700
>> > Justin Weiss <justin@...tinweiss.com> wrote:
>> >  
>> >> Jonathan Cameron <jic23@...nel.org> writes:
>> >>   
>> >> > On Fri, 11 Oct 2024 08:37:49 -0700
>> >> > Justin Weiss <justin@...tinweiss.com> wrote:
>> >> >    
>> >> >> Add read and write functions and create _available entries. Use
>> >> >> IIO_CHAN_INFO_SAMP_FREQ instead of IIO_CHAN_INFO_FREQUENCY to match
>> >> >> the BMI160 / BMI323 drivers.    
>> >> >
>> >> > Ah.  Please break dropping _FREQUENCY change out as a separate fix
>> >> > with fixes tag etc and drag it to start of the patch. It was never
>> >> > wired to anything anyway
>> >> >
>> >> > That's a straight forward ABI bug so we want that to land ahead
>> >> > of the rest of the series.    
>> >> 
>> >> Thanks, I'll pull that into its own change and make it the first patch.
>> >>   
>> >> > Does this device have a data ready interrupt and if so what affect
>> >> > do the different ODRs for each type of sensor have on that?
>> >> > If there are separate data ready signals, you probably want to 
>> >> > go with a dual buffer setup from the start as it is hard to unwind
>> >> > that later.    
>> >> 
>> >> It has data ready interrupts for both accelerometer and gyroscope and a
>> >> FIFO interrupt. I had held off on interrupts to keep this change
>> >> simpler, but if it's a better idea to get it in earlier, I can add it
>> >> alongside the triggered buffer change.  
>> >
>> > Ok. So the challenge is that IIO buffers are only described by external
>> > metadata.  We don't carry tags within them.  Hence if you are using
>> > either effectively separate datastreams (the two data ready interrupts)
>> > or a fifo that is tagged data (how this difference of speed is normally handled
>> > if it's one buffer) then when we push them into IIO buffers, they have
>> > to go into separate buffers.
>> >
>> > In older drivers this was done via the heavy weight option of registering
>> > two separate IIO devices. Today we have the ability to support multiple buffers
>> > in one driver. I'm not sure we've yet used it for this case, so I think
>> > there may still be some gaps around triggering that will matter for the
>> > separate dataready interrupt case (fifo is fine as no trigger involved).
>> > Looking again at that code, it looks like there may need to be quite
>> > a bit more work to cover this case proeprly.
>> >
>> > We may be able to have a migration path from the simple case you have
>> > (where timing is an external trigger) to multiple buffers.
>> > It would involve:
>> > 1) Initial solution where the frequencies must match if the fifo is in use.
>> >    Non fifo trigger from data ready might work but we'd need to figure out
>> >    if they run in close enough timing.
>> > 2) Solution where we add a second buffer and if the channels are enabled
>> >    in that we can allow separate timing for the two sensor types.
>> >
>> > This is one of those hardware features that seems like a good idea
>> > from the hardware design point of view but assumes a very specific
>> > sort of software model :(
>> >
>> > Jonathan  
>> 
>> Hm, that does sound tricky. If there's an example I can follow, I can
>> make an attempt at it.
>
> I don't think it ever got used for a device like this - so probably no
> examples, but I might have forgotten one. (this was a few years back).
>
>> Otherwise, if there's a change I can make now
>> that would help with migrating in the future, I can do that instead.
>> 
>> Of the devices I've looked at, only one has had the interrupts usable
>> and that one only had a single pin available.
> Lovely!  
>
>> So if this change doesn't
>> make it harder to add later if it's necessary, I would still be OK going
>> without full support for now.
> I stopped being lazy and opened the datasheet.
>
> Hmm. We have auxiliary channels as well.  oh goody.
> Considering just the fifo as that's the high performance route.
>
> Basically we can do headerless mode trivially as that's just one buffer.
> (same ODR for all sensors).
> We could do headered version but without messing with multiple buffers
> that would be only when all sensors have same ODR (after a messy
> transition period perhaps - that bit of the datasheet is less than
> intuitive!) The reason we might do headered mode is to support the
> timestamps but we can probably get those via a quick read of other
> registers after draining the fifo.

OK, that sounds good. It looks like the BMI323 driver approximates
timestamps by slicing up the time period between the last flush and the
current flush. It seems like that could also work.

If I understand it right, the simple way forward would be to use only
the fifo watermark interrupt, to set the fifo to headerless mode, and
only allow that buffer to be enabled when the ODR is the same between
the accel and gyro sensors.

Since that sounds like a fairly independent change, I can hold it for a
future patch, unless you think it belongs in this set.

Thank you for the rest of the feedback and advice, I really appreciate
it. I think I have enough for another revision soon.

Justin

> So I'm fine with just not supporting the weird corner cases unless
> we get someone turning up who
> a) cares
> b) if foolish (or motivated) enough to do the necessary work 
> c) (if they are lucky) we have the infrastructure in place because someone
>    else needed the missing bits.
>
> Jonathan
>
>
>> 
>> Justin

Powered by blists - more mailing lists