Received-SPF: pass (google.com: domain of linux-kernel+bounces-192271-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d87ae6ef-090d-4e47-bde4-4d08fd445ac1@gmail.com>
Date: Tue, 28 May 2024 15:16:07 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/6] dt-bindings: adc: ad7173: add support for ad411x
To: Conor Dooley <conor@kernel.org>, dumitru.ceclan@analog.com
Cc: 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>, David Lechner <dlechner@baylibre.com>,
 linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240527-ad4111-v3-0-7e9eddbbd3eb@analog.com>
 <20240527-ad4111-v3-1-7e9eddbbd3eb@analog.com>
 <20240527-arguably-said-361184ad848e@spud>
Content-Language: en-US
From: "Ceclan, Dumitru" <mitrutzceclan@gmail.com>
In-Reply-To: <20240527-arguably-said-361184ad848e@spud>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
>>
>> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
>>
>> AD411x family ADCs support a VCOM pin, dedicated for single-ended usage.
>> AD4111/AD4112 support current channels, usage is implemented by
>>  specifying channel reg values bigger than 15.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 122 ++++++++++++++++++++-
>>  1 file changed, 120 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.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: |
>>    primarily for measurement of signals close to DC but also delivers
>>    outstanding performance with input bandwidths out to ~10kHz.
>>  
>> +  Analog Devices AD411x ADC's:
>> +  The AD411X family encompasses a series of low power, low noise, 24-bit,
>> +  sigma-delta analog-to-digital converters that offer a versatile range of
>> +  specifications. They integrate an analog front end suitable for processing
>> +  fully differential/single-ended and bipolar voltage inputs.
>> +
>>    Datasheets for supported chips:
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
>> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
>>      https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
>> @@ -31,6 +42,11 @@ description: |
>>  properties:
>>    compatible:
>>      enum:
>> +      - adi,ad4111
>> +      - adi,ad4112
>> +      - adi,ad4114
>> +      - adi,ad4115
>> +      - adi,ad4116
>>        - adi,ad7172-2
>>        - adi,ad7172-4
>>        - adi,ad7173-8
>> @@ -129,10 +145,36 @@ patternProperties:
>>          maximum: 15
>>  
>>        diff-channels:
>> +        description: |
>> +          For using current channels specify select the current inputs
>> +           and enable the adi,current-channel property.
>> +
>> +          Family AD411x supports a dedicated VINCOM voltage input.
>> +          To select it set the second channel to 16.
>> +            (VIN2, VINCOM) -> diff-channels = <2 16>
>> +
>> +          There are special values that can be selected besides the voltage
>> +          analog inputs:
>> +            21: REF+
>> +            22: REF−
>> +          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2:
>> +            19: ((AVDD1 − AVSS)/5)+
>> +            20: ((AVDD1 − AVSS)/5)−
>> +
>>          items:
>>            minimum: 0
>>            maximum: 31
>>  
>> +      single-channel:
>> +        description: |
>> +          Models AD4111 and AD4112 support single-ended current channels.
>> +          To select the desired current input, specify the desired input pair:
>> +            (IIN2+, IIN2−) -> single-channel = <2>
>> +
>> +        items:
>> +          minimum: 1
>> +          maximum: 16
>> +
>>        adi,reference-select:
>>          description: |
>>            Select the reference source to use when converting on
>> @@ -154,9 +196,26 @@ patternProperties:
>>            - avdd
>>          default: refout-avss
>>  
>> +      adi,current-channel:
>> +        description: |
>> +          Signal that the selected inputs are current channels.
>> +          Only available on AD4111 and AD4112.
>> +        type: boolean
>> +
>> +      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.

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.

> 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

> 
> Thanks,
> Conor.