Dear "miaofng",
In message 200910241217470153391@gmail.com you wrote:
From 1f6aaba856fbf484c442eb33cf220774d57fba8d Mon Sep 17 00:00:00 2001 From: miaofng miaofng@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@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 miaofngmiaofng@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 itfree(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