Wolfgang,
Thanks for your detailed reviews!
Once I get these last few style issues resolved, what more do I need to do to
get this merged? I don't really want to spam the list with more nearly
identical copies of these patches unless I'm sure that all the necessary review
items have been taken care of.
On Mar 15, 2011, at 15:36, Wolfgang Denk wrote:
> In message <1300208664-18339-5-git-send-email-kyle.d.moff...@boeing.com> you
> wrote:
>> The eXMeritus HWW-1U-1A unit is a DO-160-certified 13lb 1U chassis
>> with 3 independent TEMPEST zones. Two independent P2020 computers may
>> be found inside each zone. Complete hardware support is included.
>
> Please run checkpatch on your submissions!
I did run it on this patch, although I forgot to run it on the resurrected
reset patch (which is also now fixed). It's a common gripe on the LKML that
the tool is overzealous about certain warnings in cases where the "fix" makes
the code less readable.
Specifically:
> ...
>> + /* Ok, now go ahead and program all of those in one go */
>> + mpc85xx_gpio_set( gpio_high|gpio_low|gpio_in,
>> + gpio_high|gpio_low,
>> + gpio_high);
>
> ERROR: space prohibited after that open parenthesis '('
> #427: FILE: board/exmeritus/hww1u1a/hww1u1a.c:100:
> + mpc85xx_gpio_set( gpio_high|gpio_low|gpio_in,
I could "fix" this code to read:
mpc85xx_gpio_set(gpio_high|gpio_low|gpio_in,
gpio_high|gpio_low, gpio_high);
And it would be much harder to visually compare the three bitmask arguments
against each other.
>> + /*
>> + * If things have been taken out of reset early (for example, by one
>> + * of the BDI3000 debuggers), then we need to put them back in reset
>> + * and delay a while before we continue.
>> + */
>> +#define GPIO_RESETS (GPIO_DIMM_RESET|GPIO_USB_RESET|GPIO_GETH0_RESET)
>> + if (mpc85xx_gpio_get(GPIO_RESETS)) {
>
> Please don;t add #defines right in the middle of the code.
Fixed, thanks!
>> +/*
>> + * This little shell function just returns whether or not it's CPU A.
>> + * It can be used to select the right device-tree when booting, etc.
>> + */
>> +int do_hww1u1a_test_cpu_a(cmd_tbl_t *cmdtp, int flag, int argc, char *
>> const argv[])
>
> 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-character
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". This thread discusses it further:
http://lkml.org/lkml/2009/12/15/490
>> +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"
>
> 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.
>> + /* Now the serial# part of the hostname */
>> + for (j = 0; serialnr[j]; j++)
>> + if (isalnum(serialnr[j]))
>> + hww1u1a_prompt[i++] = tolower(serialnr[j]);
>
> Braces needed for multiline statements.
Fixed, thanks!
>> + /* 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.
>> +/* Enable the U-Boot "memory test" */
>> +#define CONFIG_SYS_MEMTEST_START 0x00000000
>> +#define CONFIG_SYS_MEMTEST_END 0x7fffffff
>
> I think this has not been tested, right?
I'm pretty sure it's been tested, but not very recently; the memory on all the
shipped units is ECC anyways.
Cheers,
Kyle Moffett
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
