Re: [PATCH RFC russell-king 2/4] net: phy: sfp: add support for multigig RollBall modules

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, Aug 11, 2020 at 12:06:43AM +0200, Marek Behún wrote:
> This adds support for multigig copper SFP modules from RollBall/Hilink.
> These modules have a specific way to access clause 45 registers of the
> internal PHY.
> 
> We also need to wait at least 25 seconds after deasserting TX disable
> before accessing the PHY. The code waits for 30 seconds just to be sure.

I wonder why it takes so long - the 88x3310 boots very quickly.

> +static int sfp_i2c_mii_probe(struct sfp *sfp)
> +{
> +	struct mii_bus *i2c_mii;
> +	int ret;
> +
> +	if (sfp->rollball_mii)
> +		i2c_mii = mdio_i2c_rollball_alloc(sfp->dev, sfp->i2c);
> +	else
> +		i2c_mii = mdio_i2c_alloc(sfp->dev, sfp->i2c);
> +

I think I'd prefer to find a way to teach the existing mdio_i2c code
about different protocols rather than having a load of different buses
that we have to select between.  Maybe we can do that via the PHY
address, or maybe we should have a call into mdio_i2c that tells it
which protocol should be used?

The reason I don't like this approach is it seems to me to be very
heavy handed... using a sledge hammer to crack a nut.

> +	rollball = (!memcmp(id.base.vendor_name, "OEM             ", 16) &&
> +		    (!memcmp(id.base.vendor_pn, "SFP-10G-T       ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-10        ", 16) ||
> +		     !memcmp(id.base.vendor_pn, "RTSFP-2.5G      ", 16)));
> +	if (rollball) {
> +		/* TODO: try to write this to EEPROM */
> +		id.base.extended_cc = SFF8024_ECC_10GBASE_T_SFI;

Should we really be "fixing" vendors EEPROMs for them?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!