linux-kernel - Re: [PATCH v5 2/4] dt-bindings: media: allegro: Add al300 decoder IPs

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <aRHNn6fv8jiGXccq@pengutronix.de>
Date: Mon, 10 Nov 2025 12:33:51 +0100
From: Michael Tretter <m.tretter@...gutronix.de>
To: yassine.ouaissa@...egrodvt.com
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Michal Simek <michal.simek@....com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Nicolas Dufresne <nicolas@...fresne.ca>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
	Conor Dooley <conor.dooley@...rochip.com>
Subject: Re: [PATCH v5 2/4] dt-bindings: media: allegro: Add al300 decoder IPs

On Wed, 13 Aug 2025 09:54:54 +0000, Yassine Ouaissa via B4 Relay wrote:
> From: Yassine Ouaissa <yassine.ouaissa@...egrodvt.com>
> 
> Add a schema for the allegro gen3 video decoder.
> 
> Acked-by: Conor Dooley <conor.dooley@...rochip.com>
> Signed-off-by: Yassine Ouaissa <yassine.ouaissa@...egrodvt.com>
> ---
>  .../bindings/media/allegro,al300-vdec.yaml         | 76 ++++++++++++++++++++++
>  MAINTAINERS                                        |  2 +
>  2 files changed, 78 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/allegro,al300-vdec.yaml b/Documentation/devicetree/bindings/media/allegro,al300-vdec.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..12e86c306d5578d298ad03a6d59593dd1ddcff9e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allegro,al300-vdec.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/media/allegro,al300-vdec.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Allegro DVT Video IP Decoder Gen 3
> +
> +maintainers:
> +  - Yassine OUAISSA <yassine.ouaissa@...egrodvt.com>
> +
> +description:
> +  The al300-vdec represents the gen 3 of Allegro DVT IP video
> +  decoding technology, offering significant advancements over its
> +  predecessors. This new decoder features enhanced processing capabilities
> +  with improved throughput and reduced latency.

This reads more like marketing than a technical description. Maybe just
stick to the first sentence:

	The al300-vdec represents the gen 3 of Allegro DVT IP video decoding
	technology.

> +
> +  Communication between the host driver software and the MCU is implemented
> +  through a specialized mailbox interface mechanism. This mailbox system
> +  provides a structured channel for exchanging commands, parameters, and
> +  status information between the host CPU and the MCU controlling the codec
> +  engines.

I'd focus the description on the hardware/MCP and drop anything
referring to the host driver software.

> +
> +properties:
> +  compatible:
> +    const: allegro,al300-vdec
> +
> +  reg:
> +    maxItems: 2
> +
> +  reg-names:
> +    items:
> +      - const: top
> +      - const: apb
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    description: MCU core clock
> +    maxItems: 1
> +
> +  memory-region:
> +    maxItems: 1

Please add a description explaining the purpose of the memory-region.

> +
> +  firmware-name:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: False
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    axi {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        video-decoder@...20000 {
> +            compatible = "allegro,al300-vdec";
> +            reg = <0x00 0xa0120000 0x00 0x10000>, /*VDEC_TOP*/
> +                  <0x01 0x80000000 0x00 0x8000>;  /*MCU_APB*/

Drop these comments. If the register maps need documentation, these
should be a description in the binding above.

> +            reg-names = "top", "apb";
> +            interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
> +            clocks = <&mcu_core_clk>;
> +            memory-region = <&vdec_mem>;
> +            firmware-name = "al300_vdec.fw";
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 88a76452956eb3b91e7813cbdc16c361f26216a5..532d8a51df9f500767f468935d960b1634139c65 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -819,7 +819,9 @@ M:	Michael Tretter <m.tretter@...gutronix.de>
>  R:	Pengutronix Kernel Team <kernel@...gutronix.de>
>  L:	linux-media@...r.kernel.org
>  S:	Maintained
> +F:	Documentation/devicetree/bindings/media/allegro,al300-vdec.yaml
>  F:	Documentation/devicetree/bindings/media/allegro,al5e.yaml
> +F:	drivers/media/platform/allegro-dvt/al300
>  F:	drivers/media/platform/allegro-dvt/zynqmp

This feels a bit weird. You first change the maintenance to the
zynqmp subdirectory and then add the al300 to the same maintainers
entry.

Either add yourself as maintainer for the al300 directory or just leave
the maintainership on the top directory.

Also, the al300 directory should be added with the actual driver.

Michael