| Message ID | 20240826212205.187073-1-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv4,net-next] net: ag71xx: get reset control using devm api | expand |
On Mon, 26 Aug 2024 14:21:57 -0700 Rosen Penev wrote: > Currently, the of variant is missing reset_control_put in error paths. > The devm variant does not require it. > > Allows removing mdio_reset from the struct as it is not used outside the > function. > @@ -683,6 +682,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > struct device *dev = &ag->pdev->dev; > struct net_device *ndev = ag->ndev; > static struct mii_bus *mii_bus; > + struct reset_control *mdio_reset; nit: maintain the longest to shortest ordering of the variables (sorted by line length not type length) > struct device_node *np, *mnp; > int err; > > @@ -698,10 +698,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > if (!mii_bus) > return -ENOMEM; > > - ag->mdio_reset = of_reset_control_get_exclusive(np, "mdio"); > - if (IS_ERR(ag->mdio_reset)) { > + mdio_reset = devm_reset_control_get_exclusive(dev, "mdio"); > + if (IS_ERR(mdio_reset)) { > netif_err(ag, probe, ndev, "Failed to get reset mdio.\n"); > - return PTR_ERR(ag->mdio_reset); > + return PTR_ERR(mdio_reset); > } > > mii_bus->name = "ag71xx_mdio"; > @@ -712,10 +712,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > mii_bus->parent = dev; > snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); > > - if (!IS_ERR(ag->mdio_reset)) { > - reset_control_assert(ag->mdio_reset); > + if (!IS_ERR(mdio_reset)) { Are you planning to follow up to remove this check? Would be nice to do that as second patch in the same series > + reset_control_assert(mdio_reset); > msleep(100); > - reset_control_deassert(ag->mdio_reset); > + reset_control_deassert(mdio_reset); > msleep(200); > } >
On Tue, Aug 27, 2024 at 4:13 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 26 Aug 2024 14:21:57 -0700 Rosen Penev wrote: > > Currently, the of variant is missing reset_control_put in error paths. > > The devm variant does not require it. > > > > Allows removing mdio_reset from the struct as it is not used outside the > > function. > > > @@ -683,6 +682,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > struct device *dev = &ag->pdev->dev; > > struct net_device *ndev = ag->ndev; > > static struct mii_bus *mii_bus; > > + struct reset_control *mdio_reset; > > nit: maintain the longest to shortest ordering of the variables > (sorted by line length not type length) > > > struct device_node *np, *mnp; > > int err; > > > > @@ -698,10 +698,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > if (!mii_bus) > > return -ENOMEM; > > > > - ag->mdio_reset = of_reset_control_get_exclusive(np, "mdio"); > > - if (IS_ERR(ag->mdio_reset)) { > > + mdio_reset = devm_reset_control_get_exclusive(dev, "mdio"); > > + if (IS_ERR(mdio_reset)) { > > netif_err(ag, probe, ndev, "Failed to get reset mdio.\n"); > > - return PTR_ERR(ag->mdio_reset); > > + return PTR_ERR(mdio_reset); > > } > > > > mii_bus->name = "ag71xx_mdio"; > > @@ -712,10 +712,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > mii_bus->parent = dev; > > snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); > > > > - if (!IS_ERR(ag->mdio_reset)) { > > - reset_control_assert(ag->mdio_reset); > > + if (!IS_ERR(mdio_reset)) { > > Are you planning to follow up to remove this check? > Would be nice to do that as second patch in the same series I actually have no idea why this is here. I assume it's some mistake. I don't think it's meant to be optional... > > > + reset_control_assert(mdio_reset); > > msleep(100); > > - reset_control_deassert(ag->mdio_reset); > > + reset_control_deassert(mdio_reset); > > msleep(200); > > } > > >
> On Tue, Aug 27, 2024 at 4:13 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > On Mon, 26 Aug 2024 14:21:57 -0700 Rosen Penev wrote: > > > Currently, the of variant is missing reset_control_put in error paths. > > > The devm variant does not require it. > > > > > > Allows removing mdio_reset from the struct as it is not used outside the > > > function. > > > > > @@ -683,6 +682,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > struct device *dev = &ag->pdev->dev; > > > struct net_device *ndev = ag->ndev; > > > static struct mii_bus *mii_bus; > > > + struct reset_control *mdio_reset; > > > > nit: maintain the longest to shortest ordering of the variables > > (sorted by line length not type length) > > > > > struct device_node *np, *mnp; > > > int err; > > > > > > @@ -698,10 +698,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > if (!mii_bus) > > > return -ENOMEM; > > > > > > - ag->mdio_reset = of_reset_control_get_exclusive(np, "mdio"); > > > - if (IS_ERR(ag->mdio_reset)) { > > > + mdio_reset = devm_reset_control_get_exclusive(dev, "mdio"); > > > + if (IS_ERR(mdio_reset)) { > > > netif_err(ag, probe, ndev, "Failed to get reset mdio.\n"); > > > - return PTR_ERR(ag->mdio_reset); > > > + return PTR_ERR(mdio_reset); > > > } > > > > > > mii_bus->name = "ag71xx_mdio"; > > > @@ -712,10 +712,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > mii_bus->parent = dev; > > > snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); > > > > > > - if (!IS_ERR(ag->mdio_reset)) { > > > - reset_control_assert(ag->mdio_reset); > > > + if (!IS_ERR(mdio_reset)) { > > > > Are you planning to follow up to remove this check? > > Would be nice to do that as second patch in the same series > I actually have no idea why this is here. I assume it's some mistake. > I don't think it's meant to be optional... {devm,of}_reset_control_get_exclusive() will return an error if the OF node is missing. If it should be optional, it should be devm_reset_control_get_optional_exclusive(), which would return NULL if it is missing. The equivalent driver used in OpenWrt does explicitly make it optional. https://github.com/openwrt/openwrt/blob/4646aa169986036772b9f75393c08508d20ddf8b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c#L1532, while the mac_reset is mandatory. They might have a reason for that or maybe only heritage from Atheros AG7100 driver. > > > > > + reset_control_assert(mdio_reset); > > > msleep(100); > > > - reset_control_deassert(ag->mdio_reset); > > > + reset_control_deassert(mdio_reset); > > > msleep(200); > > > } Regards, Luiz
On Thu, Aug 29, 2024 at 3:20 PM Luiz Angelo Daros de Luca <luizluca@gmail.com> wrote: > > > On Tue, Aug 27, 2024 at 4:13 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > On Mon, 26 Aug 2024 14:21:57 -0700 Rosen Penev wrote: > > > > Currently, the of variant is missing reset_control_put in error paths. > > > > The devm variant does not require it. > > > > > > > > Allows removing mdio_reset from the struct as it is not used outside the > > > > function. > > > > > > > @@ -683,6 +682,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > > struct device *dev = &ag->pdev->dev; > > > > struct net_device *ndev = ag->ndev; > > > > static struct mii_bus *mii_bus; > > > > + struct reset_control *mdio_reset; > > > > > > nit: maintain the longest to shortest ordering of the variables > > > (sorted by line length not type length) > > > > > > > struct device_node *np, *mnp; > > > > int err; > > > > > > > > @@ -698,10 +698,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > > if (!mii_bus) > > > > return -ENOMEM; > > > > > > > > - ag->mdio_reset = of_reset_control_get_exclusive(np, "mdio"); > > > > - if (IS_ERR(ag->mdio_reset)) { > > > > + mdio_reset = devm_reset_control_get_exclusive(dev, "mdio"); > > > > > > > + if (IS_ERR(mdio_reset)) { > > > > netif_err(ag, probe, ndev, "Failed to get reset mdio.\n"); > > > > - return PTR_ERR(ag->mdio_reset); > > > > + return PTR_ERR(mdio_reset); > > > > } > > > > > > > > mii_bus->name = "ag71xx_mdio"; > > > > @@ -712,10 +712,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) > > > > mii_bus->parent = dev; > > > > snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); > > > > > > > > - if (!IS_ERR(ag->mdio_reset)) { > > > > - reset_control_assert(ag->mdio_reset); > > > > + if (!IS_ERR(mdio_reset)) { > > > > > > Are you planning to follow up to remove this check? > > > Would be nice to do that as second patch in the same series > > I actually have no idea why this is here. I assume it's some mistake. > > I don't think it's meant to be optional... > > {devm,of}_reset_control_get_exclusive() will return an error if the OF > node is missing. If it should be optional, it should be > devm_reset_control_get_optional_exclusive(), which would return NULL > if it is missing. I assume during upstreaming it was suggested to remove optional. > > The equivalent driver used in OpenWrt does explicitly make it > optional. https://github.com/openwrt/openwrt/blob/4646aa169986036772b9f75393c08508d20ddf8b/target/linux/ath79/files/drivers/net/ethernet/atheros/ag71xx/ag71xx_main.c#L1532, And also the wrong check (it should be NULL). > while the mac_reset is mandatory. They might have a reason for that or > maybe only heritage from Atheros AG7100 driver. > > > > > > > > + reset_control_assert(mdio_reset); > > > > msleep(100); > > > > - reset_control_deassert(ag->mdio_reset); > > > > + reset_control_deassert(mdio_reset); > > > > msleep(200); > > > > } > > Regards, > > Luiz
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c index 89cd001b385f..d81aa0ccd572 100644 --- a/drivers/net/ethernet/atheros/ag71xx.c +++ b/drivers/net/ethernet/atheros/ag71xx.c @@ -379,7 +379,6 @@ struct ag71xx { u32 fifodata[3]; int mac_idx; - struct reset_control *mdio_reset; struct clk *clk_mdio; }; @@ -683,6 +682,7 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) struct device *dev = &ag->pdev->dev; struct net_device *ndev = ag->ndev; static struct mii_bus *mii_bus; + struct reset_control *mdio_reset; struct device_node *np, *mnp; int err; @@ -698,10 +698,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) if (!mii_bus) return -ENOMEM; - ag->mdio_reset = of_reset_control_get_exclusive(np, "mdio"); - if (IS_ERR(ag->mdio_reset)) { + mdio_reset = devm_reset_control_get_exclusive(dev, "mdio"); + if (IS_ERR(mdio_reset)) { netif_err(ag, probe, ndev, "Failed to get reset mdio.\n"); - return PTR_ERR(ag->mdio_reset); + return PTR_ERR(mdio_reset); } mii_bus->name = "ag71xx_mdio"; @@ -712,10 +712,10 @@ static int ag71xx_mdio_probe(struct ag71xx *ag) mii_bus->parent = dev; snprintf(mii_bus->id, MII_BUS_ID_SIZE, "%s.%d", np->name, ag->mac_idx); - if (!IS_ERR(ag->mdio_reset)) { - reset_control_assert(ag->mdio_reset); + if (!IS_ERR(mdio_reset)) { + reset_control_assert(mdio_reset); msleep(100); - reset_control_deassert(ag->mdio_reset); + reset_control_deassert(mdio_reset); msleep(200); }
Currently, the of variant is missing reset_control_put in error paths. The devm variant does not require it. Allows removing mdio_reset from the struct as it is not used outside the function. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- v2: don't call after ag71xx_mdio_probe. Already done. v3: use devm instead. v4: resend to bump the CI drivers/net/ethernet/atheros/ag71xx.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)