[U-Boot] [RFC 1/5] CAN interface library

[Mike Frysinger <vapier@gentoo.org>]

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 <wg@denx.de>

have you really been working on this stuff since 2007 ?

> +struct can_dev {
> +	char *name;

const ?
-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/20091101/747d1ff9/attachment.pgp