Re: [PATCH v3 06/32] sifive: Drop an unnecessary #ifdef
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
> > > Suggested-by: Sean Anderson
> > > ---
> > >
> > > 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.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v3 06/32] sifive: Drop an unnecessary #ifdef
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
Suggested-by: Sean Anderson
---
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
Re: [PATCH v3 06/32] sifive: Drop an unnecessary #ifdef
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
Suggested-by: Sean Anderson
---
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
[PATCH v3 06/32] sifive: Drop an unnecessary #ifdef
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
Suggested-by: Sean Anderson
---
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
TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
struct k210_pll_config *best);
#ifndef nop
--
2.42.0.655.g421f12c284-goog
