Messages in this thread | | | Date | Wed, 08 Jan 2014 14:14:18 +0100 | From | Hans de Goede <> | Subject | Re: [PATCH v3 2/3] ata: ahci_platform: Manage SATA PHY |
| |
Hi,
On 01/08/2014 12:28 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;
-ENODEV is not the right errno to check for, if there is no phy specified in the dt then you will get -EINVAL (no phy-names property) or -ENODATA (phy-names property is there but name not found).
Also you don't want to log an error on PROBE_DEFERRAL.
Here is what I've for similar code I'm working on for ehci-platform.c and ohci-platform.c :
priv->phy = devm_phy_get(&dev->dev, "phy0"); if (IS_ERR(priv->phy)) { err = PTR_ERR(priv->phy); if (err != -EINVAL && err != -ENODATA) { if (err != -EPROBE_DEFER) dev_err(&dev->dev, "Error getting phy\n"); goto err_put_hcd; } priv->phy = NULL; }
And from then on I simply use "if (priv->phy)" checks, and of course check phy_init and phy_power_on return values.
Regards,
Hans
| |