| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cb7937f5-2045-4903-825c-71ed70097efb@kernel.org>
Date: Mon, 10 Feb 2025 18:30:49 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nas Chung <nas.chung@...psnmedia.com>, mchehab@...nel.org,
hverkuil@...all.nl, sebastian.fricke@...labora.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-imx@....com,
linux-arm-kernel@...ts.infradead.org, jackson.lee@...psnmedia.com,
lafley.kim@...psnmedia.com
Subject: Re: [PATCH 3/8] dt-bindings: media: nxp: Add Wave6 video codec device
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@...psnmedia.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@...psnmedia.com>
> + - Jackson Lee <jackson.lee@...psnmedia.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@...c0000 {
> + 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@...80000 {
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@...90000 {
> + 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@...a0000 {
> + 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@...b0000 {
> + 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@...psnmedia.com>
> +M: Jackson Lee <jackson.lee@...psnmedia.com>
> +L: linux-media@...r.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
Powered by blists - more mailing lists