[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZAntvsDrEtx/pIjA@corigine.com>
Date: Thu, 9 Mar 2023 15:31:26 +0100
From: Simon Horman <simon.horman@...igine.com>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: Russell King - ARM Linux <linux@...linux.org.uk>,
Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
David Miller <davem@...emloft.net>,
Paolo Abeni <pabeni@...hat.com>,
Eric Dumazet <edumazet@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in
lan87xx_read_status
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@...il.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
>
Powered by blists - more mailing lists