Re: [PATCH v3 6/6] iio: adc: ad4851: add ad485x driver

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 25 Oct 2024 14:55:13 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 10/25/24 9:29 AM, David Lechner wrote:
> > On 10/25/24 6:35 AM, Miclaus, Antoniu wrote:  
> >>>  
> > ...
> >   
> >>>
> >>> See the ad7380 driver as an example of how to impelemt this. [2]
> >>>
> >>> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> >>> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
> >>> cbc4acea2cfa@baylibre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
> >>> CyeikpTumyjO0qDTn7eF7Fd-
> >>> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
> >>>
> >>> Also, I would expect the .sign value to depend on how the
> >>> input is being used. If it is differential or single-ended
> >>> bipolar, then it is signed, but if it is signle-ended unipoloar
> >>> then it is unsiged.
> >>>
> >>> Typically, this is coming from the devicetree because it
> >>> depends on what is wired up to the input.  
> >>
> >> This topic is mentioned in the cover letter, maybe not argued enough there.
> >> Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
> >> But this is a request from the actual users of the driver: to have the softspan fully
> >> controlled from userspace. That's why the offset and scale implementations were added.
> >> Both these attributes are influencing the softspan.
> >>  
> >>>> +	},								\
> >>>> +}  
> >>>  
> > 
> > The cover letter did not get sent, so we did not see this.  
> 
> So please resend it so we can get the full explanation.
> 
> > 
> > Still, I have doubts about using the offset attribute for
> > this since a 0 raw value is always 0V for both unipolar
> > and bipolar cases. There is never an offset to apply to
> > the raw value.
> > 
> > So I think we will need to find a different way to control
> > this other than the offset attribute.  
> 
> I thought about this some more and I have an idea to solve the
> issue without using devicetree or the offset attribute.
> 
> But we should see what Jonathan thinks before implementing this
> in case it isn't a good idea.
> 
> We can expose each voltage input to userspace as two different
> channels, a single-ended channel and a differential channel.

That was common in early drivers - such as the max1363 because they
were well prior to having sufficiently complex bindings to specify
wired channels.  We also have drivers that do this if no channel
subnodes are provided (kind of a fallback).

> 
> For an 8 channel chip, we would have 16 IIO channels (in order
> of scan_index):
> 
> in_voltage0_raw
> in_voltage0-voltage8_raw
> in_voltage1_raw
> in_voltage1-voltage9_raw
> ...
> in_voltage7_raw
> in_voltage7-voltage15_raw
> 
> If you read the voltage using in_voltageX_raw, then the SoftSpan
> for that channel gets set to the 0V to +V value based on
> in_voltageX_scale. Likewise, if you read the in_voltageX-voltageY_raw
> attribute, the SoftSpan gets set to -V to +V according to
> in_voltageX-voltageY_scale.
> 
> For buffered reads, only one of each in_voltageX_raw/in_voltageX-voltageY_raw
> pair can be enabled at the same time (because the chip is simultaneous
> sampling).
> 
This approach is fine as it's pretty much what some existing parts
are doing even if mostly people are these days preferring the
specified channel route.

Jonathan