Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1596012277-8448-1-git-send-email-weiyi.lu@mediatek.com> <1596012277-8448-2-git-send-email-weiyi.lu@mediatek.com>
In-Reply-To: <1596012277-8448-2-git-send-email-weiyi.lu@mediatek.com>
From:   Enric Balletbo Serra <eballetbo@gmail.com>
Date:   Wed, 29 Jul 2020 11:53:22 +0200
Message-ID: <CAFqH_50+_Bt97roTACRhoq8XkfSc1UcHyQP3OCacQR5oK-viOg@mail.gmail.com>
Subject: Re: [PATCH v2 1/5] dt-bindings: ARM: Mediatek: Document bindings for MT8192
To:     Weiyi Lu <weiyi.lu@mediatek.com>
Cc:     Matthias Brugger <matthias.bgg@gmail.com>,
        Rob Herring <robh@kernel.org>, Stephen Boyd <sboyd@kernel.org>,
        Nicolas Boichat <drinkcat@chromium.org>,
        James Liao <jamesjj.liao@mediatek.com>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>, linux-clk@vger.kernel.org,
        srv_heupstream <srv_heupstream@mediatek.com>,
        Wendell Lin <wendell.lin@mediatek.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Weiyu,

Thanks for the patch, some comments below. I am not sure what
maintainers think but your patches, in general, are really big and I'm
wondering if wouldn't be better split by functionalities. Will make
your series much longer but easy to review in my opinion. Anyway, I'm
going to comment a few files but the comments can be applied to other
files.

Missatge de Weiyi Lu <weiyi.lu@mediatek.com> del dia dc., 29 de jul.
2020 a les 10:46:
>
> This patch adds the binding documentation for apmixedsys, audsys,
> camsys-raw, camsys, imgsys, imp_iic_wrap, infracfg, ipesys, mdpsys,
> mfgcfg, mmsys, msdc, pericfg, scp-adsp, topckgen, vdecsys-soc,
> vdecsys and vencsys for Mediatek MT8192.
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
>  .../bindings/arm/mediatek/mediatek,apmixedsys.txt  |  1 +
>  .../bindings/arm/mediatek/mediatek,audsys.txt      |  1 +
>  .../bindings/arm/mediatek/mediatek,camsys-raw.yaml | 40 ++++++++++++++++++++
>  .../bindings/arm/mediatek/mediatek,camsys.txt      |  1 +
>  .../bindings/arm/mediatek/mediatek,imgsys.txt      |  2 +
>  .../arm/mediatek/mediatek,imp_iic_wrap.yaml        | 43 ++++++++++++++++++++++
>  .../bindings/arm/mediatek/mediatek,infracfg.txt    |  1 +
>  .../bindings/arm/mediatek/mediatek,ipesys.txt      |  1 +
>  .../bindings/arm/mediatek/mediatek,mdpsys.yaml     | 38 +++++++++++++++++++
>  .../bindings/arm/mediatek/mediatek,mfgcfg.txt      |  1 +
>  .../bindings/arm/mediatek/mediatek,mmsys.txt       |  1 +
>  .../bindings/arm/mediatek/mediatek,msdc.yaml       | 39 ++++++++++++++++++++
>  .../bindings/arm/mediatek/mediatek,pericfg.yaml    |  1 +
>  .../bindings/arm/mediatek/mediatek,scp-adsp.yaml   | 38 +++++++++++++++++++
>  .../bindings/arm/mediatek/mediatek,topckgen.txt    |  1 +
>  .../arm/mediatek/mediatek,vdecsys-soc.yaml         | 38 +++++++++++++++++++
>  .../bindings/arm/mediatek/mediatek,vdecsys.txt     |  1 +
>  .../bindings/arm/mediatek/mediatek,vencsys.txt     |  1 +
>  18 files changed, 249 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys-raw.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,imp_iic_wrap.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,mdpsys.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,msdc.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,scp-adsp.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,vdecsys-soc.yaml
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> index bd7a0fa..6942ad4 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,apmixedsys.txt
> @@ -17,6 +17,7 @@ Required Properties:
>         - "mediatek,mt8135-apmixedsys"
>         - "mediatek,mt8173-apmixedsys"
>         - "mediatek,mt8183-apmixedsys", "syscon"
> +       - "mediatek,mt8192-apmixedsys", "syscon"
>         - "mediatek,mt8516-apmixedsys"
>  - #clock-cells: Must be 1
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt b/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt
> index 38309db..fdcb267 100644
> --- a/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,audsys.txt
> @@ -12,6 +12,7 @@ Required Properties:
>         - "mediatek,mt7622-audsys", "syscon"
>         - "mediatek,mt7623-audsys", "mediatek,mt2701-audsys", "syscon"
>         - "mediatek,mt8183-audiosys", "syscon"
> +       - "mediatek,mt8192-audsys", "syscon"
>         - "mediatek,mt8516-audsys", "syscon"
>  - #clock-cells: Must be 1
>
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys-raw.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys-raw.yaml
> new file mode 100644
> index 0000000..db6f425
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,camsys-raw.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,camsys-raw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek CAMSYS RAW Controller
> +
> +maintainers:
> +  - Weiyi Lu <weiyi.lu@mediatek.com>
> +
> +description:
> +  The Mediatek camsys raw controller provides various clocks to the system.
> +

It only provides clocks or also provides configuration registers
non-clock related?

> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - mediatek,mt8192-camsys_rawa
> +          - mediatek,mt8192-camsys_rawb
> +          - mediatek,mt8192-camsys_rawc
> +      - const: syscon
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    camsys_rawa: camsys_rawa@1a04f000 {

I think that this should be  "syscon@1a04f000", since node names are
supposed to match the class of the device instead of the name of the
device.

Just because I am curious, can you show me an example of
"mediatek,mt8192-camsys_rawb" or "mediatek,mt8192-camsys_rawc"? It's a
different address space?

> +        compatible = "mediatek,mt8192-camsys_rawa", "syscon";
> +        reg = <0 0x1a04f000 0 0x1000>;
> +        #clock-cells = <1>;
> +    };

[snip]