Re: [PATCH v2 2/7] clocksource: Add Tegra186 timers support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On Fri, Apr 03, 2020 at 07:24:55PM +0300, Dmitry Osipenko wrote:
> 01.04.2020 01:19, Thierry Reding пишет:
> ...
> > +static int tegra186_wdt_set_timeout(struct watchdog_device *wdd,
> > +				    unsigned int timeout)
> > +{
> > +	struct tegra186_wdt *wdt = to_tegra186_wdt(wdd);
> > +
> > +	tegra186_wdt_disable(wdt);
> > +	wdt->base.timeout = timeout;
> > +	tegra186_wdt_enable(wdt);
> 
> Why changing timeout enables the watchdog?

We have to disable and enable to make it accept the new settings and to
restart the counter. But you're right, we should only do that when the
WDT is actually active, so I've conditionalized the disable/enable calls
on watchdog_active().

> 
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_ops tegra186_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = tegra186_wdt_start,
> > +	.stop = tegra186_wdt_stop,
> > +	.ping = tegra186_wdt_ping,
> > +	.set_timeout = tegra186_wdt_set_timeout,
> > +};
> 
> ...
> > +static int __maybe_unused tegra186_timer_suspend(struct device *dev)
> > +{
> > +	struct tegra186_timer *tegra = dev_get_drvdata(dev);
> > +
> > +	if (tegra->wdt)
> > +		tegra186_wdt_disable(tegra->wdt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused tegra186_timer_resume(struct device *dev)
> > +{
> > +	struct tegra186_timer *tegra = dev_get_drvdata(dev);
> > +
> > +	if (tegra->wdt)
> > +		tegra186_wdt_enable(tegra->wdt);
> 
> What if watchdog is in a stopped state? Why it's enabled unconditionally?

Same as above.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]