On Sunday 01 November 2009 06:33:33 Wolfgang Grandegger wrote: > --- a/Makefile > +++ b/Makefile > @@ -203,6 +203,7 @@ LIBS += net/libnet.a > LIBS += disk/libdisk.a > LIBS += drivers/bios_emulator/libatibiosemu.a > LIBS += drivers/block/libblock.a > +LIBS += drivers/can/libcan.a
this isnt an issue specific to CAN, but i wonder if we should switch LIBS to
LIBS-y now that the top level Makefile can rely on autoconf.mk settings after
the point config.mk is included. it would save time on pointlessly recursing
into all the empty dirs and creating empty archives.
> --- /dev/null
> +++ b/drivers/can/Makefile
> @@ -0,0 +1,47 @@
> +include $(TOPDIR)/config.mk
> +
> +LIB := $(obj)libcan.a
> +
> +COBJS-$(CONFIG_CAN) += can.o
> +
> +COBJS := $(COBJS-y)
> +SRCS := $(COBJS:.o=.c)
> +OBJS := $(addprefix $(obj),$(COBJS))
> +
> +all: $(LIB)
> +
> +$(LIB): $(obj).depend $(OBJS)
> + $(AR) $(ARFLAGS) $@ $(OBJS)
> +
> +
> +#####################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +####################################################
also not specific to CAN, but i think it's time we start creating .mk files
for subdirs to include
> --- /dev/null
> +++ b/drivers/can/can.c
>
> +static char *baudrates[] = { "125K", "250K", "500K" };
so we're restricting ourselves to these three speeds ? i have passing
familiarity with CAN, but i didnt think the protocol was restricted to
specific speeds.
> +int can_register (struct can_dev* can_dev)
no space before the paren, and the * is cuddled on the wrong side of the
space. seems like a lot of this code suffers from these two issues.
> +{
> + struct can_dev* dev;
> +
> + can_dev->next = NULL;
> + if (!can_devs)
> + can_devs = can_dev;
> + else {
> + for (dev = can_devs; dev->next; dev = dev->next)
> + ;
> + dev->next = can_dev;
> + }
invert the if logic and i think the code would look "nicer" -- use braces on
the first branch instead of the second.
> +struct can_dev *can_init (int dev_num, int ibaud)
> +{
> + struct can_dev *dev;
> + int i;
> +
> + if (!can_devs) {
> + puts ("No CAN devices registered\n");
> + return NULL;
> + }
> +
> + /* Advance to selected device */
> + for (i = 0, dev = can_devs; dev; i++, dev = dev->next) {
> + if (i == dev_num)
> + break;
> + }
> +
> + if (!dev) {
> + printf ("CAN device %d does not exist\n", dev_num);
> + return NULL;
> + }
> +
> + printf ("Initializing CAN%d at 0x%08x with baudrate %s\n",
> + i, dev->base, baudrates[ibaud]);
> +
> + dev->init (dev, ibaud);
> +
> + return dev;
> +}
wonder if we should have a generic device list code base since this looks
similar to a lot of other u-boot device lists ...
> --- /dev/null
> +++ b/include/can.h
> @@ -0,0 +1,70 @@
> +/*
> + * (C) Copyright 2007-2009, Wolfgang Grandegger <w...@denx.de>
have you really been working on this stuff since 2007 ?
> +struct can_dev {
> + char *name;
const ?
-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
