Re: [PATCH net-next] net: phy: mediatek: add driver for EcoNet Fast Ethernet SoC PHYs

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Aug 26, 2026 at 02:08:50AM +0200, Caleb James DeLisle wrote:
> 
> On 25/08/2026 21:40, Andrew Lunn wrote:
> > > Also provide support for the older EN7512 Fast Ethernet SoC PHYs found
> > > in EN751221 chips with do not have the MCM switch. That is chips which
> > > do not have a "G" in the name. As these PHYs bear the ID 03a2.9412
> > > which collides with MTK_GPHY_ID_MT7530 gigabit PHY, do not match them
> > > and instead rely on the user to override the PHY ID in the device tree
> > > if they wish to use this driver.
> > Is there a way to tell them apart using other registers?
> 
> 
> The reference code differentiates this PHY from the other because this one
> does not advertise gigabit capability. I sent it like this because the idea
> of a matcher made me nervous and this felt more conservative, but I'm open
> to guidance about what is the most appropriate solution.

If it is as simple as that, i suggest you detect it at runtime.

In the driver structure, you provide a match_phy_device()
function. This get called independent of what ID value you have in the
structure. So you first need to check if the ID matches. Then check
the 1G capability.

Ideally you want the code in the same driver, because module loading
happens based on the ID. I'm not sure user space will load two drivers
if they both indicate the same ID. Something you can experiment with.

> > > +	for (;;) {
> > It is unusual to do loops like this. Can it be turned into a do while
> > loop? And without looking deep into it, it is not clear if this is
> > endless if the hardware stops responding.
> 
> 
> You raise a good point in that it's hard to reason out the default exit
> scenario, and I will figure out how to improve that. I'm not sure I can
> actually get rid of the infinite loop without making the code worse, because
> the default exit condition needs to assign ret and goto the error out label.

Consider adding comments as well. Also consider replacing the infinite
loop with a bounded loop, and return -EIO if the it goes around the
loop too many times. What you are trying to avoid is on hardware error
a CPU constantly spinning until power off.

> > > +static int en751221_fephy_config_init(struct phy_device *phydev)
> > > +{
> > > +	struct econet_socphy_shared *shared = phy_package_get_priv(phydev);
> > > +	u16 l0r26_temp;
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	if (!shared->phydev_p0) {
> > > +		phydev_err(phydev, "Port zero must be configured\n");
> > > +		return -EOPNOTSUPP;
> > > +	}
> > What is the issue here? Why must port 0 be first?
> It doesn't need to be configured first but it must be probed because these
> PHYs are not independent, they exist as a group and some configuration must
> be done on the first phy in the group. I will re-send with clarification in
> a comment.

Consider using phy_package_init_once() to configure the shared things
by the first PHY to probe, independent of what address it is.

	Andrew