Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190117151942.6092-1-saiprakash.ranjan@codeaurora.org> <154775022743.169631.1716489383315213219@swboyd.mtv.corp.google.com>
In-Reply-To: <154775022743.169631.1716489383315213219@swboyd.mtv.corp.google.com>
From:   Guenter Roeck <groeck@google.com>
Date:   Thu, 17 Jan 2019 11:09:31 -0800
Message-ID: <CABXOdTfE+_q8DrRfPE1V+3pU7x5rLoOy7k5LYMrwd+xojsBWvw@mail.gmail.com>
Subject: Re: [PATCHv2] watchdog: qcom: Add suspend/resume support
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     Guenter Roeck <groeck@chromium.org>,
        Guenter Roeck <linux@roeck-us.net>,
        Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
        Wim Van Sebroeck <wim@linux-watchdog.org>,
        linux-watchdog@vger.kernel.org,
        Rajendra Nayak <rnayak@codeaurora.org>,
        Vivek Gautam <vivek.gautam@codeaurora.org>,
        Sibi Sankar <sibis@codeaurora.org>,
        Doug Anderson <dianders@chromium.org>,
        linux-arm-kernel@lists.infradead.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jan 17, 2019 at 10:37 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Sai Prakash Ranjan (2019-01-17 07:19:42)
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index 780971318810..5dfd604477a4 100644
> > --- a/drivers/watchdog/qcom-wdt.c
> > +++ b/drivers/watchdog/qcom-wdt.c
> > @@ -245,6 +245,28 @@ static int qcom_wdt_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +static int __maybe_unused qcom_wdt_suspend(struct device *dev)
> > +{
> > +       struct qcom_wdt *wdt = dev_get_drvdata(dev);
> > +
> > +       if (watchdog_active(&wdt->wdd))
> > +               qcom_wdt_stop(&wdt->wdd);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused qcom_wdt_resume(struct device *dev)
> > +{
> > +       struct qcom_wdt *wdt = dev_get_drvdata(dev);
> > +
> > +       if (watchdog_active(&wdt->wdd))
> > +               qcom_wdt_start(&wdt->wdd);
> > +
> > +       return 0;
> > +}
>
> This looks fairly generic. For example, the Mediatek driver also stops
> and starts (but also pings after starting). Grepping for 'active' in
> drivers/watchdog/ finds more examples. Could there be some functions in
> watchdog core that do the common things like watchdog_stop() and
> watchdog_start() and watchdog_start_and_ping()? Or maybe a bit can be
> set during registration so that the 'struct class watchdog_class' can
> get PM ops to stop and start on suspend/resume of the watchdog character
> device?
>
> Nothing is wrong with the patch, I'm just bemoaning the amount of code
> duplication here.
>

Patch(es) to add the functionality to the watchdog core are welcome;
it should be possible to move the functionality into the core (maybe
to be enabled with a new watchdog API call). Doing it using the class
device sounds like an excellent idea. This should be straightforward
to implement, though the question of "should we ping on resume or not"
might be an endless source for bike shedding fun.

Guenter