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

[Andrew Lunn <andrew@lunn.ch>]

> 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?

> +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.

> +	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.

> +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.

> +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?

> +
> +	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 ?


    Andrew

---
pw-bot: cr