Re: [PATCH net-next v14 5/5] net: dsa: motorcomm: Add support for Motorcomm YT922x

[Andrew Lunn]

> +++ b/drivers/net/dsa/motorcomm/mdio_bus.c
> @@ -299,3 +299,42 @@ int yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp)
>  
>  	return 0;
>  }
> +
> +/* intif extend register read/write api */
> +int
> +yt921x_intif_ext_write(struct yt921x_priv *priv, int port, int reg, u16 val)
> +{
> +	int res;
> +
> +	if (port > priv->series->max_ports)
> +		return -ENODEV;
> +
> +	res = yt921x_intif_write(priv, port, YT92XX_PAGE_SELECT, reg);
> +	if (res)
> +		return res;
> +
> +	res = yt921x_intif_write(priv, port, YT92XX_PAGE, val);
> +	if (res)
> +		return res;
> +
> +	return 0;
> +}
> +
> +int
> +yt921x_intif_ext_read(struct yt921x_priv *priv, int port, int reg, u16 *valp)
> +{
> +	int res;
> +
> +	if (port > priv->series->max_ports)
> +		return -ENODEV;
> +
> +	res = yt921x_intif_write(priv, port, YT92XX_PAGE_SELECT, reg);
> +	if (res)
> +		return res;
> +
> +	res = yt921x_intif_read(priv, port, YT92XX_PAGE, valp);
> +	if (res)
> +		return res;
> +
> +	return 0;
> +}

Why are these in mdio_bus? They don't have anything to do with MDIO.

Since they build on yt921x_intif_read they seem to fit in yt921x.c
better? Or am i missing something?

> +static void yt922x_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
> +				 struct phylink_link_state *state)
> +{
> +	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
> +	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
> +	int port = pp->index;
> +	int res = 0;
> +	u16 data;
> +	int addr;
> +	u16 lp;
> +
> +	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII);
> +	if (addr < 0) {
> +		state->link = false;
> +		return;
> +	}
> +
> +	mutex_lock(&priv->reg_lock);
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		res = yt921x_intif_read(priv, addr, MII_BMSR, &data);
> +		if (res)
> +			goto err;
> +		res = yt921x_intif_read(priv, addr, MII_LPA, &lp);
> +		if (res)
> +			goto err;
> +		phylink_mii_c22_pcs_decode_state(state, neg_mode, data, lp);


Rather than data, call it bmsr. It then fits:

void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
				      unsigned int neg_mode, u16 bmsr, u16 lpa)

> +static void yt922x_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> +	struct yt921x_port *pp = pcs_to_yt921x_port(pcs);
> +	struct yt921x_priv *priv = yt921x_port_to_priv(pp);
> +	struct device *dev = to_device(priv);
> +	int port = pp->index;
> +	u16 data;
> +	int addr;
> +	int res;
> +
> +	mutex_lock(&priv->reg_lock);
> +	addr = yt922x_sds_phyaddr_get(port, YT922X_PHY_REG_TYPE_MII);
> +	if (addr < 0) {
> +		res = addr;
> +		goto err;
> +	}
> +	res = yt921x_intif_read(priv, addr, MII_BMCR, &data);
> +	if (res)
> +		goto err;
> +	data |= BMCR_ANRESTART;
> +	res = yt921x_intif_write(priv, addr, MII_BMCR, data);

Again, bmcr. It then looks a lot like
phylink_mii_c22_pcs_an_restart().

> +	if (res)
> +		goto err;
> +	mutex_unlock(&priv->reg_lock);
> +	return;
> +
> +err:
> +	mutex_unlock(&priv->reg_lock);
> +	if (res)
> +		dev_err(dev, "Failed to %s PCS port %d: %i\n", "an restart",
> +			port, res);
> +}
> +
> +static int yt922x_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			     phy_interface_t interface,
> +			     const unsigned long *advertising,
> +			     bool permit_pause_to_mac)
> +{

advertising does not appear to be used here. Does this pcs not support
it? That would be odd, because you looked at lpa in earlier functions.

	Andrew