Hi Timur Tabi,
On Tue, Jul 10, 2012 at 11:15 AM, Timur Tabi <ti...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
