diff mbox series

[v4,4/6] watchdog: rzg2l_wdt: Add error check for reset_control_deassert

Message ID 20220223160100.23543-5-biju.das.jz@bp.renesas.com
State New
Headers show
Series RZG2L_WDT Fixes and Improvements | expand

Commit Message

Biju Das Feb. 23, 2022, 4 p.m. UTC
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(-)

Comments

Biju Das Feb. 24, 2022, 11:03 a.m. UTC | #1
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 mbox series

Patch

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;