Message ID | 20220223160100.23543-5-biju.das.jz@bp.renesas.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | RZG2L_WDT Fixes and Improvements | expand |
Hi Biju, On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > If reset_control_deassert() fails, then we won't be able to > access the device registers. Therefore check the return code of > reset_control_deassert() and bailout in case of error. > > While at it change reset_control_assert->reset_control_reset in > rzg2l_wdt_stop() and remove unnecessary reset_control_deassert() > from rzg2l_wdt_start(). > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- a/drivers/watchdog/rzg2l_wdt.c > +++ b/drivers/watchdog/rzg2l_wdt.c > @@ -88,7 +88,6 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev) > { > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > - reset_control_deassert(priv->rstc); So before, we had a reset control imbalance? - After probe, reset was deasserted. - After start, reset was deasserted twice. Oops. You probably want to mention this in the commit description, too. > pm_runtime_get_sync(wdev->parent); > > /* Initialize time out */ > @@ -108,7 +107,7 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > pm_runtime_put(wdev->parent); > - reset_control_assert(priv->rstc); > + reset_control_reset(priv->rstc); As the reset is now deasserted after probe(), stop(), and start(), I think the reset_control_reset() in .restart() can now be removed? > > return 0; > } > @@ -204,7 +203,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) > return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), > "failed to get cpg reset"); > > - reset_control_deassert(priv->rstc); > + ret = reset_control_deassert(priv->rstc); > + if (ret) > + return dev_err_probe(dev, ret, "failed to deassert"); > + > pm_runtime_enable(&pdev->dev); > > priv->wdev.info = &rzg2l_wdt_ident; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH v4 4/6] watchdog: rzg2l_wdt: Add error check for > reset_control_deassert > > Hi Biju, > > On Wed, Feb 23, 2022 at 5:01 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > If reset_control_deassert() fails, then we won't be able to access the > > device registers. Therefore check the return code of > > reset_control_deassert() and bailout in case of error. > > > > While at it change reset_control_assert->reset_control_reset in > > rzg2l_wdt_stop() and remove unnecessary reset_control_deassert() from > > rzg2l_wdt_start(). > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/watchdog/rzg2l_wdt.c > > +++ b/drivers/watchdog/rzg2l_wdt.c > > @@ -88,7 +88,6 @@ static int rzg2l_wdt_start(struct watchdog_device > > *wdev) { > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > - reset_control_deassert(priv->rstc); > > So before, we had a reset control imbalance? > - After probe, reset was deasserted. > - After start, reset was deasserted twice. Oops. > You probably want to mention this in the commit description, too. OK, will add this in the commit description. > > > pm_runtime_get_sync(wdev->parent); > > > > /* Initialize time out */ > > @@ -108,7 +107,7 @@ static int rzg2l_wdt_stop(struct watchdog_device > *wdev) > > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); > > > > pm_runtime_put(wdev->parent); > > - reset_control_assert(priv->rstc); > > + reset_control_reset(priv->rstc); > > As the reset is now deasserted after probe(), stop(), and start(), I think > the reset_control_reset() in .restart() can now be removed? For Overflow method still we need to reset the module, so that we can update WDTSET register to change the timeout value from 60sec to 43.69 msec, so that reboot can occur after 43.69 msec which corresponds to counter value of 1. Yes, it is removed in patch#5, which use Force reset by parity error instead of WDT overflow method. Regards, Biju > > > > > return 0; > > } > > @@ -204,7 +203,10 @@ static int rzg2l_wdt_probe(struct platform_device > *pdev) > > return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), > > "failed to get cpg reset"); > > > > - reset_control_deassert(priv->rstc); > > + ret = reset_control_deassert(priv->rstc); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to deassert"); > > + > > pm_runtime_enable(&pdev->dev); > > > > priv->wdev.info = &rzg2l_wdt_ident; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
diff --git a/drivers/watchdog/rzg2l_wdt.c b/drivers/watchdog/rzg2l_wdt.c index 48dfe6e5e64f..73b667ed3e99 100644 --- a/drivers/watchdog/rzg2l_wdt.c +++ b/drivers/watchdog/rzg2l_wdt.c @@ -88,7 +88,6 @@ static int rzg2l_wdt_start(struct watchdog_device *wdev) { struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); - reset_control_deassert(priv->rstc); pm_runtime_get_sync(wdev->parent); /* Initialize time out */ @@ -108,7 +107,7 @@ static int rzg2l_wdt_stop(struct watchdog_device *wdev) struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev); pm_runtime_put(wdev->parent); - reset_control_assert(priv->rstc); + reset_control_reset(priv->rstc); return 0; } @@ -204,7 +203,10 @@ static int rzg2l_wdt_probe(struct platform_device *pdev) return dev_err_probe(&pdev->dev, PTR_ERR(priv->rstc), "failed to get cpg reset"); - reset_control_deassert(priv->rstc); + ret = reset_control_deassert(priv->rstc); + if (ret) + return dev_err_probe(dev, ret, "failed to deassert"); + pm_runtime_enable(&pdev->dev); priv->wdev.info = &rzg2l_wdt_ident;
If reset_control_deassert() fails, then we won't be able to access the device registers. Therefore check the return code of reset_control_deassert() and bailout in case of error. While at it change reset_control_assert->reset_control_reset in rzg2l_wdt_stop() and remove unnecessary reset_control_deassert() from rzg2l_wdt_start(). Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v3->v4: * Made reset usage counter balanced * Updated commit description v2->v3: * Patch reordering from Patch 2 -> Patch 3 * Updated commit description v1->v2: * Updated commit description and removed Rb tag from Guenter, since there is code change * Replaced reset_control_assert with reset_control_reset in stop and removed reset_control_deassert() from start. --- drivers/watchdog/rzg2l_wdt.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)