[U-Boot] [PATCH 2/5] Add Ethernet hardware MAC address framework to usbnet

[Mike Frysinger <vapier@gentoo.org>]

On Tuesday, April 12, 2011 17:13:57 Simon Glass wrote:
> Built-in Ethernet adapters support setting the mac address by means of a
> ethaddr environment variable for each interface (ethaddr, eth1addr,
> eth2addr).
> 
> This adds similar support to the USB network side, using the names
> usbethaddr, usbeth1addr, etc. They are kept separate since we don't want
> a USB device taking the MAC address of a built-in device or vice versa.

i dont know about that part.  it isnt like built-in devices generally "come 
and go" like USB.  maybe Wolfgang will have an opinion.

> --- a/include/net.h
> +++ b/include/net.h
> +/*
> + * Get the hardware address for an ethernet interface .
> + * Args:
> + *	base_name - base name for device (NULL for "eth")
> + *	index - device index number (0 for first)
> + *	enetaddr - returns 6 byte hardware address
> + * Returns:
> + *	1 = ok, 0 = invalid MAC address
> + */
> +extern int eth_getenv_enetaddr_by_index(char *base_name, int index,
> +					uchar *enetaddr);

the base_name must be const

might be better to document the return value the same way we document the 
ultimate func it calls: "Return true if the address is valid".

> +/*
> + * Set the hardware address for an ethernet interface based on 'eth%daddr'
> + * environment variable (or just 'ethaddr' if eth_number is 0).
> + * Args:
> + *	base_name - base name for device (NULL for "eth")
> + *	eth_number - value of %d (0 for first device of this type)
> + * Returns:
> + *	0 = ok, -1 = driver reported error trying to set address
> + */
> +int eth_set_hwaddr(struct eth_device *dev, char *base_name, int
> eth_number);

the base_name must be const.

i'd document return as "0 is success, non-zero is error status from driver".

also, the naming is a bit confusing.  better to stick with the "write" aspect 
which this ultimately calls into: eth_write_hwaddr().

> --- a/net/eth.c
> +++ b/net/eth.c
> +int eth_set_hwaddr(struct eth_device *dev, char *base_name, int
> ...
> +	if (strchr(dev->name, ' '))
> +		puts("\nWarning: eth device name has a space!\n");

this should not be relocated here.  it belongs in the registration step only.

> +		memcpy(dev->enetaddr, env_enetaddr, 6);
> +	}
> +	if (dev->write_hwaddr &&
> +		!eth_mac_skip(eth_number) &&
> +		is_valid_ether_addr(dev->enetaddr)) {
> +		ret = dev->write_hwaddr(dev);
> +	}
> +	return ret;

but new lines before/after that write_addr if block please
-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/20110412/cf22e6cc/attachment.pgp