On 2/28/07, Txema Lopez tlopez@aotek.es wrote:
Grant Likely wrote:
On 2/27/07, Txema Lopez tlopez@aotek.es wrote:
Hi all, This patch, for the 1.2.0 version, adds support for the csb535fs board embedded in a csb935fs breakout board. The set is known as i.MX21 Litekit.
With a quick perusal, the patch looks mostly okay. Unfortunately, it's inconvenient to review because it was sent as a gzipped attachment instead of inline (I can't just hit 'reply' and start typing comments).
I'm sorry, the patch size is more than the 40k U-Boot's limit, so ...
... it then makes sense to break it up into a couple of patches. :-) shorter patches are easier to review anyway. Plus with smaller patches, changes that get 'acked' aren't necessarily held up by changes that need some more work.
Here are some general comments:
- You should add your copyright to all new files that you've added.
At the moment, your copyright only appears on a few of the new files.
The cpu/arm926ejs/imx21 files have been copied from the cpu/arm920t/imx with a few modifications, so in the files with minor changes I've left the old copyright.
Hmmm, indeed. There are large sections of identical, or near identical code between the two. If I replace all the IMX21_* macros with IMX_* in imx-regs.h, it results in a file that is somewhere around 75% identical.
I know that slightly modified duplicate code is common in u-boot, so this is not an critique on your work, but I'd really like to move away from this mode of operation. Duplicating the original file and modifying it is certainly the easiest way to add support for a new board/cpu, but I think it just leads to maintenance problems down the roads. For example, if one file has been duplicated with minor mods 10 times and a bug is found in it at a later date; the bug fix needs to be applied to 10 files, not one.
Unfortunately, this situation is messy because the imx is in cpu/arm920t and imx32 is in cpu/arm926ejs. It probably requires the creation of a new directory for the common imx soc bits, but where? Perhaps under lib_arm/imx?
- Where did include/asm-arm/arch-imx21/imx21-regs.h come from? There
is no copyright notice on it at all.
It is a modification of include/asm-arm/arch-imx/imx-regs.h
ok
- There's a fair bit of inconsistent whitespace (intermixed space and
tab characters).
Please, could you be more explicit and tell me where these mistakes are .
Configure your editor to make tab characters and trailing whitespace visually distinct and you'll see them.
- lowlevel_init.S has a dozen or so lines indented with 4 spaces instead of a tab character. The coding standard is for indentation with tab characters, and tab stops are at 8 character columns. - imx21-regs.h: inconsistent throughout; but you probably inherited this whitespace - serial.c has a few lines where spaces precede tabs; pretty minor stuff.
- I do wonder at the amount of boilerplate required for each new board
port (but that's a longer ranged questions directed at the whole of u-boot).
Me too.
:-)