[U-Boot] [RFC 1/5] CAN interface library

[Wolfgang Grandegger]

Mike Frysinger wrote:
> On Sunday 01 November 2009 06:33:33 Wolfgang Grandegger wrote:
[snip]
>> --- /dev/null
>> +++ b/drivers/can/can.c
>>
>> +static char *baudrates[] = { "125K", "250K", "500K" };
> 
> so we're restricting ourselves to these three speeds ?  i have passing 
> familiarity with CAN, but i didnt think the protocol was restricted to 
> specific speeds.

Well, this is an RFC and as I wrote in the introduction some features
need to be added or extended, especially for CAN device configuration.
My idea is to have a more complete default bit-timing table, which board
specific code may overwrite using, for example:

   sja1000_register(&my_sja1000, &my_config_opts);

This would also allow to set the CAN clock, cdr and ocr registers.

>> +int can_register (struct can_dev* can_dev)
> 
> no space before the paren, and the * is cuddled on the wrong side of the 
> space.  seems like a lot of this code suffers from these two issues.

U-Boot coding style requires a space after the function name and before
"(". But the "*" is misplaced, of course.

>> +{
>> +	struct can_dev* dev;
>> +
>> +	can_dev->next = NULL;
>> +	if (!can_devs)
>> +		can_devs = can_dev;
>> +	else {
>> +		for (dev = can_devs; dev->next; dev = dev->next)
>> +			    ;
>> +		dev->next = can_dev;
>> +	}
> 
> invert the if logic and i think the code would look "nicer" -- use braces on 
> the first branch instead of the second.

OK.

>> +struct can_dev *can_init (int dev_num, int ibaud)
>> +{
>> +	struct can_dev *dev;
>> +	int i;
>> +
>> +	if (!can_devs) {
>> +		puts ("No CAN devices registered\n");
>> +		return NULL;
>> +	}
>> +
>> +	/* Advance to selected device */
>> +	for (i = 0, dev = can_devs; dev; i++, dev = dev->next) {
>> +		if (i == dev_num)
>> +			break;
>> +	}
>> +
>> +	if (!dev) {
>> +		printf ("CAN device %d does not exist\n", dev_num);
>> +		return NULL;
>> +	}
>> +
>> +	printf ("Initializing CAN%d at 0x%08x with baudrate %s\n",
>> +		i, dev->base, baudrates[ibaud]);
>> +
>> +	dev->init (dev, ibaud);
>> +
>> +	return dev;
>> +}
> 
> wonder if we should have a generic device list code base since this looks 
> similar to a lot of other u-boot device lists ...

Do we already have a generic interface?

>> --- /dev/null
>> +++ b/include/can.h
>> @@ -0,0 +1,70 @@
>> +/*
>> + * (C) Copyright 2007-2009, Wolfgang Grandegger <wg at denx.de>
> 
> have you really been working on this stuff since 2007 ?

The code was written in 2007. "2007, 2009" is more appropriate.

>> +struct can_dev {
>> +	char *name;
> 
> const ?

OK.

Wolfgang.