Re: [PATCH v10 3/3] dt-bindings: dma: uart: rename binding

[Matthias Brugger <matthias.bgg@gmail.com>]

On 18/01/2019 04:10, Long Cheng wrote:
> The filename matches mtk-uart-apdma.c.
> So using "mtk-uart-apdma.txt" should be better.
> 
> Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> ---
>  .../devicetree/bindings/dma/8250_mtk_dma.txt       |   33 --------------------
>  .../devicetree/bindings/dma/mtk-uart-apdma.txt     |   33 ++++++++++++++++++++
>  2 files changed, 33 insertions(+), 33 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
>  create mode 100644 Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
> 
> diff --git a/Documentation/devicetree/bindings/dma/8250_mtk_dma.txt b/Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
> deleted file mode 100644
> index 3fe0961..0000000
> --- a/Documentation/devicetree/bindings/dma/8250_mtk_dma.txt
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -* Mediatek UART APDMA Controller
> -
> -Required properties:
> -- compatible should contain:
> -  * "mediatek,mt2712-uart-dma" for MT2712 compatible APDMA
> -  * "mediatek,mt6577-uart-dma" for MT6577 and all of the above
> -
> -- reg: The base address of the APDMA register bank.> -
> -- interrupts: A single interrupt specifier.

One interrupt per dma-request, or 8 if no dma-requests property is present.

> -
> -- clocks : Must contain an entry for each entry in clock-names.
> -  See ../clocks/clock-bindings.txt for details.
> -- clock-names: The APDMA clock for register accesses> -
> -Examples:
> -
> -	apdma: dma-controller@11000380 {
> -		compatible = "mediatek,mt2712-uart-dma";
> -		reg = <0 0x11000380 0 0x400>;
> -		interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 64 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 65 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 66 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 67 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 68 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 69 IRQ_TYPE_LEVEL_LOW>,
> -			     <GIC_SPI 70 IRQ_TYPE_LEVEL_LOW>;
> -		clocks = <&pericfg CLK_PERI_AP_DMA>;
> -		clock-names = "apdma";
> -		#dma-cells = <1>;
> -	};
> -
> diff --git a/Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt b/Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
> new file mode 100644
> index 0000000..3fe0961
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dma/mtk-uart-apdma.txt
> @@ -0,0 +1,33 @@
> +* Mediatek UART APDMA Controller
> +
> +Required properties:
> +- compatible should contain:
> +  * "mediatek,mt2712-uart-dma" for MT2712 compatible APDMA
> +  * "mediatek,mt6577-uart-dma" for MT6577 and all of the above
> +
> +- reg: The base address of the APDMA register bank.
> +

There is one address needed for every dma-request or 8 if dma-requests property
is not present. As already mentioned in the driver review dma-requests property
is missing.

> +- interrupts: A single interrupt specifier.
> +
> +- clocks : Must contain an entry for each entry in clock-names.
> +  See ../clocks/clock-bindings.txt for details.
> +- clock-names: The APDMA clock for register accesses
> +

Missing option property dma-33bits (should be mtk,dma-33bits?).

> +Examples:
> +
> +	apdma: dma-controller@11000380 {
> +		compatible = "mediatek,mt2712-uart-dma";
> +		reg = <0 0x11000380 0 0x400>;

We are missing the io mem areas here, right?

Regards,
Matthias

> +		interrupts = <GIC_SPI 63 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 64 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 65 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 66 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 67 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 68 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 69 IRQ_TYPE_LEVEL_LOW>,
> +			     <GIC_SPI 70 IRQ_TYPE_LEVEL_LOW>;
> +		clocks = <&pericfg CLK_PERI_AP_DMA>;
> +		clock-names = "apdma";
> +		#dma-cells = <1>;
> +	};
> +
>