Hi Mike,
Thanks for your careful review. I'll resend the patch according to your remarks except below: 1)
+int register_candev(struct can_device *dev)
use "can" or "candev", dont mix the two
answer: register_candev is origin from register_netdev(...). CAN is something like network, it may has high level protocols such as open-can and etc, so i want to keep that story.
2)
+int sja1000_interrupt(struct can_device *dev)
u-boot is a polled architecture, not an interrupt based one. i guess this driver needs rewriting to do that.
answer: no, i want to keep that structure. Then linux drivers can be ported to u-boot easily. take an example, in sja1000, i only need to replace "_net" to "_can" at most of time. To gain this convience, i added the wrap layer of can_core.c. In order to work in u-boot polled mode, people only need to call sja1000_interrupt periodly. It works fine on my board.
On Saturday 24 October 2009 00:17:50 miaofng 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
this should be split into two patches -- one for the core CAN code and one for the sja1000-specific implementation. it's also lacking documentation updates to the top level README or perhaps a standalone doc/README.can file unconditionally calling can_init() is broken (in lib_arm/board.c). can is like any other driver and can be enabled/disabled. it needs a dedicated CONFIG_CAN option which would be leveraged in drviers/can/Makefile to add can_core.c to the build.
--- /dev/null +++ b/drivers/can/can_core.c +#define can_debug printf +#define can_error printf
uhh, what ? we already have debug() macros. dont go creating your own brand new stuff.
+#ifdef CONFIG_CMD_CAN
CONFIG_CMD_xxx is reserved for actual commands as in U_BOOT_CMD. you arent implementing that, so this is wrong. besides, it isnt needed once you use a proper CONFIG_CAN and add it to the top level Makefile.
+void canif_rx(struct can_frame *cf, struct can_device *dev) +{
- int len;
- //error frame?
C++ style comments are not allowed. use normal C /* comments */.
- if(cf->can_id&CAN_ERR_FLAG)
- {
you need to read the style documentation. u-boot code follows the linux kernel. that means this should have been: if (cf->can_id & CAN_ERR_FLAG) { all of your code is horribly broken in these regards
+int can_init(void) +{ +#ifdef CONFIG_CMD_CAN
- int index;
- for(index = 0; index < CONFIG_CAN_DEV_MAX; index ++) can_devs[index] = 0;
use memset() like god intended. then again, can_devs is declared static and in the .bss, so this init step is unnecessary.
- board_can_init();
+#endif
- return 0;
+}
these need to be weak's like all the other subsystems do it. look at net/eth.c and how it does it: - can_initialize() - weak board_can_init() - weak cpu_can_init()
+int register_candev(struct can_device *dev)
use "can" or "candev", dont mix the two
--- /dev/null +++ b/drivers/can/sja1000.c +//MODULE_AUTHOR("Oliver Hartkopp oliver.hartkopp@volkswagen.de"); +//MODULE_LICENSE("Dual BSD/GPL"); +//MODULE_DESCRIPTION(DRV_NAME "CAN netdevice driver");
dont leave crap behind -- delete it
+int sja1000_interrupt(struct can_device *dev)
u-boot is a polled architecture, not an interrupt based one. i guess this driver needs rewriting to do that.
+int register_sja1000dev(struct can_device *dev)
can drivers should have a standard naming convention. perhaps something like: can_<driver>_register()
+void unregister_sja1000dev(struct can_device *dev) +{
- set_reset_mode(dev);
- unregister_candev(dev);
+}
do we really need an unregister framework ? none of the other subsystems do and it hasnt been a problem.
--- /dev/null +++ b/include/candev.h @@ -0,0 +1,46 @@ +/*
- (C) Copyright 2009 miaofngmiaofng@gmail.com
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- 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
- */
+/*
- netdev.h - definitions an prototypes for network devices
- */
+#ifndef _CANDEV_H_ +#define _CANDEV_H_
clearly you've been copying & pasting code from the Linux kernel, yet you've been stripping out people's copyrights. that wont fly. -mike