[U-Boot-Users] Updated mpc86xx U-Boot Tree

[Wolfgang Denk]

Dear Jon,

in message <E1GQ3Bk-00065e-D6 at 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

-- 
Software Engineering:  Embedded and Realtime Systems,  Embedded Linux
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
 The software required `Windows 95 or better', so I installed Linux.