linux-kernel - Re: [PATCH 6/7] dt-bindings: iio: adc: add adi,ad7606c-{16,18} compatible strings

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: <zuvwoy5wtdel7qgkz6wa6valwjwajpwoqnizyoooiawghrxvc3@cuoswu32h4fl>
Date: Mon, 19 Aug 2024 15:09:42 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Alexandru Ardelean <aardelean@...libre.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, jic23@...nel.org, krzk+dt@...nel.org, robh@...nel.org, 
	lars@...afoo.de, michael.hennerich@...log.com, gstols@...libre.com
Subject: Re: [PATCH 6/7] dt-bindings: iio: adc: add adi,ad7606c-{16,18}
 compatible strings

On Mon, Aug 19, 2024 at 09:47:16AM +0300, Alexandru Ardelean wrote:
> The driver will support the AD7606C-16 and AD7606C-18.
> This change adds the compatible strings for these devices.
> 
> The AD7606C-16,18 channels also support these (individually configurable)
> types of channels:
>  - bipolar single-ended
>  - unipolar single-ended
>  - bipolar differential
> 
> This DT adds support for 'channel@X' nodes'

I don't understand this sentence, suggest to drop it.


> 
> Signed-off-by: Alexandru Ardelean <aardelean@...libre.com>
> ---
>  .../bindings/iio/adc/adi,ad7606.yaml          | 83 +++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> index 69408cae3db9..f9e177de3f8c 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7606.yaml
> @@ -14,6 +14,8 @@ description: |
>    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7605-4.pdf
>    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606_7606-6_7606-4.pdf
>    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7606B.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-16.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad7606c-18.pdf
>    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7616.pdf
>  
>  properties:
> @@ -24,6 +26,8 @@ properties:
>        - adi,ad7606-6
>        - adi,ad7606-8  # Referred to as AD7606 (without -8) in the datasheet
>        - adi,ad7606b
> +      - adi,ad7606c-16
> +      - adi,ad7606c-18
>        - adi,ad7616
>  
>    reg:
> @@ -114,6 +118,30 @@ properties:
>        assumed that the pins are hardwired to VDD.
>      type: boolean
>  
> +patternProperties:
> +  "^channel@([0-9a-f])$":

[0-7]

> +    type: object
> +    $ref: adc.yaml
> +    unevaluatedProperties: false
> +
> +    properties:
> +      reg:
> +        description: The channel number.
> +        minimum: 0
> +        maximum: 7
> +
> +      diff-channel:
> +        description: Channel is bipolar differential.

There is diff-channels property, why do we need one more?

> +        type: boolean
> +
> +      bipolar:
> +        description: |
> +          Channel is bipolar single-ended. If 'diff-channel' is set, then
> +          the value of this property will be ignored.

Then provide here allOf:if:then which makes it false if diff-channel(s)
is present. And then drop entire property, because you duplicate what's
in adc.yaml.


> +        type: boolean

Blank line.

> +    required:
> +      - reg
> +
>  required:
>    - compatible
>    - reg
> @@ -170,6 +198,21 @@ allOf:
>          adi,conversion-start-gpios:
>            maxItems: 1
>  
> +  - if:
> +      not:
> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - adi,ad7606c-16
> +                - adi,ad7606c-18
> +    then:
> +      patternProperties:
> +        "^channel@([0-9a-f])$":
> +          properties:
> +            diff-channels: false
> +            bipolar: true

? Drop, no clue what you want to say here. But more important, you are
now adding channels to other variants. Split your commit between new
device and new properties for existing devices.


Best regards,
Krzysztof