[U-Boot] [PATCH-V3 1/2] AM35x: Add support for AM3517EVM

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Monday, May 31, 2010 3:11 PM
> To: Hiremath, Vaibhav
> Cc: u-boot at lists.denx.de; tom at bumblecow.com; Paulraj, Sandeep; Premi,
> Sanjeev
> Subject: Re: [PATCH-V3 1/2] AM35x: Add support for AM3517EVM
> 
> Dear hvaibhav at ti.com,
> 
> In message <1273166585-26101-1-git-send-email-hvaibhav at ti.com> you wrote:
> > From: Vaibhav Hiremath <hvaibhav at ti.com>
> >
> > This patch adds basic support for the AM3517EVM.
> > It includes:
> > 	- Board int file (.c and .h)
> > 	- Default configuration file
> > 	- Updates for Makefile
> >
> > Changes from V2:
> > 	- Removed trailing spaces
> > 	- Updated MAINTAINERS & MAKEALL for am3517_evm
> 
> Such comments do not belong into the commit message. Please place thes
> ebelow the "---" line:
> 
[Hiremath, Vaibhav] Ok will incorporate in next post.

> > Signed-off-by: Vaibhav Hiremath <hvaibhav at ti.com>
> > Signed-off-by: Sanjeev Premi <premi at ti.com>
> > ---
> 
> ==> Comments should go here.
> 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5cbc845..0bc65e1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -798,6 +798,10 @@ Alex Z
> >  	lart		SA1100
> >  	dnp1110		SA1110
> >
> > +Vaibhav Hiremath <hvaibhav at ti.com>
> > +
> > +	am3517_evm	ARM CORTEX-A8 (AM35x SoC)
> > +
> 
> Please keep list sorted.
> 
[Hiremath, Vaibhav] How does this sorted, I could not see any relation between the entries there. 

> ...
> > diff --git a/arch/arm/include/asm/arch-omap3/mux.h
> b/arch/arm/include/asm/arch-omap3/mux.h
> > index 0c01c73..ffeb982 100644
> > --- a/arch/arm/include/asm/arch-omap3/mux.h
> > +++ b/arch/arm/include/asm/arch-omap3/mux.h
> ...
> > +/* AM3517 specific */
> > +#define CONTROL_PADCONF_CCDC_PCLK	0x01E4
> > +#define CONTROL_PADCONF_CCDC_FIELD	0x01E6
> 
> Board specific defoinitions should not be added to global header
> files. Please use a board specific header instead.
> 
[Hiremath, Vaibhav] I think this has been placed at the right place. This is mux definition and got added to mux.h file. You could see all mux definition are present in this file.

Do you see any issues with this?

> > --- /dev/null
> > +++ b/board/logicpd/am3517evm/am3517evm.c
> > @@ -0,0 +1,76 @@
> ...
> > +int board_init(void)
> > +{
> > +	DECLARE_GLOBAL_DATA_PTR;
> 
> This is bound to break. DECLARE_GLOBAL_DATA_PTR must always be used on
> file scope only; never use this on function scope.  
[Hiremath, Vaibhav] Agreed. Actually this code is derived form board/ti/evm/, so followed the same here.

I will change it in next post.

> Please check all
> your code.
> 
[Hiremath, Vaibhav] Do you see any other issues? I don't get this statement.

Thanks,
Vaibhav

> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> Genius doesn't work on an assembly line basis.  You can't simply say,
> "Today I will be brilliant."
> 	-- Kirk, "The Ultimate Computer", stardate 4731.3