| Message ID | 1451397935-23643-1-git-send-email-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier@gmail.com> wrote: >Originally, most of the platforms using this driver did not define an >mdio subnode >in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio >bus for stmmac driver") >introduced a backward compatibily issue by using of_mdiobus_register >explicitly >with an mdio subnode. This patch fixes the issue by calling the >function >mdiobus_register, when mdio subnode is not found. The driver is now >compatible >with both modes. Looks reasonable to me, though you will want to make sure the different DTSes get updated to include the proper compatible node for the MDIO bus. > >Signed-off-by: Romain Perier <romain.perier@gmail.com> Please include a Fixes tag to help keep track of changes. >--- > drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > >diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >index 16c85cc..0034de44 100644 >--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev) > if (mdio_node) { > netdev_dbg(ndev, "FOUND MDIO subnode\n"); > } else { >- netdev_err(ndev, "NO MDIO subnode\n"); >- return 0; >+ netdev_warn(ndev, "No MDIO subnode found\n"); > } > } > >@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev) > new_bus->phy_mask = mdio_bus_data->phy_mask; > new_bus->parent = priv->device; > >- err = of_mdiobus_register(new_bus, mdio_node); >+ if (IS_ENABLED(CONFIG_OF) && mdio_node) You should be able to drop the IS_ENABLED part since there is an inline provided in the non-OF case which does a fallback to mdiobus_register(). >+ err = of_mdiobus_register(new_bus, mdio_node); >+ else >+ err = mdiobus_register(new_bus); > if (err != 0) { This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated? > pr_err("%s: Cannot register as MDIO bus\n", new_bus->name); > goto bus_register_fail;
Hi all, 2016-01-02 6:51 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: > On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier@gmail.com> wrote: >>Originally, most of the platforms using this driver did not define an >>mdio subnode >>in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio >>bus for stmmac driver") >>introduced a backward compatibily issue by using of_mdiobus_register >>explicitly >>with an mdio subnode. This patch fixes the issue by calling the >>function >>mdiobus_register, when mdio subnode is not found. The driver is now >>compatible >>with both modes. > > Looks reasonable to me, though you will want to make sure the different DTSes get updated to include the proper compatible node for the MDIO bus. > >> >>Signed-off-by: Romain Perier <romain.perier@gmail.com> > > Please include a Fixes tag to help keep track of changes. Sure, sorry for my ignorance, but I did not know that we're supposed to add a Fixes tag in this situation. > >>--- >> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >>b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >>index 16c85cc..0034de44 100644 >>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c >>@@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev) >> if (mdio_node) { >> netdev_dbg(ndev, "FOUND MDIO subnode\n"); >> } else { >>- netdev_err(ndev, "NO MDIO subnode\n"); >>- return 0; >>+ netdev_warn(ndev, "No MDIO subnode found\n"); >> } >> } >> >>@@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev) >> new_bus->phy_mask = mdio_bus_data->phy_mask; >> new_bus->parent = priv->device; >> >>- err = of_mdiobus_register(new_bus, mdio_node); >>+ if (IS_ENABLED(CONFIG_OF) && mdio_node) > > You should be able to drop the IS_ENABLED part since there is an inline provided in the non-OF case which does a fallback to mdiobus_register(). Good point > >>+ err = of_mdiobus_register(new_bus, mdio_node); >>+ else >>+ err = mdiobus_register(new_bus); >> if (err != 0) { > > This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated? > I don't understand what you mean. Instead of defining mdio_node with NULL, you propose to assign it to pdev->dev.of_node as its default value... then it would be reassigned with "child_nod" if a subnode with the compatible string "snps,dwmac-mdio" is found, or kept with its default value otherwise... and in this case we could call of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the conditonnal branch) Regards, Romain
Le 02/01/2016 07:11, Romain Perier a écrit : >>> Signed-off-by: Romain Perier <romain.perier@gmail.com> >> >> Please include a Fixes tag to help keep track of changes. > > Sure, sorry for my ignorance, but I did not know that we're supposed > to add a Fixes tag in this situation. It is a good practice and definitively helps making sure that if regression entered a merge window, and a fix came in another merge window, people can easily backport the fix, moving on.. [snip] >> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated? >> > > I don't understand what you mean. Instead of defining mdio_node with > NULL, you propose to assign it to pdev->dev.of_node as its default > value... then it would be reassigned with "child_nod" if a subnode > with the compatible string "snps,dwmac-mdio" is found, or kept with > its default value otherwise... and in this case we could call > of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the > conditonnal branch) Your understanding is correct, this is what I was suggesting, thanks
Hi, 2016-01-02 18:52 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: > >>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated? >>> >> >> I don't understand what you mean. Instead of defining mdio_node with >> NULL, you propose to assign it to pdev->dev.of_node as its default >> value... then it would be reassigned with "child_nod" if a subnode >> with the compatible string "snps,dwmac-mdio" is found, or kept with >> its default value otherwise... and in this case we could call >> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the >> conditonnal branch) > > Your understanding is correct, this is what I was suggesting, thanks So in this case, I already tested this yesterday, it does not work, basically because the for loop just after of_mdiobus_register is never executed, probably because new_bus->phy_map is not populated, which is not the case by calling mdiobus_register for example. of_mdiobus_register seems to populate this array of phys only for subnodes found in the devicetree... Regards, Romain
On 03/01/16 06:36, Romain Perier wrote: > Hi, > > 2016-01-02 18:52 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: >> >>>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated? >>>> >>> >>> I don't understand what you mean. Instead of defining mdio_node with >>> NULL, you propose to assign it to pdev->dev.of_node as its default >>> value... then it would be reassigned with "child_nod" if a subnode >>> with the compatible string "snps,dwmac-mdio" is found, or kept with >>> its default value otherwise... and in this case we could call >>> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the >>> conditonnal branch) >> >> Your understanding is correct, this is what I was suggesting, thanks > > So in this case, I already tested this yesterday, it does not work, > basically because the for loop just after of_mdiobus_register is never > executed, probably because new_bus->phy_map is not populated, which is > not the case by calling mdiobus_register for example. > of_mdiobus_register seems to populate this array of phys only for > subnodes found in the devicetree... Fair enough, then you initial patch with dropping IS_ENABLED() and adding a "Fixes" tag should be resubmitted. Thank you
On 8/01/2016 3:45 AM, Florian Fainelli wrote: > On 03/01/16 06:36, Romain Perier wrote: >> Hi, >> >> 2016-01-02 18:52 GMT+01:00 Florian Fainelli <f.fainelli@gmail.com>: >>> >>>>> This looks reasonable, does it work if we just assign mdio_node to the pdev->dev.of_node to be backwards compatible with DTSes which have not yet been updated? >>>>> >>>> >>>> I don't understand what you mean. Instead of defining mdio_node with >>>> NULL, you propose to assign it to pdev->dev.of_node as its default >>>> value... then it would be reassigned with "child_nod" if a subnode >>>> with the compatible string "snps,dwmac-mdio" is found, or kept with >>>> its default value otherwise... and in this case we could call >>>> of_mdiobus_register(new_bus, mdio_node) all the time ? (it removes the >>>> conditonnal branch) >>> >>> Your understanding is correct, this is what I was suggesting, thanks >> >> So in this case, I already tested this yesterday, it does not work, >> basically because the for loop just after of_mdiobus_register is never >> executed, probably because new_bus->phy_map is not populated, which is >> not the case by calling mdiobus_register for example. >> of_mdiobus_register seems to populate this array of phys only for >> subnodes found in the devicetree... > > Fair enough, then you initial patch with dropping IS_ENABLED() and > adding a "Fixes" tag should be resubmitted. > Looks reasonable.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c index 16c85cc..0034de44 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c @@ -218,8 +218,7 @@ int stmmac_mdio_register(struct net_device *ndev) if (mdio_node) { netdev_dbg(ndev, "FOUND MDIO subnode\n"); } else { - netdev_err(ndev, "NO MDIO subnode\n"); - return 0; + netdev_warn(ndev, "No MDIO subnode found\n"); } } @@ -251,7 +250,10 @@ int stmmac_mdio_register(struct net_device *ndev) new_bus->phy_mask = mdio_bus_data->phy_mask; new_bus->parent = priv->device; - err = of_mdiobus_register(new_bus, mdio_node); + if (IS_ENABLED(CONFIG_OF) && mdio_node) + err = of_mdiobus_register(new_bus, mdio_node); + else + err = mdiobus_register(new_bus); if (err != 0) { pr_err("%s: Cannot register as MDIO bus\n", new_bus->name); goto bus_register_fail;
Originally, most of the platforms using this driver did not define an mdio subnode in the devicetree. Commit e34d65 ("stmmac: create of compatible mdio bus for stmmac driver") introduced a backward compatibily issue by using of_mdiobus_register explicitly with an mdio subnode. This patch fixes the issue by calling the function mdiobus_register, when mdio subnode is not found. The driver is now compatible with both modes. Signed-off-by: Romain Perier <romain.perier@gmail.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)