On 01/08/2014 04:58 PM, Roger Quadros wrote: > On 01/08/2014 03:35 PM, Arnd Bergmann wrote: >> On Wednesday 08 January 2014 15:29:18 Kishon Vijay Abraham I wrote: >>>> + hpriv->phy = devm_phy_get(dev, "sata-phy"); >>>> + if (IS_ERR(hpriv->phy)) { >>>> + dev_dbg(dev, "can't get sata-phy\n"); >>>> + /* return only if -EPROBE_DEFER */ >>>> + if (PTR_ERR(hpriv->phy) == -EPROBE_DEFER) { >>>> + rc = -EPROBE_DEFER; >>>> + goto disable_unprepare_clk; >>>> + } >>>> + } >> >> This should probably check for all errors except "not present" >> rather than checking for -EPROBE_DEFER. We want to abort the >> probe function for deferred probe as well as the case where we >> a PHY was listed but isn't working properly. > > OK. > >> >>>> + if (!IS_ERR(hpriv->phy)) { >>>> + phy_init(hpriv->phy); >>> >>> Don't we have to check the return values of phy_init and phy_power_on? Is it >>> not needed because it is an optional phy? >> >> Right. I think we should set hpriv->phy to NULL if it's not there and >> then call the functions only if it's actually present but bail out on >> an error. > > OK. How does this look? > > hpriv->phy = devm_phy_get(dev, "sata-phy"); > if (IS_ERR(hpriv->phy)) { > if (PTR_ERR(hpriv->phy) == -ENODEV) > goto continue; yikes :P goto phyinit; > > dev_err(dev, "couldn't get sata-phy\n"); > rc = PTR_ERR(hpriv->phy); > goto disable_unprepare_clk; > } > > continue: phyinit: > > if (!IS_ERR(hpriv->phy)) { > rc = phy_init(hpriv->phy); > if (rc) > goto disable_unprepare_clk; > > rc = phy_power_on(hpriv->phy); > if (rc) { > phy_exit(hpriv->phy); > goto disable_unprepare_clk; > } > } > > cheers, > -roger > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html