Received-SPF: pass (google.com: domain of linux-kernel+bounces-129203-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: <25cb3514-1281-49a8-9e9b-40ead9b050dc@gmail.com>
Date: Wed, 3 Apr 2024 10:43:54 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/6] dt-bindings: adc: ad7173: add support for ad411x
To: David Lechner <dlechner@baylibre.com>, 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>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240401-ad4111-v1-0-34618a9cc502@analog.com>
 <20240401-ad4111-v1-1-34618a9cc502@analog.com>
 <CAMknhBHeKAQ45=5-dL1T1tv-mZcPN+bNo3vxWJYgWpEPE+8p3Q@mail.gmail.com>
Content-Language: en-US
From: "Ceclan, Dumitru" <mitrutzceclan@gmail.com>
In-Reply-To: <CAMknhBHeKAQ45=5-dL1T1tv-mZcPN+bNo3vxWJYgWpEPE+8p3Q@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 01/04/2024 22:37, David Lechner wrote:
> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay
> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote:
>>
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>

..

>>      properties:
>>        reg:
>> +        description:
>> +          Reg values 16-19 are only permitted for ad4111/ad4112 current channels.
>>          minimum: 0
>> -        maximum: 15
>> +        maximum: 19
> 
> This looks wrong. Isn't reg describing the number of logical channels
> (# of channel config registers)?
> 
> After reviewing the driver, I see that > 16 is used as a way of
> flagging current inputs, but still seems like the wrong way to do it.
> See suggestion below.
> 

This was a suggestion from Jonathan, maybe I implemented it wrong.
Other alternative that came to my mind: attribute "adi,current-channel".
>>
>>        diff-channels:
>> +        description:
>> +          For using current channels specify only the positive channel.
>> +            (IIN2+, IIN2−) -> diff-channels = <2 0>
> 
> I find this a bit confusing since 2 is already VIN2 and 0 is already
> VIN0. I think it would make more sense to assign unique channel
> numbers individually to the negative and positive current inputs.
> Also, I think it makes sense to use the same numbers that the
> registers in the datasheet use (8 - 11 for negative and 12 to 15 for
> positive).
> 
> So: (IIN2+, IIN2−) -> diff-channels = <13 10>
> 
> 
It would mean for the user to look in the datasheet at the possible
channel INPUT configurations values, decode the bit field into two
integer values and place it here (0110101010) -> 13 10. This is
cumbersome for just choosing current input 2.

>> +
>> +          Family AD411x supports a dedicated VCOM voltage input.
>> +          To select it set the second channel to 16.
>> +            (VIN2, VCOM) -> diff-channels = <2 16>
> 
> The 411x datasheets call this pin VINCOM so calling it VCOM here is a
> bit confusing.
> 

Sure, I'll rename to VINCOM.

> Also, do we need to add a vincom-supply to get this voltage? Or is it
> safe to assume it is always connected to AVSS? The datasheet seems to
> indicate that the latter is the case. But then it also has this
> special case (at least for AD4116, didn't check all datasheets)
> "VIN10, VINCOM (single-ended or differential pair)". If it can be used
> as part of a fully differential input, we probably need some extra
> flag to indicate that case.
> 

I cannot see any configuration options for these use cases. All inputs
are routed to the same mux and routed to the differential positive and
negative ADC inputs.

"VIN10, VINCOM (single-ended or differential pair)" the only difference
between these two use cases is if you connected VINCOM to AVSS (with
unipolar coding) or not with bipolar encoding. The channel is still
measuring the difference between the two selected inputs and comparing
to the selected reference.

> Similarly, do we need special handling for ADCIN15 on AD4116? It has a
> "(pseudo differential or differential pair)" notation that other
> inputs don't. In other words, it is more like VINCOM than it is to the
> other ADCINxx pins. So we probably need an adcin15-supply for this pin
> to properly get the right channel configuration. I.e. the logic in the
> IIO driver would be if adcin15-supply is present, any channels that
> use this input are pseudo-differential, otherwise any channels that
> use it are fully differential.
> 

I cannot seem to understand what would a adcin15-supply be needed for.
This input, the same as all others, enters the mux and is routed to
either positive or negative input of the ADC.

The voltage on the ADCIN15 pin is not important to the user, just the
difference in voltage between that pin and the other one selected.

>>          items:
>>            minimum: 0
>>            maximum: 31
>> @@ -166,7 +191,6 @@ allOf:
>>    - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>
>>    # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
>> -  # Other models have [0-3] channel registers
> 
> Did you forget to remove
> 
>             reg:
>               maximum: 3
> 
> from this if statement that this comment is referring to?
> 
> 


Other way around, forgot in a previous patch to remove the comment.
I'll move this change to a precursor patch.

>>    - if:
>>        properties:
>>          compatible:
>> @@ -187,6 +211,37 @@ allOf:
>>                  - vref
>>                  - refout-avss
>>                  - avdd
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ad4114
>> +              - adi,ad4115
>> +              - adi,ad4116
>> +              - adi,ad7173-8
>> +              - adi,ad7175-8
>> +    then:
>> +      patternProperties:
>> +        "^channel@[0-9a-f]$":
>> +          properties:
>> +            reg:
>> +              maximum: 15
> 
> As with the previous reg comment, this if statement should not be
> needed since maximum should not be changed to 19.
> 

We'll see what is the best approach regarding the current channels,
perhaps the one you mentioned in the later reply with always configuring
like the temp channel.

>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - adi,ad7172-2
>> +              - adi,ad7175-2
>> +              - adi,ad7176-2
>> +              - adi,ad7177-2
>> +    then:
>> +      patternProperties:
>> +        "^channel@[0-9a-f]$":
>> +          properties:
>>              reg:
>>                maximum: 3
> 
> It looks to me like AD7172-4 actually has 8 possible channels rather
> than 16. So it would need a special condition as well. But that is a
> bug in the previous bindings and should therefore be fixed in a
> separate patch.

It is addressed already in the binding:
"
  - if:
      properties:
        compatible:
          contains:
            const: adi,ad7172-4
[...]
              maximum: 7
"