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

[Caleb James DeLisle]

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.


> > +static int en751221_fephy_r50(struct phy_device *phydev)
> > +{
> > +	struct econet_socphy_shared *shared = phy_package_get_priv(phydev);
> > +	struct compensation ctab = get_ctab(phydev);
> > +	int zcal_sz = ARRAY_SIZE(zcal_to_r50ohm);
> > +	u8 rg_zcal_ctrl = ECONET_R50_ZCAL_DEFAULT;
> Please swap these two lines.
Whoops, thanks.
> > +	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.

In any case I'll find something that's easier to reason out at a glance.


> > +static int en751221_fephy_tx_offset(struct phy_device *phydev)
> > +{
> > +	struct econet_socphy_shared *shared = phy_package_get_priv(phydev);
> > +	struct compensation ctab = get_ctab(phydev);
> > +	int initial_comp_out;
> > +	int polarity = 0;
> > +	int offset = ECONET_TXOS_DEFAULT;
> > +	u16 offset_bin;
> > +	int comp_out;
> > +	int ret = 0;
> Another reverse christmas tree issue. Please check all your functions.
Whoops, sorry for not catching this before sending.
> > +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.
> > +
> > +	for (i = 0; i < 5; i++) {
> > +		ret = en751221_fephy_r50(phydev);
> > +		if (!ret)
> > +			break;
> > +	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < 5; i++) {
> > +		ret = en751221_fephy_tx_offset(phydev);
> > +		if (!ret)
> > +			break;
> > +	}
> > +	if (ret)
> > +		return ret;
> > +
> > +	for (i = 0; i < 5; i++) {
> > +		ret = en751221_fephy_tx_amp(phydev);
> > +		if (!ret)
> > +			break;
> > +	}
> Why 5 ?


Following the reference implementation "keep trying until it's very 
clear that it's not going to work". But this should be a define and it 
slipped my mind to do so.


Thank you for your review!


Caleb

>      Andrew
>
> ---
> pw-bot: cr