Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Dec 13, 2018 at 02:27:51PM +0100, Marek Vasut wrote:
> On 12/13/2018 09:38 AM, Andrew Lunn wrote:
> >> +static int tja11xx_config_aneg(struct phy_device *phydev)
> >> +{
> >> +	phydev->autoneg = 0;
> >> +	phydev->speed = SPEED_100;
> >> +	phydev->duplex = DUPLEX_FULL;
> >> +	phydev->pause = 0;
> >> +	phydev->asym_pause = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > Hi Marek
> 
> Hi,
> 
> > That is, err, interesting. Are you saying the PHY cannot do auto-neg?
> 
> There isn't much to auto-negotiate and some of the bits in the standard
> registers behave weirdly.

Hi Marek

Hopefully the config_aneg callback is not called if you don't list
autoneg to the .features. The microchip_t1 driver just uses
genphy_config_aneg, but if a NULL works, i would prefer that.

> > Does it happen to be a 100T1 device for automotive?
> 
> Yes, that's what the commit message says, why ?

Sorry, i missed it in the commit message.  We now have enough bits in
the link_mode that we should explicitly say this is 100BaseT2_Full,
not 100BaseT_Full. So please add a new member to
ethtool_link_mode_bit_indices ...
 
> >> +
> >> +static int tja11xx_config_init(struct phy_device *phydev)
> >> +{
> >> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;

And make it part of the .features. Well, you need to add a new
PHY_BASIC_T2_FEATURES macro, which uses it, and use that in the
phy_driver structure. There should not be any need to modify it in the
config_init call.

We also need to think about what we do with the PHY_BASIC_T1_FEATURES
macro. Ideally we want to swap that to also make use of a new
ethtool_link_mode_bit_indices, but i've no idea at the moment if that
will break something.

  Andrew