Re: [PATCH v3 2/3] ata: ahci_platform: Manage SATA PHY

On 01/08/2014 03:35 PM, Arnd Bergmann wrote:

On Wednesday 08 January 2014 15:29:18 Kishon Vijay Abraham I wrote:

quoted

quoted

+     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.

quoted

quoted

+     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