[U-Boot] [PATCH v6 4/4] mpc85xx: Add board support for the eXMeritus HWW-1U-1A devices

[Moffett, Kyle D]

On Mar 21, 2011, at 16:30, Wolfgang Denk wrote:
> In message <5B9D9C87-C278-4AF3-B20C-26ECFF6C09B7 at 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