Received-SPF: pass (google.com: domain of linux-kernel+bounces-215922-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <2d9a1405-4b8d-401b-99c4-434ac4b57f6e@baylibre.com>
Date: Sat, 15 Jun 2024 10:23:31 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] dt-bindings: iio: adc: add AD4695 and similar ADCs
To: Jonathan Cameron <jic23@kernel.org>
Cc: Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Michael Hennerich <michael.hennerich@analog.com>,
 =?UTF-8?Q?Nuno_S=C3=A1?= <nuno.sa@analog.com>,
 Jonathan Corbet <corbet@lwn.net>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-doc@vger.kernel.org
References: <20240612-iio-adc-ad4695-v1-0-6a4ed251fc86@baylibre.com>
 <20240612-iio-adc-ad4695-v1-1-6a4ed251fc86@baylibre.com>
 <20240615134106.40e55e16@jic23-huawei>
Content-Language: en-US
From: David Lechner <dlechner@baylibre.com>
In-Reply-To: <20240615134106.40e55e16@jic23-huawei>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 6/15/24 7:41 AM, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 14:20:40 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> Add device tree bindings for AD4695 and similar ADCs.
>>

...

>> +
>> +  avdd-supply:
>> +    description: A 2.7 V to 5.5 V supply that powers the analog circuitry.
> 
> I'm a cynic.  Do we care about the supported voltages in this binding doc?
> Feels just somewhere we might make a mistake.

Not especially useful, I suppose. I'll clean these up a bit.

> 
>> +
>> +  ldo-in-supply:
>> +    description: A 2.4 V to 5.5 V supply connected to the internal LDO input.
>> +
>> +  vdd-supply:
>> +    description: A 1.8V supply that powers the core circuitry.
>> +
>> +  vio-supply:
>> +    description: A 1.2V to 1.8V supply for the digital inputs and outputs.
>> +
>> +  ref-supply:
>> +    description: A 2.4 V to 5.1 V supply for the external reference voltage.
>> +
>> +  refin-supply:
>> +    description: A 2.4 V to 5.1 V supply for the internal reference buffer input.
>> +
>> +  com-supply:
>> +    description: Common voltage supply for pseudo-differential analog inputs.
> 
> These last few have more info in them so definitely good to have the descriptions
> 

...

>> +
>> +patternProperties:
>> +  "^channel@[0-9a-f]$":
>> +    type: object
>> +    $ref: adc.yaml
>> +    unevaluatedProperties: false
>> +    description:
>> +      Describes each individual channel. In addition the properties defined
>> +      below, bipolar from adc.yaml is also supported.
>> +
>> +    properties:
>> +      reg:
>> +        maximum: 15
>> +        description: Input pin number (INx).
> 
> I'd drop this description as the pin pairing makes it messy.
> If you switch to diff-channels etc, just leave it as a index value not
> connected to the pin numbers.
> 
>> +
>> +      adi,pin-pairing:
>> +        description: |
>> +          The input pin pairing for the negative input. This can be:
>> +          - REFGND, normally 0V (single-ended)
>> +          - COM, normally V_REF/2, see com-supply (pseudo-differential)
>> +          - For even numbered pins, the next odd numbered pin (differential)
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        enum: [refgnd, com, next]
> 
> Next is full on differential, just provide both channels via
> diff-channels. You can constrain the particular combinations in the binding.
> 
> Refcnd is normal single ended.  Probably want to use the new single-channel
> property for that as we are mixing differential and single ended channels
> so reg is pretty much just an index.
> 
> Hmm. For comm we haven't had done a recent binding for a chip with the option
> of pseudo differential that is per channel, they've been whole device only.
> That feels like it will be common enough we need to support it cleanly
> with a 'general' scheme.

I think we have. :-)

https://lore.kernel.org/linux-iio/adc6cba9-2e79-475f-9c24-039fe9d3345d@baylibre.com/T/#mcbc1ce3a2541db502bf7870b7ea8574626a46312

> 
> Problem is I know someone will have a chip with 2 vincom pins and selecting
> between them, so we can't just have pseudo-differential as a boolean and adc.yaml
> 
> There are horrible solutions like a magic channel number that changes the
> meaning of diff-channels but that's ugly.
> Maybe pseudo-differential for now and we have to later we add
> pseudo-differential-comm  = <0> etc?
> 

I was trying to keep things simple with 1 property instead of 3, but we
can drop adi,pin-pairing and use the standard diff-channels, single-channel
and common-mode-channel properties.


> 
>> +        default: refgnd
>> +
>> +      adi,no-high-z:
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +        description: |
> 
> Do we need the | given not really formatted?

No. Was probably copy/paste.

> 
>> +          Enable this flag if the input pin requires the Analog Input High-Z
>> +          Mode to be disabled for proper operation.
>> +

...

>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    spi {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        adc@0 {
>> +            compatible = "adi,ad4695";
>> +            reg = <0>;
>> +            spi-cpol;
>> +            spi-cpha;
>> +            spi-max-frequency = <80000000>;
>> +            avdd-supply = <&supply_2_5V>;
>> +            vdd-supply = <&supply_1_8V>;
>> +            vio-supply = <&supply_1_2V>;
>> +            ref-supply = <&supply_5V>;
>> +            reset-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
>> +
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +

Using the standard adc.yaml properties, these would now be:

>> +            /* Differential channel between IN0 and IN1. */
>> +            channel@0 {
>> +                reg = <0>;

                    diff-channels = <0>, <1>;

>> +                bipolar;
>> +            };
>> +
>> +            /* Single-ended channel between IN2 and REFGND. */
>> +            channel@2 {
>> +                reg = <2>;

                    single-channel = <2>;
		    common-mode-channel = <0>;

>> +            };
>> +
>> +            /* Pseudo-differential channel between IN3 and COM. */
>> +            channel@f {
>> +                reg = <3>;

                    single-channel = <3>;
                    common-mode-channel = <1>;

>> +                bipolar;
>> +            };
>> +        };
>> +    };
> 

And I will add a header file with macros for the common mode
channel values.