Date:   Mon, 20 Nov 2017 09:35:51 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Richard Leitner <dev@...l1n.net>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "richard.leitner@...data.com" <richard.leitner@...data.com>
Subject: RE: [PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy

From: Richard Leitner <dev@...l1n.net> Sent: Monday, November 20, 2017 4:34 PM
>The fec_reset_phy function allowed only one execution during probeing.
>To make it more usable move the dt parsing and gpio allocation to the probe
>function. The parameters of the phy reset are added to the fec_enet_private
>struct. As a result the fec_reset_phy function may be called anytime after
>probe.
>
>One checkpatch.pl warning (too long line) is ignored. This is due to the fact a
>string (dt property name) otherwise needs to be split over multiple lines,
>which is counterproductive for the readability.
>
>Signed-off-by: Richard Leitner <richard.leitner@...data.com>
>---

It is better to convert to gpio descriptor, and use dts gpio flag as the gpio polarity instead of extra phy_reset_active_high variable.

Regards,
Andy

> drivers/net/ethernet/freescale/fec.h      |  4 ++
> drivers/net/ethernet/freescale/fec_main.c | 88 ++++++++++++++++----------
>-----
> 2 files changed, 50 insertions(+), 42 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index 5385074b3b7d..401c4eabf08a 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -539,6 +539,10 @@ struct fec_enet_private {
> 	int	pause_flag;
> 	int	wol_flag;
> 	u32	quirks;
>+	int	phy_reset;
>+	int	phy_reset_duration;
>+	int	phy_reset_post_delay;
>+	bool	phy_reset_active_high;
>
> 	struct	napi_struct napi;
> 	int	csum_flags;
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 610573855213..06a7caca0cee 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -3212,62 +3212,36 @@ static int fec_enet_init(struct net_device *ndev)  }
>
> #ifdef CONFIG_OF
>-static int fec_reset_phy(struct platform_device *pdev)
>+static int fec_reset_phy(struct net_device *ndev)
> {
>-	int err, phy_reset;
>-	bool active_high = false;
>-	int msec = 1, phy_post_delay = 0;
>-	struct device_node *np = pdev->dev.of_node;
>-
>-	if (!np)
>-		return 0;
>-
>-	err = of_property_read_u32(np, "phy-reset-duration", &msec);
>-	/* A sane reset duration should not be longer than 1s */
>-	if (!err && msec > 1000)
>-		msec = 1;
>+	struct fec_enet_private *fep = netdev_priv(ndev);
>
>-	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>-	if (phy_reset == -EPROBE_DEFER)
>-		return phy_reset;
>-	else if (!gpio_is_valid(phy_reset))
>+	if (!fep->phy_reset)
> 		return 0;
>
>-	err = of_property_read_u32(np, "phy-reset-post-delay",
>&phy_post_delay);
>-	/* valid reset duration should be less than 1s */
>-	if (!err && phy_post_delay > 1000)
>-		return -EINVAL;
>-
>-	active_high = of_property_read_bool(np, "phy-reset-active-high");
>+	gpio_set_value_cansleep(fep->phy_reset, fep-
>>phy_reset_active_high);
>
>-	err = devm_gpio_request_one(&pdev->dev, phy_reset,
>-			active_high ? GPIOF_OUT_INIT_HIGH :
>GPIOF_OUT_INIT_LOW,
>-			"phy-reset");
>-	if (err) {
>-		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n",
>err);
>-		return err;
>-	}
>-
>-	if (msec > 20)
>-		msleep(msec);
>+	if (fep->phy_reset_duration > 20)
>+		msleep(fep->phy_reset_duration);
> 	else
>-		usleep_range(msec * 1000, msec * 1000 + 1000);
>+		usleep_range(fep->phy_reset_duration * 1000,
>+			     fep->phy_reset_duration * 1000 + 1000);
>
>-	gpio_set_value_cansleep(phy_reset, !active_high);
>+	gpio_set_value_cansleep(fep->phy_reset, !fep-
>>phy_reset_active_high);
>
>-	if (!phy_post_delay)
>+	if (!fep->phy_reset_post_delay)
> 		return 0;
>
>-	if (phy_post_delay > 20)
>-		msleep(phy_post_delay);
>+	if (fep->phy_reset_post_delay > 20)
>+		msleep(fep->phy_reset_post_delay);
> 	else
>-		usleep_range(phy_post_delay * 1000,
>-			     phy_post_delay * 1000 + 1000);
>+		usleep_range(fep->phy_reset_post_delay * 1000,
>+			     fep->phy_reset_post_delay * 1000 + 1000);
>
> 	return 0;
> }
> #else /* CONFIG_OF */
>-static int fec_reset_phy(struct platform_device *pdev)
>+static int fec_reset_phy(struct net_device *ndev)
> {
> 	/*
> 	 * In case of platform probe, the reset has been done @@ -3400,6
>+3374,36 @@ fec_probe(struct platform_device *pdev)
> 	}
> 	fep->phy_node = phy_node;
>
>+	fep->phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>+	if (gpio_is_valid(fep->phy_reset)) {
>+		ret = of_property_read_u32(np, "phy-reset-duration",
>+					   &fep->phy_reset_duration);
>+		/* A sane reset duration should not be longer than 1s */
>+		if (!ret && fep->phy_reset_post_delay > 1000)
>+			fep->phy_reset_post_delay = 1;
>+
>+		ret = of_property_read_u32(np, "phy-reset-post-delay",
>+					   &fep->phy_reset_post_delay);
>+		/* valid post reset delay should be less than 1s */
>+		if (!ret && fep->phy_reset_post_delay > 1000)
>+			fep->phy_reset_post_delay = 1;
>+
>+		fep->phy_reset_active_high = of_property_read_bool(np,
>+"phy-reset-active-high");
>+
>+		ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset,
>+					    fep->phy_reset_active_high ?
>+					    GPIOF_OUT_INIT_HIGH :
>+					    GPIOF_OUT_INIT_LOW,
>+					    "phy-reset");
>+		if (ret) {
>+			dev_err(&pdev->dev, "failed to get reset-
>gpios: %d\n",
>+				ret);
>+			goto failed_phy;
>+		}
>+	} else {
>+		fep->phy_reset = 0;
>+	}
>+
> 	ret = of_get_phy_mode(pdev->dev.of_node);
> 	if (ret < 0) {
> 		pdata = dev_get_platdata(&pdev->dev); @@ -3472,7 +3476,7
>@@ fec_probe(struct platform_device *pdev)
> 	pm_runtime_set_active(&pdev->dev);
> 	pm_runtime_enable(&pdev->dev);
>
>-	ret = fec_reset_phy(pdev);
>+	ret = fec_reset_phy(ndev);
> 	if (ret)
> 		goto failed_reset;
>
>--
>2.11.0

Powered by blists - more mailing lists