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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c1e63a22-2dd7-48ab-9b98-ffd9f8b08a1b@kernel.org>
Date: Mon, 12 Aug 2024 15:55:58 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Trevor Gamblin <tgamblin@...libre.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 David Lechner <dlechner@...libre.com>,
 Uwe Kleine-Konig <u.kleine-koenig@...libre.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 1/3] dt-bindings: iio: adc: add AD762x/AD796x ADCs

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