| Message ID | 20200322024834.31402-8-zhengdejin5@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | introduce read_poll_timeout | expand |
On 3/21/2020 7:48 PM, 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> > --- > 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 | 16 ++++------------ > 1 file changed, 4 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index a585faf8b844..cfe7aae35084 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1059,23 +1059,15 @@ 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); You are doing some subtle changes here, the sleep used to be *before* the read of BMCR and now it will be *after*. If there were PHY devices that required 50ms at least, but would incorrectly return that BMCR_RESET is cleared *before* 50ms, then we would be breaking those. I would recommend we drop this patch for now, the rest looks good to me though.
On Sat, Mar 21, 2020 at 07:57:11PM -0700, Florian Fainelli wrote: > > > On 3/21/2020 7:48 PM, 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> > > --- > > 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 | 16 ++++------------ > > 1 file changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > > index a585faf8b844..cfe7aae35084 100644 > > --- a/drivers/net/phy/phy_device.c > > +++ b/drivers/net/phy/phy_device.c > > @@ -1059,23 +1059,15 @@ 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); > > You are doing some subtle changes here, the sleep used to be *before* > the read of BMCR and now it will be *after*. If there were PHY devices > that required 50ms at least, but would incorrectly return that > BMCR_RESET is cleared *before* 50ms, then we would be breaking those. > > I would recommend we drop this patch for now, the rest looks good to me > though. Hi Florian�� Thanks very much for your comments, I will drop this patch, and can I get your ack/reviewed-by for the rest patches? thanks again! BR, dejin > -- > Florian
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index a585faf8b844..cfe7aae35084 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1059,23 +1059,15 @@ 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; + ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET), + 50000, 600000); /* 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; } int phy_init_hw(struct phy_device *phydev)
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> --- 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 | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)