| Message ID | 20231016222835.596572-7-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Tidy up use of CONFIG_CMDLINE | expand |
On 10/17/23 00:27, Simon Glass wrote: > This code is normally compiled for sifive, but sandbox can also compile > it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is > possible to disable UNIT_TEST for sandbox. > > Drop the condition since it isn't needed. This is not what the patch does. It introduces another condition. > > Signed-off-by: Simon Glass <sjg@chromium.org> > Suggested-by: Sean Anderson <seanga2@gmail.com> > --- > > Changes in v3: > - Just drop the condition instead > > include/k210/pll.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/k210/pll.h b/include/k210/pll.h > index fd16a89cb203..6dd60b2eb4fc 100644 > --- a/include/k210/pll.h > +++ b/include/k210/pll.h > @@ -13,7 +13,7 @@ struct k210_pll_config { > u8 od; > }; > > -#ifdef CONFIG_UNIT_TEST > +#ifdef CONFIG_SANDBOX We should be able to compile with and without unit tests on the MAIX boards. Why should CONFIG_SANDBOX have any relevance here? Why don't we make k210_pll_calc_config() non-static in all cases? Best regards Heinrich > TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, > struct k210_pll_config *best); > #ifndef nop
On 10/18/23 09:51, Heinrich Schuchardt wrote: > On 10/17/23 00:27, Simon Glass wrote: >> This code is normally compiled for sifive, but sandbox can also compile >> it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is >> possible to disable UNIT_TEST for sandbox. >> >> Drop the condition since it isn't needed. > > This is not what the patch does. It introduces another condition. > >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> Suggested-by: Sean Anderson <seanga2@gmail.com> >> --- >> >> Changes in v3: >> - Just drop the condition instead >> >> include/k210/pll.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/k210/pll.h b/include/k210/pll.h >> index fd16a89cb203..6dd60b2eb4fc 100644 >> --- a/include/k210/pll.h >> +++ b/include/k210/pll.h >> @@ -13,7 +13,7 @@ struct k210_pll_config { >> u8 od; >> }; >> >> -#ifdef CONFIG_UNIT_TEST >> +#ifdef CONFIG_SANDBOX > > We should be able to compile with and without unit tests on the MAIX > boards. Why should CONFIG_SANDBOX have any relevance here? > > Why don't we make k210_pll_calc_config() non-static in all cases? U-Boot is smaller when it is static. But the forward declaration can be made unconditionally, as long as it is unused. --Sean
On Wed, Oct 18, 2023 at 01:23:13PM -0400, Sean Anderson wrote: > On 10/18/23 09:51, Heinrich Schuchardt wrote: > > On 10/17/23 00:27, Simon Glass wrote: > > > This code is normally compiled for sifive, but sandbox can also compile > > > it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is > > > possible to disable UNIT_TEST for sandbox. > > > > > > Drop the condition since it isn't needed. > > > > This is not what the patch does. It introduces another condition. > > > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > > Suggested-by: Sean Anderson <seanga2@gmail.com> > > > --- > > > > > > Changes in v3: > > > - Just drop the condition instead > > > > > > include/k210/pll.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/k210/pll.h b/include/k210/pll.h > > > index fd16a89cb203..6dd60b2eb4fc 100644 > > > --- a/include/k210/pll.h > > > +++ b/include/k210/pll.h > > > @@ -13,7 +13,7 @@ struct k210_pll_config { > > > u8 od; > > > }; > > > > > > -#ifdef CONFIG_UNIT_TEST > > > +#ifdef CONFIG_SANDBOX > > > > We should be able to compile with and without unit tests on the MAIX > > boards. Why should CONFIG_SANDBOX have any relevance here? > > > > Why don't we make k210_pll_calc_config() non-static in all cases? > > U-Boot is smaller when it is static. But the forward declaration can be made > unconditionally, as long as it is unused. I think part of the problem with this patch is confusing what it's doing. The issue here is that we can (and this is good!) and do compile drivers/clk/clk_k210.c for sandbox, so it gets coverity scans and so forth. However, if we build sandbox with UNIT_TEST=n then we get an undefined reference to 'nop'. Because sandbox doesn't define nop(). This header then provides nop for sandbox. But, we can do something clearer, now that this is spelled out.
diff --git a/include/k210/pll.h b/include/k210/pll.h index fd16a89cb203..6dd60b2eb4fc 100644 --- a/include/k210/pll.h +++ b/include/k210/pll.h @@ -13,7 +13,7 @@ struct k210_pll_config { u8 od; }; -#ifdef CONFIG_UNIT_TEST +#ifdef CONFIG_SANDBOX TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in, struct k210_pll_config *best); #ifndef nop
This code is normally compiled for sifive, but sandbox can also compile it. We should not use UNIT_TEST as a synonym for SANDBOX, since it is possible to disable UNIT_TEST for sandbox. Drop the condition since it isn't needed. Signed-off-by: Simon Glass <sjg@chromium.org> Suggested-by: Sean Anderson <seanga2@gmail.com> --- Changes in v3: - Just drop the condition instead include/k210/pll.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)