On Mar 21, 2011, at 16:30, Wolfgang Denk wrote:
In message 5B9D9C87-C278-4AF3-B20C-26ECFF6C09B7@boeing.com you wrote:
WARNING: line over 80 characters #463: FILE: board/exmeritus/hww1u1a/hww1u1a.c:136: +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
This one is only a warning, and it's much more readable to have 1 84-charac
In U-Boot, it is considered to be an ERROR.
Just looking at the last ~200 commits (actually 187, because it ignores merges):
$ git format-patch -o recent-patches -200 origin/master $ ./checkpatch.pl --no-tree --strict recent-patches/* >checkpatch.log 2>&1 $ grep 'over 80 char' checkpatch.log | wc -l 130
That's 130 lines in the last 200 patches which are over 80 characters?!?! How are those patches any different from mine?
$ grep '^ERROR:' checkpatch.log | wc -l 113
And that's 113 HARD ERRORS from checkpatch!?!?!
Of those, 32 are "Missing Signed-off-by: line(s)", 20 are "macros with complex values should be enclosed in parenthesis", 19 are inconsistent or missing whitespace issues, 4 are "(foo*) instead of "(foo *)", 3 are invalid UTF-8 errors, 4 are "return is not a function" errors, etc, etc.
Look, I'm really trying to comply with U-Boot coding standards, but I'm really of pissed off about the inconsistent requirements you are applying to my patches versus a lot of other things that YOU ARE MERGING on a regular basis.
So why are you picking on my board-specific code so hard here? This is extremely frustrating for me and a strong deterrent against us *ever* contributing to U-Boot again in the future.
ter line than split it across 2 different lines. Even still, this warning is only issued if you pass "--subjective" to checkpatch, which is documented to "enable more subjective tests".
No, I get this warning without such flags. The command I run is just "checkpatch.pl --no-tree"
What version of checkpatch are you running? I copied version 0.31 out of my latest Linux kernel tree, which identical to the latest version from here:
http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/
If U-Boot policy is to run checkpatch then you'd better either specify a particular version and command-line options or be willing to accept the default output of the latest version.
BTW - could you please restrict your line length to some 70 characters or so? Thanks.
Sorry about that, sending email through an Exchange server is no fun :-(. Hopefully I've got it fixed.
+U_BOOT_CMD(
- hww1u1a_test_cpu_a, 1, 0, do_hww1u1a_test_cpu_a,
- "Test if this is CPU A (versus B) on the eXMeritus HWW-1U-1A board",
- /* */" && <command-if-true>\n"
- "hww1u1a_test_cpu_a || <command-if-false>\n"
=20 What is this empty comment needed for?
Just a mental placeholder for the fact that the U_BOOT_CMD macro inserts the name of the command in that spot. Will remove.
We don't provide usage examples in the help text. This should be fixed in the first place.
This *IS* the help text, and not a sample usage. This is visually and effectively no different from this text in common/cmd_mp.c:
U_BOOT_CMD( cpu, CONFIG_SYS_MAXARGS, 1, cpu_cmd, "Multiprocessor CPU boot manipulation and release", "<num> reset - Reset cpu <num>\n" "cpu <num> status - Status of cpu <num>\n" "cpu <num> disable - Disable cpu <num>\n" "cpu <num> release <addr> [args] - Release cpu <num> at <addr> with [args]" #ifdef CPU_ARCH_HELP "\n" CPU_ARCH_HELP #endif );
/* Turn on the "HRESET_REQ" pin (hard-reset request) */printf("\nRESET: Hardware reset triggered, waiting...\n");out_be32(&gur->rstcr, 0x2);while (1)udelay(10000);- }
Should that not be an infinite wait here?
At this point if the board does not reset due to hardware failure it's better off hanging than silently falling through.
Why don't you simply call "hang()" then?
Didn't know it existed at the time the code was written. Will fix.
Cheers, Kyle Moffett