Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <90ebea0b-1d67-98dc-2b49-a6e3b97a2c4a@kernel.org>
Date:   Tue, 22 Mar 2022 19:04:43 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3 21/26] dt-bindings: crypto: convert rockchip-crypto to
 yaml
Content-Language: en-US
To:     Corentin Labbe <clabbe@baylibre.com>, heiko@sntech.de,
        herbert@gondor.apana.org.au, krzk+dt@kernel.org,
        mturquette@baylibre.com, robh+dt@kernel.org, sboyd@kernel.org
Cc:     devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-clk@vger.kernel.org, linux-crypto@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org
References: <20220321200739.3572792-1-clabbe@baylibre.com>
 <20220321200739.3572792-22-clabbe@baylibre.com>
From:   Krzysztof Kozlowski <krzk@kernel.org>
In-Reply-To: <20220321200739.3572792-22-clabbe@baylibre.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 21/03/2022 21:07, Corentin Labbe wrote:
> Convert rockchip-crypto to yaml
> 
> Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
> ---
>  .../crypto/rockchip,rk3288-crypto.yaml        | 84 +++++++++++++++++++
>  .../bindings/crypto/rockchip-crypto.txt       | 28 -------
>  2 files changed, 84 insertions(+), 28 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
>  delete mode 100644 Documentation/devicetree/bindings/crypto/rockchip-crypto.txt
> 
> diff --git a/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> new file mode 100644
> index 000000000000..a6be89a1c890
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/crypto/rockchip,rk3288-crypto.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/crypto/rockchip,rk3288-crypto.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip Electronics And Security Accelerator
> +
> +maintainers:
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3288-crypto
> +      - rockchip,rk3328-crypto
> +      - rockchip,rk3399-crypto

Waaaait, what? Only rockchip,rk3288-crypto is in original bindings.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 4
> +
> +  clock-names:
> +    minItems: 4
> +
> +  resets:
> +    maxItems: 1

You missed reset-names.

This patch is quite different than previous, in unexpected way. What
happened here?

> +
> +if:

Please define it after "allOf:", so it could be easily extended without
changing indentation.

> +  properties:
> +    compatible:
> +      const: rockchip,rk3399-crypto
> +then:
> +  properties:
> +    reg:
> +      minItems: 2
> +    interrupts:
> +      minItems: 2

List interrupts. This is really different than your v1. It also looks
different than original bindings and you did not mention any differences
here, nor in the commit msg. Either explain in commit msg all
differences (and why) or move them to separate commit.

You seem to change the bindings a lot (new properties, different
constraints, new compatibles), so this should all go to separate commit.
Now it is just confusing.

> +    clocks:
> +      minItems: 6

You need maxItems. Everywhere.

> +    clock-names:
> +      minItems: 6

List all items.

> +    resets:
> +      minItems: 6
> +else:
> +  if:
> +    properties:
> +      compatible:
> +        const: rockchip,rk3328-crypto
> +  then:
> +    properties:
> +      clocks:
> +        minItems: 3
> +      clock-names:
> +        minItems: 3
> +

Best regards,
Krzysztof