On Mon, Apr 18, 2011 at 12:04 PM, Wolfgang Denk <w...@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1302748176-4324-2-git-send-email-...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
