Hi Wolfgang,
On Wednesday 13 May 2009 13:32:08 Wolfgang Denk wrote:
Not sure if this patch will be accepted. It's probably not so easy to understand the history of changes in this file with this coding style cleanup. But the current condition is so bad that I think it's worth the change.
Another argument against the patch is that it makes the code in some places more ugly...
-const static pio_config_t pio_config_ns [IDE_MAX_PIO_MODE+1] = -{
- /* Setup Length Hold */
- { 70, 165, 30 }, /* PIO-Mode 0, [ns] */
- { 50, 125, 20 }, /* PIO-Mode 1, [ns] */
- { 30, 101, 15 }, /* PIO-Mode 2, [ns] */
- { 30, 80, 10 }, /* PIO-Mode 3, [ns] */
- { 25, 70, 10 }, /* PIO-Mode 4, [ns] */
+const static pio_config_t pio_config_ns[IDE_MAX_PIO_MODE + 1] = {
- /* Setup Length Hold */
- {70, 165, 30}, /* PIO-Mode 0, [ns] */
- {50, 125, 20}, /* PIO-Mode 1, [ns] */
- {30, 101, 15}, /* PIO-Mode 2, [ns] */
- {30, 80, 10}, /* PIO-Mode 3, [ns] */
- {25, 70, 10}, /* PIO-Mode 4, [ns] */
};
Instead of nice vertically aligned columns the new code is less readable.
OK, I'll change this back to the original alignment.
-static void ide_ident (block_dev_desc_t *dev_desc); -static uchar ide_wait (int dev, ulong t); +static void ide_ident(block_dev_desc_t * dev_desc);
And I really hate the space after the '*' in lines like these,
I don't like then too. I'm wondering why indent didn't do this consistent.
especially as it's done more or lessa randomly:
-static void atapi_inquiry(block_dev_desc_t *dev_desc); -ulong atapi_read (int device, lbaint_t blknr, ulong blkcnt, void *buffer); +static void atapi_inquiry(block_dev_desc_t * dev_desc); +ulong atapi_read(int device, lbaint_t blknr, ulong blkcnt, void *buffer);
We have "* dev_desc", but "*buffer" ? That's just bogus.
- printf ("\nLoading from IDE device %d, partition %d: "
"Name: %.32s Type: %.32s\n",dev, part, info.name, info.type);
- printf("\nLoading from IDE device %d, partition %d: "
"Name: %.32s Type: %.32s\n", dev, part, info.name, info.type);Also, reordering the lines here makes the code more difficult to read.
- if (ide_dev_desc[dev].block_read (dev, info.start, 1, (ulong *)addr) !=
- { - printf ("** Read error on %d:%d\n", dev, part);
show_boot_progress (-48);
- if (ide_dev_desc[dev].block_read(dev, info.start, 1, (ulong *) addr) !=
- {
Why is there a space after the cast?
No idea. I'll try to remove all those spaces.
<snip>
+U_BOOT_CMD(ide, 5, 1, do_ide,
"IDE sub-system","reset - reset IDE controller\n""ide info - show available IDE devices\n""ide device [dev] - show or set current device\n""ide part [dev] - print partition table of one or all IDE devices\n""ide read addr blk# cnt\n""ide write addr blk# cnt - read/write `cnt'"" blocks starting at block `blk#'\n"" to/from memory address `addr'\n");+U_BOOT_CMD(diskboot, 3, 1, do_diskboot,
"boot from IDE device", "loadAddr dev:part\n");And indentation should be done by TAB - I see no reason to add more space here.
Sure. I'll fix it.
I agree that this file badly needs to be cleaned up, and I even agree to a patch like above - but it has to be done more intelligently. The resulting file should adhere to the Coding Style requirements, and be consistent in style.
OK, I'll try to fix all those issues and resubmit a new version.
Thanks.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================