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 message1300707767-15682-2-git-send-email-luca.ceresoli@comelit.it you wrote:
Board support for the Comelit Group SpA CPS board, which is a custom board based on the BeagleBoardhttp://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 jschmoller@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