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

[miaofng]

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 at 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 at 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 miaofng<miaofng at 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