Re: [net-next RFC PATCH v3 3/4] net: phy: Add support for Aeonsemi AS21xxx PHYs

[Russell King (Oracle)]

On Thu, Mar 27, 2025 at 12:35:03AM +0100, Christian Marangi wrote:
> +static int as21xxx_match_phy_device(struct phy_device *phydev,
> +				    const struct phy_driver *phydrv)
> +{
> +	struct as21xxx_priv *priv;
> +	u32 phy_id;
> +	int ret;
> +
> +	/* Skip PHY that are not AS21xxx or already have firmware loaded */
> +	if (phydev->c45_ids.device_ids[MDIO_MMD_PCS] != PHY_ID_AS21XXX)
> +		return phydev->phy_id == phydrv->phy_id;

Isn't phydev->phy_id zero here for a clause 45 PHY? If the firmware
has been loaded, I believ eyou said that PHY_ID_AS21XXX won't be
used, so the if() will be true, and because we've read clause 45
IDs, phydev->phy_id will be zero meaning this will never match. So
a PHY with firmware loaded won't ever match any of these drivers.
This is probably not what you want.

I'd suggest converting the tail of phy_bus_match() so that you can
call that to do the standard matching using either C22 or C45 IDs
as appropriate without duplicating that code.

> +
> +	/* Read PHY ID to handle firmware just loaded */
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID1);
> +	if (ret < 0)
> +		return ret;
> +	phy_id = ret << 16;
> +
> +	ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MII_PHYSID2);
> +	if (ret < 0)
> +		return ret;
> +	phy_id |= ret;
> +
> +	/* With PHY ID not the generic AS21xxx one assume
> +	 * the firmware just loaded
> +	 */
> +	if (phy_id != PHY_ID_AS21XXX)
> +		return phy_id == phydrv->phy_id;
> +
> +	/* Allocate temp priv and load the firmware */
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	mutex_init(&priv->ipc_lock);
> +
> +	ret = aeon_firmware_load(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_ipc_sync_parity(phydev, priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable PTP clk if not already Enabled */
> +	ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> +			       VEND1_PTP_CLK_EN);
> +	if (ret)
> +		return ret;
> +
> +	ret = aeon_dpc_ra_enable(phydev, priv);
> +	if (ret)
> +		return ret;
> +
> +	mutex_destroy(&priv->ipc_lock);
> +	kfree(priv);
> +
> +	/* Return not maching anyway as PHY ID will change after
> +	 * firmware is loaded.

Also "This relies on the driver probe order."

> +	 */
> +	return 0;
> +}
> +
> +static struct phy_driver as21xxx_drivers[] = {
> +	{
> +		/* PHY expose in C45 as 0x7500 0x9410
> +		 * before firmware is loaded.

Also "This driver entry must be attempted first to load the firmware and
thus update the ID registers."

> +		 */
> +		PHY_ID_MATCH_EXACT(PHY_ID_AS21XXX),
> +		.name		= "Aeonsemi AS21xxx",
> +		.match_phy_device = as21xxx_match_phy_device,
> +	},
> +	{
> +		PHY_ID_MATCH_EXACT(PHY_ID_AS21011JB1),
> +		.name		= "Aeonsemi AS21011JB1",
> +		.probe		= as21xxx_probe,
> +		.match_phy_device = as21xxx_match_phy_device,
> +		.read_status	= as21xxx_read_status,
> +		.led_brightness_set = as21xxx_led_brightness_set,
> +		.led_hw_is_supported = as21xxx_led_hw_is_supported,
> +		.led_hw_control_set = as21xxx_led_hw_control_set,
> +		.led_hw_control_get = as21xxx_led_hw_control_get,
> +		.led_polarity_set = as21xxx_led_polarity_set,

If I'm reading these driver entries correctly, the only reason for
having separate entries is to be able to have a unique name printed
for each - the methods themselves are all identical.

My feeling is that is not a sufficient reason to duplicate the driver
entries, which adds bloat (not only in terms of static data, but also
the data structures necessary to support each entry in sysfs.) However,
lets see what Andrew says.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!