diff mbox series

[RFC,1/2] net: fec: make fec_reset_phy not only usable once

Message ID 20211206101326.1022527-2-philippe.schenker@toradex.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Reset PHY in fec_resume if it got powered down | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 4 of 4 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 144 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Philippe Schenker Dec. 6, 2021, 10:13 a.m. UTC
inside fec_reset_phy devm_gpio_request_once is called hence making
the function only callable once as the gpio is not stored somewhere nor
freed at the end.

Create a new function to collect the data around phy-reset-gpio from
devicetree and store it in fec_enet_private. Make fec_reset_phy use
the data stored in fec_enet_private, so this function can be called
multiple times.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 drivers/net/ethernet/freescale/fec.h      |  6 ++
 drivers/net/ethernet/freescale/fec_main.c | 94 +++++++++++++++--------
 2 files changed, 69 insertions(+), 31 deletions(-)

Comments

Andrew Lunn Dec. 6, 2021, 1:13 p.m. UTC | #1
>  #ifdef CONFIG_OF
> -static int fec_reset_phy(struct platform_device *pdev)
> +static int fec_reset_phy_probe(struct platform_device *pdev,
> +			       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;
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	int tmp, ret;
>  
>  	if (!np)
>  		return 0;
>  
> -	err = of_property_read_u32(np, "phy-reset-duration", &msec);
> +	tmp = 1;
> +	ret = of_property_read_u32(np, "phy-reset-duration", &tmp);
>  	/* A sane reset duration should not be longer than 1s */
> -	if (!err && msec > 1000)
> -		msec = 1;
> +	if (!ret && tmp > 1000)
> +		tmp = 1;
> +
> +	fep->phy_reset_duration = tmp;

If you don't change the names msec and ret, this code would be
unchanged. It then becomes a lot easier to see you have not changed,
the code, only moved it around.

    Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 7b4961daa254..466607bbf9cf 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -631,6 +631,12 @@  struct fec_enet_private {
 	int pps_enable;
 	unsigned int next_counter;
 
+	/* PHY reset signal */
+	bool phy_reset_active_high;
+	int phy_reset_duration;
+	int phy_reset_gpio;
+	int phy_reset_post_delay;
+
 	u64 ethtool_stats[];
 };
 
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index bc418b910999..92840f18c48f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3588,62 +3588,90 @@  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_probe(struct platform_device *pdev,
+			       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;
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	int tmp, ret;
 
 	if (!np)
 		return 0;
 
-	err = of_property_read_u32(np, "phy-reset-duration", &msec);
+	tmp = 1;
+	ret = of_property_read_u32(np, "phy-reset-duration", &tmp);
 	/* A sane reset duration should not be longer than 1s */
-	if (!err && msec > 1000)
-		msec = 1;
+	if (!ret && tmp > 1000)
+		tmp = 1;
+
+	fep->phy_reset_duration = tmp;
 
-	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))
+	tmp = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (tmp == -EPROBE_DEFER)
+		return tmp;
+	else if (!gpio_is_valid(tmp))
 		return 0;
 
-	err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
+	fep->phy_reset_gpio = tmp;
+
+	tmp = 0;
+	ret = of_property_read_u32(np, "phy-reset-post-delay", &tmp);
 	/* valid reset duration should be less than 1s */
-	if (!err && phy_post_delay > 1000)
+	if (!ret && tmp > 1000)
 		return -EINVAL;
 
-	active_high = of_property_read_bool(np, "phy-reset-active-high");
+	fep->phy_reset_post_delay = tmp;
 
-	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;
+	fep->phy_reset_active_high =
+		of_property_read_bool(np, "phy-reset-active-high");
+
+	ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset_gpio,
+				    fep->phy_reset_active_high ?
+				    GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				    "phy-reset");
+	if (ret) {
+		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", ret);
+		return ret;
 	}
 
-	if (msec > 20)
-		msleep(msec);
+	return 0;
+}
+
+static int fec_reset_phy(struct net_device *ndev)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+
+	gpio_set_value_cansleep(fep->phy_reset_gpio,
+				fep->phy_reset_active_high);
+
+	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_gpio,
+				!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_probe(struct platform_device *pdev,
+			       struct net_device *ndev)
+{
+	return 0;
+}
+
+static int fec_reset_phy(struct net_device *ndev)
 {
 	/*
 	 * In case of platform probe, the reset has been done
@@ -3918,7 +3946,11 @@  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_probe(pdev, ndev);
+	if (ret)
+		goto failed_reset;
+
+	ret = fec_reset_phy(ndev);
 	if (ret)
 		goto failed_reset;