On Tue, Feb 07, 2023 at 08:39:38AM +0100, Heinrich Schuchardt wrote:
On 2/7/23 01:00, Tom Rini wrote:
On Tue, Feb 07, 2023 at 12:54:03AM +0100, Heinrich Schuchardt wrote:
On 2/6/23 01:53, Simon Glass wrote:
This converts 3 usages of this option to the non-SPL form, since there is no SPL_CMD_BOOTEFI_BOOTMGR defined in Kconfig
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
boot/Makefile | 2 +- cmd/bootmenu.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/Makefile b/boot/Makefile index 69c31adb77d..73b5b19816b 100644 --- a/boot/Makefile +++ b/boot/Makefile @@ -29,7 +29,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFILOADER) += bootmeth_efi.o obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL -obj-$(CONFIG_$(SPL_TPL_)CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o +obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootflow_menu.o endif diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c index 3236ca5d799..422ab411252 100644 --- a/cmd/bootmenu.c +++ b/cmd/bootmenu.c @@ -223,7 +223,7 @@ static int prepare_bootmenu_entry(struct bootmenu_data *menu, return 1; } -#if (CONFIG_IS_ENABLED(CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENABLED(CMD_EFICONFIG)) +#if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENABLED(CMD_EFICONFIG))
There is no reason whatsoever for using different macros for the two options.
Here and elsewhere, one CONFIG is being fixed at a time. If at the end of the series this is not fixed, then that's an issue to address.
This cannot be reviewed easily. I never received the complete series.
This, and the related series, are among the most reviewed we've had in quite some time. Just FWIW.
CONFIG_IS_ENABLED() is more restrictive than IS_ENABLED(). No motivation is provided why the condition should be relaxed in the commit message.
The idea of "restrictive" is not how either of those macros should be evaluated.
Cover-letters are not in the commit history. But anyway the cover-letter does not provide any motivation for the change either.
NAK to this patch.
It's incorrect to use CONFIG_IS_ENABLED() instead of IS_ENABLED() outside of: - CONFIG_FOO, CONFIG_SPL_FOO (etc) exist - The code in question is compiled in the SPL (etc) context and we do need to know if the code block in question is required here and the implicit value of SPL_FOO being false is useful. This case is why Simon insists that adding def_bool n for SPL_EFI_LOADER, etc, is correct, but I'm not convinced.
I will be taking most of these patches in the next day or two, but I can avoid the EFI ones if you insist. They are however I believe correct.