Re: [PATCH net-next v3 11/11] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration

[Florian Fainelli <f.fainelli@gmail.com>]

On 09/14/18 01:16, Quentin Schulz wrote:
> Previously, the SerDes muxing was hardcoded to a given mode in the MAC
> controller driver. Now, the SerDes muxing is configured within the
> Device Tree and is enforced in the MAC controller driver so we can have
> a lot of different SerDes configurations.
> 
> Make use of the SerDes PHYs in the MAC controller to set up the SerDes
> according to the SerDes<->switch port mapping and the communication mode
> with the Ethernet PHY.

This looks good, just a few comments below:

[snip]

> +		err = of_get_phy_mode(portnp);
> +		if (err < 0)
> +			ocelot->ports[port]->phy_mode = PHY_INTERFACE_MODE_NA;
> +		else
> +			ocelot->ports[port]->phy_mode = err;
> +
> +		switch (ocelot->ports[port]->phy_mode) {
> +		case PHY_INTERFACE_MODE_NA:
> +			continue;

Would not you want to issue a message indicating that the Device Tree
must be updated here? AFAICT with your patch series, this should no
longer be a condition that you will hit unless you kept the old DTB
around, right?

> +		case PHY_INTERFACE_MODE_SGMII:
> +			phy_mode = PHY_MODE_SGMII;
> +			break;
> +		case PHY_INTERFACE_MODE_QSGMII:
> +			phy_mode = PHY_MODE_QSGMII;
> +			break;
> +		default:
> +			dev_err(ocelot->dev,
> +				"invalid phy mode for port%d, (Q)SGMII only\n",
> +				port);
> +			return -EINVAL;
> +		}
> +
> +		serdes = devm_of_phy_get(ocelot->dev, portnp, NULL);
> +		if (IS_ERR(serdes)) {
> +			err = PTR_ERR(serdes);
> +			if (err == -EPROBE_DEFER) {

This can be simplified into:

			if (err == -EPROBE_DEFER)
				dev_dbg();
			else
				dev_err();
			goto err_probe_ports;

> +				dev_dbg(ocelot->dev, "deferring probe\n");
> +				goto err_probe_ports;
> +			}
> +
> +			dev_err(ocelot->dev, "missing SerDes phys for port%d\n",
> +				port);
>  			goto err_probe_ports;
>  		}
> +
> +		ocelot->ports[port]->serdes = serdes;
>  	}
>  
>  	register_netdevice_notifier(&ocelot_netdevice_nb);
> 

-- 
Florian