Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[Jonathan Cameron]

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!

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.

Joanthan

> 
> - Nuno Sá
>