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

[Andrew Lunn]

> +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?

> +
> +	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.

> +
> +		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.

	Andrew