Received-SPF: pass (google.com: domain of linux-kernel+bounces-194998-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <218737c70a9215e1ed49874f075a616bc9cf1de9.camel@gmail.com>
Subject: Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: David Lechner <dlechner@baylibre.com>, "Ceclan, Dumitru"
	 <mitrutzceclan@gmail.com>, Conor Dooley <conor@kernel.org>
Cc: dumitru.ceclan@analog.com, Lars-Peter Clausen <lars@metafoo.de>, Michael
 Hennerich <Michael.Hennerich@analog.com>, Jonathan Cameron
 <jic23@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>,  Conor Dooley <conor+dt@kernel.org>,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
 linux-kernel@vger.kernel.org
Date: Thu, 30 May 2024 09:12:22 +0200
In-Reply-To: <362e006f-7856-46e3-90fa-b1610784ee9d@baylibre.com>
References: <20240527-ad4111-v3-0-7e9eddbbd3eb@analog.com>
	 <20240527-ad4111-v3-1-7e9eddbbd3eb@analog.com>
	 <20240527-arguably-said-361184ad848e@spud>
	 <d87ae6ef-090d-4e47-bde4-4d08fd445ac1@gmail.com>
	 <20240528-filtrate-cloning-b9152322a3da@spud>
	 <a1c75105-6447-4b67-b7d2-326ad9b19b82@gmail.com>
	 <362e006f-7856-46e3-90fa-b1610784ee9d@baylibre.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 (3.52.1-1.fc40) 
Precedence: bulk
MIME-Version: 1.0

On Wed, 2024-05-29 at 17:04 -0500, David Lechner wrote:
> On 5/29/24 8:38 AM, 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 R=
elay
> > > > > wrote:
> > > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > > > >=20
> > > > > > Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> > > > > >=20
> > > > > > AD411x family ADCs support a VCOM pin, dedicated for single-end=
ed usage.
> > > > > > AD4111/AD4112 support current channels, usage is implemented by
> > > > > > =C2=A0specifying channel reg values bigger than 15.
> > > > > >=20
> > > > > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > > > > ---
> > > > > > =C2=A0.../devicetree/bindings/iio/adc/adi,ad7173.yaml=C2=A0=C2=
=A0=C2=A0 | 122
> > > > > > ++++++++++++++++++++-
> > > > > > =C2=A01 file changed, 120 insertions(+), 2 deletions(-)
> > > > > >=20
> > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad71=
73.yaml
> > > > > > b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > index ea6cfcd0aff4..5b1af382dad3 100644
> > > > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> > > > > > @@ -19,7 +19,18 @@ description: |
> > > > > > =C2=A0=C2=A0 primarily for measurement of signals close to DC b=
ut also delivers
> > > > > > =C2=A0=C2=A0 outstanding performance with input bandwidths out =
to ~10kHz.
> > > > > > =C2=A0
> > > > > > +=C2=A0 Analog Devices AD411x ADC's:
> > > > > > +=C2=A0 The AD411X family encompasses a series of low power, lo=
w noise, 24-
> > > > > > bit,
> > > > > > +=C2=A0 sigma-delta analog-to-digital converters that offer a v=
ersatile range
> > > > > > of
> > > > > > +=C2=A0 specifications. They integrate an analog front end suit=
able for
> > > > > > processing
> > > > > > +=C2=A0 fully differential/single-ended and bipolar voltage inp=
uts.
> > > > > > +
> > > > > > =C2=A0=C2=A0 Datasheets for supported chips:
> > > > > > +=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD4111.pdf
> > > > > > +=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD4112.pdf
> > > > > > +=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD4114.pdf
> > > > > > +=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD4115.pdf
> > > > > > +=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD4116.pdf
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD7172-2.pdf
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD7172-4.pdf
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0
> > > > > > https://www.analog.com/media/en/technical-documentation/data-sh=
eets/AD7173-8.pdf
> > > > > > @@ -31,6 +42,11 @@ description: |
> > > > > > =C2=A0properties:
> > > > > > =C2=A0=C2=A0 compatible:
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 enum:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad4111
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad4112
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad4114
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad4115
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad4116
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad7172-2
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad7172-4
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - adi,ad7173-8
> > > > > > @@ -129,10 +145,36 @@ patternProperties:
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 maximum: 15
> > > > > > =C2=A0
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 diff-channels:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 description: |
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 For usi=
ng current channels specify select the current inputs
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 a=
nd enable the adi,current-channel property.
> > > > > > +
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Family =
AD411x supports a dedicated VINCOM voltage input.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 To sele=
ct it set the second channel to 16.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 (VIN2, VINCOM) -> diff-channels =3D <2 16>
> > > > > > +
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 There a=
re special values that can be selected besides the
> > > > > > voltage
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 analog =
inputs:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 21: REF+
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 22: REF=E2=88=92
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Support=
ed only by AD7172-2, AD7172-4, AD7175-2, AD7175-8,
> > > > > > AD7177-2:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 19: ((AVDD1 =E2=88=92 AVSS)/5)+
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 20: ((AVDD1 =E2=88=92 AVSS)/5)=E2=88=92
> > > > > > +
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 items:
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 mi=
nimum: 0
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ma=
ximum: 31
> > > > > > =C2=A0
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 single-channel:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 description: |
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Models =
AD4111 and AD4112 support single-ended current
> > > > > > channels.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 To sele=
ct the desired current input, specify the desired input
> > > > > > pair:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0 (IIN2+, IIN2=E2=88=92) -> single-channel =3D <2>
> > > > > > +
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 items:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 minimum=
: 1
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 maximum=
: 16
> > > > > > +
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adi,reference-select:
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 description: |
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Se=
lect the reference source to use when converting on
> > > > > > @@ -154,9 +196,26 @@ patternProperties:
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - =
avdd
> > > > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 default: refou=
t-avss
> > > > > > =C2=A0
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adi,current-channel:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 description: |
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Signal =
that the selected inputs are current channels.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Only av=
ailable on AD4111 and AD4112.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 type: boolean
> > > > > > +
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adi,channel-type:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 description:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Used to=
 differentiate between different channel types as the
> > > > > > device
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 r=
egister configurations are the same for all usage types.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 Both ps=
eudo-differential and single-ended channels will use
> > > > > > the
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 s=
ingle-ended specifier.
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 $ref: /schemas/type=
s.yaml#/definitions/string
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 enum:
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - singl=
e-ended
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 - diffe=
rential
> > > > > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 default: differenti=
al
> > > > >=20
> > > > > 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 g=
ot
> > > > > 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 bind=
ing.
> > > >=20
> > > > In previous thread we arrived to the conclusion single-ended and
> > > > pseudo-differential channels should be marked with the flag
> > > > "differential=3Dfalse" 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.
> > > >=20
> > > > 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.
> > >=20
> > > 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.
> > >=20
> > 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=3Dfalse within a channel definition.
> >=20
> > No, there is no uncertainty regarding pseudo-differential, it's
> > basically single-ended.
> >=20
> > We cannot use single-channel for voltage channels, two voltage
> > inputs need to be specified. And again, single-channel will be
> > used here for the current channels.=20
>=20
> Instead of using diff-channels for single-ended/pseudo-differential
> plus a property that says "actually not differential" could we just
> add a second common-mode-channel property to specify the second
> input pin that is connected to the common mode voltage source?
>=20

Yeah, I definitely don't like having diff-channels and then go "oh, not rea=
lly a
differential channel". So, if could avoid that, it would be great!

> Just to make sure I'm understanding, single-ended means common mode
> voltage is 0V (or AVSS for this chip, I guess) and pseudo-differential
> means the common mode voltage is something else other than that.
> In other words, single-ended is just a special case of pseudo-differentia=
l.
> So effectively, no difference that we need to describe?
>=20
> So something like this could work?
>=20
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* a fully differential voltag=
e input channel */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 channel@0 {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reg =3D <0>;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bipolar;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 diff-channels =3D =
<0 1>; /* VIN0 is +, VIN1 is - */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adi,reference-sele=
ct =3D "vref";
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* a single-ended voltage inpu=
t channel */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 channel@1 {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reg =3D <1>;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* no bipolar sinc=
e common mode is 0V */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 single-channel =3D=
 <2>; /* VIN2 is input */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 common-mode-channe=
l =3D <3>; /* VIN3 connected to 0V */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* a pseudo-differential volta=
ge input channel */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 channel@2 {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reg =3D <2>;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bipolar; /* since =
common mode is not 0V */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 single-channel =3D=
 <4>; /* VIN4 is input */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 common-mode-channe=
l =3D <5>; /* VIN5 connected to Vref / 2 */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adi,reference-sele=
ct =3D "vref";
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 };
>=20
> 	/* a current input channel */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 channel@3 {
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 reg =3D <3>;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 bipolar;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* 0 is not the sa=
me pin as channel@0 because of
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 * the adi,cu=
rrent-channel flag */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 single-channel =3D=
 <0>; /* using IIN0+ and IIN0- pins */
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 adi,current-channe=
l;

Unless I'm missing something, this flag is also not being used anywhere in =
the
current series?

- Nuno S=C3=A1