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

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.

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

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

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