Re: [PATCH v2 2/4] dt-bindings: media: allegro-dvt: add decoder dt-bindings for Gen3 IP

[Krzysztof Kozlowski <krzk@kernel.org>]

On 05/06/2025 14:26, Yassine Ouaissa via B4 Relay wrote:
> From: Yassine Ouaissa <yassine.ouaissa@allegrodvt.com>
> 
> Add compatible for video decoder on allegrodvt Gen 3 IP.

A nit, subject: drop second/last, redundant "dt-bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18


Subject prefix(es): still wrong. You can get them for example with `git
log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is
touching. For bindings, the preferred subjects are explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

> 
> v2:
> - Change the YAML file name, use the existing vendor-prefix.
> - Improuve the dt-bindings description.
> - Change the device compatible identifier, from "allegrodvt, al300-vdec",
>   to "allegro, al300-vdec"
> - Simplify the register property specification,
>   by using the simple min/max items constraint (Krzysztof Kozlowski)
> - Remove the clock-names property. And remove it from the required
>   properties list (Krzysztof Kozlowski) (Conor Dooley)
> - Use the simple maxItems constraint for the memory-region property.
>   Also for the firmware-name (Krzysztof Kozlowski)
> - Example changes:
>   - Use header provides definitions for the interrupts (Conor Dooley)
>   - Improuve Interrupt specification using GIC constants (Conor Dooley)
>   - Use generic node name "video-decoder" (Krzysztof Kozlowski) (Conor Dooley)
>   - Remove unused label (Krzysztof Kozlowski)
>   - Change clock reference from <&mcu_clock_dec> to <&mcu_core_clk>
>   - Use hex format for reg property (Krzysztof Kozlowski) (Conor Dooley)
>   - Reduce memory region size (Krzysztof Kozlowski) (Conor Dooley)

All this goes to changelog

> 
>   - Link v1: https://patchwork.linuxtv.org/project/linux-media/patch/20250511144752.504162-4-yassine.ouaissa@allegrodvt.com/

Drop

> 
> Signed-off-by: Yassine Ouaissa <yassine.ouaissa@allegrodvt.com>
> ---
>  .../bindings/media/allegro,al300-vdec.yaml         | 75 ++++++++++++++++++++++
>  MAINTAINERS                                        |  2 +
>  2 files changed, 77 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..26f9ac39682431b1d4828aed5d1ed43ef099e204
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/allegro,al300-vdec.yaml
> @@ -0,0 +1,75 @@
> +# 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@allegrodvt.com>
> +
> +description: The al300-vdec represents the gen 3 of Allegro DVT IP video

Blank line after description:

> +  decoding technology, offering significant advancements over its
> +  predecessors. This new decoder features enhanced processing capabilities
> +  with improved throughput and reduced latency.
> +
> +  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.
> +
> +properties:
> +  compatible:
> +    const: allegro,al300-vdec
> +
> +  reg:
> +    maxItems: 2
> +    minItems: 2

Drop

> +
> +  reg-names:
> +    items:
> +      - const: regs

base? apb is also "regs", because this is "reg" property, so "regs"
feels redundant.

Unless this is something entirely else (quite different address in
example), so maybe this should not be reg at all.

Also, make the example complete - missing memory region.

> +      - const: apb
> +
Best regards,
Krzysztof