Re: [PATCH net-next v3 2/6] net: dsa: mt7530: Extend device data ready for adding a new hardware

[Jakub Kicinski <kuba@kernel.org>]

On Fri, 4 Sep 2020 22:21:57 +0800 Landen Chao wrote:
> +static bool
> +mt7530_phy_mode_supported(struct dsa_switch *ds, int port,
> +			  const struct phylink_link_state *state)
>  {
>  	struct mt7530_priv *priv = ds->priv;
> -	u32 mcr_cur, mcr_new;
>  
>  	switch (port) {
>  	case 0: /* Internal phy */
> @@ -1363,33 +1364,114 @@ static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>  	case 3:
>  	case 4:
>  		if (state->interface != PHY_INTERFACE_MODE_GMII)
> -			return;
> +			goto unsupported;

return false;

Jumping to a label which does nothing but returns makes the code less
readable.

>  		break;
>  	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
> -		if (priv->p5_interface == state->interface)
> -			break;
>  		if (!phy_interface_mode_is_rgmii(state->interface) &&
>  		    state->interface != PHY_INTERFACE_MODE_MII &&
>  		    state->interface != PHY_INTERFACE_MODE_GMII)
> -			return;
> +			goto unsupported;
> +		break;
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> +			goto unsupported;
> +		break;
> +	default:
> +		dev_err(priv->dev, "%s: unsupported port: %i\n", __func__,
> +			port);
> +		goto unsupported;
> +	}
> +
> +	return true;
> +
> +unsupported:
> +	return false;
> +}

> +static void
> +mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
> +			  const struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 mcr_cur, mcr_new;
> +
> +	if (!mt753x_phy_mode_supported(ds, port, state))
> +		goto unsupported;
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:

case 0 ... 4:

> +		if (state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
> +		if (priv->p5_interface == state->interface)
> +			break;

> +static void
> +mt753x_phylink_validate(struct dsa_switch *ds, int port,
> +			unsigned long *supported,
> +			struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };

Please keep the variables longest to shortest (reverse xmas tree).