Messages in this thread | | | Subject | Re: [PATCH v2 13/14] usb: host: ehci-st: simplify optional reset handling | From | Philipp Zabel <> | Date | Wed, 15 Mar 2017 16:21:09 +0100 |
| |
Hi Alan,
On Wed, 2017-03-15 at 10:35 -0400, Alan Stern wrote: > On Wed, 15 Mar 2017, Philipp Zabel wrote: > > > As of commit bb475230b8e5 ("reset: make optional functions really > > optional"), the reset framework API calls use NULL pointers to describe > > optional, non-present reset controls. > > What does it use to describe genuine errors?
Negative error pointers, as before. The only difference is that instead of returning -ENOENT if no resets are specified in the device tree, the optional reset_control_get variants now return NULL.
> > This allows to return errors from devm_reset_control_get_optional_shared > > unconditionally. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > drivers/usb/host/ehci-st.c | 8 ++------ > > 1 file changed, 2 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/host/ehci-st.c b/drivers/usb/host/ehci-st.c > > index be4a2788fc582..12e803d2c98df 100644 > > --- a/drivers/usb/host/ehci-st.c > > +++ b/drivers/usb/host/ehci-st.c > > @@ -210,18 +210,14 @@ static int st_ehci_platform_probe(struct platform_device *dev) > > devm_reset_control_get_optional_shared(&dev->dev, "power"); > > if (IS_ERR(priv->pwr)) { > > err = PTR_ERR(priv->pwr); > > - if (err == -EPROBE_DEFER) > > - goto err_put_clks; > > - priv->pwr = NULL; > > + goto err_put_clks; > > } > > > > priv->rst = > > devm_reset_control_get_optional_shared(&dev->dev, "softreset"); > > if (IS_ERR(priv->rst)) { > > err = PTR_ERR(priv->rst); > > - if (err == -EPROBE_DEFER) > > - goto err_put_clks; > > - priv->rst = NULL; > > + goto err_put_clks; > > } > > > > if (pdata->power_on) { > > These changes do not agree with the patch description. If any sort of > error besides EPROBE_DEFER occurs then: > > the old code sets priv->pwr or priv->rst to NULL and continues; > > the new code returns with an error. > > The only way the patch could be equivalent to the existing code would > be if devm_reset_control_get_optional_shared() returns no errors other > than EPROBE_DEFER. But the patch description doesn't say this. > > Alan Stern
You are right, devm_reset_control_get_optional_shared can return:
-ENOMEM, returned by devres_alloc in __devm_reset_control_get,
-EILSEQ, returned by of_property_match_string in __of_reset_control_get if the "reset-names" DT property is broken,
-EINVAL, returned by of_parse_phandle_with_args in __of_reset_control_get, if there is a reset phandle specified in the device tree but it is pointing to an invalid reset controller node (missing "#reset-cells" property or number of phandle arguments does not match).
-EINVAL, returned by the reset controller driver's choice of of_xlate, also if the reset is specified but somehow invalid.
So yes, if there was a genuine error in the device tree, we would now return the error instead of silently ignoring it as before. I assume that these errors were never intended to be ignored, it just happened to be a hassle to separate them from the -ENOENT condition with the old API.
regards Philipp
| |