Re: [PATCH v4 23/42] dt-bindings: dma: Add Cirrus EP93xx
From: Krzysztof Kozlowski
Date: Fri Sep 15 2023 - 06:59:07 EST
On 15/09/2023 10:11, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
>
> Add YAML bindings for ep93xx SoC DMA.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> ---
> .../bindings/dma/cirrus,ep9301-dma-m2m.yaml | 69 ++++++++++++
> .../bindings/dma/cirrus,ep9301-dma-m2p.yaml | 121 +++++++++++++++++++++
> include/dt-bindings/dma/cirrus,ep93xx-dma.h | 26 +++++
> include/linux/platform_data/dma-ep93xx.h | 21 +---
> 4 files changed, 217 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/cirrus,ep9301-dma-m2m.yaml b/Documentation/devicetree/bindings/dma/cirrus,ep9301-dma-m2m.yaml
> new file mode 100644
> index 000000000000..80a4352bf8aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/cirrus,ep9301-dma-m2m.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/cirrus,ep9301-dma-m2m.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logick ep93xx SoC DMA controller
> +
> +maintainers:
> + - Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> + - Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> +
Missing $ref to dma-controller.yaml
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-dma-m2m
> + - items:
> + - enum:
> + - cirrus,ep9302-dma-m2m
> + - cirrus,ep9307-dma-m2m
> + - cirrus,ep9312-dma-m2m
> + - cirrus,ep9315-dma-m2m
> + - const: cirrus,ep9301-dma-m2m
> +
> + reg:
> + items:
> + - description: m2m0 channel registers
> + - description: m2m1 channel registers
> +
> + clocks:
> + items:
> + - description: m2m0 channel gate clock
> + - description: m2m1 channel gate clock
> +
> + clock-names:
> + items:
> + - const: m2m0
> + - const: m2m1
> +
> + interrupts:
> + items:
> + - description: m2m0 channel interrupt
> + - description: m2m1 channel interrupt
> +
> + '#dma-cells': true
Rather:
const: 1
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> + - clock-names
> + - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/cirrus,ep9301-clk.h>
> + dma-controller@80000100 {
> + compatible = "cirrus,ep9301-dma-m2m";
> + reg = <0x80000100 0x0040>,
> + <0x80000140 0x0040>;
> + clocks = <&eclk EP93XX_CLK_M2M0>,
> + <&eclk EP93XX_CLK_M2M1>;
> + clock-names = "m2m0", "m2m1";
> + interrupt-parent = <&vic0>;
> + interrupts = <17>, <18>;
> + #dma-cells = <1>;
> + };
> diff --git a/Documentation/devicetree/bindings/dma/cirrus,ep9301-dma-m2p.yaml b/Documentation/devicetree/bindings/dma/cirrus,ep9301-dma-m2p.yaml
> new file mode 100644
> index 000000000000..0236cc37233e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/cirrus,ep9301-dma-m2p.yaml
> @@ -0,0 +1,121 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/dma/cirrus,ep9301-dma-m2p.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logick ep93xx SoC M2P DMA controller
> +
> +maintainers:
> + - Alexander Sverdlin <alexander.sverdlin@xxxxxxxxx>
> + - Nikita Shubin <nikita.shubin@xxxxxxxxxxx>
> +
Same comments in this file
> +properties:
> + compatible:
> + oneOf:
> + - const: cirrus,ep9301-dma-m2p
> + - items:
> + - enum:
> + - cirrus,ep9302-dma-m2p
> + - cirrus,ep9307-dma-m2p
> + - cirrus,ep9312-dma-m2p
> + - cirrus,ep9315-dma-m2p
> + - const: cirrus,ep9301-dma-m2p
> +
> + reg:
> + items:
> + - description: m2p0 channel registers
> + - description: m2p1 channel registers
> + - description: m2p2 channel registers
> + - description: m2p3 channel registers
> + - description: m2p4 channel registers
> + - description: m2p5 channel registers
> + - description: m2p6 channel registers
> + - description: m2p7 channel registers
> + - description: m2p8 channel registers
> + - description: m2p9 channel registers
> +
...
> diff --git a/include/dt-bindings/dma/cirrus,ep93xx-dma.h b/include/dt-bindings/dma/cirrus,ep93xx-dma.h
> new file mode 100644
> index 000000000000..21c7324eb27e
> --- /dev/null
> +++ b/include/dt-bindings/dma/cirrus,ep93xx-dma.h
Use same filename as bindings.
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +#ifndef DT_BINDINGS_CIRRUS_EP93XX_DMA_H
> +#define DT_BINDINGS_CIRRUS_EP93XX_DMA_H
> +
> +/*
> + * M2P channels.
> + *
> + * Note that these values are also directly used for setting the PPALLOC
> + * register.
> + */
> +#define EP93XX_DMA_I2S1 0
Where is the user of these defines? IOW, who uses them? Only driver?
> +#define EP93XX_DMA_I2S2 1
> +#define EP93XX_DMA_AAC1 2
> +#define EP93XX_DMA_AAC2 3
> +#define EP93XX_DMA_AAC3 4
> +#define EP93XX_DMA_I2S3 5
> +#define EP93XX_DMA_UART1 6
> +#define EP93XX_DMA_UART2 7
> +#define EP93XX_DMA_UART3 8
> +#define EP93XX_DMA_IRDA 9
> +/* M2M channels */
> +#define EP93XX_DMA_SSP 10
> +#define EP93XX_DMA_IDE 11
> +
> +#endif /* DT_BINDINGS_CIRRUS_EP93XX_DMA_H */
> +
> diff --git a/include/linux/platform_data/dma-ep93xx.h b/include/linux/platform_data/dma-ep93xx.h> index eb9805bb3fe8..54b41d1468ef 100644
> --- a/include/linux/platform_data/dma-ep93xx.h
> +++ b/include/linux/platform_data/dma-ep93xx.h
> @@ -5,26 +5,7 @@
> #include <linux/types.h>
> #include <linux/dmaengine.h>
> #include <linux/dma-mapping.h>
> -
> -/*
> - * M2P channels.
> - *
> - * Note that these values are also directly used for setting the PPALLOC
> - * register.
> - */
> -#define EP93XX_DMA_I2S1 0
> -#define EP93XX_DMA_I2S2 1
> -#define EP93XX_DMA_AAC1 2
> -#define EP93XX_DMA_AAC2 3
> -#define EP93XX_DMA_AAC3 4
> -#define EP93XX_DMA_I2S3 5
> -#define EP93XX_DMA_UART1 6
> -#define EP93XX_DMA_UART2 7
> -#define EP93XX_DMA_UART3 8
> -#define EP93XX_DMA_IRDA 9
> -/* M2M channels */
> -#define EP93XX_DMA_SSP 10
> -#define EP93XX_DMA_IDE 11
> +#include <dt-bindings/dma/cirrus,ep93xx-dma.h>
Your headers should not include bindings.
Best regards,
Krzysztof