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

[Simon Glass]

On Thu, Apr 21, 2011 at 4:38 PM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <BANLkTikGucjpun2RhS2T2Nyq4_KB9gK8zw at 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