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

Jonathan Cameron
2024-06-29T16:40:12Z

> > +#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