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 <[email protected]>
---
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
Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
<[email protected]>:
>
> 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 <[email protected]>
> ---
> 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
>
On 9/27/20 1:09 AM, Bruno Thomsen wrote:
> Den tor. 24. sep. 2020 kl. 12.53 skrev Uwe Kleine-König
> <[email protected]>:
>>
>> 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 <[email protected]>
>> ---
>> 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
>>
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
> > <[email protected]>:
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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/ |
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
> > > <[email protected]>:
> > >>
> > >> 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 <[email protected]>
> > >> ---
> > >> 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/ |