Re: [PATCH v6 5/7] dt-bindings: MIPS: Document Ingenic SoCs binding.

[Zhou Yanjie <zhouyanjie@xxxxxxxxxxxxxx>]

Hi Rob,

On 2020年02月27日 00:29, Rob Herring wrote:
> On Fri, Feb 21, 2020 at 12:24:47AM +0800, 周琰杰 (Zhou Yanjie) wrote:
> > Document the available properties for the SoC root node and the
> > CPU nodes of the devicetree for the Ingenic XBurst SoCs.
> >
> > Tested-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
> > Tested-by: Paul Boddie <paul@xxxxxxxxxxxxx>
> > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> > ---
> >
> > Notes:
> >      v1->v2:
> >      Change the two Document from txt to yaml.
> >      
> >      v2->v3:
> >      Fix formatting errors.
> >      
> >      v3->v4:
> >      Fix bugs in the two yaml files.
> >      
> >      v4->v5:
> >      No change.
> >      
> >      v5->v6:
> >      Rewrite the two yaml files.
> >
> >   .../bindings/mips/ingenic/ingenic,cpu.yaml         | 61 ++++++++++++++++++++++
> >   .../bindings/mips/ingenic/ingenic,soc.yaml         | 34 ++++++++++++
> >   2 files changed, 95 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> >   create mode 100644 Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml b/Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> > new file mode 100644
> > index 00000000..ad1fd86
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mips/ingenic/ingenic,cpu.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mips/ingenic/ingenic,cpu.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Bindings for Ingenic XBurst family CPUs
> > +
> > +maintainers:
> > +  - 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> Blank line here.
>
> > +description: |
> Drop the '|'.
>
> > +  Ingenic XBurst family CPUs shall have the following properties.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +
> > +      - description: Ingenic XBurst®1 CPU Core
> > +        items:
> > +          - const: ingenic,xburst
> > +
> > +      - description: Ingenic XBurst®2 CPU Core
> > +        items:
> > +          - const: ingenic,xburst2
> enum:
>    - ingenic,xburst  # Ingenic XBurst®1 CPU Core
>    - ingenic,xburst2 # Ingenic XBurst®2 CPU Core
>
> Though I don't find the description really adds much.
>
> > +
> > +  reg:
> > +    description: |
> > +      The number of the CPU.
> Drop this.
>
> Add:
>
> maxItems: 1
>
> > +
> > +required:
> > +  - device_type
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/jz4780-cgu.h>
> > +
> > +    cpus {
> > +    	#address-cells = <1>;
> > +    	#size-cells = <0>;
> > +
> > +    	cpu0: cpu@0 {
> > +    		device_type = "cpu";
> > +    		compatible = "ingenic,xburst";
> > +    		reg = <0>;
> > +
> > +    		clocks = <&cgu JZ4780_CLK_CPU>;
> > +    		clock-names = "cpu";
> Not documented.
>
> > +    	};
> > +
> > +    	cpu1: cpu@1 {
> > +    		device_type = "cpu";
> > +    		compatible = "ingenic,xburst";
> > +    		reg = <1>;
> > +
> > +    		clocks = <&cgu JZ4780_CLK_CORE1>;
> > +    		clock-names = "cpu";
> > +    	};
> > +    };
> > +...
> > diff --git a/Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml b/Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml
> > new file mode 100644
> > index 00000000..8943e73
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mips/ingenic/ingenic,soc.yaml
> > @@ -0,0 +1,34 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mips/ingenic/ingenic,soc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Bindings for Ingenic SoCs with XBurst CPU inside.
> > +
> > +maintainers:
> > +  - 周琰杰 (Zhou Yanjie) <zhouyanjie@xxxxxxxxxxxxxx>
> Blank line.
>
> > +description: |
> > +  Ingenic SoCs with XBurst CPU inside shall have the following properties.
> > +
> > +properties:
> > +  $nodename:
> > +    const: '/'
> > +  compatible:
> > +    oneOf:
> > +
> > +      - description: Ingenic JZ47 Series Mobile Application Processor
> > +        items:
> > +          - const: ingenic,jz4740
> > +          - const: ingenic,jz4725b
> > +          - const: ingenic,jz4760
> > +          - const: ingenic,jz4760b
> > +          - const: ingenic,jz4770
> > +          - const: ingenic,jz4780
> This is defining the root compatible is 6 strings. You want a enum here
> I think.
>
> > +
> > +      - description: Ingenic X Series IoT Application Processor
> > +        items:
> > +          - const: ingenic,x1000
> > +          - const: ingenic,x1000e
> > +          - const: ingenic,x1500
> Same here.
>
> Did you validate your dts file with this schema using 'make dtbs_check'?

I am very sorry, to be honest, because I was not familiar with yaml before,
the previous versions have not been verified, but when v6, I have set up
the environment and checked it with "make dt_binding_check", but I
don't know why the problems you pointed out above were not prompted
during the check. I will fix these problems in the next version.

Thanks and best regards!

> Rob