Dear Stefan,
In message <200906021112.05109...@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 <w...@denx.de>
> > Cc: John Rigby <jcri...@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 <w...@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: w...@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_
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
