Re: [RFC net-next v2] net:dsa:yt922x: Add support for Motorcomm YT922x

[Andrew Lunn <andrew@lunn.ch>]

>  drivers/net/dsa/Kconfig  |   7 +-
>  drivers/net/dsa/Makefile |   2 +-
>  drivers/net/dsa/yt921x.c | 819 ++++++++++++++++++++++++++++++++++++++-

That is a lot of new code. Please try to break it up into a number of
smaller patches, with good commit messages, which are obviously
correct.

Since this is a new device, it does not need to work with a big
bang. You can slowly add the needed bits, such that when the last
patch is merged the device is functional.

> +#define YT922X_INTERNAL_SDS1_PHYADDR 0
> +#define YT922X_INTERNAL_UTP0_PHYADDR 4
> +#define YT922X_INTERNAL_UTP1_PHYADDR 5
> +#define YT922X_INTERNAL_UTP2_PHYADDR 6
> +#define YT922X_INTERNAL_UTP3_PHYADDR 7
> +#define YT922X_INTERNAL_SDS0_PHYADDR 8

These don't seem to be used. Generally, a MAC driver does not need to
know the mapping to PHY addresses, the phandles in the DT indicates
it.

>  static int yt921x_mbus_int_read(struct mii_bus *mbus, int port, int reg)
>  {
>  	struct yt921x_priv *priv = mbus->priv;
> +	int max_ports;
>  	u16 val;
>  	int res;
>  
> -	if (port >= YT921X_PORT_NUM)
> +	max_ports = priv->series_info->ports;
> +	if (port >= max_ports)

This sort of code change would make a nice simple patch. Change all
current instances of YT921X_PORT_NUM to priv->series_info->ports. Easy
to review.

> @@ -4748,6 +4766,13 @@ static int yt921x_dsa_setup(struct dsa_switch *ds)
>  	struct device_node *child;
>  	int res;
>  
> +	for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
> +		struct yt921x_port *pp = &priv->ports[i];
> +
> +		pp->index = i;
> +		INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
> +	}

This should be a patch, with an explanation why it is needed. Also
please take a look at ethtool -c stats-block-usecs, which indicates
how out of date the statistics are. It is currently not well
supported, i only learned of it recently, but setting it will help you
get the self tests passing.

> +static void
> +yt922x_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 yt921x_priv *priv = to_yt921x_priv(dp->ds);
> +	int port = dp->index;
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +	res = yt922x_port_down(priv, port);
> +	mutex_unlock(&priv->reg_lock);
> +
> +	if (res)
> +		dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring down",
> +			port, res);
> +}

This appears to be a cut/paste of
yt921x_phylink_mac_link_down(). Don't do that. Refactor existing
functions to make them generic.

> +static int yt922x_port_sds_init(struct yt921x_priv *priv, int port,
> +				phy_interface_t interface)
> +{
> +	int addr;
> +	u16 data;
> +	int res;
> +
> +	addr = yt922x_sds_phyaddr_get(port,
> +				      YT922X_PHY_REG_TYPE_SDS_COMMON_EXT,
> +				      YT922X_PHY_REG_SPACE_SGMII);
> +	if (addr < 0)
> +		return -EINVAL;
> +	/* write protect */
> +	res = yt921x_intif_ext_write(priv, addr, 0x4be, 0xd);
> +	if (res)
> +		return res;
> +	/* CDR */
> +	if (interface == PHY_INTERFACE_MODE_100BASEX) {
> +		res = yt921x_intif_ext_write(priv, addr, 0x406, 0x0);
> +		if (res)
> +			return res;
> +		res = yt921x_intif_ext_write(priv, addr, 0x416, 0x3458);
> +		if (res)
> +			return res;
> +	} else {
> +		res = yt921x_intif_ext_write(priv, addr, 0x406, 0x800);
> +		if (res)
> +			return res;
> +		res = yt921x_intif_ext_write(priv, addr, 0x416, 0x4558);
> +		if (res)
> +			return res;
> +	}
> +	/* PLL */
> +	if (interface == PHY_INTERFACE_MODE_USXGMII) {
> +		res = yt921x_intif_ext_write(priv, addr, 0x43a, 0x1006);
> +		if (res)
> +			return res;
> +		res = yt921x_intif_ext_write(priv, addr, 0x43f, 0x3029);
> +		if (res)

What is this function doing?

    Andrew

---
pw-bot: cr