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

[Wolfgang Denk]

Dear Simon Glass,

In message <1302748176-4324-2-git-send-email-sjg at chromium.org> you 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.
> 
> TEST=build, test on harmony, with setenv usbethaddr c0:c1:c0:13:0b:b8, bootp, tftp ...
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>  board/davinci/common/misc.c |    2 +-
>  drivers/net/designware.c    |    2 +-
>  drivers/usb/eth/usb_ether.c |    9 ++++-
>  include/net.h               |   25 ++++++++++++++++-
>  net/eth.c                   |   65 ++++++++++++++++++++++++++----------------
>  5 files changed, 73 insertions(+), 30 deletions(-)
...
> diff --git a/drivers/usb/eth/usb_ether.c b/drivers/usb/eth/usb_ether.c
> index 7b55da3..6565ea5 100644
> --- a/drivers/usb/eth/usb_ether.c
> +++ b/drivers/usb/eth/usb_ether.c
> @@ -80,6 +80,7 @@ int is_eth_dev_on_usb_host(void)
>   */
>  static void probe_valid_drivers(struct usb_device *dev)
>  {
> +	struct eth_device *eth;
>  	int j;
>  
>  	for (j = 0; prob_dev[j].probe && prob_dev[j].get_info; j++) {
> @@ -88,9 +89,10 @@ static void probe_valid_drivers(struct usb_device *dev)
>  		/*
>  		 * ok, it is a supported eth device. Get info and fill it in
>  		 */
> +		eth = &usb_eth[usb_max_eth_dev].eth_dev;

Index for eth is usb_max_eth_dev.

> @@ -100,7 +102,10 @@ static void probe_valid_drivers(struct usb_device *dev)
>  			 * call since eth_current_changed (internally called)
>  			 * relies on it
>  			 */
> -			eth_register(&usb_eth[usb_max_eth_dev - 1].eth_dev);
> +			eth_register(eth);

You change the behaviour here.  Please confirmt his is really
intentional.

> diff --git a/include/net.h b/include/net.h
> index 95ef8ab..9256a46 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -123,7 +123,18 @@ extern int eth_get_dev_index (void);		/* get the device index */
>  extern void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
>  extern int eth_getenv_enetaddr(char *name, uchar *enetaddr);
>  extern int eth_setenv_enetaddr(char *name, const uchar *enetaddr);
> -extern int eth_getenv_enetaddr_by_index(int index, uchar *enetaddr);
> +
> +/*
> + * Get the hardware address for an ethernet interface .
> + * Args:
> + *	base_name - base name for device (NULL for "eth")

This is an atitifical decision for the API which is difficult to
understand.  It just makes the code and understanding it more
difficult.  Just pass "eth" when you mean it.

> +int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
> +		   int eth_number)
> +{
> +	unsigned char env_enetaddr[6];
> +	int ret = 0;
> +
> +	eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr);

Add error handling, or write

	(void)eth_getenv_enetaddr_by_index(...);

to indicate that you intentionally ignore the return value.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Philosophy is a game with objectives and no rules.
Mathematics is a game with rules and no objectives.