Re: [PATCH v9 1/3] dt-bindings: dma: Add Amlogic A9 SoC DMA

On 2026/6/26 13:50, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Low] Global dt-binding header defines generic 'DMA_TX' and 'DMA_RX' macros, risking namespace collisions.
> - [Low] The `#dma-cells` property lacks a description explaining the layout and purpose of its 2 cells.
> - [Low] Missing space before the opening brace in the device tree example node.
> --
> 
> commit a7b4f5e4e6207e7134f0b399abc853db92908381
> Author: Xianwei Zhao<[email protected]>
> 
> dt-bindings: dma: Add Amlogic A9 SoC DMA
> 
> This commit adds device tree binding documentation for the Amlogic A9 SoC
> general-purpose DMA controller. It also introduces a new header file containing
> specific property values for this controller.
> 
>> diff --git a/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml b/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
>> new file mode 100644
>> index 0000000000000..efd7b2602c336
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/amlogic,a9-dma.yaml
> [ ... ]
> 
>> @@ -0,0 +1,65 @@
>> +properties:
>> +  compatible:
>> +    const: amlogic,a9-dma
> [ ... ]
> 
>> +  clock-names:
>> +    const: sys
>> +
>> +  '#dma-cells':
>> +    const: 2
> [Severity: Low]
> Is a description needed for this `#dma-cells` property to explain the layout
> and purpose of its 2 cells? Without it, device tree writers won't have
> documentation clarifying which cell represents the channel ID and which
> represents the direction.
> 
Will add it.
>> +
>> +  dma-channels:
>> +    maximum: 64
> [ ... ]
> 
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    dma-controller@fe400000{
> [Severity: Low]
> This isn't a bug, but should there be a space before the opening brace here?
> Normally it is formatted as `dma-controller@fe400000 {`.
> 
Will add space.
>> +        compatible = "amlogic,a9-dma";
>> +        reg = <0xfe400000 0x4000>;
>> +        interrupts = <GIC_SPI 35 IRQ_TYPE_EDGE_RISING>;
>> +        clocks = <&clkc 45>;
>> +        #dma-cells = <2>;
>> +        dma-channels = <28>;
>> +    };
>> diff --git a/include/dt-bindings/dma/amlogic,a9-dma.h b/include/dt-bindings/dma/amlogic,a9-dma.h
>> new file mode 100644
>> index 0000000000000..c59c2fd4b9561
>> --- /dev/null
>> +++ b/include/dt-bindings/dma/amlogic,a9-dma.h
>> @@ -0,0 +1,8 @@
>> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
>> +
>> +#ifndef __DT_BINDINGS_DMA_AMLOGIC_DMA_H__
>> +#define __DT_BINDINGS_DMA_AMLOGIC_DMA_H__
>> +
>> +#define DMA_TX                       0
>> +#define DMA_RX                       1
> [Severity: Low]
> Could these macro names lead to namespace collisions since they lack a vendor
> or SoC prefix? These names are already widely used across the kernel, and
> defining them generically in a global dt-bindings header might cause
> preprocessor collisions and build failures for any driver or DTS file that
> includes this header.
> 
NO, the maintainer prefers no prefix.

>> +#endif /* __DT_BINDINGS_DMA_AMLOGIC_DMA_H__ */
> --
> Sashiko AI review ·https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic