[PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[Teresa Remmet]

There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
working in spl again.

Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
---
 drivers/watchdog/wdt-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
index 268713529604..f113a65fc187 100644
--- a/drivers/watchdog/wdt-uclass.c
+++ b/drivers/watchdog/wdt-uclass.c
@@ -51,7 +51,7 @@ int initr_watchdog(void)
 						    4 * reset_period) / 4;
 	}
 
-	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
+	if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
 		printf("WDT:   Not starting\n");
 		return 0;
 	}
-- 
2.25.1

Re: [PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[Rasmus Villemoes]

On 18/06/2021 13.14, Teresa Remmet wrote:
> There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
> So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
> working in spl again.

I suppose it doesn't make sense to introduce SPL/TPL variants of that
(if one wants to handle a watchdog early, it should be handled early),
so ack.

But this whole thing seems extremely fragile. There really should be
some kind of sanity check, maybe just scripted run over the tree once in
while, that finds such issues. A very naive approach like

git grep -E -o 'CONFIG_IS_ENABLED\(\s*[A-Z0-9a-z_]*' | cut -f2 -d'(' |
sort -u | while read x ; do if ! git grep -q "config SPL_$x" && ! git
grep -q "config TPL_$x" ; then echo "No SPL or TPL variant of CONFIG_$x"
; fi ; done

finds a lot of stuff, but most are probably in files that cannot be
built for SPL/TPL (e.g. all the CMD_ stuff), so false positives. But
there's also somewhat amusing examples like

#if CONFIG_IS_ENABLED(FIT_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT)

(which we find because there's no SPL_SPL_FIT_PRINT...).

Rasmus

Re: [PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[Stefan Roese]

On 18.06.21 13:14, Teresa Remmet wrote:
> There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
> So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
> working in spl again.
> 
> Signed-off-by: Teresa Remmet <t.remmet@phytec.de>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   drivers/watchdog/wdt-uclass.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 268713529604..f113a65fc187 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -51,7 +51,7 @@ int initr_watchdog(void)
>   						    4 * reset_period) / 4;
>   	}
>   
> -	if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
> +	if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
>   		printf("WDT:   Not starting\n");
>   		return 0;
>   	}
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[Stefan Roese]

On 18.06.21 14:52, Rasmus Villemoes wrote:
> On 18/06/2021 13.14, Teresa Remmet wrote:
>> There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
>> So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
>> working in spl again.
> 
> I suppose it doesn't make sense to introduce SPL/TPL variants of that
> (if one wants to handle a watchdog early, it should be handled early),
> so ack.
> 
> But this whole thing seems extremely fragile. There really should be
> some kind of sanity check, maybe just scripted run over the tree once in
> while, that finds such issues.

I whole-heartily agree. I'm pretty sure, that the U-Boot source tree is
cluttered with misuses of these constructs.

> A very naive approach like
> 
> git grep -E -o 'CONFIG_IS_ENABLED\(\s*[A-Z0-9a-z_]*' | cut -f2 -d'(' |
> sort -u | while read x ; do if ! git grep -q "config SPL_$x" && ! git
> grep -q "config TPL_$x" ; then echo "No SPL or TPL variant of CONFIG_$x"
> ; fi ; done
> 
> finds a lot of stuff, but most are probably in files that cannot be
> built for SPL/TPL (e.g. all the CMD_ stuff), so false positives. But
> there's also somewhat amusing examples like
> 
> #if CONFIG_IS_ENABLED(FIT_PRINT) || CONFIG_IS_ENABLED(SPL_FIT_PRINT)
> 
> (which we find because there's no SPL_SPL_FIT_PRINT...).

It would be great if someone (you?) could come up with such a script.

Thanks,
Stefan

Re: [PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[Tim Harvey]

On Fri, Jun 18, 2021 at 4:14 AM Teresa Remmet <t.remmet@phytec.de> wrote:
>
> There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
> So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
> working in spl again.
>
> Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> ---
>  drivers/watchdog/wdt-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-uclass.c
> index 268713529604..f113a65fc187 100644
> --- a/drivers/watchdog/wdt-uclass.c
> +++ b/drivers/watchdog/wdt-uclass.c
> @@ -51,7 +51,7 @@ int initr_watchdog(void)
>                                                     4 * reset_period) / 4;
>         }
>
> -       if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
> +       if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
>                 printf("WDT:   Not starting\n");
>                 return 0;
>         }
> --
> 2.25.1
>

Maybe add a 'Fixes: 830d29ac3721 ("watchdog: Allow to use CONFIG_WDT
without starting watchdog")' to the commit message?

I'm more interested in just someone picking this up as without it SPL
watchdog is broken.

Tim

Re: [PATCH] drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

[Teresa Remmet]

Hello Tim,

Am Mittwoch, den 14.07.2021, 15:42 -0700 schrieb Tim Harvey:
> On Fri, Jun 18, 2021 at 4:14 AM Teresa Remmet <t.remmet@phytec.de>
> wrote:
> > There is no separate SPL/TPL config for WATCHDOG_AUTOSTART.
> > So use IS_ENABLED instead of CONFIG_IS_ENABLED to make watchdog
> > working in spl again.
> > 
> > Signed-off-by: Teresa Remmet <t.remmet@phytec.de>
> > ---
> >  drivers/watchdog/wdt-uclass.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/watchdog/wdt-uclass.c b/drivers/watchdog/wdt-
> > uclass.c
> > index 268713529604..f113a65fc187 100644
> > --- a/drivers/watchdog/wdt-uclass.c
> > +++ b/drivers/watchdog/wdt-uclass.c
> > @@ -51,7 +51,7 @@ int initr_watchdog(void)
> >                                                     4 *
> > reset_period) / 4;
> >         }
> > 
> > -       if (!CONFIG_IS_ENABLED(WATCHDOG_AUTOSTART)) {
> > +       if (!IS_ENABLED(CONFIG_WATCHDOG_AUTOSTART)) {
> >                 printf("WDT:   Not starting\n");
> >                 return 0;
> >         }
> > --
> > 2.25.1
> > 
> 
> Maybe add a 'Fixes: 830d29ac3721 ("watchdog: Allow to use CONFIG_WDT
> without starting watchdog")' to the commit message?

yes, this would make more clear. I will send a v2 with a updated commit
message.

Thanks,
Teresa 

> 
> I'm more interested in just someone picking this up as without it SPL
> watchdog is broken.
> 
> Tim