| Message ID | Y9nbJJP/2gvJmpnO@google.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: fec: fix conversion to gpiod API | expand |
On 31.01.2023 19:23:16, 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, there is a quirk in > gpiolib (introduced in b02c85c9458cdd15e2c43413d7d2541a468cde57) that > transparently handles "phy-reset-active-high" property. Remove handling > from the driver to avoid ending up with the double inversion/flipped > logic. > > Fixes: 468ba54bd616 ("fec: convert to gpio descriptor") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reported-by: Marc Kleine-Budde <mkl@pengutronix.de> Tested-by: Marc Kleine-Budde <mkl@pengutronix.de> Marc
On Tue, Jan 31, 2023 at 07:23:16PM -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, there is a quirk in > gpiolib (introduced in b02c85c9458cdd15e2c43413d7d2541a468cde57) that > transparently handles "phy-reset-active-high" property. Remove handling > from the driver to avoid ending up with the double inversion/flipped > logic. > > Fixes: 468ba54bd616 ("fec: convert to gpio descriptor") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Please split this into two: 1) Fix for it being optional 2) Removing support for phy-reset-active-high The breakage is in net-next, so we are not in a rush, and we don't need a minimum of patches. So since this is two logical changes, it should be two patches. Please also update the binding document to indicate that 'phy-reset-active-high' is no longer deprecated, it has actually been removed. So we want the DT checking tools to error out if such a property is found. Thanks Andrew
On Wed, Feb 01, 2023 at 07:04:29PM +0100, Andrew Lunn wrote: > On Tue, Jan 31, 2023 at 07:23:16PM -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, there is a quirk in > > gpiolib (introduced in b02c85c9458cdd15e2c43413d7d2541a468cde57) that > > transparently handles "phy-reset-active-high" property. Remove handling > > from the driver to avoid ending up with the double inversion/flipped > > logic. > > > > Fixes: 468ba54bd616 ("fec: convert to gpio descriptor") > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > Please split this into two: > > 1) Fix for it being optional > 2) Removing support for phy-reset-active-high > > The breakage is in net-next, so we are not in a rush, and we don't > need a minimum of patches. So since this is two logical changes, it > should be two patches. Hm, so you want the driver be still broken after changing the call to be devm_gpiod_get_optional()? Because you can not have this driver use gpiod API and keep parsing 'phy-reset-active-high' (by the driver itself). OK, I suppose I can do that... > > Please also update the binding document to indicate that > 'phy-reset-active-high' is no longer deprecated, it has actually been > removed. So we want the DT checking tools to error out if such a > property is found. We still parse 'phy-reset-active-high', just parsing happens elsewhere (in gpiolib), so it is not an error to have it in a binding. Thanks.
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 2716898e0b9b..1a8f1e6296f2 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> @@ -4036,7 +4037,6 @@ static int fec_enet_init(struct net_device *ndev) 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; @@ -4054,20 +4054,21 @@ static int fec_reset_phy(struct platform_device *pdev) 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"); + 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;
The reset line is optional, so we should be using devm_gpiod_get_optional() and not abort probing if it is not available. Also, there is a quirk in gpiolib (introduced in b02c85c9458cdd15e2c43413d7d2541a468cde57) that transparently handles "phy-reset-active-high" property. Remove handling from the driver to avoid ending up with the double inversion/flipped logic. Fixes: 468ba54bd616 ("fec: convert to gpio descriptor") Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- v2: dropped conversion to generic device properties from the patch. drivers/net/ethernet/freescale/fec_main.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)