| Message ID | Y9mar1COtT5z4mvT@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fec: fix conversion to gpiod API | expand |
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
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.
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)
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(-)