Date:   Thu, 9 Feb 2023 12:01:25 -0600
From:   Rob Herring <robh@kernel.org>
To:     Moudy Ho <moudy.ho@mediatek.com>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org,
        Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH v4 03/16] dt-binding: mediatek: add MediaTek mt8195 MDP3
 components
Message-ID: <20230209180125.GA557423-robh@kernel.org>
References: <20230208092209.19472-1-moudy.ho@mediatek.com>
 <20230208092209.19472-4-moudy.ho@mediatek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230208092209.19472-4-moudy.ho@mediatek.com>
Precedence: bulk

On Wed, Feb 08, 2023 at 05:21:56PM +0800, Moudy Ho wrote:
> Adds support for MT8195 MDP3 RDMA, and introduce more MDP3 components
> present in MT8195.

I'm sure I asked this before, but how are these blocks different from 
what we already have in bindings/display/mediatek/. It's all the same 
block names. If they are the same/similar h/w, then it should be 1 
binding even if you have different consumers in the kernel.

If my questions aren't answered in the patches, then I'll just keep 
asking because I won't remember...

> 
> Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> ---
>  .../bindings/media/mediatek,mdp3-rdma.yaml    |  30 +--
>  .../bindings/media/mediatek,mdp3-rsz.yaml     |   5 +-
>  .../bindings/media/mediatek,mt8195-mdp3.yaml  | 174 ++++++++++++++++++
>  3 files changed, 197 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> index 46730687c662..abc3284b21d0 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-rdma.yaml
> @@ -20,8 +20,9 @@ description: |
>  
>  properties:
>    compatible:
> -    items:
> -      - const: mediatek,mt8183-mdp3-rdma
> +    enum:
> +      - mediatek,mt8183-mdp3-rdma
> +      - mediatek,mt8195-mdp3-rdma
>  
>    reg:
>      maxItems: 1
> @@ -46,20 +47,28 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32-array
>  
>    power-domains:
> -    maxItems: 1
> +    oneOf:
> +      - items:
> +          - description: for RDMA
> +      - items:
> +          - description: for vppsys 0
> +          - description: for vppsys 1
>  
>    clocks:
> -    items:
> -      - description: RDMA clock
> -      - description: RSZ clock
> +    minItems: 2
> +    maxItems: 19
>  
>    iommus:
> -    maxItems: 1
> +    oneOf:
> +      - items:
> +          - description: RDMA port
> +      - items:
> +          - description: RDMA port
> +          - description: RDMA to WROT DL port
>  
>    mboxes:
> -    items:
> -      - description: used for 1st data pipe from RDMA
> -      - description: used for 2nd data pipe from RDMA
> +    minItems: 1
> +    maxItems: 5
>  
>    '#dma-cells':
>      const: 1
> @@ -72,7 +81,6 @@ required:
>    - power-domains
>    - clocks
>    - iommus
> -  - mboxes
>    - '#dma-cells'
>  
>  additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
> index 78f9de6192ef..4bc5ac112d2a 100644
> --- a/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-rsz.yaml
> @@ -43,12 +43,15 @@ properties:
>  
>    clocks:
>      minItems: 1
> +    maxItems: 2
> +
> +  power-domains:
> +    maxItems: 1
>  
>  required:
>    - compatible
>    - reg
>    - mediatek,gce-client-reg
> -  - mediatek,gce-events
>    - clocks
>  
>  additionalProperties: false
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml
> new file mode 100644
> index 000000000000..d2b01456c495
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mt8195-mdp3.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/mediatek,mt8195-mdp3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Media Data Path 3 display components
> +
> +maintainers:
> +  - Matthias Brugger <matthias.bgg@gmail.com>
> +  - Moudy Ho <moudy.ho@mediatek.com>
> +
> +description:
> +  A group of display pipeline components for image quality adjustment,
> +  color format conversion and data flow control, and the abbreviations
> +  are explained below.
> +  AAL    - Ambient-light Adaptive Luma.
> +  Color  - Enhance or reduce color in Y/S/H channel.
> +  FG     - Fime Grain for AV1 spec.
> +  HDR    - Perform HDR to SDR.
> +  MERGE  - Used to merge two slice-per-line into one side-by-side.
> +  OVL    - Perform alpha blending.
> +  PAD    - Predefined alpha or color value insertion.
> +  SPLIT  - Split a HDMI stream into two ouptut.
> +  STITCH - Combine multiple video frame with overlapping fields of view.
> +  TCC    - HDR gamma curve conversion support.
> +  TDSHP  - Sharpness and contrast improvement.

Each block likely needs its own schema.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt8195-mdp3-aal
> +      - mediatek,mt8195-mdp3-color
> +      - mediatek,mt8195-mdp3-fg
> +      - mediatek,mt8195-mdp3-hdr
> +      - mediatek,mt8195-mdp3-merge
> +      - mediatek,mt8195-mdp3-ovl
> +      - mediatek,mt8195-mdp3-pad
> +      - mediatek,mt8195-mdp3-split
> +      - mediatek,mt8195-mdp3-stitch
> +      - mediatek,mt8195-mdp3-tcc
> +      - mediatek,mt8195-mdp3-tdshp
> +
> +  reg:
> +    maxItems: 1
> +
> +  mediatek,gce-client-reg:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      items:
> +        - description: phandle of GCE
> +        - description: GCE subsys id
> +        - description: register offset
> +        - description: register size

Given these match up to reg values, I'm really wondering why we have 
this.

> +    description:
> +      Each GCE subsys id is mapping to a base address of display function blocks
> +      register which is defined in <include/dt-bindings/gce/mt8195-gce.h>.
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 7

You have to define what each clock is which probably depends on each 
block.

Rob