[PATCH 1/2] phy-core: Don't print an error on probe deferral or nodata

Printing an error on probe-deferral clearly is not the right thing to do.
While at it I've also silenced the error in case of -ENODATA, so that
devm_phy_get can be used to get an optional phy without causing errors to
be printed. Alternatively a new devm_phy_get_optional method could be
added for this.

While at it also factor the error handling out of the if ... else ..., as it
is identical in both branches.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/phy/phy-core.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 58e0e97..d7b992e 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c

@@ -371,16 +371,14 @@ struct phy *phy_get(struct device *dev, const char *string)
 		index = of_property_match_string(dev->of_node, "phy-names",
 			string);
 		phy = of_phy_get(dev, index);
-		if (IS_ERR(phy)) {
-			dev_err(dev, "unable to find phy\n");
-			return phy;
-		}
 	} else {
 		phy = phy_lookup(dev, string);
-		if (IS_ERR(phy)) {
-			dev_err(dev, "unable to find phy\n");
-			return phy;
-		}
+	}
+	if (IS_ERR(phy)) {
+		int err = PTR_ERR(phy);
+		if (err != -EPROBE_DEFER && err != -ENODATA)
+			dev_err(dev, "unable to get phy\n");
+		return phy;
 	}
 
 	if (!try_module_get(phy->ops->owner))

-- 
1.8.4.2

The phy-core allows phy_init and phy_power_on to be called multiple times,
but before this patch -ENOSUPP from phy_pm_runtime_get_sync would be
propagated to the caller for the 2nd and later calls.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/phy/phy-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index d7b992e..8ee6157 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c

@@ -161,7 +161,8 @@ int phy_init(struct phy *phy)
 			dev_err(&phy->dev, "phy init failed --> %d\n", ret);
 			goto out;
 		}
-	}
+	} else
+		ret = 0; /* Override possible ret == -ENOTSUPP */
 
 out:
 	mutex_unlock(&phy->mutex);

@@ -209,7 +210,8 @@ int phy_power_on(struct phy *phy)
 			dev_err(&phy->dev, "phy poweron failed --> %d\n", ret);
 			goto out;
 		}
-	}
+	} else
+		ret = 0; /* Override possible ret == -ENOTSUPP */
 
 out:
 	mutex_unlock(&phy->mutex);

-- 
1.8.4.2

Hello.

On 06-01-2014 3:06, Hans de Goede wrote:

The phy-core allows phy_init and phy_power_on to be called multiple times,
but before this patch -ENOSUPP from phy_pm_runtime_get_sync would be
propagated to the caller for the 2nd and later calls.

Signed-off-by: Hans de Goede <redacted>
---
  drivers/phy/phy-core.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

    *else* arm should have {} when the *if* arm has it.

    Same here.

WBR, Sergei

Hi,

On Monday 06 January 2014 04:36 AM, Hans de Goede wrote:

The phy-core allows phy_init and phy_power_on to be called multiple times,
but before this patch -ENOSUPP from phy_pm_runtime_get_sync would be
propagated to the caller for the 2nd and later calls.

Thanks for fixing this. Have one minor comment below.

'should use braces in both branches'.

same here..

Thanks
Kishon

On Tuesday 07 January 2014 02:26 PM, Kishon Vijay Abraham I wrote:

Hi,

On Monday 06 January 2014 04:36 AM, Hans de Goede wrote:

quoted

The phy-core allows phy_init and phy_power_on to be called multiple times,
but before this patch -ENOSUPP from phy_pm_runtime_get_sync would be
propagated to the caller for the 2nd and later calls.

Thanks for fixing this. Have one minor comment below.

quoted

Signed-off-by: Hans de Goede <redacted>
---
 drivers/phy/phy-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index d7b992e..8ee6157 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c

@@ -161,7 +161,8 @@ int phy_init(struct phy *phy)
 			dev_err(&phy->dev, "phy init failed --> %d\n", ret);
 			goto out;
 		}
-	}
+	} else
+		ret = 0; /* Override possible ret == -ENOTSUPP */

'should use braces in both branches'.

ah.. Sergei had already commented :-)

quoted

 
 out:
 	mutex_unlock(&phy->mutex);

@@ -209,7 +210,8 @@ int phy_power_on(struct phy *phy)
 			dev_err(&phy->dev, "phy poweron failed --> %d\n", ret);
 			goto out;
 		}
-	}
+	} else
+		ret = 0; /* Override possible ret == -ENOTSUPP */

same here..

Thanks
Kishon

Hi,

On 01/07/2014 09:56 AM, Kishon Vijay Abraham I wrote:

Hi,

On Monday 06 January 2014 04:36 AM, Hans de Goede wrote:

quoted

The phy-core allows phy_init and phy_power_on to be called multiple times,
but before this patch -ENOSUPP from phy_pm_runtime_get_sync would be
propagated to the caller for the 2nd and later calls.

Thanks for fixing this. Have one minor comment below.

quoted

Signed-off-by: Hans de Goede <redacted>
---
  drivers/phy/phy-core.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index d7b992e..8ee6157 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c

@@ -161,7 +161,8 @@ int phy_init(struct phy *phy)
  			dev_err(&phy->dev, "phy init failed --> %d\n", ret);
  			goto out;
  		}
-	}
+	} else
+		ret = 0; /* Override possible ret == -ENOTSUPP */

'should use braces in both branches'.

Ok, I'll do a v2 fixing this soonish, any comments on the first patch?

Regards,

Hans

Hi,

On Monday 06 January 2014 04:36 AM, Hans de Goede wrote:

Printing an error on probe-deferral clearly is not the right thing to do.
While at it I've also silenced the error in case of -ENODATA, so that
devm_phy_get can be used to get an optional phy without causing errors to

What do you mean by optional phy here?

Thanks
Kishon

Hi,

On 01/07/2014 10:31 AM, Kishon Vijay Abraham I wrote:

Hi,

On Monday 06 January 2014 04:36 AM, Hans de Goede wrote:

quoted

Printing an error on probe-deferral clearly is not the right thing to do.
While at it I've also silenced the error in case of -ENODATA, so that
devm_phy_get can be used to get an optional phy without causing errors to

What do you mean by optional phy here?

Some generic platform drivers, ie drivers/usb/host/ehci-platform.c
(with some extensions to make it more generic I'm working on), may
take a phy in the devicetree-node, but having a phy is not mandatory,
the code using the phy will all be guarded against the phy not being
there and only call phy_ functions if it is actually there.

For this use case it would be nice if in this case devm_phy_get would
return an error code, but not do a dev_err, so as to not pollute dmesg
with irrelevant / wrong error messages.

The regulator core has devm_regulator_get_optional for this, so
if you don't want this silent behavior in devm_phy_get by default,
I can do write a different patch adding devm_phy_get_optional
instead.

Also I've just realized that this check:

quoted

+		if (err != -EPROBE_DEFER && err != -ENODATA)
+			dev_err(dev, "unable to get phy\n");

Is incomplete to also be silent when no phy-names are specified,
it should be:

	if (err != -EPROBE_DEFER && err != -ENODATA && err != -EINVAL)
		dev_err(dev, "unable to get phy\n");

I'll fix this (or add a devm_phy_get_optional) in v2 of this set.

Regards,

Hans