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: <0b8a2d07-feea-409f-a850-7ee0c752a949@baylibre.com>
Date: Fri, 15 Nov 2024 11:06:55 -0600
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, lars@...afoo.de,
 Michael.Hennerich@...log.com, jic23@...nel.org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, marcelo.schmitt1@...il.com
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] dt-bindings: iio: adc: adi,ad4000: Add PulSAR

On 11/14/24 5:50 PM, Marcelo Schmitt wrote:
> Extend the AD4000 series device tree documentation to also describe
> PulSAR devices.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
> ---
>  .../bindings/iio/adc/adi,ad4000.yaml          | 115 +++++++++++++++++-
>  1 file changed, 114 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> index e413a9d8d2a2..35049071a9de 100644
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -19,6 +19,21 @@ description: |
>      https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
>      https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7685.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7686.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7687.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7688.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7690.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7691.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7693.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7694.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7942.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7946.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7980.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7982.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7983.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7984.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/ad7988-1_7988-5.pdf

It would be nice to sort these lowest number first.

>  
>  $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
> @@ -63,6 +78,38 @@ properties:
>  
>        - const: adi,adaq4003
>  
> +      - const: adi,ad7946
> +      - items:
> +          - enum:
> +              - adi,ad7942
> +          - const: adi,ad7946
> +
> +      - const: adi,ad7983
> +      - items:
> +          - enum:
> +              - adi,ad7980
> +              - adi,ad7988-5
> +              - adi,ad7686
> +              - adi,ad7685
> +              - adi,ad7694
> +              - adi,ad7988-1
> +          - const: adi,ad7983
> +
> +      - const: adi,ad7688
> +      - items:
> +          - enum:
> +              - adi,ad7693
> +              - adi,ad7687
> +          - const: adi,ad7688
> +
> +      - const: adi,ad7984
> +      - items:
> +          - enum:
> +              - adi,ad7982
> +              - adi,ad7690
> +              - adi,ad7691
> +          - const: adi,ad7984
> +

IMHO, having fallbacks just makes the bindings harder to use and doesn't
actually provide any useful benefit.

And with this many chips, it can be easy to overlook a small difference
in one chips, like ad7694 not having VIO pin, so is it really fallback
compatible? Easier to just avoid the question and not have fallbacks.

>    reg:
>      maxItems: 1
>  
> @@ -129,10 +176,76 @@ required:
>    - compatible
>    - reg
>    - vdd-supply
> -  - vio-supply
>    - ref-supply
>  
>  allOf:
> +  # AD7694 doesn't have a VIO pin

It sounds like using not: could make this if: a lot shorter.

Also, it looks like ad7983 doesn't have the pin either.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad4000
> +              - adi,ad4001
> +              - adi,ad4002
> +              - adi,ad4003
> +              - adi,ad4004
> +              - adi,ad4005
> +              - adi,ad4006
> +              - adi,ad4007
> +              - adi,ad4008
> +              - adi,ad4010
> +              - adi,ad4011
> +              - adi,ad4020
> +              - adi,ad4021
> +              - adi,ad4022
> +              - adi,adaq4001
> +              - adi,adaq4003
> +              - adi,ad7685
> +              - adi,ad7686
> +              - adi,ad7687
> +              - adi,ad7688
> +              - adi,ad7690
> +              - adi,ad7691
> +              - adi,ad7693
> +              - adi,ad7942
> +              - adi,ad7946
> +              - adi,ad7980
> +              - adi,ad7982
> +              - adi,ad7983
> +              - adi,ad7984
> +              - adi,ad7988-1
> +              - adi,ad7988-5
> +    then:
> +      required:
> +        - vio-supply
> +  # Single-channel PulSAR devices have SDI either tied to VIO, GND, or host CS.

To me, the more interesting thing to say here is that the sdi
option is omitted because these chips don't have a programmable
register.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad7685
> +              - adi,ad7686
> +              - adi,ad7687
> +              - adi,ad7688
> +              - adi,ad7690
> +              - adi,ad7691
> +              - adi,ad7693
> +              - adi,ad7694
> +              - adi,ad7942
> +              - adi,ad7946
> +              - adi,ad7980
> +              - adi,ad7982
> +              - adi,ad7983
> +              - adi,ad7984
> +              - adi,ad7988-1
> +              - adi,ad7988-5
> +    then:
> +      properties:
> +        adi,sdi-pin:
> +          enum: [ high, low, cs ]
> +          default: high

For the similar ad7944, Rob suggested that the default should be the equivalent
of "cs" since that is most like "regular" SPI. So I think it makes sense do the
same here. (The adi,spi-mode property in the ad7944 binding is named a bit
different, single = high, chain = low and _property omitted_ (default) = cs)

>    # The configuration register can only be accessed if SDI is connected to MOSI
>    - if:
>        required:


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