Re: [PATCH v2 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/08/2024 15:41, Trevor Gamblin wrote:
> 
> On 2024-08-10 8:10 a.m., Krzysztof Kozlowski wrote:
>> On 09/08/2024 20:41, Trevor Gamblin wrote:
>>> Add a binding specification for the Analog Devices Inc. AD7625,
>>> AD7626, AD7960, and AD7961 ADCs.
>>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +allOf:
>>> +  - if:
>>> +      required:
>>> +        - ref-supply
>>> +    then:
>>> +      # refin-supply is not needed if ref-supply is given
>> Not needed or not allowed? Schema says the latter.
> Yes, this is poor wording on my part. I will fix it to say "not allowed".

so just drop it. No need to repeat schema - it is obvious from the
comment. OTOH, if you want to keep any of such comments, then make it
useful - explain WHY it is not allowed. Because the WHY is not visible
from the code.

>>
>>> +      properties:
>>> +        refin-supply: false
>>> +  - if:
>>> +      required:
>>> +        - refin-supply
>>> +    then:
>>> +      # ref-supply is not needed if refin-supply is given
>>> +      properties:
>>> +        ref-supply: false
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - adi,ad7625
>>> +              - adi,ad7626
>>> +    then:
>>> +      properties:
>>> +        en2-gpios: false
>>> +        en3-gpios: false
>>> +        adi,en2-always-on: false
>>> +        adi,en3-always-on: false
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - adi,ad7960
>>> +              - adi,ad7961
>>> +    then:
>>> +      # ad796x parts must have one of the two supplies
>>> +      oneOf:
>>> +        - required: [ref-supply]
>>> +        - required: [refin-supply]
>> That's duplicating first and second if. And all three - comment, first
>> if:then: and this one here is kind of contradictory so I don't know what
>> you want to achieve.
> 
> It sounds like there's a better way for me to specify this, but I'm not 
> exactly sure how.
> 
> The AD762x parts can operate without external references, so the intent 
> was that neither REF nor REFIN was required in the bindings, but if one 
> is given then the other can't be.
> 
> For the AD796x parts, one of REF or REFIN must be provided, but not 
> both. If REFIN is provided, then REF doesn't need an input because a 
> reference voltage is generated on REF. If REF is provided, then REFIN is 
> tied to ground.
> 
> Maybe there's a simpler way for me to specify the whole block?

Ah, now I see. Looks correct. I am not sure if it could be coded simpler.

Best regards,
Krzysztof