From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wf-out-1314.google.com (wf-out-1314.google.com [209.85.200.171]) by ozlabs.org (Postfix) with ESMTP id 9214EDDF7A for ; Fri, 8 May 2009 12:49:24 +1000 (EST) Received: by wf-out-1314.google.com with SMTP id 24so935487wfg.15 for ; Thu, 07 May 2009 19:49:23 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1241640919-4650-1-git-send-email-wd@denx.de> <1241640919-4650-12-git-send-email-wd@denx.de> Date: Thu, 7 May 2009 20:49:23 -0600 Message-ID: <4b73d43f0905071949r5b8f16f8n18d70771720cfb15@mail.gmail.com> Subject: Re: [PATCH 11/12] mpc5121: Added MPC512x DMA driver. From: John Rigby To: Grant Likely Content-Type: multipart/alternative; boundary=001636ed643113312804695dadd5 Cc: linuxppc-dev@ozlabs.org, Wolfgang Denk , Piotr Ziecik List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , --001636ed643113312804695dadd5 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit On Wed, May 6, 2009 at 3:07 PM, Grant Likely wrote: > On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk wrote: > > From: Piotr Ziecik > > > > This patch adds initial version of MPC512x DMA driver. > > Only memory to memory transfers are currenly supported. > > > > Signed-off-by: Piotr Ziecik > > Signed-off-by: Wolfgang Denk > > Cc: Grant Likely > > Cc: John Rigby > > 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. > --001636ed643113312804695dadd5 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Wed, May 6, 2009 at 3:07 PM, Grant Li= kely <gra= nt.likely@secretlab.ca> wrote:
On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> From: Piotr Ziecik <kosmo@sem= ihalf.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 <gran= t.likely@secretlab.ca>
> Cc: John Rigby <jcrigby@gmail.= com>

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

> =A0drivers/dma/mpc512x_dma.c =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| = =A0642 ++++++++++++++++++++++++++
> =A0drivers/dma/mpc512x_dma.h =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0| = =A0192 ++++++++

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 @@
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0};
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dma@14000 {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl= ,mpc5121-dma2";
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 compatible =3D "fsl= ,mpc512x-dma";

Nack. =A0Compatible values should not use wildcards. =A0Be specific. = =A0And
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.=A0 There is a dma2 in some 8= xxx but last I looked it is not at all the same.=A0 I would vote for fsl,mp= c5121-dma.


> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0reg =3D <0x14000 0x1= 800>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupts =3D <65 0= x8>;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0interrupt-parent =3D &l= t; &ipic >;
> diff --git a/arch/powerpc/platforms/512x/mpc512x_shared.c b/arch/power= pc/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)
> =A0static struct of_device_id __initdata of_bus_ids[] =3D {
> =A0 =A0 =A0 =A0{ .compatible =3D "fsl,mpc5121-immr", },
> =A0 =A0 =A0 =A0{ .compatible =3D "fsl,mpc5121-localbus", },<= br> > + =A0 =A0 =A0 { .compatible =3D "fsl,mpc5121-dma", },

This doesn't look right either. =A0Shouldn't the dma device h= ang off the
IMMR node?
Yes dma is part of IMMR.


g.

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

--001636ed643113312804695dadd5--