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, ®)) { > - 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