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

[Jon Loeliger]

On Thu, 2007-03-22 at 18:17, Wolfgang Denk wrote:
> 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.

But the overall approach?  Is it better than the
previous version that hit common code?

> > -	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.

Will do.


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

[ ... ]

> > +		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.

Got it.  Will do.

> 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).

OK.  Will "reset8641" be acceptable?

> 
> > -	mpc8641_reset_board(cmdtp, flag, argc, argv);
> > +	out8(PIXIS_BASE + PIXIS_RST, 0);
> 
> Some comment seems like a good idea to me...

Hmmm. ok.

> > +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/

Right.

> 
> Best regards,
> Wolfgang Denk

Thanks for your review!

I'll respin the patch an check again before asking for a pull!

jdl