Hi Timur Tabi,
On Tue, Jul 10, 2012 at 11:15 AM, Timur Tabi timur@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