| Message ID | 1388963189-13556-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > 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 <hdegoede@redhat.com> > --- > 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)) >
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: >> 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: >> + 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
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))
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 <hdegoede@redhat.com> --- drivers/phy/phy-core.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)