Re: [PATCH net-next v14 4/4] net: dsa: add basic initial driver for MxL862xx switches

[Daniel Golle <daniel@makrotopia.org>]

On Sat, Feb 07, 2026 at 11:59:02PM +0200, Vladimir Oltean wrote:
> On Sat, Feb 07, 2026 at 03:07:27AM +0000, Daniel Golle wrote:
> > +/* PHY access via firmware relay */
> > +static int mxl862xx_phy_read_mmd(struct mxl862xx_priv *priv, int port,
> > +				 int devadd, int reg)
> > +{
> > +	struct mdio_relay_data param = {
> > +		.phy = port,
> > +		.mmd = devadd,
> > +		.reg = cpu_to_le16(reg),
> > +	};
> > +	int ret;
> > +
> > +	ret = MXL862XX_API_READ(priv, INT_GPHY_READ, param);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return le16_to_cpu(param.data);
> > +}
> > +
> > +static int mxl862xx_phy_write_mmd(struct mxl862xx_priv *priv, int port,
> > +				  int devadd, int reg, u16 data)
> > +{
> > +	struct mdio_relay_data param = {
> > +		.phy = port,
> > +		.mmd = devadd,
> > +		.reg = cpu_to_le16(reg),
> > +		.data = cpu_to_le16(data),
> > +	};
> > +
> > +	return MXL862XX_API_WRITE(priv, INT_GPHY_WRITE, param);
> > +}
> > +
> > +static int mxl862xx_phy_read_mii_bus(struct mii_bus *bus, int port, int regnum)
> > +{
> > +	return mxl862xx_phy_read_mmd(bus->priv, port, 0, regnum);
> > +}
> > +
> > +static int mxl862xx_phy_write_mii_bus(struct mii_bus *bus, int port,
> > +				      int regnum, u16 val)
> > +{
> > +	return mxl862xx_phy_write_mmd(bus->priv, port, 0, regnum, val);
> > +}
> > +
> > +static int mxl862xx_phy_read_c45_mii_bus(struct mii_bus *bus, int port,
> > +					 int devadd, int regnum)
> > +{
> > +	return mxl862xx_phy_read_mmd(bus->priv, port, devadd, regnum);
> > +}
> > +
> > +static int mxl862xx_phy_write_c45_mii_bus(struct mii_bus *bus, int port,
> > +					  int devadd, int regnum, u16 val)
> > +{
> > +	return mxl862xx_phy_write_mmd(bus->priv, port, devadd, regnum, val);
> > +}
> 
> You took inspiration from the wrong place with the mii_bus ops prototypes,
> specifically with the "int port" argument.
> 
> The second argument does not hold the port, it holds the PHY address.
> I.e. in this case:
>                 port@6 {
>                     reg = <6>;
>                     phy-handle = <&phy5>;
>                     phy-mode = "internal";
>                 };
>                 phy5: ethernet-phy@5 {
>                     reg = <5>;
>                 };
> 
> "int port" is 5, not 6.
> 
> Your source of inspiration are the prototypes of an mii_bus used as
> ds->user_mii_bus. We have a different set of requirements there, because
> ds->user_mii_bus exists for the case where the PHY is not described in
> the device tree, so the port index is given as argument and the
> user_mii_bus is responsible for internally translating the port index to
> a PHY address.
> 
> So while the use of "int port" as argument name for these operations is
> justifiable in some cases, it is not applicable to this driver, and will
> be a pitfall for anyone who has to modify or debug this code.

Ack. While not completely correct from the beginning I should have
addressed that and changed the parameter to 'int addr' when I started to
count physical ports from 0 and no longer hide the microcontroller --
from that moment on the PHY address and the port address are no longer
equal.

I will post a follow-up series to address this and also the removal of
the 'label' property from the DT binding example once this has been
merged. Both cases don't justify a "Fixes:"-tag though, but I'll just
try to be fast, so both can still be applied before net-next closes.