[PATCH v3 0/3] ata: ahci_platform: Add PHY support and OMAP support

[PATCH v3 0/3] ata: ahci_platform: Add PHY support and OMAP support

[Roger Quadros]

Hi,

Some platforms have a PHY hooked up to the SATA controller.
The PHY needs to be initialized and powered up for SATA to work.
We do that using the Generic PHY framework in PATCH 2.

In order to support SATA on the OMAP platforms we need to runtime
resume the device before use. PATCH 3 takes care of that.

PATCH 1 just updates the compatible list in the DT binding document.

Changelog:
v3:
- Addressed review comments and rebased on 3.13-rc7
- DT binding update moved to new patch.

v2:
- got rid of Texas Instruments SATA wrapper driver
- adressed review comments

cheers,
-roger

Balaji T K (1):
  ata: ahci_platform: Manage SATA PHY

Roger Quadros (2):
  ata: ahci_platform: Update DT compatible list
  ata: ahci_platform: runtime resume the device before use

 .../devicetree/bindings/ata/ahci-platform.txt      |  4 ++-
 drivers/ata/ahci.h                                 |  2 ++
 drivers/ata/ahci_platform.c                        | 41 +++++++++++++++++++++-
 3 files changed, 45 insertions(+), 2 deletions(-)

-- 
1.8.3.2

[PATCH v3 1/3] ata: ahci_platform: Update DT compatible list

[Roger Quadros]

The ahci_platform driver supports "snps,exynos5440-ahci" and
"ibm,476gtr-ahci". Add this to the DT binding information.

Also add compatible string "snps,dwc-ahci", which should be used
for Synopsis Designware SATA cores. e.g. on TI OMAP5 and DRA7 platforms.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 Documentation/devicetree/bindings/ata/ahci-platform.txt | 4 +++-
 drivers/ata/ahci_platform.c                             | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index 89de156..08ba647 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -4,7 +4,9 @@ SATA nodes are defined to describe on-chip Serial ATA controllers.
 Each SATA controller should have its own node.
 
 Required properties:
-- compatible        : compatible list, contains "snps,spear-ahci"
+- compatible        : compatible list, contains "snps,spear-ahci",
+		      "snps,exynos5440-ahci", "ibm,476gtr-ahci",
+		      or "snps,dwc-ahci"
 - interrupts        : <interrupt mapping for SATA IRQ>
 - reg               : <registers mapping>
 
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index 4b231ba..d5ced13 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -330,6 +330,7 @@ static const struct of_device_id ahci_of_match[] = {
 	{ .compatible = "snps,spear-ahci", },
 	{ .compatible = "snps,exynos5440-ahci", },
 	{ .compatible = "ibm,476gtr-ahci", },
+	{ .compatible = "snps,dwc-ahci", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, ahci_of_match);
-- 
1.8.3.2

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

[Roger Quadros]

From: Balaji T K <balajitk@ti.com>

Some platforms have a PHY hooked up to the
SATA controller. The PHY needs to be initialized
and powered up for SATA to work. We do that
using the PHY framework.

[Roger Q] Cleaned up.

CC: Tejun Heo <tj@kernel.org>
Signed-off-by: Balaji T K <balajitk@ti.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/ata/ahci.h          |  2 ++
 drivers/ata/ahci_platform.c | 28 +++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 2289efd..f9bbada 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -37,6 +37,7 @@
 
 #include <linux/clk.h>
 #include <linux/libata.h>
+#include <linux/phy/phy.h>
 
 /* Enclosure Management Control */
 #define EM_CTRL_MSG_TYPE              0x000f0000
@@ -322,6 +323,7 @@ struct ahci_host_priv {
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clk;		/* Only for platforms supporting clk */
+	struct phy		*phy;		/* If platform uses phy */
 	void			*plat_data;	/* Other platform data */
 };
 
diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index d5ced13..f61093b 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -142,6 +142,21 @@ static int ahci_probe(struct platform_device *pdev)
 		}
 	}
 
+	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;
+		}
+	}
+
+	if (!IS_ERR(hpriv->phy)) {
+		phy_init(hpriv->phy);
+		phy_power_on(hpriv->phy);
+	}
+
 	/*
 	 * Some platforms might need to prepare for mmio region access,
 	 * which could be done in the following init call. So, the mmio
@@ -151,7 +166,7 @@ static int ahci_probe(struct platform_device *pdev)
 	if (pdata && pdata->init) {
 		rc = pdata->init(dev, hpriv->mmio);
 		if (rc)
-			goto disable_unprepare_clk;
+			goto disable_phy;
 	}
 
 	ahci_save_initial_config(dev, hpriv,
@@ -221,6 +236,12 @@ static int ahci_probe(struct platform_device *pdev)
 pdata_exit:
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
+disable_phy:
+	if (!IS_ERR(hpriv->phy)) {
+		phy_power_off(hpriv->phy);
+		phy_exit(hpriv->phy);
+	}
+
 disable_unprepare_clk:
 	if (!IS_ERR(hpriv->clk))
 		clk_disable_unprepare(hpriv->clk);
@@ -239,6 +260,11 @@ static void ahci_host_stop(struct ata_host *host)
 	if (pdata && pdata->exit)
 		pdata->exit(dev);
 
+	if (!IS_ERR(hpriv->phy)) {
+		phy_power_off(hpriv->phy);
+		phy_exit(hpriv->phy);
+	}
+
 	if (!IS_ERR(hpriv->clk)) {
 		clk_disable_unprepare(hpriv->clk);
 		clk_put(hpriv->clk);
-- 
1.8.3.2

[PATCH v3 3/3] ata: ahci_platform: runtime resume the device before use

[Roger Quadros]

On OMAP platforms the device needs to be runtime resumed before
it can be accessed. The OMAP HWMOD framework takes care of
enabling the module and its resources based on the
device's runtime PM state.

In this patch we runtime resume during .probe() and runtime suspend
during .remove() (i.e. ahci_host_stop()).

We also update the runtime PM state during .resume().

Signed-off-by: Roger Quadros <rogerq@ti.com>
Signed-off-by: Balaji T K <balajitk@ti.com>
---
 drivers/ata/ahci_platform.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
index f61093b..04c4ac2 100644
--- a/drivers/ata/ahci_platform.c
+++ b/drivers/ata/ahci_platform.c
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/libata.h>
 #include <linux/ahci_platform.h>
+#include <linux/pm_runtime.h>
 #include "ahci.h"
 
 static void ahci_host_stop(struct ata_host *host);
@@ -142,6 +143,9 @@ static int ahci_probe(struct platform_device *pdev)
 		}
 	}
 
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
 	hpriv->phy = devm_phy_get(dev, "sata-phy");
 	if (IS_ERR(hpriv->phy)) {
 		dev_dbg(dev, "can't get sata-phy\n");
@@ -269,6 +273,9 @@ static void ahci_host_stop(struct ata_host *host)
 		clk_disable_unprepare(hpriv->clk);
 		clk_put(hpriv->clk);
 	}
+
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -340,6 +347,11 @@ static int ahci_resume(struct device *dev)
 
 	ata_host_resume(host);
 
+	/* We resumed so update PM runtime state */
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	return 0;
 
 disable_unprepare_clk:
