Re: [PATCH 0/2 net-next] phy: allow flagging a device as internal

On Tue, 2013-05-21 at 18:25 +0100, Florian Fainelli wrote:

2013/5/21 Ben Hutchings [off-list ref]
        On Tue, 2013-05-21 at 11:37 +0100, Florian Fainelli wrote:
        > David, Andy,
        >
        > This small patch set updates libphy to allow PHY drivers to
        flag a
        > specific PHY device as internal, which is then used to
        accurately
        > report the transceiver type (internal vs external) in
        ethtool.
        >
        > As far as I can tell we only have one in-tree driver for the
        moment
        > which is known to be for internal PHYs.
        
        
        I don't think you should make any change like this until there
        is proper
        documentation of what the 'transceiver' field actually means.
        
        It appears that XCVR_EXTERNAL was originally used for a
        transceiver
        module external to the system, usually connected to an AUI
        port.  The
        modern equivalent of that might be SFP+ and similar module
        slots, but
        they're a bit different because the Physical Coding Sublayer
        is usually
        part of the controller.
        
        Many newer drivers are using XCVR_EXTERNAL to indicate that
        the PHY is a
        separate package from the controller, which is what you seem
        to be doing
        here.  But nowhere is that specified!


Well, it depends how you define "specified", I can see quite a lot of
drivers actually defining it to match the definition you gave above,
so the de facto situation is like you describe it.

I would like to see someone (you?) provide a longer comment in ethtool.h
that says what XCVR_INTERNAL and XCVR_EXTERNAL are supposed to mean (the
other values are obviously driver-specific so don't matter as much).

        The transceiver field doesn't even really matter much unless
        the user
        has a choice of which to use. 


This is about reporting accurate information, the field is not
user-changeable using libphy's ethtool knobs.

The information cannot be accurate without also being clear.

        Does anyone make boards like that any
        more?  And it's probably entirely redundant with the port and
        phy_address fields in that case, anyway.


At least I need to know from my driver if the PHY is an internal one
(package perspective) or not because there is a different
configuration path depending on this. I could just drop the hunk which
sets cmd->transceiver then.

I think that makes more sense.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.