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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 10/02/2025 10:07, Nas Chung wrote:
> Add documents for the Wave6 video codec on NXP i.MX SoCs.
> 
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> ---
>  .../bindings/media/nxp,wave633c.yaml          | 202 ++++++++++++++++++
>  MAINTAINERS                                   |   8 +
>  2 files changed, 210 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/nxp,wave633c.yaml
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,wave633c.yaml b/Documentation/devicetree/bindings/media/nxp,wave633c.yaml
> new file mode 100644
> index 000000000000..99c3008314c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/nxp,wave633c.yaml

Filename matching compatible.

> @@ -0,0 +1,202 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/nxp,wave633c.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Chips&Media Wave6 Series multi-standard codec IP on NXP i.MX SoCs.
> +
> +maintainers:
> +  - Nas Chung <nas.chung@chipsnmedia.com>
> +  - Jackson Lee <jackson.lee@chipsnmedia.com>
> +
> +description:
> +  The Chips&Media Wave6 codec IP is a multi-standard video encoder/decoder.
> +  On NXP i.MX SoCs, Wave6 codec IP functionality is split between the VPU control device
> +  (vpu-ctrl) and the VPU device (vpu). The VPU control device manages shared resources
> +  such as firmware access and power domains, while the VPU device provides encoding
> +  and decoding capabilities. The VPU devie cannot operate independently

Typo

> +  without the VPU control device.
> +

Please wrap code according to the preferred limit expressed in Kernel
coding style (checkpatch is not a coding style description, but only a
tool).  Bindings use strict rule here.



> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - nxp,imx95-wave633c-ctrl
> +          - nxp,imx95-wave633c

I don't understand why you duplicated compatibles. You split this for
driver? That's a no. There are no two hardwares.

These compatibles are anyway weird - why imx95 is in chipmedia product?
Is this part of a SoC?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: VPU clock
> +      - description: VPU associated block clock
> +
> +  clock-names:
> +    items:
> +      - const: vpu
> +      - const: vpublk_wave
> +
> +  power-domains:
> +    minItems: 1
> +    items:
> +      - description: Main VPU power domain
> +      - description: Performance power domain
> +
> +  power-domain-names:
> +    items:
> +      - const: vpumix
> +      - const: vpuperf
> +
> +  cnm,ctrl:

What is this prefix about? Is this nxp or something else?

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of the VPU control device node. Required for VPU operation.

Explain - required for what. Operation is too generic.

If this is phandle to second device, then it's proof your split is
really wrong.

> +
> +  boot:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of the boot memory region node for the VPU control device.

No clue what is this... if memory region then use existing bindings.

Anyway, wrap your code correctly.

> +
> +  sram:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: phandle of the SRAM memory region node for the VPU control device.
> +
> +  '#cooling-cells':
> +    const: 2
> +
> +  support-follower:
> +    type: boolean
> +    description: Indicates whether the VPU domain power always on.

You cannot add new common properties in random way. Missing vendor
prefix but more important: does not look at all as hardware property but
OS policy.

> +
> +patternProperties:
> +  "^vpu-ctrl@[0-9a-f]+$":
> +    type: object
> +    properties:
> +      compatible:
> +        items:
> +          - enum:
> +              - nxp,imx95-wave633c-ctrl

Really, what? How nxp,imx95-wave633c-ctrl node can have a child with
nxp,imx95-wave633c-ctrl compatible?

NAK.


> +      reg: true
> +      clocks: true
> +      clock-names: true
> +      power-domains:
> +        items:
> +          - description: Main VPU power domain
> +          - description: Performance power domain
> +      power-domain-names:
> +        items:
> +          - const: vpumix
> +          - const: vpuperf
> +      sram: true
> +      boot: true
> +      '#cooling-cells': true
> +      support-follower: true
> +    required:
> +      - compatible
> +      - reg
> +      - clocks
> +      - clock-names
> +      - power-domains
> +      - power-domain-names
> +      - sram
> +      - boot
> +
> +    additionalProperties: false
> +
> +  "^vpu@[0-9a-f]+$":
> +    type: object
> +    properties:
> +      compatible:
> +        items:
> +          - enum:
> +              - nxp,imx95-wave633c
> +      reg: true
> +      interrupts: true
> +      clocks: true
> +      clock-names: true
> +      power-domains:
> +        maxItems: 1
> +        description: Main VPU power domain
> +      cnm,ctrl: true
> +    required:
> +      - compatible
> +      - reg
> +      - interrupts
> +      - clocks
> +      - clock-names
> +      - power-domains
> +      - cnm,ctrl

All this is just incorrect.

> +
> +    additionalProperties: false
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/nxp,imx95-clock.h>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      vpuctrl: vpu-ctrl@4c4c0000 {
> +        compatible = "nxp,imx95-wave633c-ctrl";
> +        reg = <0x0 0x4c4c0000 0x0 0x10000>;
> +        clocks = <&scmi_clk 115>,
> +            <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
> +        clock-names = "vpu", "vpublk_wave";
> +        power-domains = <&scmi_devpd 21>, <&scmi_perf 10>;
> +        power-domain-names = "vpumix", "vpuperf";
> +        #cooling-cells = <2>;
> +        boot = <&vpu_boot>;
> +        sram = <&sram1>;
> +      };
> +
> +      vpu0: vpu@4c480000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +        compatible = "nxp,imx95-wave633c";
> +        reg = <0x0 0x4c480000 0x0 0x10000>;
> +        interrupts = <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&scmi_clk 115>,
> +                <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
> +        clock-names = "vpu", "vpublk_wave";
> +        power-domains = <&scmi_devpd 21>;
> +        cnm,ctrl = <&vpuctrl>;
> +      };
> +
> +      vpu1: vpu@4c490000 {
> +        compatible = "nxp,imx95-wave633c";

Drop all duplicated examples.


> +        reg = <0x0 0x4c490000 0x0 0x10000>;
> +        interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&scmi_clk 115>,
> +                <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
> +        clock-names = "vpu", "vpublk_wave";
> +        power-domains = <&scmi_devpd 21>;
> +        cnm,ctrl = <&vpuctrl>;
> +      };
> +
> +      vpu2: vpu@4c4a0000 {
> +        compatible = "nxp,imx95-wave633c";
> +        reg = <0x0 0x4c4a0000 0x0 0x10000>;
> +        interrupts = <GIC_SPI 301 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&scmi_clk 115>,
> +                <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
> +        clock-names = "vpu", "vpublk_wave";
> +        power-domains = <&scmi_devpd 21>;
> +        cnm,ctrl = <&vpuctrl>;
> +      };
> +
> +      vpu3: vpu@4c4b0000 {
> +        compatible = "nxp,imx95-wave633c";
> +        reg = <0x0 0x4c4b0000 0x0 0x10000>;
> +        interrupts = <GIC_SPI 302 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&scmi_clk 115>,
> +                <&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
> +        clock-names = "vpu", "vpublk_wave";
> +        power-domains = <&scmi_devpd 21>;
> +        cnm,ctrl = <&vpuctrl>;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 896a307fa065..5ff5b1f1ced2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25462,6 +25462,14 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/media/cnm,wave521c.yaml
>  F:	drivers/media/platform/chips-media/wave5/
>  
> +WAVE6 VPU CODEC DRIVER
> +M:	Nas Chung <nas.chung@chipsnmedia.com>
> +M:	Jackson Lee <jackson.lee@chipsnmedia.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/media/nxp,wave633c.yaml
> +F:	drivers/media/platform/chips-media/wave6/

There is no such file/directory.

Best regards,
Krzysztof