[U-Boot] [PATCH v2 10/10] MPC512x: add support for ARIA board

[Wolfgang Denk]

Dear Stefan,

In message <200906021112.05109.sr at denx.de> you wrote:
> 
> On Saturday 16 May 2009 10:47:46 Wolfgang Denk wrote:
> > ARIA is a MPC5121E based COM Express module by Dave/DENX.
> >
> > Signed-off-by: Wolfgang Denk <wd at denx.de>
> > Cc: John Rigby <jcrigby at gmail.com>
> 
> Please find some mostly nitpicking comments below. (Sorry about the late 
> review - I just stumbled over a few issue while using this port as basis for a 
> port for an MPC5123 board from esd).

Thanks for the review.

> <snip>
> 
> > diff --git a/board/davedenx/aria/Makefile b/board/davedenx/aria/Makefile
> > new file mode 100644
> > index 0000000..48c2a83
> > --- /dev/null
> > +++ b/board/davedenx/aria/Makefile
> > @@ -0,0 +1,53 @@
> > +#
> > +# (C) Copyright 2009 Wolfgang Denk <wd at denx.de>
> > +#
> > +# See file CREDITS for list of people who contributed to this
> > +# project.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation; either version 2 of
> > +# the License, or (at your option) any later version.
> > +#
> > +# This program is distributed in the hope that it will be useful,
> > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +# GNU General Public License for more details.
> > +#
> > +# You should have received a copy of the GNU General Public License
> > +# along with this program; if not, write to the Free Software
> > +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> > +# MA 02111-1307 USA
> > +#
> > +
> > +include $(TOPDIR)/config.mk
> > +
> > +$(shell mkdir -p $(OBJTREE)/board/freescale/common)
> 
> Is this really needed?

Yes, it is - but not here. It must be added to cpu/mpc512x/Makefile,
i. e. go into the "mpc512x: Move common files to share them by several
boards" commit.

> > +$(LIB):	$(obj).depend $(OBJS)
> > +
> > +	$(AR) $(ARFLAGS) $@ $(OBJS)
> 
> Please remove this empty line above.

Will do.

> > diff --git a/board/davedenx/aria/aria.c b/board/davedenx/aria/aria.c
> > new file mode 100644
> > index 0000000..4d26713
> > --- /dev/null
> > +++ b/board/davedenx/aria/aria.c
...
> > +	if (SVR_MJREV(spridr) >= 2) {
> > +		out_be32(&im->lpc.altr, CONFIG_SYS_CS_ALETIMING);
> > +	}
> 
> Curly braces can be removed. And I suggest to add an empty line here.

Will do.

...
> > +int misc_init_r(void)
> > +{
> > +	u32 tmp;
> > +	extern int mpc5121_diu_init(void);
> 
> Please move prototype declaration to top of file or to some header.

Moved to top of file as I did't find a good header.

> > +#ifdef CONFIG_FSL_DIU_FB
> > +#if	!(defined(CONFIG_VIDEO) || defined(CONFIG_CFB_CONSOLE))
> > +	mpc5121_diu_init();
> > +#endif
> > +#endif
> > +
> > +	return 0;
> > +}
> 
> Insert empty line here.

Done.

...
> > +int checkboard (void)
> > +{
> > +	puts("Board: ARIA\n");
> > +
> > +	/* initialize function mux & slew rate IO inter alia on IO Pins  */
> > +
> > +	iopin_initialize(ioregs_init,
> > +			 sizeof(ioregs_init) / sizeof(ioregs_init[0]));
> 
> Please use ARRAY_SIZE(ioregs_init) here.

Done.

Same changes applied to board/freescale/mpc5121ads/mpc5121ads.c as
well.


As the modifications are mostly cosmetical only, I will push the
changes directly into the u-boot-mpc5xxx repo, without posting new
patches. Hope this is OK.

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
They're usually so busy thinking about what  happens  next  that  the
only  time they ever find out what is happening now is when they come
to look back on it.                 - Terry Pratchett, _Wyrd Sisters_