| Message ID | 6c4ca9e8-8b68-f730-7d88-ebb7165f6b1d@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status | expand |
On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote: > Simplify the code by using phy_clear/sert_bits(). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/phy/smsc.c | 25 ++++++++++--------------- > 1 file changed, 10 insertions(+), 15 deletions(-) The phy_clear/sert_bits changes lookg good. But I have a few nit-pick comments. > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c > index af89f3ef1..5965a8afa 100644 > --- a/drivers/net/phy/smsc.c > +++ b/drivers/net/phy/smsc.c > @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev) > static int lan87xx_read_status(struct phy_device *phydev) > { > struct smsc_phy_priv *priv = phydev->priv; > + int rc; > > - int err = genphy_read_status(phydev); > + rc = genphy_read_status(phydev); > + if (rc) > + return rc; nit: this seems like a separate change, possibly a fix. > > if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) { > /* Disable EDPD to wake up PHY */ > - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); > - if (rc < 0) > - return rc; > - > - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, > - rc & ~MII_LAN83C185_EDPWRDOWN); > + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS, > + MII_LAN83C185_EDPWRDOWN); > if (rc < 0) > return rc; > > @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev) > * an actual error. > */ > read_poll_timeout(phy_read, rc, > - rc & MII_LAN83C185_ENERGYON || rc < 0, > + rc < 0 || rc & MII_LAN83C185_ENERGYON, nit: this also seems like a separate change. > 10000, 640000, true, phydev, > MII_LAN83C185_CTRL_STATUS); > if (rc < 0) > return rc; > > /* Re-enable EDPD */ > - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); > - if (rc < 0) > - return rc; > - > - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, > - rc | MII_LAN83C185_EDPWRDOWN); > + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS, > + MII_LAN83C185_EDPWRDOWN); > if (rc < 0) > return rc; > } > > - return err; > + return 0; > } > > static int smsc_get_sset_count(struct phy_device *phydev) > -- > 2.39.2 >
On 09.03.2023 15:31, Simon Horman wrote: > On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote: >> Simplify the code by using phy_clear/sert_bits(). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/phy/smsc.c | 25 ++++++++++--------------- >> 1 file changed, 10 insertions(+), 15 deletions(-) > > The phy_clear/sert_bits changes lookg good. > But I have a few nit-pick comments. > >> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c >> index af89f3ef1..5965a8afa 100644 >> --- a/drivers/net/phy/smsc.c >> +++ b/drivers/net/phy/smsc.c >> @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev) >> static int lan87xx_read_status(struct phy_device *phydev) >> { >> struct smsc_phy_priv *priv = phydev->priv; >> + int rc; >> >> - int err = genphy_read_status(phydev); >> + rc = genphy_read_status(phydev); >> + if (rc) >> + return rc; > > nit: this seems like a separate change, possibly a fix. > There's no known problem with the current code, so the need for a fix may be questionable. But you're right, it's a separate change. IMO it just wasn't worth it to provide it as a separate patch. >> >> if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) { >> /* Disable EDPD to wake up PHY */ >> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); >> - if (rc < 0) >> - return rc; >> - >> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, >> - rc & ~MII_LAN83C185_EDPWRDOWN); >> + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS, >> + MII_LAN83C185_EDPWRDOWN); >> if (rc < 0) >> return rc; >> >> @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev) >> * an actual error. >> */ >> read_poll_timeout(phy_read, rc, >> - rc & MII_LAN83C185_ENERGYON || rc < 0, >> + rc < 0 || rc & MII_LAN83C185_ENERGYON, > > nit: this also seems like a separate change. > Same as for the remark before. >> 10000, 640000, true, phydev, >> MII_LAN83C185_CTRL_STATUS); >> if (rc < 0) >> return rc; >> >> /* Re-enable EDPD */ >> - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); >> - if (rc < 0) >> - return rc; >> - >> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, >> - rc | MII_LAN83C185_EDPWRDOWN); >> + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS, >> + MII_LAN83C185_EDPWRDOWN); >> if (rc < 0) >> return rc; >> } >> >> - return err; >> + return 0; >> } >> >> static int smsc_get_sset_count(struct phy_device *phydev) >> -- >> 2.39.2 >>
On Fri, Mar 10, 2023 at 09:51:37PM +0100, Heiner Kallweit wrote: > On 09.03.2023 15:31, Simon Horman wrote: > > On Wed, Mar 08, 2023 at 09:11:02PM +0100, Heiner Kallweit wrote: > >> Simplify the code by using phy_clear/sert_bits(). > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/net/phy/smsc.c | 25 ++++++++++--------------- > >> 1 file changed, 10 insertions(+), 15 deletions(-) > > > > The phy_clear/sert_bits changes lookg good. > > But I have a few nit-pick comments. > > > >> diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c > >> index af89f3ef1..5965a8afa 100644 > >> --- a/drivers/net/phy/smsc.c > >> +++ b/drivers/net/phy/smsc.c > >> @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev) > >> static int lan87xx_read_status(struct phy_device *phydev) > >> { > >> struct smsc_phy_priv *priv = phydev->priv; > >> + int rc; > >> > >> - int err = genphy_read_status(phydev); > >> + rc = genphy_read_status(phydev); > >> + if (rc) > >> + return rc; > > > > nit: this seems like a separate change, possibly a fix. > > > There's no known problem with the current code, so the need for a fix > may be questionable. But you're right, it's a separate change. > IMO it just wasn't worth it to provide it as a separate patch. Ok, I don't feel strongly about this. Though perhaps it would be nice to mention in the patch description if a v2 materialises. In any case, I am happy with the correctness of this patch. Signed-off-by: Simon Horman <simon.horman@corigine.com> > >> > >> if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) { > >> /* Disable EDPD to wake up PHY */ > >> - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); > >> - if (rc < 0) > >> - return rc; > >> - > >> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, > >> - rc & ~MII_LAN83C185_EDPWRDOWN); > >> + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS, > >> + MII_LAN83C185_EDPWRDOWN); > >> if (rc < 0) > >> return rc; > >> > >> @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev) > >> * an actual error. > >> */ > >> read_poll_timeout(phy_read, rc, > >> - rc & MII_LAN83C185_ENERGYON || rc < 0, > >> + rc < 0 || rc & MII_LAN83C185_ENERGYON, > > > > nit: this also seems like a separate change. > > > Same as for the remark before. Ack. > >> 10000, 640000, true, phydev, > >> MII_LAN83C185_CTRL_STATUS); > >> if (rc < 0) > >> return rc; > >> > >> /* Re-enable EDPD */ > >> - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); > >> - if (rc < 0) > >> - return rc; > >> - > >> - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, > >> - rc | MII_LAN83C185_EDPWRDOWN); > >> + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS, > >> + MII_LAN83C185_EDPWRDOWN); > >> if (rc < 0) > >> return rc; > >> } > >> > >> - return err; > >> + return 0; > >> } > >> > >> static int smsc_get_sset_count(struct phy_device *phydev) > >> -- > >> 2.39.2 > >> >
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c index af89f3ef1..5965a8afa 100644 --- a/drivers/net/phy/smsc.c +++ b/drivers/net/phy/smsc.c @@ -204,17 +204,16 @@ static int lan95xx_config_aneg_ext(struct phy_device *phydev) static int lan87xx_read_status(struct phy_device *phydev) { struct smsc_phy_priv *priv = phydev->priv; + int rc; - int err = genphy_read_status(phydev); + rc = genphy_read_status(phydev); + if (rc) + return rc; if (!phydev->link && priv->energy_enable && phydev->irq == PHY_POLL) { /* Disable EDPD to wake up PHY */ - int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); - if (rc < 0) - return rc; - - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, - rc & ~MII_LAN83C185_EDPWRDOWN); + rc = phy_clear_bits(phydev, MII_LAN83C185_CTRL_STATUS, + MII_LAN83C185_EDPWRDOWN); if (rc < 0) return rc; @@ -222,24 +221,20 @@ static int lan87xx_read_status(struct phy_device *phydev) * an actual error. */ read_poll_timeout(phy_read, rc, - rc & MII_LAN83C185_ENERGYON || rc < 0, + rc < 0 || rc & MII_LAN83C185_ENERGYON, 10000, 640000, true, phydev, MII_LAN83C185_CTRL_STATUS); if (rc < 0) return rc; /* Re-enable EDPD */ - rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS); - if (rc < 0) - return rc; - - rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, - rc | MII_LAN83C185_EDPWRDOWN); + rc = phy_set_bits(phydev, MII_LAN83C185_CTRL_STATUS, + MII_LAN83C185_EDPWRDOWN); if (rc < 0) return rc; } - return err; + return 0; } static int smsc_get_sset_count(struct phy_device *phydev)
Simplify the code by using phy_clear/sert_bits(). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/smsc.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)