Hi Guenter Roeck,
> Subject: Re: [PATCH 2/4] watchdog: rzg2l_wdt: Add error check for
> reset_control_{deassert/reset}
>
> On 12/11/21 1:26 PM, Biju Das wrote:
> > If reset_control_deassert()/reset_control_reset() fails, then we won't
> > be able to access the device registers. Therefore check the return
> > code of reset_control_deassert()/reset_control_reset() and return the
> > error code to caller in case of error.
> >
> > While at it remove the unnecessary pm_runtime_resume_and_get() from
> > probe(), as it turns on the clocks.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
>
> See note below, though.
>
> > ---
> > drivers/watchdog/rzg2l_wdt.c | 37 +++++++++++++++++++-----------------
> > 1 file changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/watchdog/rzg2l_wdt.c
> > b/drivers/watchdog/rzg2l_wdt.c index 96f2a018ab62..58fe4efd9a89 100644
> > --- a/drivers/watchdog/rzg2l_wdt.c
> > +++ b/drivers/watchdog/rzg2l_wdt.c
> > @@ -85,8 +85,14 @@ static void rzg2l_wdt_init_timeout(struct
> watchdog_device *wdev)
> > static int rzg2l_wdt_start(struct watchdog_device *wdev)
> > {
> > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > + int ret;
> > +
> > + ret = reset_control_deassert(priv->rstc);
> > + if (ret) {
> > + dev_err(wdev->parent, "failed to deassert");
> > + return ret;
> > + }
> >
> This patch introduces an error return into rzg2l_wdt_start().
> If it is indeed necessary to call this function when setting the timeout,
> the error return needs to be checked there.
Good point. After rechecking, this call is not at all needed.
For WDT stop/ settime we need to reset the module. So we should use reset_control_reset() instead.
>From platform point, it just asserts and then de-assert the module[1].
Will add checks for this call in stop/settime and return error code to caller.
I will send V2 with changes.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/renesas/rzg2l-cpg.c?h=v5.16-rc4#n685
Regards,
Biju
>
> Guenter
>
> > - reset_control_deassert(priv->rstc);
> > pm_runtime_get_sync(wdev->parent);
> >
> > /* Initialize time out */
> > @@ -115,9 +121,15 @@ static int rzg2l_wdt_restart(struct watchdog_device
> *wdev,
> > unsigned long action, void *data)
> > {
> > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > + int ret;
> >
> > /* Reset the module before we modify any register */
> > - reset_control_reset(priv->rstc);
> > + ret = reset_control_reset(priv->rstc);
> > + if (ret) {
> > + dev_err(wdev->parent, "failed to reset");
> > + return ret;
> > + }
> > +
> > pm_runtime_get_sync(wdev->parent);
> >
> > /* smallest counter value to reboot soon */ @@ -151,12 +163,11 @@
> > static const struct watchdog_ops rzg2l_wdt_ops = {
> > .restart = rzg2l_wdt_restart,
> > };
> >
> > -static void rzg2l_wdt_reset_assert_pm_disable_put(void *data)
> > +static void rzg2l_wdt_reset_assert_pm_disable(void *data)
> > {
> > struct watchdog_device *wdev = data;
> > struct rzg2l_wdt_priv *priv = watchdog_get_drvdata(wdev);
> >
> > - pm_runtime_put(wdev->parent);
> > pm_runtime_disable(wdev->parent);
> > reset_control_assert(priv->rstc);
> > }
> > @@ -204,13 +215,11 @@ 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);
> > - ret = pm_runtime_resume_and_get(&pdev->dev);
> > - if (ret < 0) {
> > - dev_err(dev, "pm_runtime_resume_and_get failed ret=%pe",
> ERR_PTR(ret));
> > - goto out_pm_get;
> > - }
> >
> > priv->wdev.info = &rzg2l_wdt_ident;
> > priv->wdev.ops = &rzg2l_wdt_ops;
> > @@ -222,7 +231,7 @@ static int rzg2l_wdt_probe(struct platform_device
> > *pdev)
> >
> > watchdog_set_drvdata(&priv->wdev, priv);
> > ret = devm_add_action_or_reset(&pdev->dev,
> > - rzg2l_wdt_reset_assert_pm_disable_put,
> > + rzg2l_wdt_reset_assert_pm_disable,
> > &priv->wdev);
> > if (ret < 0)
> > return ret;
> > @@ -235,12 +244,6 @@ static int rzg2l_wdt_probe(struct platform_device
> *pdev)
> > dev_warn(dev, "Specified timeout invalid, using default");
> >
> > return devm_watchdog_register_device(&pdev->dev, &priv->wdev);
> > -
> > -out_pm_get:
> > - pm_runtime_disable(dev);
> > - reset_control_assert(priv->rstc);
> > -
> > - return ret;
> > }
> >
> > static const struct of_device_id rzg2l_wdt_ids[] = {
> >
