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

[Heiner Kallweit <hkallweit1@gmail.com>]

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
>>