[PATCH] net: fec: fix conversion to gpiod API

[PATCH] net: fec: fix conversion to gpiod API

[Dmitry Torokhov]

The reset line is optional, so we should be using
devm_gpiod_get_optional() and not abort probing if it is not available.
Also, gpiolib already handles phy-reset-active-high, continuing handling
it directly in the driver when using gpiod API results in flipped logic.

While at this convert phy properties parsing from OF to generic device
properties to avoid #ifdef-ery.

Fixes: 468ba54bd616 ("fec: convert to gpio descriptor")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 36 ++++++++---------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 2716898e0b9b..c2b54a31541e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -27,6 +27,7 @@
 #include <linux/string.h>
 #include <linux/pm_runtime.h>
 #include <linux/ptrace.h>
+#include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/slab.h>
@@ -65,6 +66,7 @@
 #include <linux/prefetch.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
+#include <linux/property.h>
 #include <soc/imx/cpuidle.h>
 #include <linux/filter.h>
 #include <linux/bpf.h>
@@ -4032,42 +4034,38 @@ static int fec_enet_init(struct net_device *ndev)
 	return ret;
 }
 
-#ifdef CONFIG_OF
 static int fec_reset_phy(struct platform_device *pdev)
 {
 	struct gpio_desc *phy_reset;
-	bool active_high = false;
 	int msec = 1, phy_post_delay = 0;
-	struct device_node *np = pdev->dev.of_node;
 	int err;
 
-	if (!np)
-		return 0;
-
-	err = of_property_read_u32(np, "phy-reset-duration", &msec);
+	err = device_property_read_u32(&pdev->dev, "phy-reset-duration", &msec);
 	/* A sane reset duration should not be longer than 1s */
 	if (!err && msec > 1000)
 		msec = 1;
 
-	err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
+	err = device_property_read_u32(&pdev->dev, "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");
-
-	phy_reset = devm_gpiod_get(&pdev->dev, "phy-reset",
-			active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
+					    GPIOD_OUT_HIGH);
 	if (IS_ERR(phy_reset))
 		return dev_err_probe(&pdev->dev, PTR_ERR(phy_reset),
-				     "failed to get phy-reset-gpios\n");
+				     "failed to request phy-reset-gpios\n");
+
+	if (!phy_reset)
+		return 0;
 
 	if (msec > 20)
 		msleep(msec);
 	else
 		usleep_range(msec * 1000, msec * 1000 + 1000);
 
-	gpiod_set_value_cansleep(phy_reset, !active_high);
+	gpiod_set_value_cansleep(phy_reset, 0);
 
 	if (!phy_post_delay)
 		return 0;
@@ -4080,16 +4078,6 @@ static int fec_reset_phy(struct platform_device *pdev)
 
 	return 0;
 }
-#else /* CONFIG_OF */
-static int fec_reset_phy(struct platform_device *pdev)
-{
-	/*
-	 * In case of platform probe, the reset has been done
-	 * by machine code.
-	 */
-	return 0;
-}
-#endif /* CONFIG_OF */
 
 static void
 fec_enet_get_queue_num(struct platform_device *pdev, int *num_tx, int *num_rx)
-- 
2.39.1.456.gfc5497dd1b-goog


-- 
Dmitry

Re: [PATCH] net: fec: fix conversion to gpiod API

[Andrew Lunn]

On Tue, Jan 31, 2023 at 02:48:15PM -0800, Dmitry Torokhov wrote:
> The reset line is optional, so we should be using
> devm_gpiod_get_optional() and not abort probing if it is not available.

> Also, gpiolib already handles phy-reset-active-high, continuing handling
> it directly in the driver when using gpiod API results in flipped logic.

Please could you split this part into a separate patch. There is some
history here, but i cannot remember which driver it actually applies
to. It might be the FEC, it could be some other Ethernet driver.

For whatever driver it was, the initial support for GPIOs totally
ignored the polarity value in DT. The API at the time meant you needed
to take extra steps to get the polarity, and that was skipped. So it
was hard coded. But developers copy/pasted DT statement from other DT
files, putting in the opposite polarity to the hard coded
value. Nobody noticed until somebody needed the opposite polarity to
the hard coded implementation to make their board work. And then the
problem was noticed. The simple solution to actually use the polarity
in DT would break all the boards which had the wrong value. So a new
property was added.

So i would like this change in a separate patch, so if it causes
regressions, it can be reverted.

> While at this convert phy properties parsing from OF to generic device
> properties to avoid #ifdef-ery.

We also need to be careful here. If you read fsl,fec.yaml, there are a
number of deprecated properties. These need to keep working for OF,
but we clearly don't want them exposed to ACPI or anything else. So if
you use generic device properties, please ensure they are only for OF.

    Andrew

Re: [PATCH] net: fec: fix conversion to gpiod API

[Dmitry Torokhov]

Hi Andrew,

On Wed, Feb 01, 2023 at 02:31:12AM +0100, Andrew Lunn wrote:
> On Tue, Jan 31, 2023 at 02:48:15PM -0800, Dmitry Torokhov wrote:
> > The reset line is optional, so we should be using
> > devm_gpiod_get_optional() and not abort probing if it is not available.
> 
> > Also, gpiolib already handles phy-reset-active-high, continuing handling
> > it directly in the driver when using gpiod API results in flipped logic.
> 
> Please could you split this part into a separate patch. There is some
> history here, but i cannot remember which driver it actually applies
> to. It might be the FEC, it could be some other Ethernet driver.
> 
> For whatever driver it was, the initial support for GPIOs totally
> ignored the polarity value in DT. The API at the time meant you needed
> to take extra steps to get the polarity, and that was skipped. So it
> was hard coded. But developers copy/pasted DT statement from other DT
> files, putting in the opposite polarity to the hard coded
> value. Nobody noticed until somebody needed the opposite polarity to
> the hard coded implementation to make their board work. And then the
> problem was noticed. The simple solution to actually use the polarity
> in DT would break all the boards which had the wrong value. So a new
> property was added.
> 
> So i would like this change in a separate patch, so if it causes
> regressions, it can be reverted.

The quirk for the gpiod API to take into account "phy-reset-active-high"
for FEC driver is already in mainline:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib-of.c?id=b02c85c9458cdd15e2c43413d7d2541a468cde57

This is limited only to devices matching compatibles handled by FEC
driver and is being considered automatically as soon as gpiod API is
used.


> 
> > While at this convert phy properties parsing from OF to generic device
> > properties to avoid #ifdef-ery.
> 
> We also need to be careful here. If you read fsl,fec.yaml, there are a
> number of deprecated properties. These need to keep working for OF,
> but we clearly don't want them exposed to ACPI or anything else. So if
> you use generic device properties, please ensure they are only for OF.

OK, if this is a concern I'll drop this from the patch and keep of_*
APIs since we need to keep #ifdef CONFIG_OF anyway.

Thanks.

-- 
Dmitry