Re: [PATCH] stmmac: Don't exit mdio registration when mdio subnode is not found in the DTS

[Romain Perier <romain.perier@gmail.com>]

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