| Message ID | 20230201215320.528319-2-dmitry.torokhov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0719bc3a5c77091192c57e440896a969cd1cf885 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3,1/2] net: fec: restore handling of PHY reset line as optional | expand |
On Wed, Feb 01, 2023 at 01:53:20PM -0800, Dmitry Torokhov wrote: > Conversion to gpiod API done in commit 468ba54bd616 ("fec: convert > to gpio descriptor") clashed with gpiolib applying the same quirk to the > reset GPIO polarity (introduced in commit b02c85c9458c). This results in > the reset line being left active/device being left in reset state when > reset line is "active low". > > Remove handling of 'phy-reset-active-high' property from the driver and > rely on gpiolib to apply needed adjustments to avoid ending up with the > double inversion/flipped logic. I searched the in tree DT files from 4.7 to 6.0. None use phy-reset-active-high. I'm don't think it has ever had an in tree user. This property was marked deprecated Jul 18 2019. So i suggest we completely drop it. Andrew
On Wed, Feb 01, 2023 at 11:54:02PM +0100, Andrew Lunn wrote: > On Wed, Feb 01, 2023 at 01:53:20PM -0800, Dmitry Torokhov wrote: > > Conversion to gpiod API done in commit 468ba54bd616 ("fec: convert > > to gpio descriptor") clashed with gpiolib applying the same quirk to the > > reset GPIO polarity (introduced in commit b02c85c9458c). This results in > > the reset line being left active/device being left in reset state when > > reset line is "active low". > > > > Remove handling of 'phy-reset-active-high' property from the driver and > > rely on gpiolib to apply needed adjustments to avoid ending up with the > > double inversion/flipped logic. > > I searched the in tree DT files from 4.7 to 6.0. None use > phy-reset-active-high. I'm don't think it has ever had an in tree > user. > > This property was marked deprecated Jul 18 2019. So i suggest we > completely drop it. I'd be happy kill the quirk in gpiolibi-of.c if that is what we want to do, although DT people sometimes are pretty touchy about keeping backward compatibility. I believe this should not stop us from merging this patch though, as the code is currently broken when this deprecated property is not present. Thanks.
On Wed, Feb 01, 2023 at 03:21:53PM -0800, Dmitry Torokhov wrote: > On Wed, Feb 01, 2023 at 11:54:02PM +0100, Andrew Lunn wrote: > > On Wed, Feb 01, 2023 at 01:53:20PM -0800, Dmitry Torokhov wrote: > > > Conversion to gpiod API done in commit 468ba54bd616 ("fec: convert > > > to gpio descriptor") clashed with gpiolib applying the same quirk to the > > > reset GPIO polarity (introduced in commit b02c85c9458c). This results in > > > the reset line being left active/device being left in reset state when > > > reset line is "active low". > > > > > > Remove handling of 'phy-reset-active-high' property from the driver and > > > rely on gpiolib to apply needed adjustments to avoid ending up with the > > > double inversion/flipped logic. > > > > I searched the in tree DT files from 4.7 to 6.0. None use > > phy-reset-active-high. I'm don't think it has ever had an in tree > > user. FTR I believe this was added in 4.6-rc1 (as 'phy-reset-active-low' in first iteration by Bernhard Walle (CCed), so maybe he can tell us a bit more about hardware and where it is still in service and whether this quirk is still relevant. > > > > This property was marked deprecated Jul 18 2019. So i suggest we > > completely drop it. > > I'd be happy kill the quirk in gpiolibi-of.c if that is what we want to > do, although DT people sometimes are pretty touchy about keeping > backward compatibility. > > I believe this should not stop us from merging this patch though, as the > code is currently broken when this deprecated property is not present. > > Thanks. > > -- > Dmitry
On Wed, Feb 01, 2023 at 05:08:05PM -0800, Dmitry Torokhov wrote: > On Wed, Feb 01, 2023 at 03:21:53PM -0800, Dmitry Torokhov wrote: > > On Wed, Feb 01, 2023 at 11:54:02PM +0100, Andrew Lunn wrote: > > > On Wed, Feb 01, 2023 at 01:53:20PM -0800, Dmitry Torokhov wrote: > > > > Conversion to gpiod API done in commit 468ba54bd616 ("fec: convert > > > > to gpio descriptor") clashed with gpiolib applying the same quirk to the > > > > reset GPIO polarity (introduced in commit b02c85c9458c). This results in > > > > the reset line being left active/device being left in reset state when > > > > reset line is "active low". > > > > > > > > Remove handling of 'phy-reset-active-high' property from the driver and > > > > rely on gpiolib to apply needed adjustments to avoid ending up with the > > > > double inversion/flipped logic. > > > > > > I searched the in tree DT files from 4.7 to 6.0. None use > > > phy-reset-active-high. I'm don't think it has ever had an in tree > > > user. > > FTR I believe this was added in 4.6-rc1 (as 'phy-reset-active-low' in > first iteration by Bernhard Walle (CCed), so maybe he can tell us a bit > more about hardware and where it is still in service and whether this > quirk is still relevant. > > > > > > > This property was marked deprecated Jul 18 2019. So i suggest we > > > completely drop it. > > > > I'd be happy kill the quirk in gpiolibi-of.c if that is what we want to > > do, although DT people sometimes are pretty touchy about keeping > > backward compatibility. Generally, that is for in kernel users. When a new feature is added, you are also supposed to add an in kernel user. I could of missed it in my search, but i didn't find an in-kernel user. If there is one, then we should keep it. Otherwise, i would remove it. > > I believe this should not stop us from merging this patch though, as the > > code is currently broken when this deprecated property is not present. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 00115b55d0ff..c73e25f8995e 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -4036,7 +4036,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,10 +4053,8 @@ 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_optional(&pdev->dev, "phy-reset", - active_high ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW); + 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"); @@ -4070,7 +4067,7 @@ static int fec_reset_phy(struct platform_device *pdev) 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;
Conversion to gpiod API done in commit 468ba54bd616 ("fec: convert to gpio descriptor") clashed with gpiolib applying the same quirk to the reset GPIO polarity (introduced in commit b02c85c9458c). This results in the reset line being left active/device being left in reset state when reset line is "active low". Remove handling of 'phy-reset-active-high' property from the driver and rely on gpiolib to apply needed adjustments 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> --- v3: new patch, split from the original one drivers/net/ethernet/freescale/fec_main.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)