Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron <jic23@kernel.org>]

> > +#define AD4030_CHAN_CMO(_idx)  {					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx,						\
> > +	.scan_index = _idx,						\
> > +	.scan_type = {							\
> > +		.sign = 'u',						\
> > +		.storagebits = 8,					\
> > +		.realbits = 8,						\
> > +		.endianness = IIO_BE,					\
> > +	},								\
> > +}
> > +  
> 
> So, from the datasheet, figure 39 we have something like a multiplexer where we
> can have:
> 
> - averaged data;
> - normal differential;
> - test pattern (btw, useful to have it in debugfs - but can come later);

Can consider doing test pattern as simply a different channel, data is afterall
coming from a very different place.


> - 8 common mode bits;
> 
> While the average, normal and test pattern are really mutual exclusive, the
> common mode voltage is different in the way that it's appended to differential
> sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
> for us to see them as different channels from a SW perspective (even more since
> gain and offset only apply to the differential data). But there are a couple of
> things I don't like (have concerns):
> 
> * 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).
> 
> Other thing that came to mind is if we could somehow use differential = true
> here. Having something like:
> 
> in_voltage1_in_voltage0_raw - diff data
in_voltage1-voltage0_raw  so normal differential channel.

> ...
> And the only thing for CM would be:
> 
> in_voltage1_raw
> in_voltage1_scale

you can do that with normal channels.

> 
> (not sure if the above is doable with ext_info - maybe only with device_attrs)
We 'could' do the somewhat nasty

in_voltage1-voltage0_raw - so normal differential channel.

in_voltage1+voltage0_raw via a custom attribute.  Don't try to hammer that in
as channel related though.


> 
> The downside of the above is that we don't have a way to separate the scan
> elements. Meaning that we don't have a way to specify the scan_type for both the
> common mode and differential voltage. That said, I wonder if it is that useful
> to buffer the common mode stuff? Alternatively, we could just have the scan_type
> for the diff data and apps really wanting the CM voltage could still access the
> raw data. Not pretty, I know...

Hmm. That is indeed a nasty corner if we don't use a real channel.
This may be a case for just using labels for the relationship.

> 
> However, even if we go with the two separate channels there's one thing that
> concerns me. Right now we have diff data with 32 for storage bits and CM data
> with 8 storage bits which means the sample will be 40 bits and in reality we
> just have 32. Sure, now we have SW buffering so we can make it work but the
> ultimate goal is to support HW buffering where we won't be able to touch the
> sample and thus we can't lie about the sample size. Could you run any test with
> this approach on a HW buffer setup? 
> 
> I did not gave too much thought on it but I'm not sure there's a sane way to
> have multiple scan_types associated with the same channel. 

Not at the same time.

Jonathan