Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 5 Sep 2020 16:47:02 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Landen Chao <landen.chao@mediatek.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Rob Herring <robh+dt@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Sean Wang <sean.wang@mediatek.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Russell King <linux@armlinux.org.uk>, <netdev@vger.kernel.org>,
        <devicetree@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>, <frank-w@public-files.de>,
        <opensource@vdorst.com>, <dqfext@gmail.com>
Subject: Re: [PATCH net-next v3 2/6] net: dsa: mt7530: Extend device data
 ready for adding a new hardware
Message-ID: <20200905164702.72edf26c@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <3cf9909d7b06346b8aa339c346e19585cccf74c8.1599228079.git.landen.chao@mediatek.com>
References: <cover.1599228079.git.landen.chao@mediatek.com>
        <3cf9909d7b06346b8aa339c346e19585cccf74c8.1599228079.git.landen.chao@mediatek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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).