diff mbox series

drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART

Message ID 20210618111425.2647258-1-t.remmet@phytec.de
State New
Delegated to: Stefano Babic
Headers show
Series drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART | expand

Commit Message

Teresa Remmet June 18, 2021, 11:14 a.m. UTC
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(-)

Comments

Rasmus Villemoes June 18, 2021, 12:52 p.m. UTC | #1
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
Stefan Roese June 18, 2021, 2:35 p.m. UTC | #2
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
Stefan Roese June 18, 2021, 2:38 p.m. UTC | #3
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
Tim Harvey July 14, 2021, 10:42 p.m. UTC | #4
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
Teresa Remmet July 15, 2021, 6:43 a.m. UTC | #5
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
diff mbox series

Patch

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;
 	}