On Wed, May 6, 2009 at 3:07 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> From: Piotr Ziecik <kosmo@semihalf.com>
>
> This patch adds initial version of MPC512x DMA driver.
> Only memory to memory transfers are currenly supported.
>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: John Rigby <jcrigby@gmail.com>

Don't have time to review this in detail right now, but three quick comments:

>  drivers/dma/mpc512x_dma.c                    |  642 ++++++++++++++++++++++++++
>  drivers/dma/mpc512x_dma.h                    |  192 ++++++++

It looks to me like these two files should be merged.

> diff --git a/arch/powerpc/boot/dts/mpc5121ads.dts b/arch/powerpc/boot/dts/mpc5121ads.dts
> index c2d9de9..e7f0e09 100644
> --- a/arch/powerpc/boot/dts/mpc5121ads.dts
> +++ b/arch/powerpc/boot/dts/mpc5121ads.dts
> @@ -373,7 +373,7 @@
>                };
>
>                dma@14000 {
> -                       compatible = "fsl,mpc5121-dma2";
> +                       compatible = "fsl,mpc512x-dma";

Nack.  Compatible values should not use wildcards.  Be specific.  And
be specific about what it is compatible to if another part implements
the same device.
The internal name for the dma module was dma2 that is where the orginal name came from.  There is a dma2 in some 8xxx but last I looked it is not at all the same.  I would vote for fsl,mpc5121-dma.


>                        reg = <0x14000 0x1800>;
>                        interrupts = <65 0x8>;
>                        interrupt-parent = < &ipic >;
> diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/powerpc/platforms/512x/mpc512x_shared.c
> index b776e45..135fd6b 100644
> --- a/arch/powerpc/platforms/512x/mpc512x_shared.c
> +++ b/arch/powerpc/platforms/512x/mpc512x_shared.c
> @@ -95,6 +95,7 @@ void __init mpc512x_init_i2c(void)
>  static struct of_device_id __initdata of_bus_ids[] = {
>        { .compatible = "fsl,mpc5121-immr", },
>        { .compatible = "fsl,mpc5121-localbus", },
> +       { .compatible = "fsl,mpc5121-dma", },

This doesn't look right either.  Shouldn't the dma device hang off the
IMMR node?
Yes dma is part of IMMR.


g.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.