| Message ID | f5fdf1a50bb164b4f59409d3a70a2689515d59c9.1687011839.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 988e8d90b3dc482637532e61bc2d58bfc4af5167 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: at803x: Use devm_regulator_get_enable_optional() | expand |
On Sat, Jun 17, 2023 at 04:24:37PM +0200, Christophe JAILLET wrote: > Use devm_regulator_get_enable_optional() instead of hand writing it. It > saves some line of code. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hello: This patch was applied to netdev/net-next.git (main) by David S. Miller <davem@davemloft.net>: On Sat, 17 Jun 2023 16:24:37 +0200 you wrote: > Use devm_regulator_get_enable_optional() instead of hand writing it. It > saves some line of code. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested-only. > > [...] Here is the summary with links: - [net-next] net: phy: at803x: Use devm_regulator_get_enable_optional() https://git.kernel.org/netdev/net-next/c/988e8d90b3dc You are awesome, thank you!
Le 17/06/2023 à 16:24, Christophe JAILLET a écrit : > Use devm_regulator_get_enable_optional() instead of hand writing it. It > saves some line of code. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > Compile tested-only. > > If my reading is correct, regulator_disable() is still called in the same > order, should an error occur or the driver be removed. > --- > drivers/net/phy/at803x.c | 44 +++++++--------------------------------- > 1 file changed, 7 insertions(+), 37 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 656136628ffd..c1f307d90518 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -304,7 +304,6 @@ struct at803x_priv { > bool is_1000basex; > struct regulator_dev *vddio_rdev; > struct regulator_dev *vddh_rdev; > - struct regulator *vddio; > u64 stats[ARRAY_SIZE(at803x_hw_stats)]; > }; > > @@ -824,11 +823,11 @@ static int at803x_parse_dt(struct phy_device *phydev) > if (ret < 0) > return ret; > > - priv->vddio = devm_regulator_get_optional(&phydev->mdio.dev, > - "vddio"); > - if (IS_ERR(priv->vddio)) { Hi, my patch is not broken by itself, but the existing code looks spurious. If the optional regulator is not found, then -ENODEV is returned, at803x_parse_dt() will return this error and the probe will fail. It looks that the test should be more subtle. I can send a follow up patch, unless there is a better way to fix the pre-existing issue. See [1] for a more detailed explanation. CJ [1]: https://lore.kernel.org/all/ZJFqCQ8bbBoX3l1g@hovoldconsulting.com/ > + ret = devm_regulator_get_enable_optional(&phydev->mdio.dev, > + "vddio"); > + if (ret) { > phydev_err(phydev, "failed to get VDDIO regulator\n"); > - return PTR_ERR(priv->vddio); > + return ret; > } > > /* Only AR8031/8033 support 1000Base-X for SFP modules */ > @@ -856,12 +855,6 @@ static int at803x_probe(struct phy_device *phydev) > if (ret) > return ret; > > - if (priv->vddio) { > - ret = regulator_enable(priv->vddio); > - if (ret < 0) > - return ret; > - } > - > if (phydev->drv->phy_id == ATH8031_PHY_ID) { > int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG); > int mode_cfg; > @@ -869,10 +862,8 @@ static int at803x_probe(struct phy_device *phydev) > .wolopts = 0, > }; > > - if (ccr < 0) { > - ret = ccr; > - goto err; > - } > + if (ccr < 0) > + return ccr; > mode_cfg = ccr & AT803X_MODE_CFG_MASK; > > switch (mode_cfg) { > @@ -890,25 +881,11 @@ static int at803x_probe(struct phy_device *phydev) > ret = at803x_set_wol(phydev, &wol); > if (ret < 0) { > phydev_err(phydev, "failed to disable WOL on probe: %d\n", ret); > - goto err; > + return ret; > } > } > > return 0; > - > -err: > - if (priv->vddio) > - regulator_disable(priv->vddio); > - > - return ret; > -} > - > -static void at803x_remove(struct phy_device *phydev) > -{ > - struct at803x_priv *priv = phydev->priv; > - > - if (priv->vddio) > - regulator_disable(priv->vddio); > } > > static int at803x_get_features(struct phy_device *phydev) > @@ -2021,7 +1998,6 @@ static struct phy_driver at803x_driver[] = { > .name = "Qualcomm Atheros AR8035", > .flags = PHY_POLL_CABLE_TEST, > .probe = at803x_probe, > - .remove = at803x_remove, > .config_aneg = at803x_config_aneg, > .config_init = at803x_config_init, > .soft_reset = genphy_soft_reset, > @@ -2043,7 +2019,6 @@ static struct phy_driver at803x_driver[] = { > .name = "Qualcomm Atheros AR8030", > .phy_id_mask = AT8030_PHY_ID_MASK, > .probe = at803x_probe, > - .remove = at803x_remove, > .config_init = at803x_config_init, > .link_change_notify = at803x_link_change_notify, > .set_wol = at803x_set_wol, > @@ -2059,7 +2034,6 @@ static struct phy_driver at803x_driver[] = { > .name = "Qualcomm Atheros AR8031/AR8033", > .flags = PHY_POLL_CABLE_TEST, > .probe = at803x_probe, > - .remove = at803x_remove, > .config_init = at803x_config_init, > .config_aneg = at803x_config_aneg, > .soft_reset = genphy_soft_reset, > @@ -2082,7 +2056,6 @@ static struct phy_driver at803x_driver[] = { > PHY_ID_MATCH_EXACT(ATH8032_PHY_ID), > .name = "Qualcomm Atheros AR8032", > .probe = at803x_probe, > - .remove = at803x_remove, > .flags = PHY_POLL_CABLE_TEST, > .config_init = at803x_config_init, > .link_change_notify = at803x_link_change_notify, > @@ -2100,7 +2073,6 @@ static struct phy_driver at803x_driver[] = { > PHY_ID_MATCH_EXACT(ATH9331_PHY_ID), > .name = "Qualcomm Atheros AR9331 built-in PHY", > .probe = at803x_probe, > - .remove = at803x_remove, > .suspend = at803x_suspend, > .resume = at803x_resume, > .flags = PHY_POLL_CABLE_TEST, > @@ -2117,7 +2089,6 @@ static struct phy_driver at803x_driver[] = { > PHY_ID_MATCH_EXACT(QCA9561_PHY_ID), > .name = "Qualcomm Atheros QCA9561 built-in PHY", > .probe = at803x_probe, > - .remove = at803x_remove, > .suspend = at803x_suspend, > .resume = at803x_resume, > .flags = PHY_POLL_CABLE_TEST, > @@ -2183,7 +2154,6 @@ static struct phy_driver at803x_driver[] = { > .name = "Qualcomm QCA8081", > .flags = PHY_POLL_CABLE_TEST, > .probe = at803x_probe, > - .remove = at803x_remove, > .config_intr = at803x_config_intr, > .handle_interrupt = at803x_handle_interrupt, > .get_tunable = at803x_get_tunable,
+ Johan and Wolfram On Tue, Jun 20, 2023 at 07:45:24PM +0200, Christophe JAILLET wrote: > Le 17/06/2023 à 16:24, Christophe JAILLET a écrit : > > Use devm_regulator_get_enable_optional() instead of hand writing it. It > > saves some line of code. > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > Compile tested-only. > > > > If my reading is correct, regulator_disable() is still called in the same > > order, should an error occur or the driver be removed. > > --- > > drivers/net/phy/at803x.c | 44 +++++++--------------------------------- > > 1 file changed, 7 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > > index 656136628ffd..c1f307d90518 100644 > > --- a/drivers/net/phy/at803x.c > > +++ b/drivers/net/phy/at803x.c > > @@ -304,7 +304,6 @@ struct at803x_priv { > > bool is_1000basex; > > struct regulator_dev *vddio_rdev; > > struct regulator_dev *vddh_rdev; > > - struct regulator *vddio; > > u64 stats[ARRAY_SIZE(at803x_hw_stats)]; > > }; > > @@ -824,11 +823,11 @@ static int at803x_parse_dt(struct phy_device *phydev) > > if (ret < 0) > > return ret; > > - priv->vddio = devm_regulator_get_optional(&phydev->mdio.dev, > > - "vddio"); > > - if (IS_ERR(priv->vddio)) { > > Hi, > > my patch is not broken by itself, but the existing code looks spurious. > > If the optional regulator is not found, then -ENODEV is returned, > at803x_parse_dt() will return this error and the probe will fail. > > It looks that the test should be more subtle. > > I can send a follow up patch, unless there is a better way to fix the > pre-existing issue. > > See [1] for a more detailed explanation. > > CJ > > [1]: https://lore.kernel.org/all/ZJFqCQ8bbBoX3l1g@hovoldconsulting.com/ My understanding is that this patch cleans things up (good) and retains some dubious behaviour (not making it worse or better). Meanwhile, a solution to the dubious behaviour was discussed in the context of [1] and moreover a patch linked from there and associated solution from Wolfram. If so. I guess we can either: 1) Move forwards with this patch and then follow-up with the preferred solution for the dubious behaviour, once that solution is decided upon and available. 2) Skip this patch and follow-up later. FWIIW, either seems fine to me. > > + ret = devm_regulator_get_enable_optional(&phydev->mdio.dev, > > + "vddio"); > > + if (ret) { > > phydev_err(phydev, "failed to get VDDIO regulator\n"); > > - return PTR_ERR(priv->vddio); > > + return ret; > > } > > /* Only AR8031/8033 support 1000Base-X for SFP modules */ > > @@ -856,12 +855,6 @@ static int at803x_probe(struct phy_device *phydev) > > if (ret) > > return ret; > > - if (priv->vddio) { > > - ret = regulator_enable(priv->vddio); > > - if (ret < 0) > > - return ret; > > - } > > - > > if (phydev->drv->phy_id == ATH8031_PHY_ID) { > > int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG); > > int mode_cfg; > > @@ -869,10 +862,8 @@ static int at803x_probe(struct phy_device *phydev) > > .wolopts = 0, > > }; > > - if (ccr < 0) { > > - ret = ccr; > > - goto err; > > - } > > + if (ccr < 0) > > + return ccr; > > mode_cfg = ccr & AT803X_MODE_CFG_MASK; > > switch (mode_cfg) { > > @@ -890,25 +881,11 @@ static int at803x_probe(struct phy_device *phydev) > > ret = at803x_set_wol(phydev, &wol); > > if (ret < 0) { > > phydev_err(phydev, "failed to disable WOL on probe: %d\n", ret); > > - goto err; > > + return ret; > > } > > } > > return 0; > > - > > -err: > > - if (priv->vddio) > > - regulator_disable(priv->vddio); > > - > > - return ret; > > -} > > - > > -static void at803x_remove(struct phy_device *phydev) > > -{ > > - struct at803x_priv *priv = phydev->priv; > > - > > - if (priv->vddio) > > - regulator_disable(priv->vddio); > > } > > static int at803x_get_features(struct phy_device *phydev) > > @@ -2021,7 +1998,6 @@ static struct phy_driver at803x_driver[] = { > > .name = "Qualcomm Atheros AR8035", > > .flags = PHY_POLL_CABLE_TEST, > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .config_aneg = at803x_config_aneg, > > .config_init = at803x_config_init, > > .soft_reset = genphy_soft_reset, > > @@ -2043,7 +2019,6 @@ static struct phy_driver at803x_driver[] = { > > .name = "Qualcomm Atheros AR8030", > > .phy_id_mask = AT8030_PHY_ID_MASK, > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .config_init = at803x_config_init, > > .link_change_notify = at803x_link_change_notify, > > .set_wol = at803x_set_wol, > > @@ -2059,7 +2034,6 @@ static struct phy_driver at803x_driver[] = { > > .name = "Qualcomm Atheros AR8031/AR8033", > > .flags = PHY_POLL_CABLE_TEST, > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .config_init = at803x_config_init, > > .config_aneg = at803x_config_aneg, > > .soft_reset = genphy_soft_reset, > > @@ -2082,7 +2056,6 @@ static struct phy_driver at803x_driver[] = { > > PHY_ID_MATCH_EXACT(ATH8032_PHY_ID), > > .name = "Qualcomm Atheros AR8032", > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .flags = PHY_POLL_CABLE_TEST, > > .config_init = at803x_config_init, > > .link_change_notify = at803x_link_change_notify, > > @@ -2100,7 +2073,6 @@ static struct phy_driver at803x_driver[] = { > > PHY_ID_MATCH_EXACT(ATH9331_PHY_ID), > > .name = "Qualcomm Atheros AR9331 built-in PHY", > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .suspend = at803x_suspend, > > .resume = at803x_resume, > > .flags = PHY_POLL_CABLE_TEST, > > @@ -2117,7 +2089,6 @@ static struct phy_driver at803x_driver[] = { > > PHY_ID_MATCH_EXACT(QCA9561_PHY_ID), > > .name = "Qualcomm Atheros QCA9561 built-in PHY", > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .suspend = at803x_suspend, > > .resume = at803x_resume, > > .flags = PHY_POLL_CABLE_TEST, > > @@ -2183,7 +2154,6 @@ static struct phy_driver at803x_driver[] = { > > .name = "Qualcomm QCA8081", > > .flags = PHY_POLL_CABLE_TEST, > > .probe = at803x_probe, > > - .remove = at803x_remove, > > .config_intr = at803x_config_intr, > > .handle_interrupt = at803x_handle_interrupt, > > .get_tunable = at803x_get_tunable, > >
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 656136628ffd..c1f307d90518 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -304,7 +304,6 @@ struct at803x_priv { bool is_1000basex; struct regulator_dev *vddio_rdev; struct regulator_dev *vddh_rdev; - struct regulator *vddio; u64 stats[ARRAY_SIZE(at803x_hw_stats)]; }; @@ -824,11 +823,11 @@ static int at803x_parse_dt(struct phy_device *phydev) if (ret < 0) return ret; - priv->vddio = devm_regulator_get_optional(&phydev->mdio.dev, - "vddio"); - if (IS_ERR(priv->vddio)) { + ret = devm_regulator_get_enable_optional(&phydev->mdio.dev, + "vddio"); + if (ret) { phydev_err(phydev, "failed to get VDDIO regulator\n"); - return PTR_ERR(priv->vddio); + return ret; } /* Only AR8031/8033 support 1000Base-X for SFP modules */ @@ -856,12 +855,6 @@ static int at803x_probe(struct phy_device *phydev) if (ret) return ret; - if (priv->vddio) { - ret = regulator_enable(priv->vddio); - if (ret < 0) - return ret; - } - if (phydev->drv->phy_id == ATH8031_PHY_ID) { int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG); int mode_cfg; @@ -869,10 +862,8 @@ static int at803x_probe(struct phy_device *phydev) .wolopts = 0, }; - if (ccr < 0) { - ret = ccr; - goto err; - } + if (ccr < 0) + return ccr; mode_cfg = ccr & AT803X_MODE_CFG_MASK; switch (mode_cfg) { @@ -890,25 +881,11 @@ static int at803x_probe(struct phy_device *phydev) ret = at803x_set_wol(phydev, &wol); if (ret < 0) { phydev_err(phydev, "failed to disable WOL on probe: %d\n", ret); - goto err; + return ret; } } return 0; - -err: - if (priv->vddio) - regulator_disable(priv->vddio); - - return ret; -} - -static void at803x_remove(struct phy_device *phydev) -{ - struct at803x_priv *priv = phydev->priv; - - if (priv->vddio) - regulator_disable(priv->vddio); } static int at803x_get_features(struct phy_device *phydev) @@ -2021,7 +1998,6 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm Atheros AR8035", .flags = PHY_POLL_CABLE_TEST, .probe = at803x_probe, - .remove = at803x_remove, .config_aneg = at803x_config_aneg, .config_init = at803x_config_init, .soft_reset = genphy_soft_reset, @@ -2043,7 +2019,6 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm Atheros AR8030", .phy_id_mask = AT8030_PHY_ID_MASK, .probe = at803x_probe, - .remove = at803x_remove, .config_init = at803x_config_init, .link_change_notify = at803x_link_change_notify, .set_wol = at803x_set_wol, @@ -2059,7 +2034,6 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm Atheros AR8031/AR8033", .flags = PHY_POLL_CABLE_TEST, .probe = at803x_probe, - .remove = at803x_remove, .config_init = at803x_config_init, .config_aneg = at803x_config_aneg, .soft_reset = genphy_soft_reset, @@ -2082,7 +2056,6 @@ static struct phy_driver at803x_driver[] = { PHY_ID_MATCH_EXACT(ATH8032_PHY_ID), .name = "Qualcomm Atheros AR8032", .probe = at803x_probe, - .remove = at803x_remove, .flags = PHY_POLL_CABLE_TEST, .config_init = at803x_config_init, .link_change_notify = at803x_link_change_notify, @@ -2100,7 +2073,6 @@ static struct phy_driver at803x_driver[] = { PHY_ID_MATCH_EXACT(ATH9331_PHY_ID), .name = "Qualcomm Atheros AR9331 built-in PHY", .probe = at803x_probe, - .remove = at803x_remove, .suspend = at803x_suspend, .resume = at803x_resume, .flags = PHY_POLL_CABLE_TEST, @@ -2117,7 +2089,6 @@ static struct phy_driver at803x_driver[] = { PHY_ID_MATCH_EXACT(QCA9561_PHY_ID), .name = "Qualcomm Atheros QCA9561 built-in PHY", .probe = at803x_probe, - .remove = at803x_remove, .suspend = at803x_suspend, .resume = at803x_resume, .flags = PHY_POLL_CABLE_TEST, @@ -2183,7 +2154,6 @@ static struct phy_driver at803x_driver[] = { .name = "Qualcomm QCA8081", .flags = PHY_POLL_CABLE_TEST, .probe = at803x_probe, - .remove = at803x_remove, .config_intr = at803x_config_intr, .handle_interrupt = at803x_handle_interrupt, .get_tunable = at803x_get_tunable,
Use devm_regulator_get_enable_optional() instead of hand writing it. It saves some line of code. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Compile tested-only. If my reading is correct, regulator_disable() is still called in the same order, should an error occur or the driver be removed. --- drivers/net/phy/at803x.c | 44 +++++++--------------------------------- 1 file changed, 7 insertions(+), 37 deletions(-)