Re: [net-next,v8,2/4] net: phy: realtek: add RTL8224 pair order support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: phy: realtek: add RTL8224 pair order support

The RTL8224 has a register to configure a pair swap (from ABCD order to
DCBA) providing PCB designers more flexbility when wiring the chip. The
swap parameter has to be set correctly for each of the 4 ports before
the chip can detect a link.

> diff --git a/drivers/net/phy/realtek/Kconfig b/drivers/net/phy/realtek/Kconfig
> index b05c2a1e9024..a741b34d193e 100644
> --- a/drivers/net/phy/realtek/Kconfig
> +++ b/drivers/net/phy/realtek/Kconfig

[ ... ]

> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> index 530b4e26d16e..2feb085f5c9e 100644
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c

[ ... ]

> @@ -1820,6 +1822,66 @@ static int rtl8224_cable_test_get_status(struct phy_device *phydev, bool *finish
>  	return rtl8224_cable_test_report(phydev, finished);
>  }
>
> +static int rtl8224_package_modify_mmd(struct phy_device *phydev, int devad,
> +				      u32 regnum, u16 mask, u16 set)
> +{
> +	int val, ret;
> +
> +	phy_lock_mdio_bus(phydev);
> +
> +	val = __phy_package_read_mmd(phydev, 0, devad, regnum);
> +	if (val < 0) {
> +		ret = val;
> +		goto exit;
> +	}
> +
> +	val &= ~mask;
> +	val |= set;
> +
> +	ret = __phy_package_write_mmd(phydev, 0, devad, regnum, val);
> +
> +exit:
> +	phy_unlock_mdio_bus(phydev);
> +	return ret;
> +}
> +
> +static int rtl8224_mdi_config_order(struct phy_device *phydev)
> +{
> +	struct device_node *np = phydev->mdio.dev.of_node;
> +	u8 port_offset = phydev->mdio.addr & 3;
> +	u32 order = 0;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "enet-phy-pair-order", &order);
> +
> +	/* Do nothing in case the property is not present */
> +	if (ret == -EINVAL)
> +		return 0;

Does this handle the CONFIG_OF=n case correctly?

When CONFIG_OF is disabled, of_property_read_u32() returns -ENOSYS (see
include/linux/of.h:736), not -EINVAL. The check above only handles -EINVAL,
so the subsequent error check propagates -ENOSYS:

static int rtl8224_mdi_config_order(struct phy_device *phydev)
{
	...
	ret = of_property_read_u32(np, "enet-phy-pair-order", &order);

	if (ret == -EINVAL)
		return 0;

	if (ret)
		return ret;  /* -ENOSYS propagates here */
	...
}

This causes rtl8224_config_init() to return -ENOSYS, which then propagates
through phy_init_hw(), failing RTL8224 PHY initialization on non-OF kernel
configurations.

REALTEK_PHY has no 'depends on OF' in its Kconfig, so CONFIG_OF=n is a valid
build configuration. Before this patch, RTL8224 had no config_init callback,
so this is a new failure path.

Could the code handle -ENOSYS in addition to -EINVAL, or use
device_property_read_u32() which is firmware-agnostic?

> +
> +	if (ret)
> +		return ret;
> +
> +	if (order & ~1)
> +		return -EINVAL;
> +
> +	return rtl8224_package_modify_mmd(phydev, MDIO_MMD_VEND1,
> +					  RTL8224_VND1_MDI_PAIR_SWAP,
> +					  BIT(port_offset),
> +					  order ? BIT(port_offset) : 0);
> +}
> +
> +static int rtl8224_config_init(struct phy_device *phydev)
> +{
> +	return rtl8224_mdi_config_order(phydev);
> +}

[ ... ]
-- 
pw-bot: cr