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;
dev_err(dev, "couldn't get sata-phy\n");
rc = PTR_ERR(hpriv->phy);
goto disable_unprepare_clk;
}
continue:
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
