* [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function
2020-09-24 10:52 ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-09-24 10:52 ` Uwe Kleine-König
2020-09-27 7:50 ` Bruno Thomsen
2020-09-24 10:52 ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: Uwe Kleine-König @ 2020-09-24 10:52 UTC (permalink / raw)
To: Alexandre Belloni, Qiang Zhao, Bruno Thomsen
Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck
The obvious advantages are:
- The linker can drop the watchdog functions if CONFIG_WATCHDOG is off.
- All watchdog stuff grouped together with only a single function call
left in generic code.
- Watchdog register is only read when it is actually used.
- Less #ifdefery
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/rtc/rtc-pcf2127.c | 56 ++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 25 deletions(-)
diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ed6316992cbb..5b1f1949b5e5 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -335,6 +335,36 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
.set_timeout = pcf2127_wdt_set_timeout,
};
+static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
+{
+ u32 wdd_timeout;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_WATCHDOG))
+ return 0;
+
+ pcf2127->wdd.parent = dev;
+ pcf2127->wdd.info = &pcf2127_wdt_info;
+ pcf2127->wdd.ops = &pcf2127_watchdog_ops;
+ pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
+ pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
+ pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
+ pcf2127->wdd.min_hw_heartbeat_ms = 500;
+ pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
+
+ watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
+
+ /* Test if watchdog timer is started by bootloader */
+ ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
+ if (ret)
+ return ret;
+
+ if (wdd_timeout)
+ set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
+
+ return devm_watchdog_register_device(dev, &pcf2127->wdd);
+}
+
/* Alarm */
static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
@@ -536,7 +566,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
int alarm_irq, const char *name, bool has_nvmem)
{
struct pcf2127 *pcf2127;
- u32 wdd_timeout;
int ret = 0;
dev_dbg(dev, "%s\n", __func__);
@@ -575,17 +604,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
}
- pcf2127->wdd.parent = dev;
- pcf2127->wdd.info = &pcf2127_wdt_info;
- pcf2127->wdd.ops = &pcf2127_watchdog_ops;
- pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
- pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
- pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
- pcf2127->wdd.min_hw_heartbeat_ms = 500;
- pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
-
- watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
-
if (has_nvmem) {
struct nvmem_config nvmem_cfg = {
.priv = pcf2127,
@@ -615,19 +633,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
return ret;
}
- /* Test if watchdog timer is started by bootloader */
- ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
- if (ret)
- return ret;
-
- if (wdd_timeout)
- set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
-
-#ifdef CONFIG_WATCHDOG
- ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
- if (ret)
- return ret;
-#endif /* CONFIG_WATCHDOG */
+ pcf2127_watchdog_init(dev, pcf2127);
/*
* Disable battery low/switch-over timestamp and interrupts.
--
2.28.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function
2020-09-24 10:52 ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
@ 2020-09-27 7:50 ` Bruno Thomsen
0 siblings, 0 replies; 22+ messages in thread
From: Bruno Thomsen @ 2020-09-27 7:50 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Alexandre Belloni, Qiang Zhao, linux-rtc, Alessandro Zummo,
linux-watchdog,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
Guenter Roeck
Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>:
>
> The obvious advantages are:
>
> - The linker can drop the watchdog functions if CONFIG_WATCHDOG is off.
> - All watchdog stuff grouped together with only a single function call
> left in generic code.
> - Watchdog register is only read when it is actually used.
> - Less #ifdefery
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/rtc/rtc-pcf2127.c | 56 ++++++++++++++++++++++-----------------
> 1 file changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ed6316992cbb..5b1f1949b5e5 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
> @@ -335,6 +335,36 @@ static const struct watchdog_ops pcf2127_watchdog_ops = {
> .set_timeout = pcf2127_wdt_set_timeout,
> };
>
> +static int pcf2127_watchdog_init(struct device *dev, struct pcf2127 *pcf2127)
> +{
> + u32 wdd_timeout;
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_WATCHDOG))
> + return 0;
> +
> + pcf2127->wdd.parent = dev;
> + pcf2127->wdd.info = &pcf2127_wdt_info;
> + pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> + pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
> + pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
> + pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
> + pcf2127->wdd.min_hw_heartbeat_ms = 500;
> + pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> +
> + watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> +
> + /* Test if watchdog timer is started by bootloader */
> + ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> + if (ret)
> + return ret;
> +
> + if (wdd_timeout)
> + set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> +
> + return devm_watchdog_register_device(dev, &pcf2127->wdd);
> +}
> +
> /* Alarm */
> static int pcf2127_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> {
> @@ -536,7 +566,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> int alarm_irq, const char *name, bool has_nvmem)
> {
> struct pcf2127 *pcf2127;
> - u32 wdd_timeout;
> int ret = 0;
>
> dev_dbg(dev, "%s\n", __func__);
> @@ -575,17 +604,6 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> pcf2127->rtc->ops = &pcf2127_rtc_alrm_ops;
> }
>
> - pcf2127->wdd.parent = dev;
> - pcf2127->wdd.info = &pcf2127_wdt_info;
> - pcf2127->wdd.ops = &pcf2127_watchdog_ops;
> - pcf2127->wdd.min_timeout = PCF2127_WD_VAL_MIN;
> - pcf2127->wdd.max_timeout = PCF2127_WD_VAL_MAX;
> - pcf2127->wdd.timeout = PCF2127_WD_VAL_DEFAULT;
> - pcf2127->wdd.min_hw_heartbeat_ms = 500;
> - pcf2127->wdd.status = WATCHDOG_NOWAYOUT_INIT_STATUS;
> -
> - watchdog_set_drvdata(&pcf2127->wdd, pcf2127);
> -
> if (has_nvmem) {
> struct nvmem_config nvmem_cfg = {
> .priv = pcf2127,
> @@ -615,19 +633,7 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
> return ret;
> }
>
> - /* Test if watchdog timer is started by bootloader */
> - ret = regmap_read(pcf2127->regmap, PCF2127_REG_WD_VAL, &wdd_timeout);
> - if (ret)
> - return ret;
> -
> - if (wdd_timeout)
> - set_bit(WDOG_HW_RUNNING, &pcf2127->wdd.status);
> -
> -#ifdef CONFIG_WATCHDOG
> - ret = devm_watchdog_register_device(dev, &pcf2127->wdd);
> - if (ret)
> - return ret;
> -#endif /* CONFIG_WATCHDOG */
> + pcf2127_watchdog_init(dev, pcf2127);
The code refactoring seems like a good idea Uwe, but the new
pcf2127_watchdog_init() is not a void function. If the return
value is not handled, it will change driver behavior. Correct
handling should look like this:
ret = pcf2127_watchdog_init(dev, pcf2127);
if (ret)
return ret;
/Bruno
> /*
> * Disable battery low/switch-over timestamp and interrupts.
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
2020-09-24 10:52 ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
2020-09-24 10:52 ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
@ 2020-09-24 10:52 ` Uwe Kleine-König
2020-09-27 8:09 ` Bruno Thomsen
2020-10-26 7:23 ` [PATCH 0/2] " Qiang Zhao
2020-11-30 9:51 ` Rasmus Villemoes
3 siblings, 1 reply; 22+ messages in thread
From: Uwe Kleine-König @ 2020-09-24 10:52 UTC (permalink / raw)
To: Alexandre Belloni, Qiang Zhao, Bruno Thomsen
Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck
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;
pcf2127->wdd.parent = dev;
--
2.28.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
2020-09-24 10:52 ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-09-27 8:09 ` Bruno Thomsen
2020-09-27 15:54 ` Guenter Roeck
0 siblings, 1 reply; 22+ messages in thread
From: Bruno Thomsen @ 2020-09-27 8:09 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Alexandre Belloni, Qiang Zhao, linux-rtc, Alessandro Zummo,
linux-watchdog,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
Guenter Roeck
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 (!IS_ENABLED(CONFIG_WATCHDOG))
return 0;
if (!device_property_read_bool(dev, "has-watchdog"))
return 0;
/Bruno
>
> pcf2127->wdd.parent = dev;
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
2020-09-27 8:09 ` Bruno Thomsen
@ 2020-09-27 15:54 ` Guenter Roeck
2020-09-28 8:43 ` Uwe Kleine-König
0 siblings, 1 reply; 22+ messages in thread
From: Guenter Roeck @ 2020-09-27 15:54 UTC (permalink / raw)
To: Bruno Thomsen, Uwe Kleine-König
Cc: Alexandre Belloni, Qiang Zhao, linux-rtc, Alessandro Zummo,
linux-watchdog,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck
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
>>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
2020-09-27 15:54 ` Guenter Roeck
@ 2020-09-28 8:43 ` Uwe Kleine-König
2020-09-28 16:26 ` Guenter Roeck
0 siblings, 1 reply; 22+ messages in thread
From: Uwe Kleine-König @ 2020-09-28 8:43 UTC (permalink / raw)
To: Guenter Roeck
Cc: Bruno Thomsen, linux-rtc, Alessandro Zummo, Alexandre Belloni,
linux-watchdog,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
Qiang Zhao
[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]
On Sun, Sep 27, 2020 at 08:54:47AM -0700, Guenter Roeck wrote:
> 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.
This is wrong. For || the second expression isn't evaluated if the first
evaluates to true (and the whole expression becomes true). This is the
intended behaviour: If CONFIG_WATCHDOG is off, we don't need to check
for the dt property and just skip the watchdog part.
> 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.
AFAICT this is save and used in other places in the kernel, too. This
is one of the reasons why you cannot compile the kernel with -O0.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available
2020-09-28 8:43 ` Uwe Kleine-König
@ 2020-09-28 16:26 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2020-09-28 16:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Bruno Thomsen, linux-rtc, Alessandro Zummo, Alexandre Belloni,
linux-watchdog,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list, Rob Herring, Sascha Hauer, Wim Van Sebroeck,
Qiang Zhao
On Mon, Sep 28, 2020 at 10:43:43AM +0200, Uwe Kleine-König wrote:
> On Sun, Sep 27, 2020 at 08:54:47AM -0700, Guenter Roeck wrote:
> > 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.
>
> This is wrong. For || the second expression isn't evaluated if the first
> evaluates to true (and the whole expression becomes true). This is the
> intended behaviour: If CONFIG_WATCHDOG is off, we don't need to check
> for the dt property and just skip the watchdog part.
>
Sorry, I meant to say "If the first part of the expression is always true".
Guenter
> > 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.
>
> AFAICT this is save and used in other places in the kernel, too. This
> is one of the reasons why you cannot compile the kernel with -O0.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 22+ messages in thread
* RE: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
2020-09-24 10:52 ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
2020-09-24 10:52 ` [PATCH 1/2] rtc: pcf2127: move watchdog initialisation to a separate function Uwe Kleine-König
2020-09-24 10:52 ` [PATCH 2/2] [RFC] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
@ 2020-10-26 7:23 ` Qiang Zhao
2020-10-26 20:48 ` Alexandre Belloni
2020-11-30 9:51 ` Rasmus Villemoes
3 siblings, 1 reply; 22+ messages in thread
From: Qiang Zhao @ 2020-10-26 7:23 UTC (permalink / raw)
To: Uwe Kleine-König, Alexandre Belloni, Bruno Thomsen
Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck
Any update for this patchset?
Best Regards
Qiang Zhao
> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Sent: 2020年9月24日 18:53
> To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Qiang Zhao
> <qiang.zhao@nxp.com>; Bruno Thomsen <bruno.thomsen@gmail.com>
> Cc: linux-rtc@vger.kernel.org; a.zummo@towertech.it;
> linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; robh+dt@kernel.org; kernel@pengutronix.de;
> Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> <linux@roeck-us.net>
> Subject: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
>
> Hello,
>
> now that there are two people stumbling over the pcf2127 driver providing a
> non-functional watchdog device, here comes an RFC patch to address this.
>
> Note this is only compile tested and dt-documentation is still missing.
> Still send this series because the cleanup is nice independent of this discussion
> and to have something to argue about.
>
> Does someone can offer a better name than "has-watchdog", is there a scheme
> that could be used already that I'm not aware of?
>
> Best regards
> Uwe
>
> Uwe Kleine-König (2):
> rtc: pcf2127: move watchdog initialisation to a separate function
> [RFC] rtc: pcf2127: only use watchdog when explicitly available
>
> drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
> 1 file changed, 32 insertions(+), 25 deletions(-)
>
> --
> 2.28.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
2020-10-26 7:23 ` [PATCH 0/2] " Qiang Zhao
@ 2020-10-26 20:48 ` Alexandre Belloni
2020-10-26 21:29 ` Guenter Roeck
0 siblings, 1 reply; 22+ messages in thread
From: Alexandre Belloni @ 2020-10-26 20:48 UTC (permalink / raw)
To: Qiang Zhao
Cc: Uwe Kleine-König, Bruno Thomsen, linux-rtc, a.zummo,
linux-watchdog, devicetree, linux-kernel, robh+dt, kernel,
Wim Van Sebroeck, Guenter Roeck
On 26/10/2020 07:23:26+0000, Qiang Zhao wrote:
> Any update for this patchset?
>
The whole point would be to get the DT and the watchdog maintainers
agree on the property name. Once done, the driver implementation is
trivial and will get applied.
> Best Regards
> Qiang Zhao
>
> > -----Original Message-----
> > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Sent: 2020年9月24日 18:53
> > To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Qiang Zhao
> > <qiang.zhao@nxp.com>; Bruno Thomsen <bruno.thomsen@gmail.com>
> > Cc: linux-rtc@vger.kernel.org; a.zummo@towertech.it;
> > linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; robh+dt@kernel.org; kernel@pengutronix.de;
> > Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> > <linux@roeck-us.net>
> > Subject: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
> >
> > Hello,
> >
> > now that there are two people stumbling over the pcf2127 driver providing a
> > non-functional watchdog device, here comes an RFC patch to address this.
> >
> > Note this is only compile tested and dt-documentation is still missing.
> > Still send this series because the cleanup is nice independent of this discussion
> > and to have something to argue about.
> >
> > Does someone can offer a better name than "has-watchdog", is there a scheme
> > that could be used already that I'm not aware of?
> >
> > Best regards
> > Uwe
> >
> > Uwe Kleine-König (2):
> > rtc: pcf2127: move watchdog initialisation to a separate function
> > [RFC] rtc: pcf2127: only use watchdog when explicitly available
> >
> > drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
> > 1 file changed, 32 insertions(+), 25 deletions(-)
> >
> > --
> > 2.28.0
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
2020-10-26 20:48 ` Alexandre Belloni
@ 2020-10-26 21:29 ` Guenter Roeck
0 siblings, 0 replies; 22+ messages in thread
From: Guenter Roeck @ 2020-10-26 21:29 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Qiang Zhao, Uwe Kleine-König, Bruno Thomsen, linux-rtc,
a.zummo, linux-watchdog, devicetree, linux-kernel, robh+dt,
kernel, Wim Van Sebroeck
On Mon, Oct 26, 2020 at 09:48:11PM +0100, Alexandre Belloni wrote:
> On 26/10/2020 07:23:26+0000, Qiang Zhao wrote:
> > Any update for this patchset?
> >
>
> The whole point would be to get the DT and the watchdog maintainers
> agree on the property name. Once done, the driver implementation is
> trivial and will get applied.
>
DT maintainers make that decision.
Guenter
> > Best Regards
> > Qiang Zhao
> >
> > > -----Original Message-----
> > > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Sent: 2020年9月24日 18:53
> > > To: Alexandre Belloni <alexandre.belloni@bootlin.com>; Qiang Zhao
> > > <qiang.zhao@nxp.com>; Bruno Thomsen <bruno.thomsen@gmail.com>
> > > Cc: linux-rtc@vger.kernel.org; a.zummo@towertech.it;
> > > linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; robh+dt@kernel.org; kernel@pengutronix.de;
> > > Wim Van Sebroeck <wim@linux-watchdog.org>; Guenter Roeck
> > > <linux@roeck-us.net>
> > > Subject: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
> > >
> > > Hello,
> > >
> > > now that there are two people stumbling over the pcf2127 driver providing a
> > > non-functional watchdog device, here comes an RFC patch to address this.
> > >
> > > Note this is only compile tested and dt-documentation is still missing.
> > > Still send this series because the cleanup is nice independent of this discussion
> > > and to have something to argue about.
> > >
> > > Does someone can offer a better name than "has-watchdog", is there a scheme
> > > that could be used already that I'm not aware of?
> > >
> > > Best regards
> > > Uwe
> > >
> > > Uwe Kleine-König (2):
> > > rtc: pcf2127: move watchdog initialisation to a separate function
> > > [RFC] rtc: pcf2127: only use watchdog when explicitly available
> > >
> > > drivers/rtc/rtc-pcf2127.c | 57 ++++++++++++++++++++++-----------------
> > > 1 file changed, 32 insertions(+), 25 deletions(-)
> > >
> > > --
> > > 2.28.0
> >
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
2020-09-24 10:52 ` [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available Uwe Kleine-König
` (2 preceding siblings ...)
2020-10-26 7:23 ` [PATCH 0/2] " Qiang Zhao
@ 2020-11-30 9:51 ` Rasmus Villemoes
2020-12-04 9:27 ` Alexandre Belloni
3 siblings, 1 reply; 22+ messages in thread
From: Rasmus Villemoes @ 2020-11-30 9:51 UTC (permalink / raw)
To: Uwe Kleine-König, Alexandre Belloni, Qiang Zhao, Bruno Thomsen
Cc: linux-rtc, a.zummo, linux-watchdog, devicetree, linux-kernel,
robh+dt, kernel, Wim Van Sebroeck, Guenter Roeck
On 24/09/2020 12.52, Uwe Kleine-König wrote:
> Hello,
>
> now that there are two people stumbling over the pcf2127 driver
> providing a non-functional watchdog device, here comes an RFC patch to
> address this.
I just want to add a "me too" here, as I'm also now affected by the
pcf2127 exposing a watchdog device (1) it didn't use to (affecting what
/dev/watchdog0 means) and (2) is not actually hooked up in hardware.
So can we please move forward with adding the has-watchdog opt-in DT
property so existing boards will not be affected?
Thanks,
Rasmus
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/2] rtc: pcf2127: only use watchdog when explicitly available
2020-11-30 9:51 ` Rasmus Villemoes
@ 2020-12-04 9:27 ` Alexandre Belloni
0 siblings, 0 replies; 22+ messages in thread
From: Alexandre Belloni @ 2020-12-04 9:27 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Uwe Kleine-König, Qiang Zhao, Bruno Thomsen, linux-rtc,
a.zummo, linux-watchdog, devicetree, linux-kernel, robh+dt,
kernel, Wim Van Sebroeck, Guenter Roeck
On 30/11/2020 10:51:41+0100, Rasmus Villemoes wrote:
> On 24/09/2020 12.52, Uwe Kleine-König wrote:
> > Hello,
> >
> > now that there are two people stumbling over the pcf2127 driver
> > providing a non-functional watchdog device, here comes an RFC patch to
> > address this.
>
> I just want to add a "me too" here, as I'm also now affected by the
> pcf2127 exposing a watchdog device (1) it didn't use to (affecting what
> /dev/watchdog0 means) and (2) is not actually hooked up in hardware.
>
> So can we please move forward with adding the has-watchdog opt-in DT
> property so existing boards will not be affected?
>
Well, someone has to get that property reviewed by Rob. Maybe this could
be reset-source as we have wakeup-source.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread