[U-Boot] [RFC 2/5] CAN device test command

[Mike Frysinger]

On Sunday 01 November 2009 06:33:34 Wolfgang Grandegger wrote:
> +	if (op == 's') {
> +	else if (op == 'i') {
> +	else if (op == 'r') {
> +	} else if (op == 'x') {
> +	} else {

your if style here is inconsistent, but ignoring that, shouldnt this really be 
a switch() ?  although, by only checking the first char, you allow people to 
encode typos into their commands and not realize it until some point in the 
future where things get stricter.  i.e. people can do `can ilovecandy ...`

> +		unsigned int dev_num = 0, ibaud = 0;
> +		struct can_dev *dev;
> +
> +		if (argc > 2)
> +			dev_num = simple_strtoul (argv[2], NULL, 10);
> +		if (argc > 3) {
> +			ibaud = simple_strtoul (argv[3], NULL, 10);
> +			if (ibaud > 2)
> +				ibaud = 2;
> +		}
> +		dev = can_init (dev_num, ibaud);
> +		if (!dev)
> +			return 1;
> +		can_dev = dev;

if i told CAN to init an unknown device, i would expect to get an error and 
the command state to remain in said error state until i selected a proper CAN 
device.  otherwise, a script that didnt check the can init status would 
incorrectly operate on the previously selected can device.

how do other commands work ?  am i complaining about common convention here ?

> +		printf ("Usage:\n%s\n", cmdtp->usage);

cmd_usage() ?

> +	can, 3,	1, do_can,
> +	"can - CAN bus commands\n",
> +	"can status [level]\n"
> +	"can init [dev] [baud-index]\n"
> +	"can xmit [id] [d0] [d1] ... [d7]\n"
> +	"can recv, abort with CTRL-C\n"

does the help really display correctly here ?  i think the "can status" line 
will have too many "can"'s ?
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091101/7d9b8d17/attachment.pgp