Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Wed, 21 Apr 2021 09:42:28 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Francesco Zanella <francesco.zanella@vimar.com>
Cc:     linux-watchdog@vger.kernel.org, wim@linux-watchdog.org,
        devicetree@vger.kernel.org, robh+dt@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] watchdog: gpio_wdt: add "start-at-boot" feature
Message-ID: <20210421164228.GB110463@roeck-us.net>
References: <20210421162621.24910-1-francesco.zanella@vimar.com>
 <20210421162621.24910-3-francesco.zanella@vimar.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210421162621.24910-3-francesco.zanella@vimar.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Apr 21, 2021 at 06:26:21PM +0200, Francesco Zanella wrote:
> If "start-at-boot" property is present in the device tree, start pinging
> hw watchdog at probe, in order to take advantage of kernel configs:
> - WATCHDOG_HANDLE_BOOT_ENABLED: Avoid possible reboot if hw watchdog was
>   been enabled before the kernel (by uboot for example) and userspace
>   doesn't take control of /dev/watchdog in time;
> - WATCHDOG_OPEN_TIMEOUT: Reboot if userspace doesn't take control of
>   /dev/watchdog within the timeout.
> 
> Signed-off-by: Francesco Zanella <francesco.zanella@vimar.com>
> ---
>  drivers/watchdog/gpio_wdt.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/gpio_wdt.c b/drivers/watchdog/gpio_wdt.c
> index 0923201ce874..1e6f0322ab7a 100644
> --- a/drivers/watchdog/gpio_wdt.c
> +++ b/drivers/watchdog/gpio_wdt.c
> @@ -31,6 +31,7 @@ struct gpio_wdt_priv {
>  	struct gpio_desc	*gpiod;
>  	bool			state;
>  	bool			always_running;
> +	bool			start_at_boot;
>  	unsigned int		hw_algo;
>  	struct watchdog_device	wdd;
>  };
> @@ -147,6 +148,9 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>  	priv->always_running = of_property_read_bool(np,
>  						     "always-running");
>  
> +	priv->start_at_boot = of_property_read_bool(np,
> +						    "start-at-boot");
> +
>  	watchdog_set_drvdata(&priv->wdd, priv);
>  
>  	priv->wdd.info		= &gpio_wdt_ident;
> @@ -161,7 +165,7 @@ static int gpio_wdt_probe(struct platform_device *pdev)
>  
>  	watchdog_stop_on_reboot(&priv->wdd);
>  
> -	if (priv->always_running)
> +	if (priv->always_running || priv->start_at_boot)
>  		gpio_wdt_start(&priv->wdd);

So the only real difference to always_running is that always_running
doesn't stop the watchdog on close but keeps it running.

Does that really warrant another property ? Why not just use
always-running ?

The special use case of being able to stop the watchdog doesn't seem
to be worth the trouble. Please explain your use case.

Guenter