Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 22 Mar 2020 19:18:32 +0000
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Dejin Zheng <zhengdejin5@gmail.com>
Cc:     andrew@lunn.ch, f.fainelli@gmail.com, hkallweit1@gmail.com,
        davem@davemloft.net, mchehab+samsung@kernel.org, corbet@lwn.net,
        gregkh@linuxfoundation.org, broonie@kernel.org, tglx@linutronix.de,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 08/10] net: phy: use phy_read_poll_timeout()
 to simplify the code
Message-ID: <20200322191832.GW25745@shell.armlinux.org.uk>
References: <20200322174943.26332-1-zhengdejin5@gmail.com>
 <20200322174943.26332-9-zhengdejin5@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200322174943.26332-9-zhengdejin5@gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 23, 2020 at 01:49:41AM +0800, Dejin Zheng wrote:
> use phy_read_poll_timeout() to replace the poll codes for
> simplify the code in phy_poll_reset() function.
> 
> Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com>
> ---
> v4 -> v5:
> 	- it can add msleep before call phy_read_poll_timeout()
> 	  to keep the code more similar. so add it.
> v3 -> v4:
> 	- drop it.
> v2 -> v3:
> 	- adapt to it after modifying the parameter order of the
> 	  newly added function
> v1 -> v2:
> 	- remove the handle of phy_read()'s return error.
> 
> 
>  drivers/net/phy/phy_device.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a585faf8b844..24297ee7f626 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1059,23 +1059,16 @@ EXPORT_SYMBOL(phy_disconnect);
>  static int phy_poll_reset(struct phy_device *phydev)
>  {
>  	/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
> -	unsigned int retries = 12;
> -	int ret;
> -
> -	do {
> -		msleep(50);
> -		ret = phy_read(phydev, MII_BMCR);
> -		if (ret < 0)
> -			return ret;
> -	} while (ret & BMCR_RESET && --retries);
> -	if (ret & BMCR_RESET)
> -		return -ETIMEDOUT;
> +	int ret, val;
>  
> +	msleep(50);
> +	ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
> +				    50000, 550000);
>  	/* Some chips (smsc911x) may still need up to another 1ms after the
>  	 * BMCR_RESET bit is cleared before they are usable.
>  	 */
>  	msleep(1);
> -	return 0;
> +	return ret;

This isn't actually equivaent behaviour.  If the read timed out, the old
code didn't wait 1ms.  Your new code does.  However, since we've waited
about 600ms already, it probably doesn't matter.

These sorts of things should be documented in the commit log, IMHO, so
it's obvious that it's been considered.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up