Re: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec device

[Krzysztof Kozlowski <krzk@kernel.org>]

On 09/05/2025 11:59, Nas Chung wrote:
>>> +examples:
>>> +  - |
>>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> +    #include <dt-bindings/clock/nxp,imx95-clock.h>
>>> +
>>> +    soc {
>>> +      #address-cells = <2>;
>>> +      #size-cells = <2>;
>>> +
>>> +      vpu: video-codec {
>>
>> Why this device does not have MMIO? Sorry, but makes little sense and if
>> you posted and tested your entire DTS you would see why.
> 
> I initially thought that if the reg property is declared in the child,
> it would not need to be declared in the parent node.
> I based this approach on the mediatek,mt8195-jpegenc.yaml binding,
> where the parent node does not include MMIO.

Do you have access to mt8195 datasheet? What is the memory/register
layout there?

If you do not have access, why do you think these are similar devices?

> 
> But, if this structure is problematic, I will address it in patch v3.
> 
>>
>> Can we see the entire DTS?
> 
> Sure !
> Below is the cnm.wave633c.example.dts file created from dt_binding_check.

This is not the entire DTS.

> 
> /dts-v1/;
> /plugin/; // silence any missing phandle references

This is bindings example. I want to see entire DTS or DTSI of the SoC.
Once you see entire DTS, you will notice that your current split is just
not correct - it should be pretty obvious.

And that's why we should keep rejecting such works which do not bring
any DTS user, because the no one - neither we nor the contributors - see
big picture and if someone saw the big picture then immediately would
notice - it's just bollocks.

What you claim is:

soc@0 {
  // MMIO bus

  video-codec {
    // which is a non-MMIO device and clearly a no-go.

Just look how DTS is organized and learn from it.

> 
> /{
>     compatible = "foo";
>     model = "foo";
>     #address-cells = <1>;
>     #size-cells = <1>;
> 
> 
>     example-0 {
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         
>         interrupt-parent = <&fake_intc0>;
>         fake_intc0: fake-interrupt-controller {
>             interrupt-controller;
>             #interrupt-cells = < 3 >;
>         };
> 

All of above are wrong for the SoC...

> 
>         #include <dt-bindings/interrupt-controller/arm-gic.h>
>         #include <dt-bindings/clock/nxp,imx95-clock.h>
>         
>         soc {
>           #address-cells = <2>;
>           #size-cells = <2>;
>         
>           vpu: video-codec {
>             compatible = "nxp,imx95-vpu", "cnm,wave633c";

What does this device represent? It is not "ctrl", because you made ctrl
separate device node. Your binding description suggests that is the VPU
control region.

>             clocks = <&scmi_clk 115>,
>                      <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;

For which sub devices these clocks are valid? For all of them?

>             clock-names = "vpu", "vpublk_wave";
>             power-domains = <&scmi_devpd 21>;
>             #address-cells = <2>;
>             #size-cells = <2>;
>             ranges;
>         
>             vpucore0: video-core@4c480000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c480000 0x0 0x10000>;
>               interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpucore1: video-core@4c490000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c490000 0x0 0x10000>;
>               interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpucore2: video-core@4c4a0000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c4a0000 0x0 0x10000>;
>               interrupts = <GIC_SPI 301 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpucore3: video-core@4c4b0000 {
>               compatible = "nxp,imx95-vpu-core";
>               reg = <0x0 0x4c4b0000 0x0 0x10000>;
>               interrupts = <GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>;
>             };
>         
>             vpuctrl: video-controller@4c4c0000 {
>               compatible = "nxp,imx95-vpu-ctrl";
>               reg = <0x0 0x4c4c0000 0x0 0x10000>;
>               memory-region = <&vpu_boot>;
>               power-domains = <&scmi_perf 10>;
>               #cooling-cells = <2>;
>               sram = <&sram1>;
>             };
>           };
>         };
> 
>     };
> };
> 
>>
>> Best regards,
>> Krzysztof
> 


Best regards,
Krzysztof