Hi all,
2016-01-02 6:51 GMT+01:00 Florian Fainelli <f.fainelli at gmail.com>:
> On December 29, 2015 6:05:35 AM PST, Romain Perier <romain.perier at 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 at 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
