Re: [PATCH] sh_eth: call of_mdiobus_register() to register phys

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 02/18/2014 07:00 PM, Ben Dooks wrote:

>>>>> If the sh_eth device is registered using OF, then the driver

>>>>     Which is not supported yet as my DT patch hasn't been merged.
>>>> This patch seems somewhat premature.

>>> I've got your OF patches in my local tree to test with, this
>>> is what I found during that testing.

>>     The issue is that I didn't post my v3 patch to netdev due to
>> net-next.git repo being closed at this moment and DaveM not wanting to
>> see any patch targeted to it during this time. I've now posted v4 of my
>> Ether DT patch to netdev.

> Ok, I will look for these tomorrow.

    No significant changes there...

>>>>> should call of_mdiobus_register() to register any PHYs connected
>>>>> to the system.

>>>>     That's not necessary (but good to have).

>>> Well, it is necessary if you then want any PHYS bound to
>>> the device to have their OF information to hand,

>>     Ether DT support worked for me without this fragment, at least.

> Yes, it just that the PHY is not being linked to the relevant
> OF node. The PHY gets bound, it will not be able to find the
> DT info passed.

    With no DT support in the PHY driver, I don't see how it matters. Perhaps 
it has to do with your "init-regs" prop patch though...

>>>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>>>> ---
>>>>>   drivers/net/ethernet/renesas/sh_eth.c | 13 +++++++++++++
>>>>>   1 file changed, 13 insertions(+)

>>>>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>>>>> b/drivers/net/ethernet/renesas/sh_eth.c
>>>>> index 06970ac..165f0c4 100644
>>>>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>>>>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
>>>> [...]
>>>>> @@ -2629,6 +2630,18 @@ static int sh_mdio_init(struct net_device
>>>>> *ndev, int id,
>>>>>       snprintf(mdp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>>>>>            mdp->pdev->name, id);
>>>>>
>>>>> +    if (ndev->dev.parent->of_node) {
>>>>> +        dev_set_drvdata(&ndev->dev, mdp->mii_bus);
>>>>> +        ret = of_mdiobus_register(mdp->mii_bus,
>>>>> +                      ndev->dev.parent->of_node);
>>>>> +        if (ret != 0) {
>>>>> +            dev_err(&ndev->dev, "of_mdiobus_register() failed\n");
>>>>> +            goto out_free_bus;
>>>>> +        }

>>> I should probably only set the drvdata if the
>>> of_mdiobus_register() succeeds.

>>     Yes. Probably should use *goto* as well since in that case the
>> success path would be the same as the existing one.

> I will look into that, not the biggest fan of gotos for
> success cases.

    Can also try to use *else* branch to call mdiobus_register()...

>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>>       /* PHY IRQ */
>>>>>       mdp->mii_bus->irq = devm_kzalloc(&ndev->dev,
>>>>>                        sizeof(int) * PHY_MAX_ADDR,

[...]

WBR, Sergei