Re: [PATCH net-next] net: phy: add helpers for comparing phy IDs

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, May 19, 2023 at 05:59:02PM +0200, Andrew Lunn wrote:
> On Fri, May 19, 2023 at 02:03:59PM +0100, Russell King wrote:
> > There are several places which open code comparing PHY IDs. Provide a
> > couple of helpers to assist with this, using a slightly simpler test
> > than the original:
> > 
> > - phy_id_compare() compares two arbitary PHY IDs and a mask of the
> >   significant bits in the ID.
> > - phydev_id_compare() compares the bound phydev with the specified
> >   PHY ID, using the bound driver's mask.
> 
> Hi Russell
> 
> I think these are useful, but i'm wondering about naming. In the PHY
> drivers we use these macros:
> 
> #define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 0)
> #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
> #define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 10)
> 
> when creating the tables. 
> 
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index 3f81bb8dac44..2094d49025a7 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -637,7 +637,7 @@ static int ksz8051_ksz8795_match_phy_device(struct phy_device *phydev,
> >  {
> >  	int ret;
> >  
> > -	if ((phydev->phy_id & MICREL_PHY_ID_MASK) != PHY_ID_KSZ8051)
> > +	if (!phy_id_compare(phydev->phy_id, PHY_ID_KSZ8051, MICREL_PHY_ID_MASK))
> >  		return 0;
> 
> This could be phy_id_compare_model() 

Note that this can't access phydev->drv, because this is a match method
and in all likelyhood, phydev->drv is NULL here.

> phy_id_compare_exact() could be used in a number of places, eg.
> 
> vitesse.c:			(phydev->drv->phy_id == PHY_ID_VSC8234 ||
> vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8244 ||
> vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8572 ||
> vitesse.c:			 phydev->drv->phy_id == PHY_ID_VSC8601) ?
> motorcomm.c:	} else if (phydev->drv->phy_id == PHY_ID_YT8531S) {
> marvell10g.c:	if (phydev->drv->phy_id != MARVELL_PHY_ID_88X3310)

However, these are not comparing the PHY ID, they're comparing the
driver's idea of the PHY ID. I would expect phy_id_compare_exact()
to be comparing phydev->phy_id!

Many places can use phydev->drv->phy_id == foo to identify which
driver was matched. If the driver's phy_id is set my a macro
definition, then that same macro identifier can be used with a
straight compare like the above, and I think open coding those
like that is entirely reasonable and proper.

However, phydev->phy_id is much more complex because it generally
needs the revision masking away, which is what these helpers are
achieving.

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