Re: [PATCH v2 01/12] dt-binding: mediatek: add bindings for MediaTek 8195 MDP3 components

["Moudy Ho (何宗原)" <Moudy.Ho@mediatek.com>]

Hi Rob,

Thanks for helping to review this series.

On Fri, 2022-12-09 at 15:06 -0600, Rob Herring wrote:
> On Wed, Dec 07, 2022 at 04:44:01PM +0800, Moudy Ho wrote:
> > This patch adds support for MT8195 MDP3 RDMA, and introduce more
> > MDP3 components present in MT8195.
> > 
> > Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> > ---
> >  .../bindings/media/mediatek,mdp3-aal.yaml     | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-color.yaml   | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-fg.yaml      | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-hdr.yaml     | 63 +++++++++++++++
> >  .../bindings/media/mediatek,mdp3-merge.yaml   | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-ovl.yaml     | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-pad.yaml     | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-rdma.yaml    |  5 +-
> >  .../bindings/media/mediatek,mdp3-split.yaml   | 76
> > +++++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-stitch.yaml  | 64
> > ++++++++++++++++
> >  .../bindings/media/mediatek,mdp3-tcc.yaml     | 63 +++++++++++++++
> >  .../bindings/media/mediatek,mdp3-tdshp.yaml   | 64
> > ++++++++++++++++
> >  12 files changed, 717 insertions(+), 2 deletions(-)
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,mdp3-aal.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,mdp3-color.yaml
> >  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-merge.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,mdp3-ovl.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,mdp3-pad.yaml
> >  create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,mdp3-split.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/media/mediatek,mdp3-
> > aal.yaml b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > aal.yaml
> > new file mode 100644
> > index 000000000000..5b59e0802357
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,mdp3-
> > aal.yaml
> > @@ -0,0 +1,64 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/media/mediatek,mdp3-aal.yaml*__;Iw!!CTRNKA9wMg0ARbw!gDIFKWscMU7roNTbdA1-AyzzrxCwA7ErPkkF-OaHo9UU8Y53_SGcVhOwp6dgB9hRhLzmmol53Sqlqg$ 
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!gDIFKWscMU7roNTbdA1-AyzzrxCwA7ErPkkF-OaHo9UU8Y53_SGcVhOwp6dgB9hRhLzmmolTzbOS7Q$ 
> >  
> > +
> > +title: MediaTek Media Data Path 3 AAL Device Tree Bindings
> 
> Drop ' Device Tree Bindings' from titles.
> 
> > +
> > +maintainers:
> > +  - Matthias Brugger <matthias.bgg@gmail.com>
> > +  - Moudy Ho <moudy.ho@mediatek.com>
> > +
> > +description: |
> 
> Don't need '|'
> 
> > +  One of Media Data Path 3 (MDP3) components is responsible for
> > backlight
> > +  power saving and sunlight visibility improving.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - mediatek,mt8195-mdp3-aal
> > +
> > +  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
> > +    description: The register of client driver can be configured
> > by gce with
> > +      4 arguments defined in this property. Each GCE subsys id is
> > mapping to
> > +      a client defined in the header include/dt-
> > bindings/gce/<chip>-gce.h.
> 
> How many 4 cell entries? This allows any number. If 1, then you need 
> 'maxItems: 1' with the outer 'items'.
> 

Sorry for the confusion here, since the GCE clients are all unique,
I'll add a "maxItems: 1" in this property.

> > +
> > +  clocks:
> > +    minItems: 1
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - mediatek,gce-client-reg
> > +  - clocks
> > +  - power-domains
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/mt8195-clk.h>
> > +    #include <dt-bindings/gce/mt8195-gce.h>
> > +    #include <dt-bindings/power/mt8195-power.h>
> > +
> > +    mdp3-aal0@14005000 {
> > +      compatible = "mediatek,mt8195-mdp3-aal";
> > +      reg = <0x14005000 0x1000>;
> > +      mediatek,gce-client-reg = <&gce1 SUBSYS_1400XXXX 0x5000
> > 0x1000>;
> > +      clocks = <&vppsys0 CLK_VPP0_MDP_AAL>;
> > +      power-domains = <&spm MT8195_POWER_DOMAIN_VPPSYS0>;
> 
> Indent by 4 spaces here.
> 

Apologies for the indentation mismatch, it will be corrected in next
version.

> > +    };
> 
> Same comments for the rest...
> 

Sincerely,
Moudy