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

[Mike Frysinger <vapier@gentoo.org>]

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 miaofng<miaofng@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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20091024/a0aa315c/attachment.pgp