On Sat, 2024-09-14 at 12:25 +0100, Jonathan Cameron wrote: > On Fri, 13 Sep 2024 15:46:17 +0200 > Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > On Fri, 2024-09-13 at 12:55 +0000, Esteban Blanc wrote: > > > On Fri Sep 13, 2024 at 10:18 AM UTC, Nuno Sá wrote: > > > > On Fri, 2024-09-13 at 09:55 +0000, Esteban Blanc wrote: > > > > > On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote: > > > > > > On Thu, 22 Aug 2024 14:45:20 +0200 > > > > > > Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote: > > > > > > > +static const unsigned long ad4630_channel_masks[] = { > > > > > > > + /* Differential only */ > > > > > > > + BIT(0) | BIT(2), > > > > > > > + /* Differential with common byte */ > > > > > > > + GENMASK(3, 0), > > > > > > The packing of data isn't going to be good. How bad to shuffle > > > > > > to put the two small channels next to each other? > > > > > > Seems like it means you will want to combine your deinterleave > > > > > > and channel specific handling above, which is a bit fiddly but > > > > > > not much worse than current code. > > > > > > > > > > I can do it since that was what I had done in the RFC in the first place. > > > > > Nuno asked for in this email > > > > > https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@xxxxxxxxx/ > > > > > : > > > > > > > > > > > > > * You're pushing the CM channels into the end. So when we a 2 channel > > > > > > > > device > > > > > > > > we'll have: > > > > > > > > > > > > > in_voltage0 - diff > > > > > > > > in_voltage1 - diff > > > > > > > > in_voltage2 - CM associated with chan0 > > > > > > > > in_voltage0 - CM associated with chan1 > > > > > > > > > > > > > > > > I think we could make it so the CM channel comes right after the > > > > > > > > channel > > > > > > > > where > > > > > > > > it's data belongs too. So for example, odd channels would be CM > > > > > > > > channels > > > > > > > > (and > > > > > > > > labels could also make sense). > > > > > > > > > > So that's what I did here :D > > > > > > > > > > For the software side off things here it doesn't change a lot of things > > > > > since we have to manipulate the data anyway, putting the extra byte at the > > > > > end or in between is no extra work. > > > > > For the offload engine however, it should be easier to ask for 24 bits > > > > > then 8 bits for each channel as it would return two u32 per "hardware > > > > > channel". > > > > > > > > > > In order to avoid having two different layouts, I was kind of sold by > > > > > Nuno's idea of having the CM in between each diff channel. > > > > > > > > > > > > > Tbh, I was not even thinking about the layout when I proposed the > > > > arrangement. > > > > Just > > > > made sense to me (from a logical point of view) to have them together as they > > > > relate > > > > to the same physical channel. FWIW, we're also speaking bytes in here so not > > > > sure > > > > if > > > > it's that important (or bad). > > > > > > The best we can do (if we managed to do it HDL wise) is to reorder the > > > data to get both CM byte in a single u32 after the 2 u32 of both diff > > > channel. That would be 3 u32 instead of 4. > > Entirely up to you. :) > > > > > > > We are starting to see more and more devices that do stuff like this. Have one > > physical channel that reflects in more than one IIO channel. For SW buffering > > it's > > not really a big deal but for HW buffering it's not ideal. > > > > I feel that at some point we should think about having a way to map a channel > > scan > > element (being kind of a virtual scan element) into the storage_bits of another > > one. > > So in this case, one sample (for one channel) would be the 32bits and things > > should > > work the same either in SW or HW buffering. > > > > That said, it's probably easier said than done in practice :) > > Yeah. That could get ugly fast + All existing userspace will fail to handle it > so I'm not keen. Maybe it's doable if we assume the 'virtual channels' are all > meta data we don't mind loosing with existing software stacks and define > a non overlapping ABI to identify the metadata. Still smells bad to me so > I'll take quite a bit of convincing! Naturally it would have to be done in a way that drivers not defining the "special" scan elements would not be affected. > > Adding something to clearly 'associate' multiple related channels would be fine > as that wouldn't change the data interpretation, just provide more info on top. > Kind of a structured _label > > Maybe a _channelgroup attribute? Would be const and all the channels with > the same index would reflect that they were measured on same 'thing'. > Typically thing might be a pin or differential pair, but we might be measuring > different types of signals - e.g. current and power. > Sounds reasonable but I think the tricky part is always to have a sane way of saying that multiple scan elements relate to just one storage_bits so we could say something like (taking this as example): scan0: //diff channel which describing the physical HW in terms of real size .storage_bits = 32 .real_bits = 24 .shift = 8 scan1: //CM data //.storage - relates to scan0 so should add nothing to the sample size if both enabled .real_bits = 8 Likely not what you meant but one thing I took from your '_channelgroup' idea was to have something similar to extended_info maybe with a small top level description and then an array of channels (that would form the group/aggregated channel). Only on the top level description we would be allowed to define the size of the scan element (in case of buffering). Still seems tricky to me :). Anyways, Right now, I have no time for something like this but eventually would like to try something. But if someone wants to propose something sooner, please :) - Nuno Sá