Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> Hi Russell,
> 
> On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> >  drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> >  
> > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > +{
> > +	int retries, val, err;
> > +
> > +	if (!reset)
> > +		return 0;
> 
> You could also call mv3310_maybe_reset after testing the 'reset'
> condition, that would make it easier to read the code.

I'm not too convinced:

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ef1ed9415d9f..3daf73e61dff 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
 				  MV_V2_PORT_CTRL_PWRDOWN);
 }
 
-static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
+static int mv3310_reset(struct phy_device *phydev, u32 unit)
 {
 	int retries, val, err;
 
-	if (!reset)
-		return 0;
-
 	err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
 			     MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
 	if (err < 0)
@@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
 
 	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
 				     MV_PCS_CSCR1_MDIX_MASK, val);
-	if (err < 0)
+	if (err <= 0)
 		return err;
 
-	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+	return mv3310_reset(phydev, MV_PCS_BASE_T);
 }
 
 static int mv3310_config_aneg(struct phy_device *phydev)

The change from:

	if (err < 0)

to:

	if (err <= 0)

could easily be mistaken as a bug, and someone may decide to try to
"fix" that back to being the former instead.  The way I have the code
makes the intention explicit.

> 
> >  static struct phy_driver mv3310_drivers[] = {
> >  	{
> >  		.phy_id		= MARVELL_PHY_ID_88X3310,
> > @@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
> >  		.name		= "mv88x3310",
> >  		.get_features	= mv3310_get_features,
> >  		.soft_reset	= genphy_no_soft_reset,
> > -		.config_init	= mv3310_config_init,
> 
> Having a quick look at the code, it seems this is a leftover and you
> don't actually want to remove config_init for the 3310.

Hmm, I wonder how that crept in... it shouldn't be there!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up