Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

[Clément Léger]

Le Thu, 14 Apr 2022 19:55:55 +0200,
Andrew Lunn <andrew@xxxxxxx> a écrit :

>  +static int a5psw_mdio_reset(struct mii_bus *bus)
> > +{
> > +	struct a5psw *a5psw = bus->priv;
> > +	unsigned long rate;
> > +	unsigned long div;
> > +	u32 cfgstatus;
> > +
> > +	rate = clk_get_rate(a5psw->hclk);
> > +	div = ((rate / a5psw->mdio_freq) / 2);
> > +	if (div >= 511 || div <= 5) {
> > +		dev_err(a5psw->dev, "MDIO clock div %ld out of range\n", div);
> > +		return -ERANGE;
> > +	}
> > +
> > +	cfgstatus = FIELD_PREP(A5PSW_MDIO_CFG_STATUS_CLKDIV, div);
> > +
> > +	a5psw_reg_writel(a5psw, A5PSW_MDIO_CFG_STATUS, cfgstatus);  
> 
> I don't see anything here which does an actual reset. So i think this
> function has the wrong name. Please also pass the frequency as a
> parameter, because at a quick glance it was not easy to see where it
> was used. There does not seem to be any need to store it in a5psw.

Indeed, the reset callback can be removed entirely and the mdio bus
could be setup directly from a5psw_probe_mdio().

> 
> > +static int a5psw_probe_mdio(struct a5psw *a5psw)
> > +{
> > +	struct device *dev = a5psw->dev;
> > +	struct device_node *mdio_node;
> > +	struct mii_bus *bus;
> > +	int err;
> > +
> > +	if (of_property_read_u32(dev->of_node, "clock-frequency",
> > +				 &a5psw->mdio_freq))
> > +		a5psw->mdio_freq = A5PSW_MDIO_DEF_FREQ;
> > +
> > +	bus = devm_mdiobus_alloc(dev);
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	bus->name = "a5psw_mdio";
> > +	bus->read = a5psw_mdio_read;
> > +	bus->write = a5psw_mdio_write;
> > +	bus->reset = a5psw_mdio_reset;  
> 
> As far as i can see, the read and write functions don't support
> C45. Please return -EOPNOTSUPP if they are passed C45 addresses.

Ok.

> 
>      Andrew



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com