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