| Message ID | 20240321114201.359381-1-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | reset: reset-rzg2l-usbphy-ctrl: Ensure reset line is asserted in error path | expand |
Hi Prabhakar, On Thu, Mar 21, 2024 at 12:43 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > In the event of failure during devm_reset_controller_register(), ensure > that the reset line is asserted back. > > Fixes: bee08559701fb ("reset: renesas: Add RZ/G2L usbphy control driver") > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c > +++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c > @@ -132,8 +132,10 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev) > priv->rcdev.dev = dev; > > error = devm_reset_controller_register(dev, &priv->rcdev); > - if (error) > + if (error) { > + reset_control_assert(priv->rstc); > return error; > + } > > spin_lock_init(&priv->lock); > dev_set_drvdata(dev, priv); Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Alternatively, you could register a cleanup handler with devm_add_action_or_reset(), like many drivers already do. Or better, turn devm_aspeed_peci_reset_control_deassert() into a generic helper, and convert all drivers[1] to use that. That would just leave us with undoing pm_runtime_enable(), which can be automated using devm_pm_runtime_enable() instead. [1] git grep -w devm.*reset_control_assert Gr{oetje,eeting}s, Geert
Hi Geert, Thank you for the review. On Wed, Apr 17, 2024 at 3:38 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Thu, Mar 21, 2024 at 12:43 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > In the event of failure during devm_reset_controller_register(), ensure > > that the reset line is asserted back. > > > > Fixes: bee08559701fb ("reset: renesas: Add RZ/G2L usbphy control driver") > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c > > +++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c > > @@ -132,8 +132,10 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev) > > priv->rcdev.dev = dev; > > > > error = devm_reset_controller_register(dev, &priv->rcdev); > > - if (error) > > + if (error) { > > + reset_control_assert(priv->rstc); > > return error; > > + } > > > > spin_lock_init(&priv->lock); > > dev_set_drvdata(dev, priv); > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Alternatively, you could register a cleanup handler with > devm_add_action_or_reset(), like many drivers already do. > Or better, turn devm_aspeed_peci_reset_control_deassert() into a > generic helper, and convert all drivers[1] to use that. > Good point, I'll send an RFC patch for this. > That would just leave us with undoing pm_runtime_enable(), which can > be automated using devm_pm_runtime_enable() instead. > Agreed. > [1] git grep -w devm.*reset_control_assert > Cheers, Prabhakar
diff --git a/drivers/reset/reset-rzg2l-usbphy-ctrl.c b/drivers/reset/reset-rzg2l-usbphy-ctrl.c index a8dde4606360..43eb01105596 100644 --- a/drivers/reset/reset-rzg2l-usbphy-ctrl.c +++ b/drivers/reset/reset-rzg2l-usbphy-ctrl.c @@ -132,8 +132,10 @@ static int rzg2l_usbphy_ctrl_probe(struct platform_device *pdev) priv->rcdev.dev = dev; error = devm_reset_controller_register(dev, &priv->rcdev); - if (error) + if (error) { + reset_control_assert(priv->rstc); return error; + } spin_lock_init(&priv->lock); dev_set_drvdata(dev, priv);