Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

[Russell King (Oracle)]

On Mon, Mar 17, 2025 at 02:34:52PM +0800, Jim Liu wrote:
> Use "BRCM_PHY_MODEL" can be applied to the entire 5221 family of PHYs.
> 
> Fixes: 3abbd0699b67 ("net: phy: broadcom: add support for BCM5221 phy")
> Signed-off-by: Jim Liu <jim.t90615@xxxxxxxxx>

Looking at BRCM_PHY_MODEL() and BRCM_PHY_REV(), I think there's more
issues with this driver. E.g.:

#define BRCM_PHY_MODEL(phydev) \
        ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)

#define BRCM_PHY_REV(phydev) \
        ((phydev)->drv->phy_id & ~((phydev)->drv->phy_id_mask))

#define PHY_ID_BCM50610                 0x0143bd60
#define PHY_ID_BCM50610M                0x0143bd70

        if ((BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610 ||
             BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610M) &&
            BRCM_PHY_REV(phydev) >= 0x3) {

and from the PHY driver table:

        .phy_id         = PHY_ID_BCM50610,
        .phy_id_mask    = 0xfffffff0,

        .phy_id         = PHY_ID_BCM50610M,
        .phy_id_mask    = 0xfffffff0,

BRCM_PHY_REV() looks at _this_ .phy_id in the table, and tries to match
it against the revision field bits 0-3 being >= 3 - but as we can see,
this field is set to the defined value which has bits 0-3 always as
zero. So, this if() statement is always false.

So, BRCM_PHY_REV() should be:

#define BRCM_PHY_REV(phydev) \
	((phydev)->phy_id & ~(phydev)->drv->phy_id_mask)


Next, I question why BRCM_PHY_MODEL() exists in the first place.
phydev->drv->phy_id is initialised to the defined value(s), and then
we end up doing:

	(phydev->drv->phy_id & phydev->drv->phy_id_mask) ==
		one-of-those-defined-values

which is pointless, because we know that what is in phydev->drv->phy_id
/is/ one-of-those-defined-values.

Therefore, I would suggest:

#define BRCM_PHY_MODEL(phydev) ((phydev)->drv->phy_id)

is entirely sufficient, and with such a simple definition, I question
the value of BRCM_PHY_MODEL() existing.

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