[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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