Re: [v4,4/4] net: phy: realtek: add RTL8224 polarity 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 polarity support

This patch adds MDI polarity swap support for RTL8224. The polarity
configuration uses 4 bits per port (one for each pair A-D) and is read
from the device tree property enet-phy-lane-polarity.

> diff --git a/drivers/net/phy/realtek/realtek_main.c b/drivers/net/phy/realtek/realtek_main.c
> --- a/drivers/net/phy/realtek/realtek_main.c
> +++ b/drivers/net/phy/realtek/realtek_main.c

[ ... ]

> +static int rtl8224_mdi_config_polarity(struct phy_device *phydev)
> +{
> +	struct device_node *np = phydev->mdio.dev.of_node;
> +	u8 port_offset = phydev->mdio.addr & 3;
> +	u32 polarity = 0;
> +	int ret, val;
> +
> +	ret = of_property_read_u32(np, "enet-phy-lane-polarity", &polarity);
> +
> +	/* Do nothing if the property is not present */
> +	if (ret == -EINVAL)
> +		return 0;
> +
> +	if (ret)
> +		return ret;
> +
> +	if (polarity & ~0xf)
> +		return -EINVAL;
> +
> +	val = __phy_package_read_mmd(phydev, 0, MDIO_MMD_VEND1,
> +				     RTL8224_VND1_MDI_POLARITY_SWAP);
> +	if (val < 0)
> +		return val;
> +
> +	val &= ~(0xf << port_offset);
> +	val |= polarity << port_offset;
            ^^^^^^^^^^^^^^^^^^^^^^^^

The shift amount appears incorrect for a 4-bit-per-port register layout.
Since polarity uses 4 bits (for pairs A-D) and port_offset ranges from
0 to 3, should the shift be (port_offset * 4) rather than just port_offset?

With the current code:
  - Port 0 (offset=0): writes to bits 0-3 (correct)
  - Port 1 (offset=1): writes to bits 1-4 (overlaps port 0)
  - Port 2 (offset=2): writes to bits 2-5 (overlaps ports 0-1)
  - Port 3 (offset=3): writes to bits 3-6 (overlaps ports 0-2)

The earlier v1 patch posted to lore used "(phydev->mdio.addr & 3) * 4"
for the offset calculation. Was the multiplication by 4 accidentally
dropped during refactoring?

For comparison, rtl8224_mdi_config_order() uses "1 << port_offset" which
is correct for a 1-bit-per-port layout, but the polarity register needs
4 bits per port.

> +	return __phy_package_write_mmd(phydev, 0, MDIO_MMD_VEND1,
> +				       RTL8224_VND1_MDI_POLARITY_SWAP, val);
> +}