Dear Stefan Roese,
In message 1242207497-21212-1-git-send-email-sr@denx.de you wrote:
While trying to fix a problem in the IDE detection of the CPCI750 I noticed the extreme bad coding style in the cmd_ide.c file. Before fixing the real problem I ran Lindent on this file. I also removed some "#if 0" parts.
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.
-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, 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) != 1) {
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) != 1) {
Why is there a space after the cast?
- switch (genimg_get_format ((void *)addr)) {
- switch (genimg_get_format((void *)addr)) {
But not so here?
case IMAGE_FORMAT_LEGACY:
hdr = (image_header_t *)addr;
hdr = (image_header_t *) addr;
But here again?
s = getenv("ide_doreset");if (s && strcmp(s, "on") == 0)+#endif
{/* Need to soft reset the device in case it's an ATAPI... */
Resulting line too long...
pio_mode = 0; /* Force it to dead slow, and hope for the best... */
pio_mode = 0; /* Force it to dead slow, and hope for the best... */
Ditto.
if (pwrsave) {c = ide_wait(device, IDE_SPIN_UP_TIME_OUT); /* may take up to 4 sec */
And again...
+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.
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.
Best regards,
Wolfgang Denk