Manfred Spraul wrote:
> Carl-Daniel Hailfinger wrote:
> 
[...]
>> +
>> +    //wait for 500ms
>> +    mdelay(500);
>>  
>>
> Waiting for phy reset is also evil - it should be done either in a
> separate thread or asynchroneously. Not urgent, we can fix it later.

Did you look at the mail I sent in response to similar criticism some
hours ago?

>> +
>> +    // check auto negotiation is complete
>> +    mii_status = mii_rw(dev, np->phyaddr, MII_BMSR, MII_READ);
>> +    while (!(mii_status & BMSR_ANEGCOMPLETE)) {
>> +        udelay(NV_MIIBUSY_DELAY);
>> +        mii_status = mii_rw(dev, np->phyaddr, MII_BMSR, MII_READ);
>> +        microseconds++;
>> +        if (microseconds == 20) {
>> +            microseconds = 0;
>> +            milliseconds++;
>> +        }
>> +        if (milliseconds > 1200) {
>> +            printk(KERN_INFO "%s: phy init failed to autoneg.\n",
>> dev->name);
>> +            return PHY_TIMEOUT;
>> +        }
>>  
>>
> Dito.

Dito.

> The phy code needs a big rewrite and support for ethtool anyway. I'd
> propose to merge the patch after removing the bitfields - everything
> else looks good. Perhaps there is a bit too much code duplication with
> the v1/v2 functions, but that's also not fatal.

The code duplication will be addressed soon. Right now I'd like to change
the union "u" of v1 and v2 structs to an anonymous union for better
readability.

IMHO the split in v[12] functions makes sense for nv_rx_process and
nv_tx_done due to heavy use of different flags.
nv_alloc_rx and nv_start_xmit versions should be merged with the help of
macros.

New patch attached (untested).

Regards,
Carl-Daniel
-- 
http://www.hailfinger.org/