[U-Boot-Users] PATCH] Add 8641reset commmand for mpc8641hpcn board

[Wolfgang Denk]

Dear Jon,

in message <1174588314.32390.41.camel at ld0161-tx32> you wrote:
>
> Does this look better/acceptable to you this way?

The implementation still lacks a lot.

> -	cmd = argv[1][1];
> +	cmd = argv[1][0];

Instead of switching on single characters, ignoring all the  rest  of
the strings, please use a string compare like done everywhere else in
U-Boot.

> +		if (argc < 5) {
> +			puts("\nUsage: 8641reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLLratio>\n");

How many different usage messages are there for this command? Please
be consistent, and implement this just once.

> +	case 'h':	/* help */

Note the 'h' case.

> +my_usage:
> +		puts("\nUsage: 8641reset cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>\n");
> +		puts("       8641reset altbank [cf <SYSCLK freq> <COREPLL ratio> <MPXPLL ratio>]\n");
> +		puts("       8641reset altbank [wd]\n");

The 'h' cxase is not documented.

> +		puts("For example:   8641reset cf 40 2.5 10\n");
> +		puts("See MPC8641HPCN Design Workbook for valid values of command line parameters.\n");

Please don't provide examples in the usage message. These  should  go
into the documentation, not the code.

> +U_BOOT_CMD(
> +	8641reset, CFG_MAXARGS, 1, mpc8641_reset_board,
> +	"8641reset 	- Reset the CPU to current bank or alternative bank\n",
> +	NULL
> +);

Now *this* is the real usage message. Please use this, and  drop  the
"my_usage" code.

Also, I object against the command name. It  should  follow  standard
name  rules  (enven  though  these  are  not strictly enforced by the
current command line parser), i.  e.  only  consist  of  alphanumeric
characters  and  underscores,  and  begin  with  an  underscore or an
alphabetic character (and I hereby discourage to use an underscore as
the first character).


> -	mpc8641_reset_board(cmdtp, flag, argc, argv);
> +	out8(PIXIS_BASE + PIXIS_RST, 0);

Some comment seems like a good idea to me...

> +5. 8641reset command
> +--------------------
> +A new command "8641reset" is introduced for reset mpc8641hpcn board to
> +current bank or alternative bank, with/without wathdog timer enabled,

Typo: s/th/tch/


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I like your game but we have to change the rules."