Re: [PATCH 1/3] net: Add MDIO bus driver for the Hisilicon FEMAC

On 2016/6/13 21:32, Andrew Lunn wrote:

On Mon, Jun 13, 2016 at 02:07:54PM +0800, Dongpo Li wrote:

quoted

This patch adds a separate driver for the MDIO interface of the
Hisilicon Fast Ethernet MAC.

Reviewed-by: Jiancheng Xue <redacted>
Signed-off-by: Dongpo Li <redacted>
---
 .../bindings/net/hisilicon-femac-mdio.txt          |  22 +++
 drivers/net/phy/Kconfig                            |   8 +
 drivers/net/phy/Makefile                           |   1 +
 drivers/net/phy/mdio-hisi-femac.c                  | 165 +++++++++++++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/hisilicon-femac-mdio.txt
 create mode 100644 drivers/net/phy/mdio-hisi-femac.c

[...]

quoted

+

Hi Dongpo

Overall this looks good. Just some minor comments

quoted

+static int hisi_femac_mdio_wait_ready(struct mii_bus *bus)
+{
+	struct hisi_femac_mdio_data *data = bus->priv;

You could just pass data here. Your read and write functions already
have it.

Thank you, I will fix it in next patch version.

quoted

+	data->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(data->clk)) {
+		ret = -ENODEV;
+		goto err_out_free_mdiobus;
+	}

Return the error which devm_clk_get() gives you.

ok, I will fix it.

quoted

+
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		goto err_out_free_mdiobus;
+
+	ret = of_mdiobus_register(bus, np);
+	if (ret)
+		goto err_out_free_mdiobus;

You leave the clock prepared and enabled on error.

ok, I will fix it.

    Andrew

.