[U-Boot] [PATCH v2 2/7] spl: nand: rename the SYS_NAND_U_BOOT_OFFS Kconfig option

[Boris Brezillon]

On Fri, 03 Jun 2016 20:08:49 -0500
Scott Wood <oss at buserror.net> wrote:

> On Wed, 2016-06-01 at 13:23 +0200, Boris Brezillon wrote:
> > The SYS_NAND_U_BOOT_OFFS is quite generic, but the Kconfig entry is forced
> > to explicitly depend on platforms that are not already defining it in their
> > include/configs/<board>.h header.
> > 
> > Rename this Kconfig option into SPL_NAND_U_BOOT_OFFS, remove the dependency
> > on NAND_SUNXI and make it dependent on SPL selection.
> > 
> > common/spl/spl_nand.c now sets CONFIG_SYS_NAND_U_BOOT_OFFS to
> > CONFIG_SPL_NAND_U_BOOT_OFFS only when it's undefined. This way we stay
> > compatible with the existing behavior.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
> > Acked-by: Hans de Goede <hdegoede at redhat.com>
> > ---
> >  common/spl/spl_nand.c             | 4 ++++
> >  drivers/mtd/nand/Kconfig          | 9 +++++----
> >  drivers/mtd/nand/sunxi_nand_spl.c | 8 ++++----
> >  3 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
> > index bbd9546..612bd4a 100644
> > --- a/common/spl/spl_nand.c
> > +++ b/common/spl/spl_nand.c
> > @@ -10,6 +10,10 @@
> >  #include <asm/io.h>
> >  #include <nand.h>
> >  
> > +#ifndef CONFIG_SYS_NAND_U_BOOT_OFFS
> > +#define CONFIG_SYS_NAND_U_BOOT_OFFS CONFIG_SPL_NAND_U_BOOT_OFFS
> > +#endif  
> [snip]
> > -# Enhance depends when converting drivers to Kconfig which use this config
> > -config SYS_NAND_U_BOOT_OFFS
> > +if SPL
> > +
> > +# This option should be used in replacement of CONFIG_SYS_NAND_U_BOOT_OFFS.
> > +# CONFIG_SYS_NAND_U_BOOT_OFFS is still preferred if defined.
> > +config SPL_NAND_U_BOOT_OFFS
> >  	hex "Location in NAND to read U-Boot from"
> >  	default 0x8000 if NAND_SUNXI
> > -	depends on NAND_SUNXI
> >  	help
> >  	Set the offset from the start of the nand where u-boot should be
> >  	loaded from.  
> 
> This doesn't work.  CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined when SPL is defined, and the user will be forced to enter a value before kconfig will continue (or kconfig will error out in an automated build).

Yes, CONFIG_SPL_NAND_U_BOOT_OFFS will always be defined, but won't be
used if CONFIG_SYS_NAND_U_BOOT_OFFS is defined in the config header
file.
And for the "user will forced to enter a value before Kconfig
continue" comment, we could just have

config SPL_NAND_U_BOOT_OFFS
  	hex "Location in NAND to read U-Boot from"
  	default 0x8000 if NAND_SUNXI
	default 0x0
	...

> 
> If you want to do this there needs to be a separate bool config that controls whether the hex config exists.

I can add an extra Kconfig option, but is it really necessary:
if people are relying on it they will choose a valid value, and leave
it to 0 otherwise.
It's just a detail, so I'm fine adding this extra option if you think
it's really useful.

> And there'd be no need to rename hex symbol.

Well, functionally there's no problem keeping the existing SYS_ prefix
if we add this extra option to activate the U_OFFS config in Kconfig,
but I'm not sure this is a good idea to reuse config header names in
Kconfig.

And what happens if the user enabled this option (some like to enable
everything :-)) and CONFIG_SYS_NAND_U_BOOT_OFFS is also defined in the
board config header?
That's what I was trying to avoid. By renaming the Kconfig option we
make sure the value defined in include/configs/<board>.h always take
precedence over the Kconfig value.



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com