Tested the patch, it works for me. Thus I'll attach a pre-emptive
Acked-by: Linas Vepstas <[EMAIL PROTECTED]>
However, some quibbbles, which I think would be nice to see fixed:
On Mon, Feb 12, 2007 at 09:35:34PM +0100, Jens Osterkamp wrote:
>
> Index: linux-2.6.20/drivers/net/sungem_phy.c
> ===================================================================
> --- linux-2.6.20.orig/drivers/net/sungem_phy.c
> +++ linux-2.6.20/drivers/net/sungem_phy.c
>
> +#define BCM5421_MODE_MASK (1 << 5)
Customary practice is to have these in the heder file ...
> + mode = (phy_reg & BCM5421_MODE_MASK) >> 5;
> +
> + if ( mode == BCM54XX_COPPER)
All this shifting makes the code hard to read and
hard to verify for correctness. Part of the problem
seems to be that you are trying to re-cycle the
BCM5421_COPPER and BCM5461_COPPER which are in
different locations.
It would have been clearer to simply have
#define BCM5421_MODE_MASK (1 << 5)
#define BCM5421_COPPER 0
if (phy_reg & BCM5421_MODE_MASK == BCM5421_COPPER)
> + if ( (phy_reg & 0x0080) >> 7)
There is no need for the shift. The if statement is
just as true (or false) with or without the shift.
> +#define BCM5461_FIBER_LINK (1 << 2)
> +#define BCM5461_MODE_MASK (3 << 1)
> +
> + mode = (phy_reg & BCM5461_MODE_MASK ) >> 1;
More confusing shifting ....
> +enum {
> + BCM54XX_COPPER,
> + BCM54XX_FIBER,
> + BCM54XX_GBIC,
> + BCM54XX_SGMII,
Things that are being used for bitmak tests should probably
not be declared in an enum. For me, at least, there's a
cognitive dissonance in doing things this way.
-- linas
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
