| Message ID | 1472121901-15629-10-git-send-email-sean.wang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sean, small nitpick inline On 25/08/2016 12:45, Sean Wang wrote: > a lot of parts in the driver uses devm_* APIs to gain benefits from the > device resource management, so devm_mdiobus_alloc is also used instead > of mdiobus_alloc to have more elegant code flow. > > Signed-off-by: Sean Wang <sean.wang@mediatek.com> > --- > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index e3baa63..05d85da 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth) > goto err_put_node; > } > > - eth->mii_bus = mdiobus_alloc(); > + eth->mii_bus = devm_mdiobus_alloc(eth->dev); > if (!eth->mii_bus) { > err = -ENOMEM; > goto err_put_node; > @@ -318,18 +318,9 @@ static int mtk_mdio_init(struct mtk_eth *eth) > > snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name); > err = of_mdiobus_register(eth->mii_bus, mii_np); > - if (err) > - goto err_free_bus; > - of_node_put(mii_np); > - > - return 0; > - > -err_free_bus: > - mdiobus_free(eth->mii_bus); > > err_put_node: > of_node_put(mii_np); > - eth->mii_bus = NULL; > return err; you might want to rename err to ret. that would make it more readable. right now it looks like the code always flows through the error path. John > } > > @@ -339,8 +330,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth) > return; > > mdiobus_unregister(eth->mii_bus); > - of_node_put(eth->mii_bus->dev.of_node); > - mdiobus_free(eth->mii_bus); > } > > static inline void mtk_irq_disable(struct mtk_eth *eth, u32 mask) >
On Date: Thu, 25 Aug 2016 15:39:08 +0200, John Crispin wrote: >Hi Sean, > >small nitpick inline > >On 25/08/2016 12:45, Sean Wang wrote: >> a lot of parts in the driver uses devm_* APIs to gain benefits from the .... >> - eth->mii_bus = mdiobus_alloc(); >> + eth->mii_bus = devm_mdiobus_alloc(eth->dev); >> if (!eth->mii_bus) { >> err = -ENOMEM; >> goto err_put_node; >> @@ -318,18 +318,9 @@ static int mtk_mdio_init(struct mtk_eth *eth) >> >> snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name); >> err = of_mdiobus_register(eth->mii_bus, mii_np); >> - if (err) >> - goto err_free_bus; >> - of_node_put(mii_np); >> - >> - return 0; >> - >> -err_free_bus: >> - mdiobus_free(eth->mii_bus); >> >> err_put_node: >> of_node_put(mii_np); >> - eth->mii_bus = NULL; >> return err; > >you might want to rename err to ret. that would make it more readable. >right now it looks like the code always flows through the error path. > > John > okay, i will change this from err to ret for readable code and my thought is using common code provided by the framework helps to 1) have simplified the code flow as [1] says 2) decrease the risk of incorrect error handling by human 3) only a few drivers used it since it ware proposed on linux 3.16, so just hope to promote for this. [1] https://patchwork.ozlabs.org/patch/344093/ >> } >> >> @@ -339,8 +330,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth) >> return; >> >> mdiobus_unregister(eth->mii_bus); >> - of_node_put(eth->mii_bus->dev.of_node); >> - mdiobus_free(eth->mii_bus); >> }
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index e3baa63..05d85da 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -304,7 +304,7 @@ static int mtk_mdio_init(struct mtk_eth *eth) goto err_put_node; } - eth->mii_bus = mdiobus_alloc(); + eth->mii_bus = devm_mdiobus_alloc(eth->dev); if (!eth->mii_bus) { err = -ENOMEM; goto err_put_node; @@ -318,18 +318,9 @@ static int mtk_mdio_init(struct mtk_eth *eth) snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%s", mii_np->name); err = of_mdiobus_register(eth->mii_bus, mii_np); - if (err) - goto err_free_bus; - of_node_put(mii_np); - - return 0; - -err_free_bus: - mdiobus_free(eth->mii_bus); err_put_node: of_node_put(mii_np); - eth->mii_bus = NULL; return err; } @@ -339,8 +330,6 @@ static void mtk_mdio_cleanup(struct mtk_eth *eth) return; mdiobus_unregister(eth->mii_bus); - of_node_put(eth->mii_bus->dev.of_node); - mdiobus_free(eth->mii_bus); } static inline void mtk_irq_disable(struct mtk_eth *eth, u32 mask)
a lot of parts in the driver uses devm_* APIs to gain benefits from the device resource management, so devm_mdiobus_alloc is also used instead of mdiobus_alloc to have more elegant code flow. Signed-off-by: Sean Wang <sean.wang@mediatek.com> --- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)