From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Grandegger <wg@grandegger.com>
Date: Sun, 01 Nov 2009 17:24:59 +0100
Subject: [U-Boot] [RFC 2/5] CAN device test command
In-Reply-To: <200911010945.12254.vapier@gentoo.org>
References: <1257075217-26623-1-git-send-email-wg@grandegger.com>
	<1257075217-26623-2-git-send-email-wg@grandegger.com>
	<1257075217-26623-3-git-send-email-wg@grandegger.com>
	<200911010945.12254.vapier@gentoo.org>
Message-ID: <4AEDB65B.4080102@grandegger.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Mike Frysinger wrote:
> 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 ...`

Will fix.

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

can_init will already print an error message. But that might be changed.

> how do other commands work ?  am i complaining about common convention here ?
> 
>> +		printf ("Usage:\n%s\n", cmdtp->usage);
> 
> cmd_usage() ?

OK.

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

I think the output was OK. But I will check later next week.

Note that this is a RFC trying to discuss the real requirements of a CAN
interface in U-Boot. I think it would also be nice to have can_xmit()
and can_recv() with a timeout parameter, e.g.:

  can_xmit(struct can_dev *dev, int timeout_us);

And maybe also a can_xmit_done() function.

Wolfgang.