Re: [PATCH NET V4 1/2] net: phy: Add phy loopback support in net phy framework

[Yunsheng Lin]

Hi, Andrew

On 2017/6/24 21:44, Andrew Lunn wrote:
>> @@ -1087,7 +1087,7 @@ int phy_suspend(struct phy_device *phydev)
>>  {
>>  	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>>  	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
>> -	int ret = 0;
>> +	int ret = -EOPNOTSUPP;
>>  
>>  	/* If the device has WOL enabled, we cannot suspend the PHY */
>>  	phy_ethtool_get_wol(phydev, &wol);
>> @@ -1109,7 +1109,7 @@ int phy_suspend(struct phy_device *phydev)
>>  int phy_resume(struct phy_device *phydev)
>>  {
>>  	struct phy_driver *phydrv = to_phy_driver(phydev->mdio.dev.driver);
>> -	int ret = 0;
>> +	int ret = -EOPNOTSUPP;
>>  
>>  	if (phydev->drv && phydrv->resume)
>>  		ret = phydrv->resume(phydev);
>> @@ -1123,6 +1123,39 @@ int phy_resume(struct phy_device *phydev)
>>  }
> 
> These changes should be in a separate patch, since they have nothing
> to do with loopback. You want lots of small patches, making them
> easier to both describe the why it is needed, and easier to review.
> 
Thanks for pointing out. Will remove it next version.
> What are the implications of this change? Are you sure the power core
> code does not understand EOPNOTSUPP as being a real error?
> I was thinking phy_suspend/resume was making the mistake not returing error when
function point is null, as I did in phy_loopback. I am not sure if power core
understand EOPNOTSUPP.
But I think we should fix it if it is fixable, if not, then add a comment. because
it is not consitent with other phy_* . For anyone who is not familiar with phy core,
it is confusing.