| Message ID | 20210618111425.2647258-1-t.remmet@phytec.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | drivers: watchdog: wdt-uclass: Use IS_ENABLED for WATCHDOG_AUTOSTART | expand |
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
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
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
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
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 --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; }
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(-)