RE: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec device
From: Nas Chung
Date: Fri May 09 2025 - 06:00:02 EST
Hi, Krzysztof.
Thanks for your feedback.
>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>Sent: Friday, April 25, 2025 7:34 PM
>To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>Cc: mchehab@xxxxxxxxxx; hverkuil@xxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx;
>robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx; linux-
>media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; linux-imx@xxxxxxx; marex@xxxxxxx; jackson.lee
><jackson.lee@xxxxxxxxxxxxxxx>; lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>
>Subject: Re: [PATCH v2 2/8] dt-bindings: media: nxp: Add Wave6 video codec
>device
>
>On Tue, Apr 22, 2025 at 06:31:13PM GMT, Nas Chung wrote:
>> Add documents for the Wave6 video codec on NXP i.MX SoCs.
>>
>> Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>> ---
>> .../bindings/media/cnm,wave633c.yaml | 165 ++++++++++++++++++
>> MAINTAINERS | 7 +
>> 2 files changed, 172 insertions(+)
>> create mode 100644
>Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>b/Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>> new file mode 100644
>> index 000000000000..5bb572e8ca18
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/cnm,wave633c.yaml
>> @@ -0,0 +1,165 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/media/cnm,wave633c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Chips&Media Wave6 Series multi-standard codec IP.
>
>Drop full stop
I see, I will remove it.
>
>> +
>> +maintainers:
>> + - Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
>> + - Jackson Lee <jackson.lee@xxxxxxxxxxxxxxx>
>> +
>> +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
>
>... this and ...
>
>> + the VPU control region and the VPU core region.
>> + The VPU control region manages shared resources such as firmware
>memory,
>> + while the VPU core region provides encoding and decoding
>> + capabilities. The VPU core cannot operate independently without
>> + the VPU control region.
>> +
>> +properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - nxp,imx95-vpu
>> + - const: cnm,wave633c
>
>... your drivers seem to use soc specific compatible, so I do not see
>value in generic compatible. It would have to be good enough alone for
>drivers, but it is not.
I agree with you. I will drop the generic compatible name and
change the yaml file name to nxp,imx95-vpu.yaml.
>
>> +
>> + clocks:
>> + items:
>> + - description: VPU clock
>> + - description: VPU associated block clock
>> +
>> + clock-names:
>> + items:
>> + - const: vpu
>> + - const: vpublk_wave
>> +
>> + power-domains:
>> + maxItems: 1
>> + description: Main VPU power domain
>
>Drop description
Okay. I will remove the description for power-domains.
>
>> +
>> + "#address-cells": true
>
>instead const
>
>> +
>> + "#size-cells": true
>
>const
I see, I will change "#address-cells" and "#size-cells" to use const.
>
>> +
>> + ranges: true
>> +
>> +patternProperties:
>> + "^video-core@[0-9a-f]+$":
>> + type: object
>> +
>> + properties:
>> + compatible:
>> + items:
>
>Drop items and keep just enum
Okay, I will use enum directly.
>
>> + - enum:
>> + - nxp,imx95-vpu-core
>
>So this is another proof that cnm,wave633c is wrong. How cnm,wave633c
>can come with nxp,imx95-vpu-core child?
I understand your point.
I will address this in my previous comment. Thanks for pointing it out.
>
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + required:
>> + - compatible
>> + - reg
>> + - interrupts
>> +
>> + additionalProperties: false
>
>Put this immediately after type:object
Okay.
>
>> +
>> + "^video-controller@[0-9a-f]+$":
>> + type: object
>
>Same here goes additionalProps.
Okay.
>
>> +
>> + properties:
>> + compatible:
>> + items:
>> + - enum:
>> + - nxp,imx95-vpu-ctrl
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + memory-region:
>> + maxItems: 1
>> +
>> + power-domains:
>> + maxItems: 1
>> + description: Performance power domain
>> +
>> + '#cooling-cells':
>
>Keep consistent quotes, either ' or "
I see, I will use consistent quoting.
>
>> + const: 2
>> +
>> + sram:
>> + $ref: /schemas/types.yaml#/definitions/phandle
>> + description: phandle of the SRAM memory region node.
>> +
>> + required:
>> + - compatible
>> + - reg
>> + - memory-region
>> + - power-domains
>> +
>> + additionalProperties: false
>> +
>> +required:
>> + - compatible
>> + - clocks
>> + - power-domains
>> +
>> +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>;
>> +
>> + 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.
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.
/dts-v1/;
/plugin/; // silence any missing phandle references
/{
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 >;
};
#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";
clocks = <&scmi_clk 115>,
<&vpu_blk_ctrl IMX95_CLK_VPUBLK_WAVE>;
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