-- 
1.8.3.2

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

[Kishon Vijay Abraham I]

Hi Roger,

On Wednesday 08 January 2014 03:17 PM, Roger Quadros wrote:
> From: Balaji T K <balajitk-l0cyMroinI0@public.gmane.org>
> 
> Some platforms have a PHY hooked up to the
> SATA controller. The PHY needs to be initialized
> and powered up for SATA to work. We do that
> using the PHY framework.
> 
> [Roger Q] Cleaned up.
> 
> CC: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Balaji T K <balajitk-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Roger Quadros <rogerq-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/ata/ahci.h          |  2 ++
>  drivers/ata/ahci_platform.c | 28 +++++++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 2289efd..f9bbada 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -37,6 +37,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/libata.h>
> +#include <linux/phy/phy.h>
>  
>  /* Enclosure Management Control */
>  #define EM_CTRL_MSG_TYPE              0x000f0000
> @@ -322,6 +323,7 @@ struct ahci_host_priv {
>  	u32			em_buf_sz;	/* EM buffer size in byte */
>  	u32			em_msg_type;	/* EM message type */
>  	struct clk		*clk;		/* Only for platforms supporting clk */
> +	struct phy		*phy;		/* If platform uses phy */
>  	void			*plat_data;	/* Other platform data */
>  };
>  
> diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c
> index d5ced13..f61093b 100644
> --- a/drivers/ata/ahci_platform.c
> +++ b/drivers/ata/ahci_platform.c
> @@ -142,6 +142,21 @@ static int ahci_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	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;
> +		}
> +	}
> +
> +	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?

Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Arnd Bergmann]

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.

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

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Roger Quadros]

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

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

[Roger Quadros]

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
> 

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

[Arnd Bergmann]

On Wednesday 08 January 2014 16:58:15 Roger Quadros wrote:
> 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;
>         }
> }

As I said, I'd prefer to set hpriv->phy to NULL in case of -ENODEV,
but functionally it seems right (with the fixup from your other mail).

One more comment: you shouldn't print an error message before
returning -EPROBE_DEFER, so if you want to keep that message,
you have to check for that return value after all.

	Arnd

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

[Hans de Goede]

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

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

[Hans de Goede]

Hi,

On 01/08/2014 02:14 PM, Hans de Goede wrote:
> 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;
>                  }

Scrap that I was reading the code wrong. Indeed if there is no
devicetree phy info it will return -ENODEV. Actually it will only ever
return one of -ENOPROBE or -ENODEV, so only checking for -ENOPROBE and
then fail silently be enough. This also has the advantage of being
future proof, if the error code for no phy found ever changes,
things won't break.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Roger Quadros]

On 01/08/2014 05:16 PM, Arnd Bergmann wrote:
> On Wednesday 08 January 2014 16:58:15 Roger Quadros wrote:
>> 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;
>>         }
>> }
> 
> As I said, I'd prefer to set hpriv->phy to NULL in case of -ENODEV,
> but functionally it seems right (with the fixup from your other mail).
> 

Why do you prefer setting hpriv->phy to NULL instead of using IS_ERR() check
before hpriv->phy is used?
The latter seems to be the norm at least among clock framework users.

> One more comment: you shouldn't print an error message before
> returning -EPROBE_DEFER, so if you want to keep that message,
> you have to check for that return value after all.
>
Right.

cheers,
-roger

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

[Arnd Bergmann]

On Thursday 09 January 2014, Roger Quadros wrote:
> >> 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;
> >>         }
> >> }
> > 
> > As I said, I'd prefer to set hpriv->phy to NULL in case of -ENODEV,
> > but functionally it seems right (with the fixup from your other mail).
> > 
> 
> Why do you prefer setting hpriv->phy to NULL instead of using IS_ERR() check
> before hpriv->phy is used?
> The latter seems to be the norm at least among clock framework users.

Two reasons:

1. It feels more natural to read "if (clk)" in driver code as a check for
   "a clock exists" than "if (!IS_ERR(clk))".
2. It clarifies that this code path is only there to check for the
   clk-not-present case, not for other errors.

Obviously the first check after clk_get needs to be IS_ERR() because that
is the documented interface, but then you should decide on the action
based on the specific error.

	Arnd