Re: [PATCH 2/2] net: phy: Add driver for Motorcomm yt8821 2.5G ethernet phy

[Simon Horman]

On Sat, Jul 27, 2024 at 02:20:31AM -0700, Frank.Sae wrote:
>  Add a driver for the motorcomm yt8821 2.5G ethernet phy.
>  Verified the driver on
>  BPI-R3(with MediaTek MT7986(Filogic 830) SoC) development board,
>  which is developed by Guangdong Bipai Technology Co., Ltd..
>  On the board, yt8821 2.5G ethernet phy works in
>  AUTO_BX2500_SGMII or FORCE_BX2500 interface,
>  supports 2.5G/1000M/100M/10M speeds, and wol(magic package).
>  Since some functions of yt8821 are similar to YT8521
>  so some functions for yt8821 can be reused.
> 
> Signed-off-by: Frank.Sae <Frank.Sae@xxxxxxxxxxxxxx>

Hi Frank,

This is not a full review. And setting up expectations,
as per the form letter below, net-next is currently closed.
But nonetheless I've provided some minor review below.

## Form letter - net-next-closed

(Adapted from text by Jakub)

The merge window for v6.11 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after 29th July.

RFC patches sent for review only are welcome at any time.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle

pw-bot: defer

> ---
>  drivers/net/phy/motorcomm.c | 639 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 636 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c

...

> +/**
> + * yt8821_probe() - read dts to get chip mode
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * returns 0 or negative errno code
> + */

nit: please document return values using a "Return:" or "Returns:" section.

Flagged by W=1 allmodconfig builds and ./scripts/kernel-doc -none -Warn

...

> +/**
> + * yt8821_config_init() - phy initializatioin
> + * @phydev: a pointer to a &struct phy_device
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8821_config_init(struct phy_device *phydev)
> +{
> +	struct yt8821_priv *priv = phydev->priv;
> +	int ret, val;
> +
> +	phydev->irq = PHY_POLL;
> +
> +	val = ytphy_read_ext_with_lock(phydev, YT8521_CHIP_CONFIG_REG);

val is set here but otherwise unused.
Should val be checked for an error here?

Flagged by W=1 builds.

> +	if (priv->chip_mode == YT8821_CHIP_MODE_AUTO_BX2500_SGMII) {
> +		ret = ytphy_modify_ext_with_lock(phydev,
> +						 YT8521_CHIP_CONFIG_REG,
> +						 YT8521_CCR_MODE_SEL_MASK,
> +						 FIELD_PREP(YT8521_CCR_MODE_SEL_MASK, 0));
> +		if (ret < 0)
> +			return ret;
> +
> +		__assign_bit(PHY_INTERFACE_MODE_2500BASEX,
> +			     phydev->possible_interfaces,
> +			     true);
> +		__assign_bit(PHY_INTERFACE_MODE_SGMII,
> +			     phydev->possible_interfaces,
> +			     true);
> +
> +		phydev->rate_matching = RATE_MATCH_NONE;
> +	} else if (priv->chip_mode == YT8821_CHIP_MODE_FORCE_BX2500) {
> +		ret = ytphy_modify_ext_with_lock(phydev,
> +						 YT8521_CHIP_CONFIG_REG,
> +						 YT8521_CCR_MODE_SEL_MASK,
> +						 FIELD_PREP(YT8521_CCR_MODE_SEL_MASK, 1));
> +		if (ret < 0)
> +			return ret;
> +
> +		phydev->rate_matching = RATE_MATCH_PAUSE;
> +	}
> +
> +	ret = yt8821gen_init(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* disable auto sleep */
> +	ret = yt8821_auto_sleep_config(phydev, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* soft reset */
> +	yt8821_soft_reset(phydev);
> +
> +	return ret;
> +}
> +
> +/**
> + * yt8821_adjust_status() - update speed and duplex to phydev
> + * @phydev: a pointer to a &struct phy_device
> + * @val: read from YTPHY_SPECIFIC_STATUS_REG
> + */
> +static void yt8821_adjust_status(struct phy_device *phydev, int val)
> +{
> +	int speed_mode, duplex;
> +	int speed_mode_low, speed_mode_high;
> +	int speed = SPEED_UNKNOWN;

nit: Please consider arranging local variables in reverse xmas tree order -
     longest line to shortest.

     This can be helpful: https://github.com/ecree-solarflare/xmastree

> +
> +	duplex = FIELD_GET(YTPHY_SSR_DUPLEX, val);
> +
> +	speed_mode_low = FIELD_GET(GENMASK(15, 14), val);
> +	speed_mode_high = FIELD_GET(BIT(9), val);
> +	speed_mode = FIELD_PREP(BIT(2), speed_mode_high) |
> +			FIELD_PREP(GENMASK(1, 0), speed_mode_low);
> +	switch (speed_mode) {
> +	case YTPHY_SSR_SPEED_10M:
> +		speed = SPEED_10;
> +		break;
> +	case YTPHY_SSR_SPEED_100M:
> +		speed = SPEED_100;
> +		break;
> +	case YTPHY_SSR_SPEED_1000M:
> +		speed = SPEED_1000;
> +		break;
> +	case YT8821_SSR_SPEED_2500M:
> +		speed = SPEED_2500;
> +		break;
> +	default:
> +		speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +
> +	phydev->speed = speed;
> +	phydev->duplex = duplex;
> +}

...