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

[Daniel Golle]

On Tue, Jan 27, 2026 at 11:41:00AM +0100, Paolo Abeni wrote:
> On 1/22/26 4:42 AM, Daniel Golle wrote:
> > +static int mxl862xx_send_cmd(struct mxl862xx_priv *priv, u16 cmd, u16 size,
> > +			     bool quiet)
> > +{
> > +	int ret;
> > +
> > +	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_LEN_RET, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mxl862xx_reg_write(priv, MXL862XX_MMD_REG_CTRL,
> > +				 cmd | CTRL_BUSY_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mxl862xx_busy_wait(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mxl862xx_reg_read(priv, MXL862XX_MMD_REG_LEN_RET);
> > +	/* handle errors returned by the firmware as -EIO
> > +	 * The firmware is based on Zephyr OS and uses the errors as
> > +	 * defined in errno.h of Zephyr OS. See
> > +	 * https://github.com/zephyrproject-rtos/zephyr/blob/v3.7.0/lib/libc/minimal/include/errno.h
> > +	 */
> > +	if ((s16)ret < 0) {
> 
> The cast is likely not needed above? if `ret` values < S16_MIN are
> possible this will return such values to the caller without the IO err
> printk.

Right, it should rather be

if (ret > S16_MAX && ret <= U16_MAX)

to really only catch the range of numbers which are negative 16-bit
signed values represented as positive 32-bit signed values.

mxl862xx_reg_read() primarily returns a signed 32-bit integer, as it is
basically just a wrapper around __mdiodev_c45_read(). Negative values of
that 32-bit integer mean that the MDIO Clause-45 read has somehow
failed, ie. it's the error the MDIO bus .read_c45() operation has
returned.

In case __mdiodev_c45_read() succeeds it returns the 16-bit value of the
register read. In this case, those 16-bit should be interpreted as a
16-bit signed integer here. A negative value denotes an error returned
from the firmware running on the switch (see comment above the code).

> 
> > +		if (!quiet)
> > +			dev_err(&priv->mdiodev->dev,
> > +				"CMD %04x returned error %d\n", cmd, (s16)ret);
> > +		return -EIO;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +int mxl862xx_api_wrap(struct mxl862xx_priv *priv, u16 cmd, void *_data,
> > +		      u16 size, bool read, bool quiet)
> > +{
> > +	__le16 *data = _data;
> > +	u16 max, i;
> > +	int ret, cmd_ret;
> 
> Minor nit: reverse christmas tree above.

Oh right.... I anyway found another minor issue, so I'm going to send
v9 with the above as well as the other minor issue fixed.

> BTW the initial port isolation LGTM, but I would appreciate some DSA
> expert second opinion.

+1