Re: [PATCH net-next 5/6] net: phy: dp83869: Support SGMII SFP modules

[Andrew Lunn]

> +static int dp83869_connect_phy(void *upstream, struct phy_device *phy)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +
> +	dp83869 = phydev->priv;
> +
> +	if (dp83869->mode != DP83869_RGMII_SGMII_BRIDGE)
> +		return 0;
> +
> +	if (!phy->drv) {
> +		dev_warn(&phy->mdio.dev, "No driver bound to SFP module phy!\n");

more instances which could be phydev_{err|warn|info}().

> +		return 0;
> +	}
> +
> +	phy_support_asym_pause(phy);

That is unusual. This is normally used by a MAC driver to indicate it
supports asym pause. It is the MAC which implements pause, but the PHY
negotiates it. So this tells phylib what to advertise to the link
partner. Why would a PHY do this? What if the MAC does not support
asym pause?

> +	linkmode_set_bit(PHY_INTERFACE_MODE_SGMII, phy->host_interfaces);
> +	phy->interface = PHY_INTERFACE_MODE_SGMII;
> +	phy->port = PORT_TP;
> +
> +	phy->speed = SPEED_UNKNOWN;
> +	phy->duplex = DUPLEX_UNKNOWN;
> +	phy->pause = MLO_PAUSE_NONE;
> +	phy->interrupts = PHY_INTERRUPT_DISABLED;
> +	phy->irq = PHY_POLL;
> +	phy->phy_link_change = &dp83869_sfp_phy_change;
> +	phy->state = PHY_READY;

I don't know of any other PHY which messes with the state machine like
this. This needs some explanation.

> +
> +	dp83869->mod_phy = phy;
> +
> +	return 0;
> +}
> +
> +static void dp83869_disconnect_phy(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +
> +	dp83869 = phydev->priv;
> +	dp83869->mod_phy = NULL;
> +}
> +
> +static int dp83869_module_start(void *upstream)
> +{
> +	struct phy_device *phydev = upstream;
> +	struct dp83869_private *dp83869;
> +	struct phy_device *mod_phy;
> +	int ret;
> +
> +	dp83869 = phydev->priv;
> +	mod_phy = dp83869->mod_phy;
> +	if (!mod_phy)
> +		return 0;
> +
> +	ret = phy_init_hw(mod_phy);
> +	if (ret) {
> +		dev_err(&mod_phy->mdio.dev, "Failed to initialize PHY hardware: error %d", ret);
> +		return ret;
> +	}
> +
> +	phy_start(mod_phy);

Something else no other PHY driver does....

	Andrew