| Message ID | 79df054046224bbb0716a8c5c2082650290eec86.1621616013.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1d0d3d818eafe1963ec1eaf302175cd14938188e |
| Headers | show |
| Series | usb: dwc3: meson-g12a: Disable the regulator in the error handling path of the probe | expand |
Hi Christophe, On Fri, May 21, 2021 at 6:55 PM Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > If an error occurs after a successful 'regulator_enable()' call, > 'regulator_disable()' must be called. > > Fix the error handling path of the probe accordingly. > > The remove function doesn't need to be fixed, because the > 'regulator_disable()' call is already hidden in 'dwc3_meson_g12a_suspend()' > which is called via 'pm_runtime_set_suspended()' in the remove function. > > Fixes: c99993376f72 ("usb: dwc3: Add Amlogic G12A DWC3 glue") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> for me this makes sense so: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > Please review carefully. > > I'm not that sure about: > The remove function doesn't need to be fixed, because the > 'regulator_disable()' call is already hidden in 'dwc3_meson_g12a_suspend()' > which is called via 'pm_runtime_set_suspended()' in the remove function. > > This is more a guess than anything else! I am not 100% sure about the suspend code-path, so I am hoping that someone else (for example Neil) can also review your patch Best regards, Martin
On 23/05/2021 18:03, Martin Blumenstingl wrote: > Hi Christophe, > > On Fri, May 21, 2021 at 6:55 PM Christophe JAILLET > <christophe.jaillet@wanadoo.fr> wrote: >> >> If an error occurs after a successful 'regulator_enable()' call, >> 'regulator_disable()' must be called. >> >> Fix the error handling path of the probe accordingly. >> >> The remove function doesn't need to be fixed, because the >> 'regulator_disable()' call is already hidden in 'dwc3_meson_g12a_suspend()' >> which is called via 'pm_runtime_set_suspended()' in the remove function. >> >> Fixes: c99993376f72 ("usb: dwc3: Add Amlogic G12A DWC3 glue") >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > for me this makes sense so: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Acked-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- >> Please review carefully. >> >> I'm not that sure about: >> The remove function doesn't need to be fixed, because the >> 'regulator_disable()' call is already hidden in 'dwc3_meson_g12a_suspend()' >> which is called via 'pm_runtime_set_suspended()' in the remove function. >> >> This is more a guess than anything else! > I am not 100% sure about the suspend code-path, so I am hoping that > someone else (for example Neil) can also review your patch Not 100% sure aswell, but the probe function is wrong anyway and must be fixed by this patch, so LGTM. Neil > > > Best regards, > Martin >
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c index bdf1f98dfad8..804957525130 100644 --- a/drivers/usb/dwc3/dwc3-meson-g12a.c +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c @@ -772,13 +772,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) ret = priv->drvdata->usb_init(priv); if (ret) - goto err_disable_clks; + goto err_disable_regulator; /* Init PHYs */ for (i = 0 ; i < PHY_COUNT ; ++i) { ret = phy_init(priv->phys[i]); if (ret) - goto err_disable_clks; + goto err_disable_regulator; } /* Set PHY Power */ @@ -816,6 +816,10 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) for (i = 0 ; i < PHY_COUNT ; ++i) phy_exit(priv->phys[i]); +err_disable_regulator: + if (priv->vbus) + regulator_disable(priv->vbus); + err_disable_clks: clk_bulk_disable_unprepare(priv->drvdata->num_clks, priv->drvdata->clks);
If an error occurs after a successful 'regulator_enable()' call, 'regulator_disable()' must be called. Fix the error handling path of the probe accordingly. The remove function doesn't need to be fixed, because the 'regulator_disable()' call is already hidden in 'dwc3_meson_g12a_suspend()' which is called via 'pm_runtime_set_suspended()' in the remove function. Fixes: c99993376f72 ("usb: dwc3: Add Amlogic G12A DWC3 glue") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- Please review carefully. I'm not that sure about: The remove function doesn't need to be fixed, because the 'regulator_disable()' call is already hidden in 'dwc3_meson_g12a_suspend()' which is called via 'pm_runtime_set_suspended()' in the remove function. This is more a guess than anything else! --- drivers/usb/dwc3/dwc3-meson-g12a.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)