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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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