Dear Jon,
in message E1GQ3Bk-00065e-D6@jdl.com you wrote:
I have updated the u-boot-86xx tree on jdl.com to have the recent modifications for the build system introduced in the denx.de mainline.
http://www.jdl.com/git_repos/
I had a look at your U-Boot tree.
I think you need to rework it to make it usable.
First: there is not a single CHANGELOG entry for any of the many commits you checked in!!!
Second: in the git log I see things like "Conflicts: board/stxxtc/Makefile" - what does that mean? Have such conflicts been sorted out? Are they still present in the tree? Have the board maintainers been contacted?
Third: there are many coding style violations like trailing white space in doc/README.mpc8641hpcn, drivers/tsec.c, include/asm-ppc/immap_86xx.h, net/bootp.c; bad indentation (spaces instead of TABs) in cpu/mpc86xx/start.S, drivers/tsec.c, include/asm-ppc/immap_86xx.h; excessive empty lines in include/configs/MPC8641HPCN.h; trailing empty lines in board/cds/common/via.c, board/mpc8641hpcn/init.S, cpu/mpc86xx/start.S; bad brace style (missing space between function name and '(' nearly everywhere); bad indentation (not by multiple of 8 chars = using TABs in many files).
Some code is so ugly that I'm not going to accept it. For example "cpu/mpc86xx/i2c.c":
195 if (i2c_wait4bus() < 0) 196 goto exit; 197 198 if (i2c_write_addr(dev, I2C_WRITE, 0) == 0) 199 goto exit; 200 201 if (__i2c_write(&a[4 - alen], alen) != alen) 202 goto exit; 203 204 if (i2c_write_addr(dev, I2C_READ, 1) == 0) 205 goto exit; 206 207 i = __i2c_read(data, length); 208 209 exit: 210 writeb(MPC86xx_I2CCR_MEN, I2CCCR); 211 212 return !(i == length);
Grrrgh...
Also, I don't want to see big blocks of code with local variable declarations (without need) like here:
"cpu/mpc86xx/spd_sdram.c":
799 { 800 unsigned int refresh_clk; 801 unsigned int refresh_time_ns[8] = { ... 824 }
Either declare variables at function entry, or move the code into a separate function, but don't add such blocks of declarations + code.
For long #ifdef / #endif sequences I want to see a comment with the #endif which makes clear which #ifdef it closes, especially for long and/or nested constructs.
Also, please don't write code like this:
"cpu/mpc86xx/spd_sdram.c":
1260 if (!ddr1_enabled && !ddr2_enabled) 1261 return 0; 1262 else { 1263 printf("Non-interleaved"); 1264 return memsize_total * 1024 * 1024; 1265 }
If the "else" needs parens, then parens shall be used for the "if", too.
And here the "else" is just bogus because of the previous "return 0". It just adds unnecessary indentation and makes the code hard to read.
Please add short comments at the end of the lines, i. e. avoid code like this:
1300 /* 8K */ 1301 dma_xfer((uint *)0x2000, 0x2000, (uint *)0); 1302 /* 16K */ 1303 dma_xfer((uint *)0x4000, 0x4000, (uint *)0); 1304 /* 32K */ 1305 dma_xfer((uint *)0x8000, 0x8000, (uint *)0); 1306 /* 64K */ 1307 dma_xfer((uint *)0x10000, 0x10000, (uint *)0); 1308 /* 128k */ 1309 dma_xfer((uint *)0x20000, 0x20000, (uint *)0); 1310 /* 256k */ 1311 dma_xfer((uint *)0x40000, 0x40000, (uint *)0); 1312 /* 512k */ 1313 dma_xfer((uint *)0x80000, 0x80000, (uint *)0); 1314 /* 1M */ 1315 dma_xfer((uint *)0x100000, 0x100000, (uint *)0); 1316 /* 2M */ 1317 dma_xfer((uint *)0x200000, 0x200000, (uint *)0); 1318 /* 4M */ 1319 dma_xfer((uint *)0x400000, 0x400000, (uint *)0);
This is ugly and difficult to read. Reformat for example like this:
dma_xfer((uint *)0x002000, 0x002000, (uint *)0); /* 8K */ dma_xfer((uint *)0x004000, 0x004000, (uint *)0); /* 16K */ dma_xfer((uint *)0x008000, 0x008000, (uint *)0); /* 32K */ dma_xfer((uint *)0x010000, 0x010000, (uint *)0); /* 64K */ dma_xfer((uint *)0x020000, 0x020000, (uint *)0); /* 128k */ dma_xfer((uint *)0x040000, 0x040000, (uint *)0); /* 256k */ dma_xfer((uint *)0x080000, 0x080000, (uint *)0); /* 512k */ dma_xfer((uint *)0x100000, 0x100000, (uint *)0); /* 1M */ dma_xfer((uint *)0x200000, 0x200000, (uint *)0); /* 2M */ dma_xfer((uint *)0x400000, 0x400000, (uint *)0); /* 4M */
etc. etc.
Note that your current repo is so wrecked that I cannot pull from it, even if you add fixes for the aforementioned problems on top of it. Instead, the fixes should to be implemented on a patch by patch base. If possible I would like to be able to pull from a (new) repo, but of course I will accept standard patches through the mailing list as well.
Best regards,
Wolfgang Denk