linux-kernel - Re: [PATCH v4 28/33] dt-bindings: crypto: rockchip: convert to new driver bindings

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: <30305936-4b69-e1ce-44c2-0d1d113b460e@linaro.org>
Date:   Sat, 2 Apr 2022 13:53:58 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Corentin Labbe <clabbe@...libre.com>, heiko@...ech.de,
        herbert@...dor.apana.org.au, krzk+dt@...nel.org, robh+dt@...nel.org
Cc:     devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v4 28/33] dt-bindings: crypto: rockchip: convert to new
 driver bindings

On 01/04/2022 22:17, Corentin Labbe wrote:
> The latest addition to the rockchip crypto driver need to update the
> driver bindings.
> 
> Signed-off-by: Corentin Labbe <clabbe@...libre.com>
> ---
>  .../crypto/rockchip,rk3288-crypto.yaml        | 68 +++++++++++++++++--
>  1 file changed, 63 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> index 66db671118c3..e6c00bc8bebf 100644
> --- a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> @@ -11,8 +11,18 @@ maintainers:
>  
>  properties:
>    compatible:
> -    enum:
> -      - rockchip,rk3288-crypto
> +    oneOf:
> +      - description: crypto IP present on RK3288 SoCs
> +        items:
> +          - const: rockchip,rk3288-crypto
> +      - description: crypto IP present on RK3328 SoCs

These two comments are not helping, so this should be just enum.

> +        items:
> +          - const: rockchip,rk3328-crypto
> +      - description: crypto IPs present on RK3399. crypto0 is the first IP with
> +                     RSA support, crypto1 is the second IP without RSA.

The second part of this comment is helpful, first not. You have chosen
enum in your first patch, so just extend it with comments. Additionally
indexing does not scale. What if next generation reverses it and crypto0
does not have RSA and crypto1 has?

Something like:

properties:
  compatible:
    enum:
      - rockchip,rk3288-crypto
      - rockchip,rk3328-crypto
        # With RSA
      - rockchip,rk3399-crypto-rsa
        # Without RSA
      - rockchip,rk3399-crypto-norsa

> +        enum:
> +          - rockchip,rk3399-crypto0
> +          - rockchip,rk3399-crypto1
>  
>    reg:
>      maxItems: 1
> @@ -21,16 +31,65 @@ properties:
>      maxItems: 1
>  
>    clocks:
> +    minItems: 3
>      maxItems: 4
>  
>    clock-names:
> +    minItems: 3
>      maxItems: 4
>  
>    resets:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 3
>  
>    reset-names:
> -    maxItems: 1
> +    deprecated: true

Why reset-names are being deprecated? Did we talk about this?

> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3288-crypto
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: "aclk"
> +            - const: "hclk"
> +            - const: "sclk"
> +            - const: "apb_pclk"
> +          minItems: 4

minItems for clocks
max for resets and reset-names

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: rockchip,rk3328-crypto
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: "hclk_master"
> +            - const: "hclk_slave"
> +            - const: "sclk"
> +          maxItems: 3

min/max for clocks
max for resets and reset-names

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - rockchip,rk3399-crypto0
> +              - rockchip,rk3399-crypto1
> +    then:
> +      properties:
> +        clock-names:
> +          items:
> +            - const: "hclk_master"
> +            - const: "hclk_slave"
> +            - const: "sclk"
> +          maxItems: 3
> +        resets:
> +          minItems: 3

Similarly.


Best regards,
Krzysztof