From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Moudy Ho <moudy.ho@mediatek.com>,
Chun-Kuang Hu <chunkuang.hu@kernel.org>,
Philipp Zabel <p.zabel@pengutronix.de>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc: AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
dri-devel@lists.freedesktop.org,
linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 3/3] dt-binding: mediatek: add MediaTek mt8195 MDP3 components
Date: Tue, 12 Sep 2023 10:19:54 +0200 [thread overview]
Message-ID: <e4119fa6-a4b7-f59e-7115-044fa83c9063@linaro.org> (raw)
In-Reply-To: <20230912075651.10693-4-moudy.ho@mediatek.com>
On 12/09/2023 09:56, Moudy Ho wrote:
> Introduce more MDP3 components present in MT8195.
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
>
> Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> ---
> .../display/mediatek/mediatek,aal.yaml | 2 +-
> .../display/mediatek/mediatek,color.yaml | 2 +-
> .../display/mediatek/mediatek,merge.yaml | 1 +
> .../display/mediatek/mediatek,ovl.yaml | 2 +-
> .../display/mediatek/mediatek,split.yaml | 1 +
> .../bindings/media/mediatek,mdp3-fg.yaml | 61 +++++++++++++++++++
> .../bindings/media/mediatek,mdp3-hdr.yaml | 60 ++++++++++++++++++
> .../bindings/media/mediatek,mdp3-pad.yaml | 61 +++++++++++++++++++
> .../bindings/media/mediatek,mdp3-rdma.yaml | 16 ++---
> .../bindings/media/mediatek,mdp3-stitch.yaml | 61 +++++++++++++++++++
> .../bindings/media/mediatek,mdp3-tcc.yaml | 60 ++++++++++++++++++
> .../bindings/media/mediatek,mdp3-tdshp.yaml | 61 +++++++++++++++++++
> 12 files changed, 378 insertions(+), 10 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-fg.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-hdr.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-pad.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-stitch.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-tcc.yaml
> create mode 100644 Documentation/devicetree/bindings/media/mediatek,mdp3-tdshp.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> index 7fd42c8fdc32..04b1314d00f2 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> @@ -24,6 +24,7 @@ properties:
> - enum:
> - mediatek,mt8173-disp-aal
> - mediatek,mt8183-disp-aal
> + - mediatek,mt8195-mdp3-aal
> - items:
> - enum:
> - mediatek,mt2712-disp-aal
> @@ -63,7 +64,6 @@ properties:
> required:
> - compatible
> - reg
> - - interrupts
Why? commit msg tells nothing about it. Why interrupt is not erquired in
mt8173? How dropping such requirement is anyhow related to mt8195?
> - power-domains
> - clocks
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> index f21e44092043..8e97b0a6a7b3 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,color.yaml
> @@ -26,6 +26,7 @@ properties:
> - mediatek,mt2701-disp-color
> - mediatek,mt8167-disp-color
> - mediatek,mt8173-disp-color
> + - mediatek,mt8195-mdp3-color
> - items:
> - enum:
> - mediatek,mt7623-disp-color
> @@ -66,7 +67,6 @@ properties:
> required:
> - compatible
> - reg
> - - interrupts
Why?
> - power-domains
> - clocks
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
> index eead5cb8636e..401498523404 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,merge.yaml
> @@ -24,6 +24,7 @@ properties:
> - enum:
> - mediatek,mt8173-disp-merge
> - mediatek,mt8195-disp-merge
> + - mediatek,mt8195-mdp3-merge
> - items:
> - const: mediatek,mt6795-disp-merge
> - const: mediatek,mt8173-disp-merge
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
> index 3e1069b00b56..10d4d4f64e09 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
> @@ -26,6 +26,7 @@ properties:
> - mediatek,mt8173-disp-ovl
> - mediatek,mt8183-disp-ovl
> - mediatek,mt8192-disp-ovl
> + - mediatek,mt8195-mdp3-ovl
> - items:
> - enum:
> - mediatek,mt7623-disp-ovl
> @@ -76,7 +77,6 @@ properties:
> required:
> - compatible
> - reg
> - - interrupts
Why?
> - power-domains
> - clocks
> - iommus
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,split.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,split.yaml
> index a8a5c9608598..a96b271e3240 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,split.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,split.yaml
> @@ -23,6 +23,7 @@ properties:
> oneOf:
> - enum:
> - mediatek,mt8173-disp-split
> + - mediatek,mt8195-mdp3-split
> - items:
> - const: mediatek,mt6795-disp-split
> - const: mediatek,mt8173-disp-split
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-fg.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-fg.yaml
> new file mode 100644
> index 000000000000..71fd449de8b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-fg.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mdp3-fg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Media Data Path 3 FG
> +
> +maintainers:
> + - Matthias Brugger <matthias.bgg@gmail.com>
> + - Moudy Ho <moudy.ho@mediatek.com>
> +
> +description:
> + One of Media Data Path 3 (MDP3) components used to add film grain
> + according to AV1 spec.
> +
> +properties:
> + compatible:
> + enum:
> + - mediatek,mt8195-mdp3-fg
> +
> + reg:
> + maxItems: 1
> +
> + mediatek,gce-client-reg:
> + description:
> + The register of display function block to be set by gce. There are 4 arguments,
> + such as gce node, subsys id, offset and register size. The subsys id that is
> + mapping to the register of display function blocks is defined in the gce header
> + include/dt-bindings/gce/<chip>-gce.h of each chips.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + items:
> + - description: phandle of GCE
> + - description: GCE subsys id
> + - description: register offset
> + - description: register size
> + maxItems: 1
> +
> + clocks:
> + minItems: 1
This must be maxItems. Use existing code as an example, do not re-invent it.
> +
> +required:
> + - compatible
> + - reg
> + - mediatek,gce-client-reg
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/mt8195-clk.h>
> + #include <dt-bindings/gce/mt8195-gce.h>
> +
> + display@14002000 {
> + compatible = "mediatek,mt8195-mdp3-fg";
> + reg = <0x14002000 0x1000>;
> + mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0x2000 0x1000>;
> + clocks = <&vppsys0 CLK_VPP0_MDP_FG>;
> + };
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-hdr.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-hdr.yaml
> new file mode 100644
> index 000000000000..fb1bb5a9e57f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-hdr.yaml
> @@ -0,0 +1,60 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mdp3-hdr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Media Data Path 3 HDR
> +
> +maintainers:
> + - Matthias Brugger <matthias.bgg@gmail.com>
> + - Moudy Ho <moudy.ho@mediatek.com>
> +
> +description:
> + One of Media Data Path 3 (MDP3) components used to perform HDR to SDR
> +
> +properties:
> + compatible:
> + enum:
> + - mediatek,mt8195-mdp3-hdr
> +
> + reg:
> + maxItems: 1
> +
> + mediatek,gce-client-reg:
> + description:
> + The register of display function block to be set by gce. There are 4 arguments,
> + such as gce node, subsys id, offset and register size. The subsys id that is
> + mapping to the register of display function blocks is defined in the gce header
> + include/dt-bindings/gce/<chip>-gce.h of each chips.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + items:
> + - description: phandle of GCE
> + - description: GCE subsys id
> + - description: register offset
> + - description: register size
> + maxItems: 1
> +
> + clocks:
> + minItems: 1
Here as well. Why is this minitems?
> +
> +required:
> + - compatible
> + - reg
> + - mediatek,gce-client-reg
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/mt8195-clk.h>
> + #include <dt-bindings/gce/mt8195-gce.h>
> +
> + display@14004000 {
> + compatible = "mediatek,mt8195-mdp3-hdr";
> + reg = <0x14004000 0x1000>;
> + mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0x4000 0x1000>;
> + clocks = <&vppsys0 CLK_VPP0_MDP_HDR>;
> + };
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-pad.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-pad.yaml
> new file mode 100644
> index 000000000000..13b66c5985fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-pad.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mdp3-pad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Media Data Path 3 PADDING
> +
> +maintainers:
> + - Matthias Brugger <matthias.bgg@gmail.com>
> + - Moudy Ho <moudy.ho@mediatek.com>
> +
> +description:
> + One of Media Data Path 3 (MDP3) components used to insert
> + pre-defined color or alpha value to arbitrary side of image.
> +
> +properties:
> + compatible:
> + enum:
> + - mediatek,mt8195-mdp3-pad
And you cannot add it to existing padding because?
> +
> + reg:
> + maxItems: 1
> +
> + mediatek,gce-client-reg:
> + description:
> + The register of display function block to be set by gce. There are 4 arguments,
> + such as gce node, subsys id, offset and register size. The subsys id that is
> + mapping to the register of display function blocks is defined in the gce header
> + include/dt-bindings/gce/<chip>-gce.h of each chips.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + items:
> + - description: phandle of GCE
> + - description: GCE subsys id
> + - description: register offset
> + - description: register size
> + maxItems: 1
> +
> + clocks:
> + minItems: 1
Nope
> +
> +required:
> + - compatible
> + - reg
> + - mediatek,gce-client-reg
> + - clocks
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/mt8195-clk.h>
> + #include <dt-bindings/gce/mt8195-gce.h>
> +
> + display@1400a000 {
> + compatible = "mediatek,mt8195-mdp3-pad";
> + reg = <0x1400a000 0x1000>;
> + mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0xa000 0x1000>;
> + clocks = <&vppsys0 CLK_VPP0_PADDING>;
> + };
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> index 0c22571d8c22..17cd5b587e23 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> @@ -23,6 +23,7 @@ properties:
> enum:
> - mediatek,mt8183-mdp3-rdma
> - mediatek,mt8195-vdo1-rdma
> + - mediatek,mt8195-mdp3-rdma
m is before v
>
> reg:
> maxItems: 1
> @@ -50,17 +51,19 @@ properties:
> maxItems: 1
>
> clocks:
> - items:
> - - description: RDMA clock
> - - description: RSZ clock
> + oneOf:
> + - items:
> + - description: RDMA clock
> + - description: SRAM shared component clock
> + - items:
> + - description: RDMA clock
Why now mt8183 can have SRAM clock optional? How changing mt8183 is
related to this patch?
I'll finish the review, sorry fix basics here.
Best regards,
Krzysztof
next prev parent reply other threads:[~2023-09-12 8:20 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-12 7:56 [PATCH v5 0/3] introduce more MDP3 components in mt8195 Moudy Ho
2023-09-12 7:56 ` [PATCH v5 1/3] dt-binding: mediatek: correct MDP3 node with generic names Moudy Ho
2023-09-12 7:56 ` [PATCH v5 2/3] dt-binding: mediatek: integrate MDP RDMA to one binding Moudy Ho
2023-09-12 8:16 ` Krzysztof Kozlowski
2023-09-13 3:04 ` Moudy Ho (何宗原)
2023-09-13 6:37 ` Krzysztof Kozlowski
2023-09-12 7:56 ` [PATCH v5 3/3] dt-binding: mediatek: add MediaTek mt8195 MDP3 components Moudy Ho
2023-09-12 8:19 ` Krzysztof Kozlowski [this message]
2023-09-13 8:39 ` Moudy Ho (何宗原)
2023-09-13 8:58 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e4119fa6-a4b7-f59e-7115-044fa83c9063@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=airlied@gmail.com \
--cc=angelogioacchino.delregno@collabora.com \
--cc=chunkuang.hu@kernel.org \
--cc=conor+dt@kernel.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=hverkuil-cisco@xs4all.nl \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=moudy.ho@mediatek.com \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).