On Wed, 2008-05-14 at 01:02 +0100, Maciej W. Rozycki wrote:
quoted text
>  Thanks for your changes -- they do not seem to break my system, but I
> have no way to verify your additions at the run time as I only have
> BCM5421 PHYs to try with.  A few comments follow.

I didn't mention this before, but I did test these myself at least on a
board with a BCM5482S in both SGMII->Copper and 1000Base-X modes.

quoted text
>  Please do provide these macros!  The size does not matter (duh, what a
> truism!).  What really matters is the meaning of the numbers.  What seems
> obvious now, may not be such a couple of changes into the future.  
> Besides, it is much more effective to grep, etc. for macros than for
> numbers.

I went ahead and revised my patch to include these.  I certainly don't
disagree with your reasoning in general.  Laziness is a big factor and
it felt faster/easier to get things merged if the change had a smaller
footprint :)

quoted text
>  We have magic numbers elsewhere, true, but they are results of
> reverse-engineering, so the exact meaning is unknown.  Of course if you
> have a possibility to document them properly, you are welcome to do so.
> But for everything that's known, let's keep the code as it should be from
> the beginning.

FYI: My guess for the 5481 magic bits is that they are accessing shadow
values at 0x18 similar to the way my 5481 patch uses the 0x1c shadow
registers.  It looks the same as the 5482's "Misc Control
Register" (shadow value "111", which is where the 0x7 comes from).

quoted text
> > +#define MII_BCM54XX_RSV0	0x15	/* Reserved / Expansion registers */
> 
>  Hmm, the name seems confusing -- is it the manufacturer's official name
> of the register?  Based on the semantics I would call it
> MII_BCM54XX_EXP_DATA or suchlike.

I'm not aware of any "official" manufacturer name for this register.
The register map has 0x15 as "reserved", but 0x15 is used for accessing
expansion registers.  I agree with your interpretation of how it's used
so I renamed it as you did in v2 of my patch.

quoted text
> 
> > +#define MII_BCM54XX_EXP		0x17	/* Expansion register access */
> 
>  And then perhaps MII_BCM54XX_EXP_SEL for consistency.

Done

quoted text
>  Formatting, please -- Linux wants operators at the end of the line and 
> indentation to mark where the continuation correlates to the line above.  
> In this case either:
[snip]

Done (this makes me wonder why Lindent doesn't pass "-nbbo" to indent
though).

quoted text
>  Hmm, you have reached the 80th column -- that causes problems sometimes,
> I would wrap the arguments.

Done

quoted text
>  Hmm, out of curiosity -- does such enforcement play well with tools like 
> ethtool or mii-tool?

I did test with 'ethtool' and link detection worked fine.  The only
catch is that it will show 1000 mbit/s even when there is no link (many
drivers report 10 mbit/s with no link present), but I wouldn't think we
should ever trust 'speed' without a link present.

v2 of the patch will follow shortly.

-- 
Nate Case <ncase@xes-inc.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html