Re: [PATCH 1/2] dt-bindings: remoteproc: Add bindings for new Ingenic SoCs.

[Paul Cercueil]

Hi Zhou,

Le sam., juil. 24 2021 at 17:11:37 +0800, 周琰杰 (Zhou Yanjie) 
<zhouyanjie@xxxxxxxxxxxxxx> a écrit :
> Add the remoteproc bindings for the JZ4760 SoC, the JZ4760B SoC,
> the JZ4775 SoC, and the JZ4780 SoC from Ingenic.
>
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> ---
>  .../bindings/remoteproc/ingenic,vpu.yaml           | 74 
> ++++++++++++++++------
>  1 file changed, 56 insertions(+), 18 deletions(-)
>
> diff --git 
> a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml 
> b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> index d0aa91b..6154596 100644
> --- a/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/ingenic,vpu.yaml
> @@ -17,31 +17,52 @@ maintainers:
>
>  properties:
>    compatible:
> -    const: ingenic,jz4770-vpu-rproc
> +    enum:
> +      - ingenic,jz4760-vpu-rproc
> +      - ingenic,jz4760b-vpu-rproc
> +      - ingenic,jz4770-vpu-rproc
> +      - ingenic,jz4775-vpu-rproc
> +      - ingenic,jz4780-vpu-rproc
>
>    reg:
> -    items:
> -      - description: aux registers
> -      - description: tcsm0 registers
> -      - description: tcsm1 registers
> -      - description: sram registers
> +    oneOf:
> +      - items:
> +          - description: aux registers
> +          - description: tcsm0 registers
> +          - description: tcsm1 registers
> +          - description: sram registers
> +      - items:
> +          - description: aux registers
> +          - description: tcsm registers
> +          - description: sram registers

Since we have "reg-names" already, we don't really need any 
description, so you could just have:

reg:
 minItems: 3
 maxItems: 4

>    reg-names:
> -    items:
> -      - const: aux
> -      - const: tcsm0
> -      - const: tcsm1
> -      - const: sram
> +    oneOf:
> +      - items:
> +          - const: aux
> +          - const: tcsm0
> +          - const: tcsm1
> +          - const: sram
> +      - items:
> +          - const: aux
> +          - const: tcsm
> +          - const: sram

You could just add "tcsm" to the items list, and add:
minItems: 3
maxItems: 4

>    clocks:
> -    items:
> -      - description: aux clock
> -      - description: vpu clock
> +    oneOf:
> +      - items:
> +          - description: aux clock
> +          - description: vpu clock
> +      - items:
> +          - description: vpu clock

Same as above, since we already have clock-names, the descriptions 
don't bring much.

You can replace with:

clocks:
 minItems: 1
 maxItems: 2

>    clock-names:
> -    items:
> -      - const: aux
> -      - const: vpu
> +    oneOf:
> +      - items:
> +          - const: aux
> +          - const: vpu
> +      - items:
> +          - const: vpu

I think you could just add:
minItems: 1

Cheers,
-Paul

>    interrupts:
>      maxItems: 1
> @@ -60,7 +81,7 @@ examples:
>    - |
>      #include <dt-bindings/clock/jz4770-cgu.h>
>
> -    vpu: video-decoder@132a0000 {
> +    video-decoder@132a0000 {
>        compatible = "ingenic,jz4770-vpu-rproc";
>
>        reg = <0x132a0000 0x20>, /* AUX */
> @@ -75,3 +96,20 @@ examples:
>        interrupt-parent = <&cpuintc>;
>        interrupts = <3>;
>      };
> +  - |
> +    #include <dt-bindings/clock/jz4780-cgu.h>
> +
> +    video-decoder@132a0000 {
> +      compatible = "ingenic,jz4780-vpu-rproc";
> +
> +      reg = <0x132a0000 0x20>, /* AUX */
> +            <0x132c0000 0x8000>, /* TCSM */
> +            <0x132f0000 0x4000>; /* SRAM */
> +      reg-names = "aux", "tcsm", "sram";
> +
> +      clocks = <&cgu JZ4780_CLK_VPU>;
> +      clock-names = "vpu";
> +
> +      interrupt-parent = <&intc>;
> +      interrupts = <62>;
> +    };
> --
> 2.7.4