Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Andy Duan <fugang.duan@nxp.com>
To:     Richard Leitner <dev@g0hl1n.net>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "richard.leitner@skidata.com" <richard.leitner@skidata.com>
Subject: RE: [PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy
Thread-Topic: [PATCH v2 1/3] net: ethernet: freescale: simplify fec_reset_phy
Thread-Index: AQHTYdqJuZTX/cW+7kiBFwhFWspG5aMdAVKg
Date:   Mon, 20 Nov 2017 09:35:51 +0000
Message-ID: <AM4PR0401MB22606BEB23DB9364E12C0F11FF220@AM4PR0401MB2260.eurprd04.prod.outlook.com>
References: <20171120083417.32558-1-dev@g0hl1n.net>
 <20171120083417.32558-2-dev@g0hl1n.net>
In-Reply-To: <20171120083417.32558-2-dev@g0hl1n.net>
Accept-Language: zh-CN, en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 1d5eb1d7-081a-44f5-de35-08d52ffa1717
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Nov 2017 09:35:51.7781
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0401MB2259
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

From: Richard Leitner <dev@g0hl1n.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 prob=
e
>function. The parameters of the phy reset are added to the fec_enet_privat=
e
>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 f=
act 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@skidata.com>
>---

It is better to convert to gpio descriptor, and use dts gpio flag as the gp=
io 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 =3D false;
>-	int msec =3D 1, phy_post_delay =3D 0;
>-	struct device_node *np =3D pdev->dev.of_node;
>-
>-	if (!np)
>-		return 0;
>-
>-	err =3D of_property_read_u32(np, "phy-reset-duration", &msec);
>-	/* A sane reset duration should not be longer than 1s */
>-	if (!err && msec > 1000)
>-		msec =3D 1;
>+	struct fec_enet_private *fep =3D netdev_priv(ndev);
>
>-	phy_reset =3D of_get_named_gpio(np, "phy-reset-gpios", 0);
>-	if (phy_reset =3D=3D -EPROBE_DEFER)
>-		return phy_reset;
>-	else if (!gpio_is_valid(phy_reset))
>+	if (!fep->phy_reset)
> 		return 0;
>
>-	err =3D 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 =3D of_property_read_bool(np, "phy-reset-active-high");
>+	gpio_set_value_cansleep(fep->phy_reset, fep-
>>phy_reset_active_high);
>
>-	err =3D 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 =3D phy_node;
>
>+	fep->phy_reset =3D of_get_named_gpio(np, "phy-reset-gpios", 0);
>+	if (gpio_is_valid(fep->phy_reset)) {
>+		ret =3D 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 =3D 1;
>+
>+		ret =3D 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 =3D 1;
>+
>+		fep->phy_reset_active_high =3D of_property_read_bool(np,
>+"phy-reset-active-high");
>+
>+		ret =3D 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 =3D 0;
>+	}
>+
> 	ret =3D of_get_phy_mode(pdev->dev.of_node);
> 	if (ret < 0) {
> 		pdata =3D 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 =3D fec_reset_phy(pdev);
>+	ret =3D fec_reset_phy(ndev);
> 	if (ret)
> 		goto failed_reset;
>
>--
>2.11.0

From 1584573521511900384@xxx Mon Nov 20 08:37:49 +0000 2017
X-GM-THRID: 1584573521511900384
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread