Re: [PATCH net-next] net: phy: smsc: use phy_clear/set_bits in lan87xx_read_status

[Simon Horman <simon.horman@corigine.com>]

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
>