Dear "miaofng",
In message <200910241217470153...@gmail.com> you wrote:
> From 1f6aaba856fbf484c442eb33cf220774d57fba8d Mon Sep 17 00:00:00 2001
> From: miaofng <miao...@gmail.com>
> Date: Fri, 23 Oct 2009 17:06:50 +0800
> Subject: [PATCH] [can] add u-boot sja1000/can support
>
> Signed-off-by: miaofng <miao...@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<miao...@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: w...@denx.de
365 Days of drinking Lo-Cal beer. = 1 Lite-year
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
