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

[Simon Glass]

On Mon, Apr 18, 2011 at 12:04 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <1302748176-4324-2-git-send-email-sjg at chromium.org> you wrote:
>> @@ -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.

Yes. Since I am using an 'eth' pointer I don't need to index the array
again. The behaviour is the same as before.

>
>> diff --git a/include/net.h b/include/net.h

>> @@ -123,7 +123,18 @@ extern int eth_get_dev_index (void);             /* get the device index */

>> +/*
>> + * 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.

The intention was to avoid everyone having to pass the correct value -
potential for error, etc. I could have created a #define, but decided
on this.

I have changed it as you request.

>
>> +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.

Done

>
> Best regards,
>
> Wolfgang Denk

Regards,
Simon