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

[Timur Tabi]

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.

> 
>> +               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().

>> 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. :-)

> 
>> +                      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.

-- 
Timur Tabi
Linux kernel developer at Freescale