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

[Kyle Switch]

On 10/13/26 09:51, Andrew Lunn wrote:
> > +++ 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?

These two interfaces access the extend registers of the Phy/serdes.

Following the existing code style-where the PHY/SerDes MII registers

were placed in the mdio_bus.c,these two interfaces are also placed

in the mdio_bus.c.

> > +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)
Okay.
> > +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().
Okay.
> > +	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.

advertising and neg_mode will been handled in next patch version.

> 	Andrew