Re: [PATCH v1 01/14] media: dt-bindings: Convert MediaTek mt8173-mdp bindings to YAML

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/09/2025 19:52, Ariel D'Alessandro wrote:
> Krzysztof,
> 
> On 8/21/25 3:46 AM, Krzysztof Kozlowski wrote:
>> On Wed, Aug 20, 2025 at 02:12:49PM -0300, Ariel D'Alessandro wrote:
>>> Convert the existing text-based DT bindings for MediaTek MT8173 Media Data Path
>>> to a YAML schema.
>>
>> Please wrap commit message according to Linux coding style / submission
>> process (neither too early nor over the limit):
>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 
> Thanks. Looks like my editor was misconfigured, sorry. Will fix in v2.
> 
>>
>>>
>>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>>> ---
>>>   .../bindings/media/mediatek,mt8173-mdp.yaml   | 174 ++++++++++++++++++
>>>   .../bindings/media/mediatek-mdp.txt           |  95 ----------
>>>   2 files changed, 174 insertions(+), 95 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8173-mdp.yaml
>>>   delete mode 100644 Documentation/devicetree/bindings/media/mediatek-mdp.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8173-mdp.yaml b/Documentation/devicetree/bindings/media/mediatek,mt8173-mdp.yaml
>>> new file mode 100644
>>> index 0000000000000..f3a08afc305b1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/mediatek,mt8173-mdp.yaml
>>> @@ -0,0 +1,174 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/mediatek,mt8173-mdp.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MediaTek MT8173 Media Data Path
>>> +
>>> +maintainers:
>>> +  - Ariel D'Alessandro <ariel.dalessandro@collabora.com>
>>> +
>>> +description:
>>> +  Media Data Path is used for scaling and color space conversion.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>
>> Just enum, no items here
> 
> See below.
> 
>>
>>
>>> +          - enum:
>>> +              - mediatek,mt8173-mdp-rdma
>>> +              - mediatek,mt8173-mdp-rsz
>>> +              - mediatek,mt8173-mdp-wdma
>>> +              - mediatek,mt8173-mdp-wrot
>>> +      - items:
>>> +          - enum:
>>> +              - mediatek,mt8173-mdp-rdma
>>> +              - mediatek,mt8173-mdp-rsz
>>> +              - mediatek,mt8173-mdp-wdma
>>> +              - mediatek,mt8173-mdp-wrot
>>> +          - const: mediatek,mt8173-mdp
>>
>> This makes no sense. How devices can be compatible and can not be
>> compatible.
> 
> According to the driver source code (and the previous txt mt8173-mdp 
> bindings), there must be a "controller node" with compatible 
> `mediatek,mt8173-mdp`. Then its sibling nodes (including itself) should 

But you did not define "mediatek,mt8173-mdp" here, so what are you
talking about?

I talk here about "wrot" and others, I thought it is obvious from the
mistake in the schema.


> be one of the component node ids, listed in `struct of_device_id 
> mtk_mdp_comp_dt_ids[]`.
> 
> Is there a proper/different way to describe this compatible binding in 
> the yaml? Or you're saying the driver doesn't make sense here?
> 
> [0] drivers/media/platform/mediatek/mdp/mtk_mdp_core.c
> 
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks: true
>>
>> No, there's no such syntax. Look at other bindings.
> 
> Ack.
> 
>>
>>
>>> +
>>> +  power-domains:
>>> +    maxItems: 1
>>> +
>>> +  iommus:
>>> +    description: |
>>
>> Drop |
> 
> Ack.
> 
>>
>>> +      This property should point to the respective IOMMU block with master port as argument,
>>> +      see Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml for details.
>>
>> Drop entire description, completely redundant. I don't know why my patch
>> fixing this was not applied, so you keep repeating same mistakes...
> 
> Ack.
> 
>>
>>> +    maxItems: 1
>>> +
>>> +  mediatek,vpu:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      Describes point to vpu.
>>
>> Useless description. We see that from the property name. Explain the
>> purpose in the hardware.
> 
> Ack.
> 
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clocks
>>> +  - power-domains
>>> +
>>> +allOf:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: mediatek,mt8173-mdp-rdma
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: Main clock
>>> +            - description: Mutex clock
>>> +    else:
>>> +      properties:
>>> +        clocks:
>>> +          items:
>>> +            - description: Main clock
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - mediatek,mt8173-mdp-rdma
>>> +              - mediatek,mt8173-mdp-wdma
>>> +              - mediatek,mt8173-mdp-wrot
>>> +    then:
>>> +      required:
>>> +        - iommus
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: mediatek,mt8173-mdp
>>
>> This makes no sense either.
> 
> Same question above about compatibles.

How same question? Do you understand this code? It is nothing the same -
you have here contains!


Best regards,
Krzysztof