Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available

From: Guenter Roeck <linux@roeck-us.net>
To: "Bruno Thomsen" <bruno.thomsen@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>,
	Qiang Zhao <qiang.zhao@nxp.com>,
	linux-rtc@vger.kernel.org,
	Alessandro Zummo <a.zummo@towertech.it>,
	linux-watchdog@vger.kernel.org,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	open list <linux-kernel@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Sascha Hauer <kernel@pengutronix.de>,
	Wim Van Sebroeck <wim@linux-watchdog.org>
Subject: Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
Date: Sun, 27 Sep 2020 08:54:47 -0700	[thread overview]
Message-ID: <8f88f2a2-4a6d-021f-4404-f05518b0477d@roeck-us.net> (raw)
In-Reply-To: <CAH+2xPAVvMpTgT3W=0AsKy=9jkS8qd6eB65Qebw51YKRQshaGQ@mail.gmail.com>

On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de>:
>>
>> Most boards using the pcf2127 chip (in my bubble) don't make use of the
>> watchdog functionality and the respective output is not connected. The
>> effect on such a board is that there is a watchdog device provided that
>> doesn't work.
>>
>> So only register the watchdog if the device tree has a "has-watchdog"
>> property.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/rtc/rtc-pcf2127.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
>> index 5b1f1949b5e5..8bd89d641578 100644
>> --- a/drivers/rtc/rtc-pcf2127.c
>> +++ b/drivers/rtc/rtc-pcf2127.c
>> @@ -340,7 +340,8 @@ static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
>>         u32 wdd_timeout;
>>         int ret;
>>
>> -       if (!IS_ENABLED(CONFIG_WATCHDOG))
>> +       if (!IS_ENABLED(CONFIG_WATCHDOG) ||
>> +           !device_property_read_bool(dev, "has-watchdog"))
>>                 return 0;
> 
> I don't think the compiler can remove the function if
> CONFIG_WATCHDOG is disabled due to the device tree
> value check. Maybe it can if split into 2 conditions.
> 

If the first part of the expression is always false, the second
part should not even be evaluated. Either case, the code now
hard depends on the compiler optimizing the code away.
It calls devm_watchdog_register_device() which doesn't exist
if CONFIG_WATCHDOG is not enabled. I didn't know that this is safe,
and I would personally not want to rely on it, but we live and
learn.

Guenter

> if (!IS_ENABLED(CONFIG_WATCHDOG))
>         return 0;
> if (!device_property_read_bool(dev, "has-watchdog"))
>         return 0;
> 
> /Bruno
> 
>>
>>         pcf2127->wdd.parent = dev;
>> --
>> 2.28.0
>>