On Saturday 24 October 2009 00:17:50 miaofng 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
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.hartk...@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<miao...@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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
