Message-ID: <d286ec0b-c8dc-4103-9aa3-2f40e0ade4a3@collabora.com>
Date: Mon, 8 Sep 2025 14:52:23 -0300
From: Ariel D'Alessandro <ariel.dalessandro@...labora.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: airlied@...il.com, amergnat@...libre.com, andrew+netdev@...n.ch,
 andrew-ct.chen@...iatek.com, angelogioacchino.delregno@...labora.com,
 broonie@...nel.org, chunkuang.hu@...nel.org, ck.hu@...iatek.com,
 conor+dt@...nel.org, davem@...emloft.net, dmitry.torokhov@...il.com,
 edumazet@...gle.com, flora.fu@...iatek.com, houlong.wei@...iatek.com,
 jeesw@...fas.com, jmassot@...labora.com, kernel@...labora.com,
 krzk+dt@...nel.org, kuba@...nel.org,
 kyrie.wu@...iatek.corp-partner.google.com, lgirdwood@...il.com,
 linus.walleij@...aro.org, louisalexis.eyraud@...labora.com,
 maarten.lankhorst@...ux.intel.com, matthias.bgg@...il.com,
 mchehab@...nel.org, minghsiu.tsai@...iatek.com, mripard@...nel.org,
 p.zabel@...gutronix.de, pabeni@...hat.com, robh@...nel.org,
 sean.wang@...nel.org, simona@...ll.ch, support.opensource@...semi.com,
 tiffany.lin@...iatek.com, tzimmermann@...e.de, yunfei.dong@...iatek.com,
 devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
 linux-gpio@...r.kernel.org, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 linux-mediatek@...ts.infradead.org, linux-sound@...r.kernel.org,
 netdev@...r.kernel.org
Subject: Re: [PATCH v1 01/14] media: dt-bindings: Convert MediaTek mt8173-mdp
 bindings to YAML

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@...labora.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@...labora.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 
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.

> 
>> +    then:
>> +      required:
>> +        - mediatek,vpu
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/mt8173-clk.h>
>> +    #include <dt-bindings/memory/mt8173-larb-port.h>
>> +    #include <dt-bindings/power/mt8173-power.h>
>> +
>> +    soc {
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +
>> +        mdp_rdma0: rdma@...01000 {
> 
> One example is enough. Two could be fine if they differ significantly.

Sounds good. Will keep just a single example, including a node for the 
controller node and one for each of the components.

Thanks a lot for the feedback!

-- 
Ariel D'Alessandro
Software Engineer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK 
Registered in England & Wales, no. 5513718

Powered by blists - more mailing lists