Date:   Mon, 23 Jan 2023 23:09:57 +0800
Message-ID: <875ycxi8xm.wl-me@linux.beauty>
From:   Li Chen <me@linux.beauty>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     Li Chen <lchen@ambarella.com>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 04/15] dt-bindings: arm: add support for Ambarella SoC
In-Reply-To: <7d191871-1025-43a3-20bf-8fc6b3f92c89@linaro.org>
References: <20230123073305.149940-1-lchen@ambarella.com>
        <20230123073305.149940-5-lchen@ambarella.com>
        <7d191871-1025-43a3-20bf-8fc6b3f92c89@linaro.org>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0
 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Mon, 23 Jan 2023 16:07:32 +0800,
Krzysztof Kozlowski wrote:
>
> On 23/01/2023 08:32, Li Chen wrote:
> > Create a vendor directory for Ambarella, and add
> > cpuid, rct, scratchpad documents.
> >
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: I2c29e45c08666489b0d9b588ac37d713f5b723d1
>
> Please run scripts/checkpatch.pl and fix reported warnings.
>
> Applies to all your patches. Also test them... I have doubts that you
> tested if you actually ignored checkpatch :/

Yeah, I checkpatch all patches, and have planned to fix Change-Id finally(manually),
but forget it before sending mails, my bad, sorry. I will remove it in v2.

> > ---
> >  .../arm/ambarella/ambarella,cpuid.yaml        | 24 +++++++++++++++++++
> >  .../bindings/arm/ambarella/ambarella,rct.yaml | 24 +++++++++++++++++++
> >  .../arm/ambarella/ambarella,scratchpad.yaml   | 24 +++++++++++++++++++
> >  .../bindings/arm/ambarella/ambarella.yaml     | 22 +++++++++++++++++
> >  MAINTAINERS                                   |  4 ++++
> >  5 files changed, 98 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
> > new file mode 100644
> > index 000000000000..1f4d9cec8f92
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>
> This goes to soc

Thanks, I wasn't aware that there is a document dir named soc. I will move cpuid yaml
to bindings/soc/ambarella/, and leave other yaml still here.

> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,cpuid.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella SoC ID
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
>
> Missing description.

Sorry, description will be added in v2. BTW, does other YAMLs in this patch
also need descriptions?

> > +
> > +properties:
> > +  compatible:
> > +    const: "ambarella,cpuid", "syscon"
>
> Drop quotes (applies to all your patches)

OK, thanks!

> Missing SoC specific compatible.
>
> > +
> > +  reg:
> > +    maxItems: 1
>
> Missing additionalProperties. sorry, start from scratch from some
> existing recent bindings or better example-schema.

Good to know that there is example-schema, thanks!
 
> > +
> > +examples:
> > +  - |
> > +    cpuid_syscon: cpuid@e0000000 {
> > +        compatible = "ambarella,cpuid", "syscon";
> > +        reg = <0xe0000000 0x1000>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> > new file mode 100644
> > index 000000000000..7279bab17d9e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,rct.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella RCT module
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: "ambarella,rct", "syscon"
>
> All the same problems.

Well noted.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +examples:
> > +  - |
> > +		rct_syscon: rct_syscon@ed080000 {
>
> Really? Just take a look and you will see wrong indentation. Also drop
> underscores in node names and "rct". Node names should be generic.

Sorry for the wrong indentation, will fix it in v2.

Is it ok to contain underscores in lable? if so, I will change it into

rct_syscon: syscon@ed080000 {

in v2.

>
> > +        compatible = "ambarella,rct", "syscon";
> > +        reg = <0xed080000 0x1000>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> > new file mode 100644
> > index 000000000000..5d2bd243b5c9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,scratchpad.yaml#
>
> That's not a clock controller!

Sorry, will fix it in v2.

> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella Scratchpad
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: "ambarella,scratchpad", "syscon"
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +examples:
> > +  - |
> > +    scratchpad_syscon: scratchpad_syscon@e0022000 {
>
> All the same problems.

Well noted.

> > +        compatible = "ambarella,scratchpad", "syscon";
> > +        reg = <0xe0022000 0x100>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> > new file mode 100644
> > index 000000000000..5991bd745c05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/ambarella.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella SoC Device Tree Bindings
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  $nodename:
> > +    const: "/"
> > +  compatible:
> > +    oneOf:
> > +      - description: Ambarella SoC based platforms
> > +        items:
> > +          - enum:
> > +              - ambarella,s6lm
>
> What is this? How do you expect it to apply? Can you try by yourself?

Sorry, I didn't find this file is duplicited with outside ambarella.yaml.
I will remove it in v2.

Thanks for your review!

Regards,
Li