Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver

[Jie Luo <luoj@codeaurora.org>]

On 8/30/2021 9:48 PM, Andrew Lunn wrote:
> On Mon, Aug 30, 2021 at 07:07:32PM +0800, Luo Jie wrote:
>> qca8081 is a single port ethernet phy chip that supports
>> 10/100/1000/2500 Mbps mode.
>>
>> Signed-off-by: Luo Jie <luoj@codeaurora.org>
>> ---
>>   drivers/net/phy/at803x.c | 389 ++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 338 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index ecae26f11aa4..2b3563ae152f 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -33,10 +33,10 @@
>>   #define AT803X_SFC_DISABLE_JABBER		BIT(0)
>>   
>>   #define AT803X_SPECIFIC_STATUS			0x11
>> -#define AT803X_SS_SPEED_MASK			(3 << 14)
>> -#define AT803X_SS_SPEED_1000			(2 << 14)
>> -#define AT803X_SS_SPEED_100			(1 << 14)
>> -#define AT803X_SS_SPEED_10			(0 << 14)
>> +#define AT803X_SS_SPEED_MASK			GENMASK(15, 14)
>> +#define AT803X_SS_SPEED_1000			2
>> +#define AT803X_SS_SPEED_100			1
>> +#define AT803X_SS_SPEED_10			0
> This looks like an improvement, and nothing to do with qca8081. Please
> make it an separate patch.
will make it an separate patch in the next patch series.
>>   #define AT803X_SS_DUPLEX			BIT(13)
>>   #define AT803X_SS_SPEED_DUPLEX_RESOLVED		BIT(11)
>>   #define AT803X_SS_MDIX				BIT(6)
>> @@ -158,6 +158,8 @@
>>   #define QCA8337_PHY_ID				0x004dd036
>>   #define QCA8K_PHY_ID_MASK			0xffffffff
>>   
>> +#define QCA8081_PHY_ID				0x004dd101
>> +
> Maybe keep all the PHY_ID together?
will move it to make PHY_ID together in the next patch series.
>
>>   #define QCA8K_DEVFLAGS_REVISION_MASK		GENMASK(2, 0)
>>   
>>   #define AT803X_PAGE_FIBER			0
>> @@ -167,7 +169,73 @@
>>   #define AT803X_KEEP_PLL_ENABLED			BIT(0)
>>   #define AT803X_DISABLE_SMARTEEE			BIT(1)
>>   
>> @@ -711,11 +779,18 @@ static void at803x_remove(struct phy_device *phydev)
>>   
>>   static int at803x_get_features(struct phy_device *phydev)
>>   {
>> -	int err;
>> +	int val;
> Why? The driver pretty consistently uses err for return values which
> are errors.
will keep err unchanged in the next patch set.
>
>>   
>> -	err = genphy_read_abilities(phydev);
>> -	if (err)
>> -		return err;
>> +	val = genphy_read_abilities(phydev);
>> +	if (val)
>> +		return val;
>> +
>> +	if (at803x_match_phy_id(phydev, QCA8081_PHY_ID)) {
>> +		val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE);
> You don't check if val indicates if there was an error.
thanks Andrew for the comment, will add the check here.
>
>> +
>> +		linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT, phydev->supported,
>> +				val & MDIO_PMA_NG_EXTABLE_2_5GBT);
>> +	}
>>   
>>   	if (!at803x_match_phy_id(phydev, ATH8031_PHY_ID))
>>   		return 0;
>> @@ -935,44 +1010,44 @@ static void at803x_link_change_notify(struct phy_device *phydev)
>>   	}
>>   }
>>   
>> -static int at803x_read_status(struct phy_device *phydev)
>> +static int at803x_read_specific_status(struct phy_device *phydev)
>>   {
>> -	int ss, err, old_link = phydev->link;
>> -
>> -	/* Update the link, but return if there was an error */
>> -	err = genphy_update_link(phydev);
>> -	if (err)
>> -		return err;
>> -
>> -	/* why bother the PHY if nothing can have changed */
>> -	if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
>> -		return 0;
>> +	int val;
>>   
>> -	phydev->speed = SPEED_UNKNOWN;
>> -	phydev->duplex = DUPLEX_UNKNOWN;
>> -	phydev->pause = 0;
>> -	phydev->asym_pause = 0;
>> +	val = phy_read(phydev, AT803X_SPECIFIC_FUNCTION_CONTROL);
>> +	if (val < 0)
>> +		return val;
>>   
>> -	err = genphy_read_lpa(phydev);
>> -	if (err < 0)
>> -		return err;
>> +	switch (FIELD_GET(AT803X_SFC_MDI_CROSSOVER_MODE_M, val)) {
>> +	case AT803X_SFC_MANUAL_MDI:
>> +		phydev->mdix_ctrl = ETH_TP_MDI;
>> +		break;
>> +	case AT803X_SFC_MANUAL_MDIX:
>> +		phydev->mdix_ctrl = ETH_TP_MDI_X;
>> +		break;
>> +	case AT803X_SFC_AUTOMATIC_CROSSOVER:
>> +		phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
>> +		break;
>> +	}
>>   
>>   	/* Read the AT8035 PHY-Specific Status register, which indicates the
>>   	 * speed and duplex that the PHY is actually using, irrespective of
>>   	 * whether we are in autoneg mode or not.
>>   	 */
>> -	ss = phy_read(phydev, AT803X_SPECIFIC_STATUS);
>> -	if (ss < 0)
>> -		return ss;
>> +	val = phy_read(phydev, AT803X_SPECIFIC_STATUS);
>> +	if (val < 0)
>> +		return val;
> What was actually wrong with ss?
>
> Is this another case of just copying code from your other driver,
> rather than cleanly extending the existing driver?
>
> There are two many changes here all at once. Please break this patch
> up. You are aiming for lots of small patches which are obviously
> correct. Part of being obviously correct is having a good commit
> message, and that gets much easier when a patch is small.
>
> 	 Andrew

Hi Andrew,

i separate the phy specific status into a new function 
at803x_read_specific_status, since this function

need to be used for both at803x phy driver and qca8081 phy driver.

i will break the patch into the minimal changes and provide the commit 
message in detail in the next

patch series.

thanks for your helpful comments.