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

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: <pvocii4kji7xsci5fcztenhkkjbsyu4dgz5fu6nuwykezzoyuy@35i4phytxyzc>
Date: Thu, 5 Jun 2025 13:29:31 +0000
From: Yassine OUAISSA <yassine.ouaissa@...egrodvt.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Michael Tretter <m.tretter@...gutronix.de>, 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
Subject: Re: [PATCH v2 2/4] dt-bindings: media: allegro-dvt: add decoder
 dt-bindings for Gen3 IP

Hi Krzysztof,

On 05.06.2025 15:01, Krzysztof Kozlowski wrote:
>On 05/06/2025 14:26, Yassine Ouaissa via B4 Relay wrote:
>> From: Yassine Ouaissa <yassine.ouaissa@...egrodvt.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
>

Got it, thanks.
>>
>> 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
>

I'll move it to the 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@...egrodvt.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@...egrodvt.com>
>> +
>> +description: The al300-vdec represents the gen 3 of Allegro DVT IP video
>
>Blank line after description:

I'll change it in the next version.
>
>> +  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

I'll change it in the next version.
>
>> +
>> +  reg-names:
>> +    items:
>> +      - const: regs
>
>base? apb is also "regs", because this is "reg" property, so "regs"
>feels redundant.
>

the regs, is where the device is mapped, but the apb is used by the MCU.
the reg 'apb' is used to map the MCU peripherals.

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

I'll change it in the next version.
>> +      - const: apb
>> +
>Best regards,
>Krzysztof

Best regards,
Yassine OUAISSA