Received-SPF: pass (google.com: domain of linux-kernel+bounces-143838-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240413151152.165682-1-alisa.roman@analog.com> <20240413151152.165682-3-alisa.roman@analog.com>
In-Reply-To: <20240413151152.165682-3-alisa.roman@analog.com>
From: David Lechner <dlechner@baylibre.com>
Date: Sat, 13 Apr 2024 13:05:01 -0500
Message-ID: <CAMknhBEwS2O3gWnJcc2RDXbeiUZ54pPm8PskGTTKL9xKYBOZ4A@mail.gmail.com>
Subject: Re: [PATCH v5 2/5] dt-bindings: iio: adc: ad7192: Add aincom supply
To: Alisa-Dariana Roman <alisadariana@gmail.com>
Cc: michael.hennerich@analog.com, linux-iio@vger.kernel.org, 
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	alexandru.tachici@analog.com, lars@metafoo.de, jic23@kernel.org, 
	robh@kernel.org, krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	lgirdwood@gmail.com, broonie@kernel.org, andy@kernel.org, nuno.sa@analog.com, 
	marcelo.schmitt@analog.com, bigunclemax@gmail.com, okan.sahin@analog.com, 
	fr0st61te@gmail.com, alisa.roman@analog.com, marcus.folkesson@gmail.com, 
	schnelle@linux.ibm.com, liambeguin@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Apr 13, 2024 at 10:12=E2=80=AFAM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> AINCOM should actually be a supply. If present and it has a non-zero
> voltage, the pseudo-differential channels are configured as single-ended
> with an offset. Otherwise, they are configured as differential channels
> between AINx and AINCOM pins.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com>
> ---
>  .../devicetree/bindings/iio/adc/adi,ad7192.yaml          | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/=
Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> index 16def2985ab4..ba506af3b73e 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
> @@ -41,6 +41,14 @@ properties:
>    interrupts:
>      maxItems: 1
>
> +  aincom-supply:
> +    description: |
> +      Optional AINCOM voltage supply. If present and it has a non-zero v=
oltage,
> +      the pseudo-differential channels are configured as single-ended ch=
annels
> +      with the AINCOM voltage as offset. Otherwise, the pseudo-different=
ial
> +      channels are configured as differential channels: voltageX-voltage=
0, with
> +      AINCOM as the negative input.

This description doesn't sound quite right to me. The datasheet has no
mention of single-ended inputs. And how each AINx input is used is
independent of whether this property is present or not. And
voltageX-voltage0 is a driver implementation detail that doesn't
really belong in the bindings.

Also, just FYI, in a similar case, Jonathan recently mentioned that he
would prefer that these sorts of supplies would be required rather
than optional [1]. But in this case, I think it needs to be optional
for backwards compatibility since we are modifying existing DT
bindings. But the point still stands that this property being present
or not doesn't mean anything special (other than we might assume the
AINCOM pin is connected to GND if the property is omitted).

[1]: https://lore.kernel.org/linux-iio/20240413181025.39d1a62e@jic23-huawei=
/

In any case, a better description would be one more like what the
datasheet says for the AINCOM pin:

"Analog inputs AIN1 to AIN4 are referenced to this input when
configured for pseudodifferential operation."

> +
>    dvdd-supply:
>      description: DVdd voltage supply
>
> @@ -117,6 +125,7 @@ examples:
>              clock-names =3D "mclk";
>              interrupts =3D <25 0x2>;
>              interrupt-parent =3D <&gpio>;
> +            aincom-supply =3D <&aincom>;
>              dvdd-supply =3D <&dvdd>;
>              avdd-supply =3D <&avdd>;
>              vref-supply =3D <&vref>;
> --
> 2.34.1
>