Re: [PATCH net-next v3 7/8] dpaa2-mac: configure the SerDes phy on a protocol change

From: Ioana Ciornei <ioana.ciornei@nxp.com>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: "davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"kishon@ti.com" <kishon@ti.com>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	Leo Li <leoyang.li@nxp.com>,
	"linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	Hongxing Zhu <hongxing.zhu@nxp.com>
Subject: Re: [PATCH net-next v3 7/8] dpaa2-mac: configure the SerDes phy on a protocol change
Date: Thu, 10 Mar 2022 15:57:10 +0000	[thread overview]
Message-ID: <20220310155709.xfbqjspok2duka3n@skbuf> (raw)
In-Reply-To: <YioTznpNwldCnJpm@shell.armlinux.org.uk>

On Thu, Mar 10, 2022 at 03:05:50PM +0000, Russell King (Oracle) wrote:
> On Thu, Mar 10, 2022 at 04:51:59PM +0200, Ioana Ciornei wrote:
> > This patch integrates the dpaa2-eth driver with the generic PHY
> > infrastructure in order to search, find and reconfigure the SerDes lanes
> > in case of a protocol change.
> > 
> > On the .mac_config() callback, the phy_set_mode_ext() API is called so
> > that the Lynx 28G SerDes PHY driver can change the lane's configuration.
> > In the same phylink callback the MC firmware is called so that it
> > reconfigures the MAC side to run using the new protocol.
> > 
> > The consumer drivers - dpaa2-eth and dpaa2-switch - are updated to call
> > the dpaa2_mac_start/stop functions newly added which will
> > power_on/power_off the associated SerDes lane.
> > 
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> 
> Looks better, there's a minor thing that I missed, sorry:
> 
> > +	if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
> > +	    !phy_interface_mode_is_rgmii(mac->if_mode) &&
> > +	    is_of_node(dpmac_node)) {
> > +		serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);
> > +
> > +		if (IS_ERR(serdes_phy)) {
> > +			if (PTR_ERR(serdes_phy) == -ENODEV)
> > +				serdes_phy = NULL;
> > +			else
> > +				return PTR_ERR(serdes_phy);
> > +		} else {
> > +			phy_init(serdes_phy);
> > +		}
> 
> Would:
> 		if (PTR_ERR(serdes_phy) == -ENODEV)
> 			serdes_phy = NULL;
> 		else if (IS_ERR(serdes_phy))
> 			return PTR_ERR(serdes_phy);
> 		else
> 			phy_init(serdes_phy);
> 

Yes, it wouldn't be an if inside another if statement.

> be neater? There is no need to check IS_ERR() before testing PTR_ERR().
> One may also prefer the pointer-comparison approach:
> 
> 		if (serdes_phy == ERR_PTR(-ENODEV))
> 
> to remove any question about PTR_ERR(p) on a !IS_ERR(p) value too, but
> it really doesn't make any difference.
> 
> I suspect this is just a code formatting issue, I'd think the compiler
> would generate reasonable code either way, so as I said above, it's
> quite minor.
> 

As you said, since it's quite minor I am going to wait to see if more
comments will appear, if not I am going to fix this up in another patch.

Thanks!
-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy