[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson <lrj@arlinx.com>]

Ben Warren wrote:
> Larry Johnson wrote:
>> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this
>> symbol
>> is defined, the PHY will advertise it's capabilities for autonegotiation
>> based on the capabilities shown in the PHY's status registers, including
>> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
>> advertise hard-coded capabilities, as before.
>>
>>   
> <snip>
>> -
>>  /*****************************************************************************
>>
>>   *
>> - * Determine the ethernet speed (10/100).
>> + * Determine the ethernet speed (10/100/1000).  Return 10 on error.
>>   */
>>  int miiphy_speed (char *devname, unsigned char addr)
>>  {
>> -    unsigned short reg;
>> +    u16 bmcr;
>>
>>  #if defined(CONFIG_PHY_GIGE)
>> -    if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
>> -        printf ("PHY 1000BT Status read failed\n");
>> -    } else {
>> -        if (reg != 0xFFFF) {
>> -            if ((reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))
>> !=0) {
>> -                return (_1000BASET);
>> -            }
>> +    u16 btsr;
>> +
>> +#if defined(CONFIG_PHY_DYNAMIC_ANEG)
>> +    u16 bmsr;
>> +
>> +    /* Check for 1000BASE-X. */
>> +    if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
>> +        printf ("PHY status");
>> +        goto miiphy_read_failed;
>> +    }
>> +    if (bmsr & PHY_BMSR_EXT_STAT) {
>> +        u16 exsr;
>>   
> Please don't define variables in code blocks. The original code used a
> single variable called 'reg' that worked well enough. The bitmasks
> (PHY_BMSR_* and so on) effectively tell the reader what register he/she
> is looking at.
> 
> I'm going to stop here and wait for you to separate content from
> cosmetic, and hopefully fix up what I've mentioned here.
> 
> regards,
> Ben
> 

Hi Ben,

You bring up two issues here, and I'm wondering how you and others feel
about them.  The first is whether all variables should be declared at
the start of the function, or within the smallest block in which they
are used.  I prefer to declare variables close to where they are used,
but don't mind doing it either way.

I do think that using separate dedicated variables for to hold the
different registers is a better coding practice.  The compiler can do a
better job keeping track of variable lifetimes than I can, and someone
modifying the code later won't have to check to see if it's safe to
modify 'reg' at some point or other.  Also, the register and bitmask
definitions make it obvious that the code is correct, as in

    "if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {"

and

    "if (bmsr & PHY_BMSR_EXT_STAT) {".

Those are the advantage I see, what do you see as the disadvantages?

Best regards, Larry