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

[Andrew Lunn <andrew@lunn.ch>]

> Imho it would be nice to introduce unlock __mdiodev_c45_* helpers in
> include/linux/mdio.h, ie.
> 
> static inline int __mdiodev_c45_read(struct mdio_device *mdiodev, int devad,
> 				     u16 regnum)
> {
> 	return __mdiobus_c45_read(mdiodev->bus, mdiodev->addr, devad, regnum);
> }
> 
> static inline int __mdiodev_c45_write(struct mdio_device *mdiodev, u32 devad,
> 				      u16 regnum, u16 val)
> {
> 	return __mdiobus_c45_write(mdiodev->bus, mdiodev->addr, devad, regnum,
> 				   val);
> }

https://elixir.bootlin.com/linux/v6.18/source/drivers/net/phy/mdio_bus.c#L531

> static int mxl862xx_reg_read(struct mxl862xx_priv *priv, u32 addr)
> {
> 	return __mdiodev_c45_read(priv->mdiodev, MDIO_MMD_VEND1, addr);
> }
> 
> static int mxl862xx_reg_write(struct mxl862xx_priv *priv, u32 addr, u16 data)
> {
> 	return __mdiodev_c45_write(priv->mdiodev, MDIO_MMD_VEND1, addr, data);
> }
> 
> static int mxl862xx_ctrl_read(struct mxl862xx_priv *priv)
> {
> 	return mxl862xx_reg_read(priv, MXL862XX_MMD_REG_CTRL);
> }
> 
> static int mxl862xx_busy_wait(struct mxl862xx_priv *priv)
> {
> 	int val;
> 
> 	return readx_poll_timeout(mxl862xx_ctrl_read, priv, val,
> 				  !(val & CTRL_BUSY_MASK), 15, 10000);
> }
> 
> Do you agree?

This part, yes.

> > > +	if (result < 0) {
> > > +		ret = result;
> > > +		goto out;
> > > +	}
> > 
> > If i'm reading mxl862xx_send_cmd() correct, result is the value of a
> > register. It seems unlikely this is a Linux error code?
> 
> Only someone with insights into the use of error codes by the uC
> firmware can really answer that. However, as also Russell pointed out,
> the whole use of s16 here with negative values being interpreted as
> errors is fishy here, because in the end this is also used to read
> registers from external MDIO connected PHYs which may return arbitrary
> 16-bit values...
> Someone in MaxLinear will need to clarify here.

It looks wrong, and since different architectures use different error
code values, it is hard to get right. I would suggest you just return
EPROTO or EIO and add a netdev_err() to print the value of result.

> > > +#define MXL862XX_API_WRITE(dev, cmd, data) \
> > > +	mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), false)
> > > +#define MXL862XX_API_READ(dev, cmd, data) \
> > > +	mxl862xx_api_wrap(dev, cmd, &(data), sizeof((data)), true)
> > 
> > > +/* 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 = reg & 0xffff,
> > > +	};
> > > +	int ret;
> > > +
> > > +	ret = MXL862XX_API_READ(priv, INT_GPHY_READ, param);
> > 
> > That looks a bit ugly, using a macro as a function name. I would
> > suggest tiny functions rather than macros. The compiler should do the
> > right thing.
> 
> The thing is that the macro way allows to use MXL862XX_API_* on
> arbitrary types, such as the packed structs. Using a function would
> require the type of the parameter to be defined, which would result
> in a lot of code duplication in this case.

How many different invocations of these macros are there? For MDIO you
need two. How many more are there? 

     Andrew