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

From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Corentin Labbe <clabbe@baylibre.com>,
	heiko@sntech.de, herbert@gondor.apana.org.au, krzk+dt@kernel.org,
	robh+dt@kernel.org
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH v4 28/33] dt-bindings: crypto: rockchip: convert to new driver bindings
Date: Sat, 2 Apr 2022 13:53:58 +0200	[thread overview]
Message-ID: <30305936-4b69-e1ce-44c2-0d1d113b460e@linaro.org> (raw)
In-Reply-To: <20220401201804.2867154-29-clabbe@baylibre.com>

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@baylibre.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