| Message ID | 20170130114116.22089-10-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Philipp, sorry for the late reply. unfortunately my GXBB board (which is supported by the driver below) is dead. I CC'ed Jerome Brunet - maybe he can give it a go on one of his (GXBB) boards. On Mon, Jan 30, 2017 at 12:41 PM, Philipp Zabel <p.zabel@pengutronix.de> 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. > > This allows to return errors from devm_reset_control_get_optional_shared > and to call reset_control_reset unconditionally. > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> (based on reading the code along with the highly appreciated changes from bb475230b8e5) > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Cc: Kevin Hilman <khilman@baylibre.com> > Cc: Carlo Caione <carlo@caione.org> > Cc: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/phy/phy-meson8b-usb2.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/drivers/phy/phy-meson8b-usb2.c b/drivers/phy/phy-meson8b-usb2.c > index 33c9f4ba157d1..87168f1fe3af6 100644 > --- a/drivers/phy/phy-meson8b-usb2.c > +++ b/drivers/phy/phy-meson8b-usb2.c > @@ -141,12 +141,10 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) > struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); > int ret; > > - if (!IS_ERR_OR_NULL(priv->reset)) { > - ret = reset_control_reset(priv->reset); > - if (ret) { > - dev_err(&phy->dev, "Failed to trigger USB reset\n"); > - return ret; > - } > + ret = reset_control_reset(priv->reset); > + if (ret) { > + dev_err(&phy->dev, "Failed to trigger USB reset\n"); > + return ret; > } > > ret = clk_prepare_enable(priv->clk_usb_general); > @@ -241,7 +239,7 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev) > return PTR_ERR(priv->clk_usb); > > priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); > - if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > + if (PTR_ERR(priv->reset)) > return PTR_ERR(priv->reset); > > priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1); > -- > 2.11.0 >
On Wed, 2017-02-15 at 00:36 +0100, Martin Blumenstingl wrote: > Hi Philipp, > > sorry for the late reply. > unfortunately my GXBB board (which is supported by the driver below) > is dead. > I CC'ed Jerome Brunet - maybe he can give it a go on one of his > (GXBB) boards. > > On Mon, Jan 30, 2017 at 12:41 PM, Philipp Zabel <p.zabel@pengutronix. > de> 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. > > > > This allows to return errors from > > devm_reset_control_get_optional_shared > > and to call reset_control_reset unconditionally. > > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > Acked-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > (based on reading the code along with the highly appreciated changes > from bb475230b8e5) > > > > > Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > Cc: Kevin Hilman <khilman@baylibre.com> > > Cc: Carlo Caione <carlo@caione.org> > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > --- > > drivers/phy/phy-meson8b-usb2.c | 12 +++++------- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/phy/phy-meson8b-usb2.c b/drivers/phy/phy- > > meson8b-usb2.c > > index 33c9f4ba157d1..87168f1fe3af6 100644 > > --- a/drivers/phy/phy-meson8b-usb2.c > > +++ b/drivers/phy/phy-meson8b-usb2.c > > @@ -141,12 +141,10 @@ static int phy_meson8b_usb2_power_on(struct > > phy *phy) > > struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); > > int ret; > > > > - if (!IS_ERR_OR_NULL(priv->reset)) { > > - ret = reset_control_reset(priv->reset); > > - if (ret) { > > - dev_err(&phy->dev, "Failed to trigger USB > > reset\n"); > > - return ret; > > - } > > + ret = reset_control_reset(priv->reset); > > + if (ret) { > > + dev_err(&phy->dev, "Failed to trigger USB > > reset\n"); > > + return ret; > > } > > > > ret = clk_prepare_enable(priv->clk_usb_general); > > @@ -241,7 +239,7 @@ static int phy_meson8b_usb2_probe(struct > > platform_device *pdev) > > return PTR_ERR(priv->clk_usb); > > > > priv->reset = devm_reset_control_get_optional_shared(&pdev- > > >dev, NULL); > > - if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > > + if (PTR_ERR(priv->reset)) This is wrong and will always exit on error. It should be "IS_ERR". Clearly the bug was there before your patch, but since you are changing the faulty line, would you mind using IS_ERR instead ? With this changed: Tested-by: Jerome Brunet <jbrunet@baylibre.com> > > return PTR_ERR(priv->reset); > > > > priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev- > > >dev.of_node, -1); > > -- > > 2.11.0 > >
Hi Jerome, On Wed, 2017-02-15 at 17:43 +0100, Jerome Brunet wrote: [...] > > > @@ -241,7 +239,7 @@ static int phy_meson8b_usb2_probe(struct > > > platform_device *pdev) > > > return PTR_ERR(priv->clk_usb); > > > > > > priv->reset = devm_reset_control_get_optional_shared(&pdev- > > > >dev, NULL); > > > - if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > > > + if (PTR_ERR(priv->reset)) > > This is wrong and will always exit on error. It should be "IS_ERR". > Clearly the bug was there before your patch, but since you are changing > the faulty line, would you mind using IS_ERR instead ? > > With this changed: > Tested-by: Jerome Brunet <jbrunet@baylibre.com> Thanks for catching this, I should have changed this to IS_ERR when removing the error value comparison. regards Philipp
diff --git a/drivers/phy/phy-meson8b-usb2.c b/drivers/phy/phy-meson8b-usb2.c index 33c9f4ba157d1..87168f1fe3af6 100644 --- a/drivers/phy/phy-meson8b-usb2.c +++ b/drivers/phy/phy-meson8b-usb2.c @@ -141,12 +141,10 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy); int ret; - if (!IS_ERR_OR_NULL(priv->reset)) { - ret = reset_control_reset(priv->reset); - if (ret) { - dev_err(&phy->dev, "Failed to trigger USB reset\n"); - return ret; - } + ret = reset_control_reset(priv->reset); + if (ret) { + dev_err(&phy->dev, "Failed to trigger USB reset\n"); + return ret; } ret = clk_prepare_enable(priv->clk_usb_general); @@ -241,7 +239,7 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev) return PTR_ERR(priv->clk_usb); priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL); - if (PTR_ERR(priv->reset) == -EPROBE_DEFER) + if (PTR_ERR(priv->reset)) return PTR_ERR(priv->reset); priv->dr_mode = of_usb_get_dr_mode_by_phy(pdev->dev.of_node, -1);
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 and to call reset_control_reset unconditionally. Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> Cc: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Cc: Kevin Hilman <khilman@baylibre.com> Cc: Carlo Caione <carlo@caione.org> Cc: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/phy/phy-meson8b-usb2.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)