Re: [PATCH net-next 2/2] net: mdio: add a driver for PIC64-HPSC/HX MDIO controller

[Charles Perry]

On Thu, Mar 19, 2026 at 05:55:44PM +0100, Andrew Lunn wrote:
> > +static int pic64hpsc_mdio_probe(struct platform_device *pdev)
> > +{
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct device *dev = &pdev->dev;
> > +	struct pic64hpsc_mdio_dev *priv;
> > +	struct mii_bus *bus;
> > +	unsigned long rate;
> > +	struct clk *clk;
> > +	u32 bus_freq;
> > +	u32 div;
> > +	int ret;
> > +
> > +	bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> > +	if (!bus)
> > +		return -ENOMEM;
> > +
> > +	priv = bus->priv;
> > +
> > +	priv->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->regs))
> > +		return PTR_ERR(priv->regs);
> > +
> > +	bus->name = KBUILD_MODNAME;
> > +	bus->read = pic64hpsc_mdio_read;
> > +	bus->write = pic64hpsc_mdio_write;
> > +	snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> > +	bus->parent = dev;
> > +
> > +	clk = devm_clk_get_optional_enabled(dev, NULL);
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> 
> What is the use case for not listing the clock? Optional clocks are
> generally because it was forgotten about in the initial driver, and
> added later. In order to not break backwards compatibility, the clock
> needs to be optional.
> 
> But this is a new driver. Why not make it required?
> 

My idea is that if someone wants to use whatever is the hardware default
or what was set by the bootloader, they have an option to do so. For that
reason, I made the clock and the clock-frequency optional. This is
something I can do without if you think it will homogenize better with new
drivers.

Now I just realized that I can achieve this by just making the
clock-frequency optional and not the clock.

Looking at some other MDIO drivers, I can see that there's different
policies on the "clock-frequency" not specified case:

 - mdio-airoha.c: use 2.5MHz if not specified
 - mdio-ipq4019.c: detect if the prescaler is the out of reset value,
                   choose 2.5MHz if that's the case.
 - mdio-mscc-miim.c and mdio-bcm-unimac.c: keep the current settings

I can implement any of the three policy above and don't have strong
opinions about this.

> > +
> > +	of_property_read_u32(np, "clock-frequency", &bus_freq);
> > +
> > +	if (bus_freq) {
> > +		if (!clk) {
> > +			dev_err(dev,
> > +				"cannot use clock-frequency without a clock\n");
> > +			return -EINVAL;
> > +		}
> 
> And this then gets simpler.
> 

Ok, I'll make the clock mandatory.

> > +
> > +		rate = clk_get_rate(clk);
> > +
> > +		div = DIV_ROUND_UP(rate, 2 * bus_freq) - 1;
> > +		if (div == 0 || div & ~MDIO_CFG_PRESCALE_MASK) {
> > +			dev_err(dev, "Incorrect MDIO clock frequency\n");
> 
> I think "Out of range" is more correct.

Ack.

Thank you,
Charles

> 
> 	Andrew