On Tue, 15 Oct 2024 18:20:24 -0700 Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote: > Jonathan Cameron <jic23@xxxxxxxxxx> writes: > > > On Sun, 13 Oct 2024 13:55:36 -0700 > > Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote: > > > >> Jonathan Cameron <jic23@xxxxxxxxxx> writes: > >> > >> > On Sat, 12 Oct 2024 19:45:18 -0700 > >> > Justin Weiss <justin@xxxxxxxxxxxxxxx> wrote: > >> > > >> >> Jonathan Cameron <jic23@xxxxxxxxxx> writes: > >> >> > >> >> > On Fri, 11 Oct 2024 08:37:49 -0700 > >> >> > Justin Weiss <justin@xxxxxxxxxxxxxxx> 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. Indeed fine to leave it as it stands for this series. We've established a compatible path forwards if those features get added so all looks good to me. Jonathan > > 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
