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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250425-romantic-truthful-dove-3ef949@kuoka>
Date: Fri, 25 Apr 2025 12:34:09 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Nas Chung <nas.chung@...psnmedia.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl, sebastian.fricke@...labora.com, 
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	linux-media@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-imx@....com, marex@...x.de, jackson.lee@...psnmedia.com, 
	lafley.kim@...psnmedia.com
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@...psnmedia.com>
> ---
>  .../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

> +
> +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

... 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.

> +
> +  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

> +
> +  "#address-cells": true

instead const

> +
> +  "#size-cells": true

const

> +
> +  ranges: true
> +
> +patternProperties:
> +  "^video-core@[0-9a-f]+$":
> +    type: object
> +
> +    properties:
> +      compatible:
> +        items:

Drop items and keep just enum

> +          - 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?

> +
> +      reg:
> +        maxItems: 1
> +
> +      interrupts:
> +        maxItems: 1
> +
> +    required:
> +      - compatible
> +      - reg
> +      - interrupts
> +
> +    additionalProperties: false

Put this immediately after type:object

> +
> +  "^video-controller@[0-9a-f]+$":
> +    type: object

Same here goes additionalProps.

> +
> +    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 "

> +        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.

Can we see the entire DTS?

Best regards,
Krzysztof