LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jie Luo <luoj@codeaurora.org>
To: Andrew Lunn <andrew@lunn.ch>
Cc: hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
	kuba@kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, sricharan@codeaurora.org
Subject: Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver
Date: Tue, 31 Aug 2021 22:10:43 +0800	[thread overview]
Message-ID: <af224018-1190-ac78-2035-c9763c1d46ae@codeaurora.org> (raw)
In-Reply-To: <YSzhtF8g42Ccv2h0@lunn.ch>


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.



  reply	other threads:[~2021-08-31 14:11 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-30 11:07 [PATCH v1 0/3] net: phy: Add " Luo Jie
2021-08-30 11:07 ` [PATCH v1 1/3] net: phy: improve the wol feature of at803x Luo Jie
2021-08-30 13:32   ` Andrew Lunn
2021-08-31 14:01     ` Jie Luo
2021-08-30 11:07 ` [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver Luo Jie
2021-08-30 11:39   ` Russell King (Oracle)
2021-08-31 13:48     ` Jie Luo
2021-08-30 13:48   ` Andrew Lunn
2021-08-31 14:10     ` Jie Luo [this message]
2021-08-30 11:07 ` [PATCH v1 3/3] net: phy: add cdt feature of qca8081 phy Luo Jie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=af224018-1190-ac78-2035-c9763c1d46ae@codeaurora.org \
    --to=luoj@codeaurora.org \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=hkallweit1@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=netdev@vger.kernel.org \
    --cc=sricharan@codeaurora.org \
    --subject='Re: [PATCH v1 2/3] net: phy: add qca8081 ethernet phy driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).