[U-Boot] [PATCH 2/2] [v3] net: abort network initialization if the PHY driver fails

[Joe Hershberger]

Hi Timur Tabi,

On Tue, Jul 10, 2012 at 11:15 AM, Timur Tabi <timur at freescale.com> wrote:
> Joe Hershberger wrote:
>
>>> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
>>> index eee41d7..5700552 100644
>>> --- a/drivers/net/fec_mxc.c
>>> +++ b/drivers/net/fec_mxc.c
>>> @@ -510,7 +510,13 @@ static int fec_open(struct eth_device *edev)
>>>                 fec_eth_phy_config(edev);
>>>         if (fec->phydev) {
>>>                 /* Start up the PHY */
>>> -               phy_startup(fec->phydev);
>>> +               int ret = phy_startup(fec->phydev);
>>> +
>>
>> Why a blank line here when it isn't in the rest of the implementations?
>
> The coding standard requires blank lines after variable declarations.
>
>>> +               if (ret) {
>>> +                       printf("Could not initialize PHY %s\n",
>>> +                              fec->phydev->dev->name);
>>> +                       return ret;
>>> +               }
>>>                 speed = fec->phydev->speed;
>>>         } else {
>>>                 speed = _100BASET;
>>> diff --git a/drivers/net/fm/eth.c b/drivers/net/fm/eth.c
>>> index f34f4db..2b616ad 100644
>>> --- a/drivers/net/fm/eth.c
>>> +++ b/drivers/net/fm/eth.c
>>> @@ -363,6 +363,9 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd)
>>>  {
>>>         struct fm_eth *fm_eth;
>>>         struct fsl_enet_mac *mac;
>>> +#ifdef CONFIG_PHYLIB
>>> +       int ret;
>>> +#endif
>>>
>>>         fm_eth = (struct fm_eth *)dev->priv;
>>>         mac = fm_eth->mac;
>>> @@ -384,7 +387,11 @@ static int fm_eth_open(struct eth_device *dev, bd_t *bd)
>>>         fmc_tx_port_graceful_stop_disable(fm_eth);
>>>
>>>  #ifdef CONFIG_PHYLIB
>>> -       phy_startup(fm_eth->phydev);
>>> +       ret = phy_startup(fm_eth->phydev);
>>> +       if (ret) {
>>> +               printf("%s: Could not initialize\n", fm_eth->phydev->dev->name);
>>
>> Why is this string different from the others? Consistency?
>
> Yes.  I tried to keep the messages consistent with the other messages in
> the function.

Should you not at least keep the core message the same?  "Could not
initialize PHY"

>>
>>> +               return ret;
>>> +       }
>>>  #else
>>>         fm_eth->phydev->speed = SPEED_1000;
>>>         fm_eth->phydev->link = 1;
>>> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
>>> index bb57e4d..268d884 100644
>>> --- a/drivers/net/sh_eth.c
>>> +++ b/drivers/net/sh_eth.c
>>> @@ -415,7 +415,11 @@ static int sh_eth_config(struct sh_eth_dev *eth, bd_t *bd)
>>>                 goto err_phy_cfg;
>>>         }
>>>         phy = port_info->phydev;
>>> -       phy_startup(phy);
>>> +       ret = phy_startup(phy);
>>> +       if (ret) {
>>> +               printf(SHETHER_NAME ": phy startup failure\n");
>>
>> Why is this string different from the others?  Consistency?
>
> Yes, it looks like the other messages in sh_eth_config().

Same here, at least the core message "Could not initialize PHY"

>>> diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c
>>> index 7854a04..d777144 100644
>>> --- a/drivers/net/xilinx_axi_emac.c
>>> +++ b/drivers/net/xilinx_axi_emac.c
>>> @@ -272,7 +272,11 @@ static int setup_phy(struct eth_device *dev)
>>>         phydev->advertising = phydev->supported;
>>>         priv->phydev = phydev;
>>>         phy_config(phydev);
>>> -       phy_startup(phydev);
>>> +       if (phy_startup(phydev)) {
>>> +               printf("axiemac: could not initialize PHY %s\n",
>>
>> Lower-case "could" in string?  Consistency?
>
> Yes, consistency. :-)

OK... I'm not sold, but I don't care enough.

>>
>>> +                      phydev->dev->name);
>>> +               return 0;
>>
>> Why are you returning 0 here and not the return value from phy_startup()?
>
> I answered that in another email.  This is what setup_phy() is supposed to
> return on failure.

OK.

Thanks,
-Joe