On Thu, Apr 21, 2011 at 4:38 PM, Wolfgang Denk wd@denx.de wrote:
Dear Simon Glass,
In message BANLkTikGucjpun2RhS2T2Nyq4_KB9gK8zw@mail.gmail.com you wrote:
- 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.
No, it is not. Before, we were accessing entry N-1 here. Now we use entry N. usb_max_eth_dev != usb_max_eth_dev - 1
Hi Wolfgang,
Hmmm. If you see the usb_max_eth_dev++ below, it is increasing the index variable to keep eth_register() happy. I have kept that behaviour.
So let's say usb_max_eth_dev is 3. The sequence is:
- set eth to point to item 3 - increase usb_max_eth_dev to 4 as required by eth_register() - call eth_register() with item 3 (i.e. eth pointer hasn't changed) - call eth_write_hwaddr with item 3
Maybe look at the whole code with my patch applied?
Let me know if I am missing something.
+ eth)) { /* found proper driver */ /* register with networking stack */ 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); + if (eth_write_hwaddr(eth, "usbeth", + usb_max_eth_dev - 1)) + puts("Warning: failed to set MAC address\n"); break; }
- 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.
Ummm... but having everyone to pass the correct value is actually a really good thing to have!
OK fair enough, it is in there now.
Regards, Simon