Re: [PATCH net-next 5/5] net: phy: adin1100: Skip the soft reset on ADIN1110/ADIN2111

[Andrew Lunn <andrew@lunn.ch>]

> > @@ -232,6 +232,15 @@ static int adin_soft_reset(struct phy_device *phydev)
> >  {
> >  	int ret;
> >  
> > +	/* On the ADIN1110/ADIN2111 MAC-PHYs, CRSM_SFT_RST resets the whole
> > +	 * chip, undoing the MAC driver's setup, and MDIO reads return stale
> > +	 * data until it completes. The MAC driver has already reset the
> > +	 * chip, so skip it here.
> > +	 */
> > +	if (phy_id_compare_model(phydev->phy_id, PHY_ID_ADIN1110) ||
> > +	    phy_id_compare_model(phydev->phy_id, PHY_ID_ADIN2111))
> > +		return 0;
> 
> It seems more common to have a separate struct phy_driver entry in the
> array passed to module_phy_driver() with a more precise
> phy_id/mask. Then for ADIN1110/ADIN2111 you'd omit the
> .soft_reset. Maybe Andrew knows what is preferred here?

Humm...

#define PHY_ID_ADIN1100				0x0283bc81
#define PHY_ID_ADIN1110				0x0283bc91
#define PHY_ID_ADIN2111				0x0283bca1

static struct phy_driver adin_driver[] = {
	{
		.phy_id			= PHY_ID_ADIN1100,
		.phy_id_mask		= 0xffffffcf,
		.name			= "ADIN1100",

That phy_id_mask is funky.

static const struct mdio_device_id __maybe_unused adin_tbl[] = {
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN1100) },
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN1110) },
	{ PHY_ID_MATCH_MODEL(PHY_ID_ADIN2111) },
	{ }

And this contradicts it.

Please leaf through the datasheet for these devices and check how the
ID is defined. Often the lowest nibble is the revision, so 0x0283bc81
and 0x0283bc8f would be the same device, just different bugs.
However, they can be different devices. This is why we have:

#define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = PHY_ID_MATCH_EXTACT_MASK
#define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = PHY_ID_MATCH_MODEL_MASK
#define PHY_ID_MATCH_VENDOR(id) .phy_id = (id), .phy_id_mask = PHY_ID_MATCH_VENDOR_MASK

Since they are different devices, it would be good to have multiple
struct phy_driver, if that can be done without breaking backwards
compatibility.

	Andrew