[U-Boot] [PATCH 1/2] net: add Calxeda xgmac driver

[Mike Frysinger <vapier@gentoo.org>]

On Friday 02 December 2011 15:21:48 Rob Herring wrote:
> --- /dev/null
> +++ b/drivers/net/calxedaxgmac.c
>
> +	writel(value, dev->iobase + XGMAC_CORE_CONFIG);

you should declare a C struct that represents the hardware's register layout, 
and then use that rather than iobase+register_offset

> +static int xgmac_init(struct eth_device *dev, bd_t * bis)
> +{
> ...
> +	/* check that there is a valid MAC address */
> +	if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) == 0) {
> +		printf("ERROR: ethaddr not set!\n");
> +		return -1;
> +	}

i'd just omit this

> +int calxedaxgmac_initialize(u32 id, ulong base_addr)
> +{
> ...
> +	priv = (struct calxeda_eth_dev *)memalign(32, sizeof(*priv));

no need for the cast

> +	if (!priv) {
> +		free(dev);
> +		return -ENOMEM;

return 0

> +	if (readl(dev->iobase + XGMAC_CORE_VERSION) != 0x1012)
> +		return -EINVAL;

return -1

> +	macaddr[1] = readl(dev->iobase + XGMAC_CORE_MACADDR0HI);
> +	macaddr[0] = readl(dev->iobase + XGMAC_CORE_MACADDR0LO);
> +	memcpy(dev->enetaddr, macaddr, 6);

does the initial mac regs really start off with useful info ?

> +	sprintf(enetvar, id ? "eth%daddr" : "ethaddr", id);
> +	eth_setenv_enetaddr(enetvar, dev->enetaddr);

NAK: delete this
-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/20111202/c795e902/attachment.pgp>