Dear Stefan,
In message 200906021112.05109.sr@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@denx.de Cc: John Rigby jcrigby@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@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