Wolfgang, thanks for your patient and prompt review, and sorry for having overlooked checkpatch.
Il 21/03/2011 12:55, Wolfgang Denk ha scritto: > Dear Luca Ceresoli, > > In message<1300707767-15682-2-git-send-email-luca.ceres...@comelit.it> you > wrote: >> Board support for the Comelit Group SpA CPS board, which is a custom board >> based on the BeagleBoard<http://beagleboard.org/> by Texas Instruments. >> >> The board support is based on the BeagleBoard implementation. > > It appears you base this code on an old version of U-Boot. Please > update your code base first. Strange. My patch is based on current master, on the latest commit that I see today on git://git.denx.de/u-boot.git: $ git show HEAD^ commit cc1dd33f273f8c96cbd7539b4a2d1d7aa12773cd Author: John Schmoller <jschmol...@xes-inc.com> Date: Thu Mar 10 16:09:26 2011 -0600 mpc8[5/6]xx: Ensure POST word does not get reset ... The "next" branch is much older, and I haven't found any more recent head in either u-boot-arm nor u-boot-ti. Where should I rebase my code on? > > ALso make sure to run your patch to checkpatch before submitting: > > [PATCH] ARMV7: OMAP3: Add support for Comelit CPS board > total: 4 errors, 325 warnings, 976 lines checked > > Please fix these! Fixed most of them for v2. There are still a few warnings, though, that I think are false positives: > WARNING: Use #include <linux/io.h> instead of <asm/io.h> > #169: FILE: board/comelit/cps/cps.c:39: > +#include <asm/io.h> I guess I can safely ignore this one. > WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt > #1011: FILE: include/configs/omap3_cps.h:305: > +extern volatile unsigned int boot_flash_env_addr; I don't know why it is volatile, but since it's defined this way in arch/arm/cpu/armv7/omap3/mem.c I think there's a reason. > >> --- /dev/null >> +++ b/board/comelit/cps/config.mk > ... >> +CONFIG_SYS_TEXT_BASE = 0x80008000 > > We don't accept such config.mk files any more. Please move definition > into your board config file. Meaning I should remove config.mk entirely, right? > >> + /* GPIO list >> + - 159 OUT (GPIO5+31): reset for remote camera interface connector. >> + - 19 OUT (GPIO1+19): integrated speaker amplifier (1=on, 0=shdn). >> + - 20 OUT (GPIO1+20): handset amplifier (1=on, 0=shdn). >> + */ > > Incorrect multiline comment style. Will fix in v2. > > ... >> --- a/boards.cfg >> +++ b/boards.cfg >> @@ -117,6 +117,7 @@ omap3_pandora arm armv7 >> pandora - >> igep0020 arm armv7 igep0020 >> isee omap3 >> igep0030 arm armv7 igep0030 >> isee omap3 >> am3517_evm arm armv7 am3517evm >> logicpd omap3 >> +omap3_cps arm armv7 cps >> comelit omap3 >> omap3_zoom1 arm armv7 zoom1 >> logicpd omap3 >> omap3_zoom2 arm armv7 zoom2 >> logicpd omap3 >> omap3_beagle arm armv7 beagle >> ti omap3 > > Please name your board just "cps" (or chose a better name). There > shall be no SoC-prefix in the board names. Will be called dig297 in v2. > >> --- /dev/null >> +++ b/include/configs/omap3_cps.h >> @@ -0,0 +1,315 @@ > ... >> +#define CONFIG_ARMV7 1 /* This is an ARM V7 CPU core */ >> +#define CONFIG_OMAP 1 /* in a TI OMAP core */ >> +#define CONFIG_OMAP34XX 1 /* which is a 34XX */ >> +#define CONFIG_OMAP3430 1 /* which is in a 3430 */ >> +#define CONFIG_OMAP3_CPS 1 /* working with CPS board */ > > #defines like these which select a feature shall not have values > assigned. Remove all these '1' here. Please fix globally. > Ok except for OMAP34XX, since arch/arm/cpu/armv7/start.S at line 114 does: > #if (CONFIG_OMAP34XX) Is it intended? If that's a mistake, I can submit a trivial patch to fix it. > Best regards, > > Wolfgang Denk > Luca _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot