Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.03.2020 08:37, Oleksij Rempel wrote:
> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
> configured in device tree by setting compatible =
> "ethernet-phy-id0180.dc81".
> 
> PHY 1 has less suported registers and functionality. For current driver
> it will affect only the HWMON support.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/nxp-tja11xx.c | 43 +++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> index b705d0bd798b..52090cfaa54e 100644
> --- a/drivers/net/phy/nxp-tja11xx.c
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -15,6 +15,7 @@
>  #define PHY_ID_MASK			0xfffffff0
>  #define PHY_ID_TJA1100			0x0180dc40
>  #define PHY_ID_TJA1101			0x0180dd00
> +#define PHY_ID_TJA1102			0x0180dc80
>  
>  #define MII_ECTRL			17
>  #define MII_ECTRL_LINK_CONTROL		BIT(15)
> @@ -190,6 +191,7 @@ static int tja11xx_config_init(struct phy_device *phydev)
>  			return ret;
>  		break;
>  	case PHY_ID_TJA1101:
> +	case PHY_ID_TJA1102:
>  		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
>  		if (ret)
>  			return ret;
> @@ -337,6 +339,31 @@ static int tja11xx_probe(struct phy_device *phydev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	/* Use the phyid to distinguish between port 0 and port 1 of the
> +	 * TJA1102. Port 0 has a proper phyid, while port 1 reads 0.
> +	 */
> +	if ((phydev->phy_id & PHY_ID_MASK) == PHY_ID_TJA1102) {
> +		int ret;
> +		u32 id;
> +
> +		ret = phy_read(phydev, MII_PHYSID1);
> +		if (ret < 0)
> +			return ret;
> +
> +		id = ret;
> +		ret = phy_read(phydev, MII_PHYSID2);
> +		if (ret < 0)
> +			return ret;
> +
> +		id |= ret << 16;
> +
> +		/* TJA1102 Port 1 has phyid 0 and doesn't support temperature
> +		 * and undervoltage alarms.
> +		 */
> +		if (id == 0)
> +			return 0;

I'm not sure I understand what you're doing here. The two ports of the chip
are separate PHY's on individual MDIO bus addresses?
Reading the PHY ID registers here seems to repeat what phylib did already
to populate phydev->phy_id. If port 1 has PHD ID 0 then the driver wouldn't
bind and tja11xx_probe() would never be called (see phy_bus_match)

> +	}
> +
>  	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>  	if (!priv->hwmon_name)
>  		return -ENOMEM;
> @@ -385,6 +412,21 @@ static struct phy_driver tja11xx_driver[] = {
>  		.get_sset_count = tja11xx_get_sset_count,
>  		.get_strings	= tja11xx_get_strings,
>  		.get_stats	= tja11xx_get_stats,
> +	}, {
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1102),
> +		.name		= "NXP TJA1102",
> +		.features       = PHY_BASIC_T1_FEATURES,
> +		.probe		= tja11xx_probe,
> +		.soft_reset	= tja11xx_soft_reset,
> +		.config_init	= tja11xx_config_init,
> +		.read_status	= tja11xx_read_status,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.set_loopback   = genphy_loopback,
> +		/* Statistics */
> +		.get_sset_count = tja11xx_get_sset_count,
> +		.get_strings	= tja11xx_get_strings,
> +		.get_stats	= tja11xx_get_stats,
>  	}
>  };
>  
> @@ -393,6 +435,7 @@ module_phy_driver(tja11xx_driver);
>  static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
> +	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1102) },
>  	{ }
>  };
>  
>