Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5551 bytes --]

On Wed, May 29, 2024 at 04:38:53PM +0300, Ceclan, Dumitru wrote:
> On 28/05/2024 20:52, Conor Dooley wrote:
> > On Tue, May 28, 2024 at 03:16:07PM +0300, Ceclan, Dumitru wrote:
> >> On 27/05/2024 20:48, Conor Dooley wrote:
> >>> On Mon, May 27, 2024 at 08:02:34PM +0300, Dumitru Ceclan via B4 Relay wrote:
> >>>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> >>>> +      adi,channel-type:
> >>>> +        description:
> >>>> +          Used to differentiate between different channel types as the device
> >>>> +           register configurations are the same for all usage types.
> >>>> +          Both pseudo-differential and single-ended channels will use the
> >>>> +           single-ended specifier.
> >>>> +        $ref: /schemas/types.yaml#/definitions/string
> >>>> +        enum:
> >>>> +          - single-ended
> >>>> +          - differential
> >>>> +        default: differential
> >>>
> >>> I dunno if my brain just ain't workin' right today, or if this is not
> >>> sufficiently explained, but why is this property needed? You've got
> >>> diff-channels and single-channels already, why can you not infer the
> >>> information you need from them? What should software do with this
> >>> information?
> >>> Additionally, "pseudo-differential" is not explained in this binding.
> >>
> >> In previous thread we arrived to the conclusion single-ended and
> >> pseudo-differential channels should be marked with the flag
> >> "differential=false" in the IIO channel struct. This cannot
> >> really be inferred as any input pair could be used in that
> >> manner and the only difference would be in external wiring.
> >>
> >> Single-channels cannot be used to define such a channel as
> >> two voltage inputs need to be selected. Also, we are already
> >> using single-channel to define the current channels.
> > 
> > If I understand correctly, the property could be simplified to a flag
> > then, since it's only the pseudo differential mode that you cannot be
> > sure of?
> > You know when you're single-ended based on single-channel, so the
> > additional info you need is only in the pseudo-differential case.
> > 
> Yes, it could just be a boolean flag. The only thing I have against
> that is the awkwardness of having both diff-channels and
> differential=false within a channel definition.

What I was suggesting was more like "adi,pseudo-differential" (you don't
need to set the =false or w/e, flag properties work based on present/not
present). I think that would avoid the awkwardness?

> >> As for explaining the pseudo-differential, should it be explained?
> >> A voltage channel within the context of these families is actually
> >> differential(as there are always two inputs selected).
> >> The single-ended and pseudo-diff use case is actually wiring up a
> >> constant voltage to the selected negative input.
> >>
> >> I did not consider that this should be described, as there is no
> >> need for an attribute to describe it.
> > 
> > I dunno, adding an explanation of it in the text for the channel type
> > seems trivial to do. "Both pseudo-differential mode (where the
> > one of differential inputs is connected to a constant voltage) and
> > single-ended channels will..."
> >
> >>> Also, what does "the device register configurations are the same for
> >>> all uses types" mean? The description here implies that you'd be reading
> >>> the registers to determine the configuration, but as far as I understand
> >>> it's the job of drivers to actually configure devices.
> >>> The only way I could interpret this that makes sense to me is that you're
> >>> trying to say that the device doesn't have registers that allow you to
> >>> do runtime configuration detection - but that's the norm and I would not
> >>> call it out here.
> >>
> >> No, I meant that the same register configuration will be set for
> >> both fully differential and single-ended. 
> >>
> >> The user will set diff-channels = <0, 1>, bipolar(or not) and
> >> then they can wire whatever to those pins: 
> >> - a differential signal
> >> - AVSS to 1 and a single-ended signal to 0
> >> - AVSS+offset to 1 and a single-ended signal to 0
> >> 	(which is called pseudo-differential in some datasheets)
> >>
> >> All these cases will look the same in terms of configuration
> > 
> > In that case, I'd just remove this sentence from the description then.
> > How you configure the registers to use the device doesn't really have
> > anything to do with describing the configuration of the hardware.
> > Given it isn't related to configuration detection at runtime, what
> > you've got written here just makes it seem like the property is
> > redundant because the register settings do not change.
> >
> > Instead, use the description to talk about when the property should be
> > used and what software should use it to determine, e.g. "Software can
> > use vendor,channel-type to determine whether or not the measured voltage
> > is absolute or relative". I pulled that outta my ass, it might not
> > be what you're actually doing, but I figure you just want to know if
> > you're measuring from the origin or either side of it.

> >It's more to the "software can this property to correctly mark the channel

Your quoting is scuffed here, I didn't write this!

> as differential or not". Hope this is acceptable. But got it, thanks.

As long as you've got a description that tells the OS what the property
actually represents, I'm happy.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]