Message-ID: <2b319d9453f4fe8842e4c306d9e2071ad031c0e7.camel@gmail.com>
Date: Mon, 16 Sep 2024 08:12:24 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Esteban Blanc <eblanc@...libre.com>, Lars-Peter Clausen
 <lars@...afoo.de>,  Michael Hennerich <Michael.Hennerich@...log.com>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>, Jonathan Corbet
 <corbet@....net>,  linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
 linux-kernel@...r.kernel.org, David Lechner <dlechner@...libre.com>, 
 linux-doc@...r.kernel.org
Subject: Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and
 ad4630-16

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@...il.com> 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@...libre.com> 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@gmail.com/
> > > > > :
> > > > >   
> > > > > > > > * 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á 

Powered by blists - more mailing lists