Re: [PATCH v2 net-next 2/3] net: phy: tja11xx: replace "nxp,rmii-refclk-in" with "nxp,phy-output-refclk"

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3747 bytes --]

On Thu, Aug 22, 2024 at 09:37:11AM +0000, Wei Fang wrote:
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > Sent: 2024年8月22日 16:47
> > To: Wei Fang <wei.fang@nxp.com>
> > Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; andrew@lunn.ch; f.fainelli@gmail.com;
> > hkallweit1@gmail.com; linux@armlinux.org.uk; Andrei Botila (OSS)
> > <andrei.botila@oss.nxp.com>; netdev@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v2 net-next 2/3] net: phy: tja11xx: replace
> > "nxp,rmii-refclk-in" with "nxp,phy-output-refclk"
> > 
> > On Thu, Aug 22, 2024 at 09:37:20AM +0800, Wei Fang wrote:
> > > As the new property "nxp,phy-output-refclk" is added to instead of the
> > > "nxp,rmii-refclk-in" property, so replace the "nxp,rmii-refclk-in"
> > > property used in the driver with the "nxp,reverse-mode" property and
> > > make slight modifications.
> > 
> > Can you explain what makes this backwards compatible please?
> > 
> It does not backward compatible, the related PHY nodes in DTS also
> need to be updated. I have not seen "nxp,rmii-refclk-in" used in the
> upstream.

Since you have switched the polarity, devicestrees that contain
"nxp,rmii-refclk-in" would actually not need an update to preserve
functionality. However...

> For nodes that do not use " nxp,rmii-refclk-in", they need
> to be updated, but unfortunately I cannot confirm which DTS use
> TJA11XX PHY, and there may be no relevant nodes in upstream DTS.

...as you say here, all tja11xx phy nodes that do not have the property
would need to be updated to retain functionality. Given you can't even
determine which devicetrees would need to be updated, I'm going to have
to NAK this change as an unnecessary ABI break.

Thanks,
Conor.

> 
> > >
> > > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > > ---
> > > V2 changes:
> > > 1. Changed the property name.
> > > ---
> > >  drivers/net/phy/nxp-tja11xx.c | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/nxp-tja11xx.c
> > > b/drivers/net/phy/nxp-tja11xx.c index 2c263ae44b4f..7aa0599c38c3
> > > 100644
> > > --- a/drivers/net/phy/nxp-tja11xx.c
> > > +++ b/drivers/net/phy/nxp-tja11xx.c
> > > @@ -78,8 +78,7 @@
> > >  #define MII_COMMCFG			27
> > >  #define MII_COMMCFG_AUTO_OP		BIT(15)
> > >
> > > -/* Configure REF_CLK as input in RMII mode */
> > > -#define TJA110X_RMII_MODE_REFCLK_IN       BIT(0)
> > > +#define TJA11XX_REVERSE_MODE		BIT(0)
> > >
> > >  struct tja11xx_priv {
> > >  	char		*hwmon_name;
> > > @@ -274,10 +273,10 @@ static int tja11xx_get_interface_mode(struct
> > phy_device *phydev)
> > >  		mii_mode = MII_CFG1_REVMII_MODE;
> > >  		break;
> > >  	case PHY_INTERFACE_MODE_RMII:
> > > -		if (priv->flags & TJA110X_RMII_MODE_REFCLK_IN)
> > > -			mii_mode = MII_CFG1_RMII_MODE_REFCLK_IN;
> > > -		else
> > > +		if (priv->flags & TJA11XX_REVERSE_MODE)
> > >  			mii_mode = MII_CFG1_RMII_MODE_REFCLK_OUT;
> > > +		else
> > > +			mii_mode = MII_CFG1_RMII_MODE_REFCLK_IN;
> > >  		break;
> > >  	default:
> > >  		return -EINVAL;
> > > @@ -517,8 +516,8 @@ static int tja11xx_parse_dt(struct phy_device
> > *phydev)
> > >  	if (!IS_ENABLED(CONFIG_OF_MDIO))
> > >  		return 0;
> > >
> > > -	if (of_property_read_bool(node, "nxp,rmii-refclk-in"))
> > > -		priv->flags |= TJA110X_RMII_MODE_REFCLK_IN;
> > > +	if (of_property_read_bool(node, "nxp,phy-output-refclk"))
> > > +		priv->flags |= TJA11XX_REVERSE_MODE;
> > >
> > >  	return 0;
> > >  }
> > > --
> > > 2.34.1
> > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]