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.
This allows to return errors from devm_reset_control_get_optional_shared
unconditionally.
Signed-off-by: Philipp Zabel <[email protected]>
---
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) {
--
2.11.0
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?
> This allows to return errors from devm_reset_control_get_optional_shared
> unconditionally.
>
> Signed-off-by: Philipp Zabel <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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