LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Luo Jie <luoj@codeaurora.org>
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: Mon, 30 Aug 2021 15:48:36 +0200	[thread overview]
Message-ID: <YSzhtF8g42Ccv2h0@lunn.ch> (raw)
In-Reply-To: <20210830110733.8964-3-luoj@codeaurora.org>

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.

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

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

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

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


  parent reply	other threads:[~2021-08-30 13:48 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 [this message]
2021-08-31 14:10     ` Jie Luo
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=YSzhtF8g42Ccv2h0@lunn.ch \
    --to=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=luoj@codeaurora.org \
    --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).