Re: [PATCH v7 02/16] dt-bindings: media: mediatek: mdp3: merge the indentical RDMA under display

[AngeloGioacchino Del Regno]

Il 18/10/23 05:06, Moudy Ho (何宗原) ha scritto:
> On Fri, 2023-10-13 at 08:46 +0200, Krzysztof Kozlowski wrote:
> >   	
>
> Hi Krzysztof,
>
> Thank you for assisting with the review.
>
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >   On 12/10/2023 10:40, Moudy Ho wrote:
> >
> > >   
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: mediatek,mt8183-mdp3-rdma
> > > +
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          items:
> > > +            - description: RDMA clock
> > > +            - description: RSZ clock (shared SRAM with RDMA)
> > > +
> > > +        mboxes:
> > > +          items:
> > > +            - description: used for 1st data pipe from RDMA
> > > +            - description: used for 2nd data pipe from RDMA
> >
> > interrupts:
> >    false
>
> As Angelo provided additional clarification in [15/16], explaining that
> certain conditions in [2/16] and [3/16] were intentionally omitted due
> to the need to integrate the same IP with different operations.
> Apologies for any inconvenience this has caused you.

MT8183's MDP3 RDMA interrupt property was omitted in the devicetree that we
have upstream because it was either unused in the driver, or MTK didn't want
to actually use it for reasons, but that SoC *definitely does* have a mdp_rdma0
IRQ and a mdp_rdma1 IRQ.

That's the same for MT8186 and MT8188... and it's probably the same for all
MediaTek SoCs, so interrupts shouldn't be disallowed in this binding.

> > > +
> > > +      required:
> > > +        - mboxes
> > > +        - mediatek,gce-events
> > > +
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: mediatek,mt8195-vdo1-rdma
> > > +
> > > +    then:
> > > +      properties:
> > > +        clocks:
> > > +          items:
> > > +            - description: RDMA clock
> >
> > mboxes: false
> > mediatek,gce-events: false
> >
> > I am not so sure it is actually "simpler" to merge these. They are
> > quite
> > different. You will end up with unmanageable allOf  with a lot of
> > branches (which supposedly you want to remove).

It's the same thing as "split"... All of the display and mdp/mdp3 components of
MediaTek SoC do support GCE mailboxes by HW, so it's not limited to "split", but
literally all of them.

Disallowing mboxes and/or mediatek,gce-events on *any* of those is actually wrong.

Cheers,
Angelo

> >
>
> Upon examining the minor hardware changes in MDP for MT8183 and MT8195
> RDMA ([3/16]), it appears that branching cannot be avoided. However,
> consolidating these changes has the additional advantage of addressing
> Rob's concerns from v4. Perhaps we can consider the current changes as
> a form of progress.
>
> Sincerely,
> Moudy
>
> > > +
> > >   additionalProperties: false
> > >   
> > >   examples:
> >
> > Best regards,
> > Krzysztof