Re: [PATCH net-next v2 2/2] net: phy: dp83td510: Add support for the DP83TD510 Ethernet PHY

[Andrew Lunn]

On Wed, May 04, 2022 at 01:06:55PM +0200, Oleksij Rempel wrote:
> The DP83TD510E is an ultra-low power Ethernet physical layer transceiver
> that supports 10M single pair cable.
> 
> This driver was tested with NXP SJA1105, STMMAC and ASIX AX88772B USB Ethernet
> controller.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>

Thanks for the re-write. This looks a lot better.

> +		ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L);
> +		if (ret < 0)
> +			return ret;
> +
> +		cfg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M);
> +		if (cfg < 0)
> +			return cfg;
> +
> +		if (ret & MDIO_AN_T1_ADV_L_FORCE_MS) {
> +			if (cfg & MDIO_AN_T1_ADV_M_MST)
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_MASTER_FORCE;
> +			else
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_SLAVE_FORCE;
> +		} else {
> +			if (cfg & MDIO_AN_T1_ADV_M_MST)
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_MASTER_PREFERRED;
> +			else
> +				phydev->master_slave_get =
> +					MASTER_SLAVE_CFG_SLAVE_PREFERRED;
> +		}

Is this identical to genphy_c45_baset1_read_status()? I don't think
there are any odd vendor registers here. So i suggest exporting it and
making use of it.

> +static int dp83td510_config_master_slave(struct phy_device *phydev)
> +{
> +	int ctl = 0;
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		ctl = MDIO_PMA_PMD_BT1_CTRL_CFG_MST;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return phy_modify_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_PMD_BT1_CTRL,
> +			     MDIO_PMA_PMD_BT1_CTRL_CFG_MST, ctl);

Is this identical to what is in genphy_c45_pma_setup_forced()? If so,
i would pull that code out into a helper and make use of it.


Thanks
	Andrew