[U-Boot] [PATCH] add u-boot sja1000/can support

[Wolfgang Denk]

Dear "miaofng",

In message <200910241217470153391 at gmail.com> you wrote:
> From 1f6aaba856fbf484c442eb33cf220774d57fba8d Mon Sep 17 00:00:00 2001
> From: miaofng <miaofng at gmail.com>
> Date: Fri, 23 Oct 2009 17:06:50 +0800
> Subject: [PATCH] [can] add u-boot sja1000/can support
> 
> Signed-off-by: miaofng <miaofng at gmail.com>

Please provide a real name here and in all Copyright entries.

...
> diff --git a/drivers/can/can_core.c b/drivers/can/can_core.c
> new file mode 100755
> index 0000000..a723e8a
> --- /dev/null
> +++ b/drivers/can/can_core.c
> @@ -0,0 +1,266 @@
> +/*
> + * Copyright (C) 2009 miaofng<miaofng at gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the version 2 of the GNU General Public License
> + * as published by the Free Software Foundation
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <common.h>
> +#include <can.h>
> +#include <malloc.h>
> +
> +#define can_debug printf

Why not
	#define can_debug debug
?

And why not use plain debug() in the first place?

> +#define can_error printf

What is this good for? I don't see "can_error" used anywhere?


> +#ifdef CONFIG_CMD_CAN
> +#ifndef CONFIG_CAN_DEV_MAX
> +#define CONFIG_CAN_DEV_MAX	8
> +#endif
> +
> +static struct can_device *can_devs[CONFIG_CAN_DEV_MAX];
> +#endif
> +
> +static inline struct can_device* index_to_cdev(int index)
> +{
> +	return (index<CONFIG_CAN_DEV_MAX)? can_devs[index]:0;
> +}
> +
> +static inline int cdev_to_index(struct can_device *dev)
> +{
> +	return (dev)?dev->index:-1;
> +}
> +
> +static void canif_tx(struct can_device *dev)
> +{
> +	struct can_frame *cf;
> +	int len;
> +
> +	len = buf_pop(&dev->tbuf, (char*)&cf, sizeof(cf));
> +	if(len == sizeof(cf)) {
> +		dev->start_xmit(cf, dev);
> +		free(cf);
> +	}
> +}

This look like Linux code to me which is somewhat out of place in a
U-Boot context?

> +void canif_rx(struct can_frame *cf, struct can_device *dev)
> +{
> +	int len;
> +
> +	//error frame?

Please do not use any C++ comments. [Fix globally]

> +	if(cf->can_id&CAN_ERR_FLAG)
> +	{

Incorrect brace style. Please fix globally.

> +		//error frame, drop it
> +		free(cf);
> +		return;
> +	}
> +
> +	//soft id filter?
> +
> +	//store to cirbuf
> +	if(dev->rbuf.size == dev->rbuf.totalsize) {
> +		can_debug("CAN: rx buffer is full\n");
> +		dev->stats.rx_dropped ++;
> +		free(cf);
> +		return;
> +	}
> +
> +	buf_push(&dev->rbuf, (char*)&cf, sizeof(cf));
> +}
> +
> +void canif_wake_queue(struct can_device *dev)
> +{
> +	canif_start_queue(dev);
> +	canif_tx(dev);
> +}

Are you sure we could need this in U-Boot? Keep in mind that U-Bootis
strictly single-tasking.

> +int can_init(void)
> +{
> +#ifdef CONFIG_CMD_CAN
> +	int index;
> +	for(index = 0; index < CONFIG_CAN_DEV_MAX; index ++) can_devs[index] = 0;

Incorrect indentation.

> +	board_can_init();
> +#endif
> +	return 0;
> +}

No can_init() should be performed at all (and thus not compiled
either) if CONFIG_CMD_CAN is not defined.

> +/*
> + * Register the CAN network device
> + */
> + #ifndef CONFIG_CAN_BUF_TX_SZ
> + #define CONFIG_CAN_BUF_TX_SZ (8*sizeof(void*))
> + #endif
> +
> + #ifndef CONFIG_CAN_BUF_RX_SZ
> + #define CONFIG_CAN_BUF_RX_SZ (8*sizeof(void*))
> + #endif

Please don't do this right in the middle of a source file. Move to
header.

