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

[Andrew Lunn]

> +static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
> +			     u16 regnum)
> +{
> +	/* Write the desired MMD Devad */
> +	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL, devad);
> +
> +	/* Write the desired MMD register address */
> +	__mdiobus_write(bus, phy_addr, MII_MMD_DATA, regnum);
> +
> +	/* Select the Function : DATA with no post increment */
> +	__mdiobus_write(bus, phy_addr, MII_MMD_CTRL,
> +			devad | MII_MMD_CTRL_NOINCR);
> +}

Please make the version in phy-core.c global scope.

A better explanation of what is going on here would be good.

> +	/* This PHY supports only C22 MDIO opcodes. We can use only indirect
> +	 * access.
> +	 */
> +	mmd_phy_indirect(bus, phy_addr, devad, regnum);

This comment suggests it is because it cannot do C45. But the core
should handle this, it would use indirect access. However, you have
hijacked phydev->drv->read_mmd to allow you to translate standard
registers to vendor registers. This bypasses the cores fallback to
indirect access.

> +static struct phy_driver dp83td510_driver[] = {
> +{
> +	PHY_ID_MATCH_MODEL(DP83TD510E_PHY_ID),
> +	.name		= "TI DP83TD510E",
> +
> +	.config_aneg	= genphy_c45_config_aneg,
> +	.read_status	= genphy_c45_read_status,
> +	.get_features	= dp83td510_get_features,
> +	.config_intr	= dp83td510_config_intr,
> +	.handle_interrupt = dp83td510_handle_interrupt,
> +
> +	.suspend	= genphy_suspend,
> +	.resume		= genphy_resume,
> +	.read_mmd	= dp83td510_read_mmd,
> +	.write_mmd	= dp83td510_write_mmd,

Given how far this PHY is away from standards, you might get a smaller
simpler driver if you ignore genphy all together, write your own
config_aneg and read_status, and don't mess with .read_mmd and
write_mmd.

	Andrew