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