Re: [PATCH net-next v9 3/3] net: dsa: yt921x: Add support for Motorcomm YT921x

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sat, Sep 13, 2025 at 12:44:01PM +0800, David Yang wrote:
> +static int
> +yt921x_port_config(struct yt921x_priv *priv, int port, unsigned int mode,
> +		   phy_interface_t interface)
> +{
> +	struct device *dev = to_device(priv);
> +	u32 mask;
> +	u32 ctrl;
> +	int res;
> +
> +	if (!yt921x_port_is_external(port)) {
> +		if (interface != PHY_INTERFACE_MODE_INTERNAL) {
> +			dev_err(dev, "Wrong mode %d on port %d\n",
> +				interface, port);
> +			return -EINVAL;
> +		}
> +		return 0;
> +	}
> +
> +	switch (interface) {
> +	/* SGMII */
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_100BASEX:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		mask = YT921X_SGMII_CTRL_PORTn(port);
> +		res = yt921x_reg_set_bits(priv, YT921X_SGMII_CTRL, mask);
> +		if (res)
> +			return res;
> +
> +		mask = YT921X_XMII_CTRL_PORTn(port);
> +		res = yt921x_reg_clear_bits(priv, YT921X_XMII_CTRL, mask);
> +		if (res)
> +			return res;
> +
> +		mask = YT921X_SGMII_MODE_M;
> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_SGMII:
> +			ctrl = YT921X_SGMII_MODE_SGMII_PHY;

Does this mean that "SGMII" here means you are sending SGMII
speed/duplex to a host MAC (in other words, immitating a PHY) ?

If yes, then I think we want PHY_INTERFACE_MODE_REVSGMII added.

> +static void
> +yt921x_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
> +			     phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct dsa_switch *ds = dp->ds;
> +	struct yt921x_priv *priv = to_yt921x_priv(ds);
> +	struct device *dev = to_device(priv);
> +	int port = dp->index;
> +	int res;

Reverse christmas tree ordering please, and do we need "struct device
*dev..." because ds->dev should be this same device. Maybe:

	struct dsa_port *dp = dsa_phylink_to_port(config);
	struct yt921x_priv *priv = to_yt921x_priv(dp->ds);
	int port = dp->index;
	int res;

> +
> +	cancel_delayed_work(&priv->ports[port].mib_read);
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt921x_port_down(priv, port);
> +	mutex_unlock(&priv->reg_lock);
> +
> +	if (res)
> +		dev_err(dev, "Failed to %s port %d: %i\n", "bring down",
> +			port, res);

Then:
		dev_err(dp->ds->dev, ...

Same comment for the other phylink methods.

Thanks.

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