> +int register_candev(struct can_device *dev)
> +{
> +	int ret;
> +	int index;
> +
> +	//insert into the list
> +	dev->index = -1;
> +	for(index =0; index < CONFIG_CAN_DEV_MAX; index ++)
> +	{
> +		if(!can_devs[index]) {
> +			can_devs[index] = dev;
> +			dev->index = index;
> +			break;
> +		}
> +	}
> +	if( dev->index < 0)
> +		can_debug("CAN: too many can devices\n");
> +
> +	//allocate circbuf for tx and rx
> +	ret = buf_init(&dev->tbuf, CONFIG_CAN_BUF_TX_SZ);
> +	if(ret != 1) {
> +		can_debug("CAN: cann't init cirbuf of tx\n");
> +		can_devs[index] = 0;
> +		return -1;
> +	}
> +	ret = buf_init(&dev->rbuf, CONFIG_CAN_BUF_RX_SZ);
> +	if(ret != 1) {
> +		can_debug("CAN: cann't init cirbuf of rx\n");
> +		can_devs[index] = 0;
> +		buf_free(&dev->tbuf);
> +		return -1;
> +	}
> +	dev->qstatus = CAN_QUEUE_INIT;
> +
> +	//success
> +	printf("CAN:   %s @index %d\n", dev->name, dev->index);
> +	return 0;
> +}

Looks like overkill to me, and doesn;t seem to fit into U-Boot design
rules. Please see http://www.denx.de/wiki/U-Boot/DesignPrinciples

> +/*
> + * Unregister the CAN network device
> + */
> +void unregister_candev(struct can_device *dev)
> +{
> +	int index = cdev_to_index(dev);
> +
> +	if(index < 0) {
> +		can_debug("CAN: invalid para\n");
> +		return;
> +	}
> +
> +	//close the dev first
> +	can_close((int)dev);
> +
> +	//release tx/rx buffer
> +	buf_free(&dev->tbuf);
> +	buf_free(&dev->rbuf);
> +
> +	//remove from the list
> +	can_devs[index] = 0;
> +}

Ditto.

> +//api
> +int can_setbrg(int brg, int index)
> +{
> +	struct can_device *dev = index_to_cdev(index);
> +
> +	if(!dev) {
> +		can_debug("CAN: invalid para\n");
> +		return -1;
> +	}
> +
> +	if( dev->state != CAN_STATE_STOPPED) {
> +		can_debug("CAN: invalid dev status<%d>\n", dev->state);
> +		return -1;
> +	}
> +
> +	if(dev->setbrg)
> +		return dev->setbrg(brg, dev);
> +
> +	can_debug("CAN: op not supported by the dev\n");
> +	return -1;
> +}

These error messages should be that, i. e. error messages, no debug.
And they could be a bit more self-explanatory. "op not supported" -
what's an "op"? Ans which "op" is not supported? Etc.

> +int can_open(int index)
> +{
> +	struct can_device *dev = index_to_cdev(index);
> +
> +	if(!dev) {
> +		can_debug("CAN: invalid para\n");
> +		return 0;
> +	}
> +
> +	if(dev->state != CAN_STATE_STOPPED) {
> +		can_debug("CAN: invalid dev status<%d>\n", dev->state);
> +		return 0;
> +	}
> +
> +	if(dev && dev->open) dev->open(dev);
> +	return (int)dev;
> +}
> +
> +void can_close(int dev_id)
> +{
> +	struct can_device *dev = (struct can_device*) dev_id;
> +	struct can_frame *cf;
> +
> +	if(!dev) {
> +		can_debug("CAN: invalid para\n");
> +		return;
> +	}
> +
> +	//can device close
> +	if(dev->close) dev->close(dev);
> +
> +	//tbuf,rbuf dump
> +	while(buf_pop(&dev->tbuf, (char*)&cf, sizeof(cf)) ) free(cf);
> +	while(buf_pop(&dev->rbuf, (char*)&cf, sizeof(cf)) ) free(cf);
> +}

Do we need this? In a single-threaded contxt?

> +//non block write
> +int can_send(struct can_frame *cf, int dev_id)
> +{
...
> +//non block read
> +struct can_frame* can_recv(int dev_id)
> +{
...

Please explain how you think this fits into U-Boot context, which is
single-threaded by design.

> diff --git a/include/candev.h b/include/candev.h
> new file mode 100755
> index 0000000..0e0dee7
> --- /dev/null
> +++ b/include/candev.h
...
> +/*
> + * netdev.h - definitions an prototypes for network devices
> + */

Inappropriate comment, it seems?

> diff --git a/lib_arm/board.c b/lib_arm/board.c
> old mode 100644
> new mode 100755
> index 5e3d7f6..aa11dd3
> --- a/lib_arm/board.c
> +++ b/lib_arm/board.c
> @@ -356,6 +356,8 @@ void start_armboot (void)
>  	serial_initialize();
>  #endif
>  
> +	can_init();
> +
>  	/* IP Address */
>  	gd->bd->bi_ip_addr = getenv_IPaddr ("ipaddr");

Such a change would have to go into a separate patch.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
365 Days of drinking Lo-Cal beer.                       = 1 Lite-year