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

[Ben Warren]

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