Re: [PATCH] Add ethtool to mii advertisment conversion helpers

["Matt Carlson" <mcarlson@broadcom.com>]

On Wed, Nov 16, 2011 at 05:29:42PM -0800, Ben Hutchings wrote:
> On Wed, 2011-11-16 at 17:16 -0800, Matt Carlson wrote:
> > On Wed, Nov 16, 2011 at 04:34:37PM -0800, Ben Hutchings wrote:
> [...]
> > > > +#define mii_lpa_to_ethtool_100bt(lpa)	mii_adv_to_ethtool_100bt(lpa)
> > > 
> > > Shouldn't this additionally translate LPA_LPACK into ADVERTISED_Autoneg?
> > 
> > You mean, like this?
> > 
> > static inline u32 mii_lpa_to_ethtool_100bt(u32 lpa)
> > {
> > 	u32 result = 0;
> > 
> > 	if (lpa & LPA_LPACK)
> > 		result |= ADVERTISED_Autoneg;
> > 
> > 	return result | mii_adv_to_ethtool_100bt(lpa);
> > }
> > 
> > Yes, that looks like a better implementation.
> 
> Think so.
> 
> And I think the mii_adv_to_ethtool_* functions should add
> ADVERTISED_Autoneg unconditionally.  But I'm not entirely sure that's
> right.

The primary purpose of these functions is to translate the information
between two representations.  It seems wise to be careful not to add
from it or take anything away from it.  It is certainly possible to
have a valid AN advertisement register configuration, but not have
autoneg enabled.  Keeping the ADVERTISED_Autoneg out could prevent
misuse.  Do you agree?

> > > Shouldn't there be mii_lpa_to_ethtool_1000X (or
> > > mii_lpa_to_ethtool_lpa_x)?
> > 
> > Yes.  You're right.  Should it just be a preprocessor definition that
> > points to mii_adv_to_ethtool_1000X()?
> 
> I think that would need to handle LPA_LPACK as well.

I was wondering if that was present in 1000Base-X mode.  I didn't see it
in my passing glance at a spec.