Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49062 )
Change subject: mb/kontron/bsl6: Reorder selects alphabetically ......................................................................
mb/kontron/bsl6: Reorder selects alphabetically
Change-Id: I4f0a1742556d11757990891c58fa2e3431b989a5 Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/kontron/bsl6/Kconfig 1 file changed, 10 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/62/49062/1
diff --git a/src/mainboard/kontron/bsl6/Kconfig b/src/mainboard/kontron/bsl6/Kconfig index fd52d3e..efab85c 100644 --- a/src/mainboard/kontron/bsl6/Kconfig +++ b/src/mainboard/kontron/bsl6/Kconfig @@ -3,17 +3,17 @@ config BOARD_KONTRON_BSL6_COMMON def_bool n select BOARD_ROMSIZE_KB_16384 - select SOC_INTEL_SKYLAKE - select SKYLAKE_SOC_PCH_H - select EXCLUDE_NATIVE_SD_INTERFACE - select NO_FADT_8042 - select HAVE_ACPI_TABLES - select HAVE_OPTION_TABLE - select HAVE_CMOS_DEFAULT - select MAINBOARD_HAS_LPC_TPM - select EC_KONTRON_KEMPLD - select MAINBOARD_HAS_LIBGFXINIT select DRIVERS_I2C_NCT7802Y + select EC_KONTRON_KEMPLD + select EXCLUDE_NATIVE_SD_INTERFACE + select HAVE_ACPI_TABLES + select HAVE_CMOS_DEFAULT + select HAVE_OPTION_TABLE + select MAINBOARD_HAS_LIBGFXINIT + select MAINBOARD_HAS_LPC_TPM + select NO_FADT_8042 + select SKYLAKE_SOC_PCH_H + select SOC_INTEL_SKYLAKE
config BOARD_KONTRON_BSL6_OPTIONS bool
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49062 )
Change subject: mb/kontron/bsl6: Reorder selects alphabetically ......................................................................
Patch Set 1:
I don't see any positive affect in alphabetical order. Actually, I've only seen things turn into a mess since people started to order alpha- betical: You can't group things together anymore unless they share a prefix and alphabetical order is broken after a few patches anyway.
So beside producing more work (to keep things sorted) for the future, what's the intended effect?
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49062 )
Change subject: mb/kontron/bsl6: Reorder selects alphabetically ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
I don't see any positive affect in alphabetical order. Actually, I've only seen things turn into a mess since people started to order alpha- betical: You can't group things together anymore unless they share a prefix and alphabetical order is broken after a few patches anyway.
So beside producing more work (to keep things sorted) for the future, what's the intended effect?
Grouping options by their usage or effect is subjective, I think. Options could be related to multiple groups and they might not be understood by others, which makes the group concept useless if not documented anywhere. Ordering the options alphabetical, makes them way more readable and it looks cleaner :)
Attention is currently required from: Felix Singer. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49062 )
Change subject: mb/kontron/bsl6: Reorder selects alphabetically ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
Grouping options by their usage or effect is subjective, I think.
I meant grouping things that obviously belong together, e.g. because they are defined in the same Kconfig file, but don't share a prefix. For instance the
EXCLUDE_NATIVE_SD_INTERFACE
here. I have no idea what it is about. From its position on the left hand side of the diff, I'd say it's soc/skylake/ related. Of course, this could be handled by better naming of options. But if you look at the SOC_INTEL_ COMMON_ prefix for instance, there are many places already where the prefix makes up most of the name. In a long select block, that makes things harder to find (manually), IMHO. Same with variables in program code btw., if one starts prefixing them, chances are high that variables with the same prefix get mixed up.
tl;dr I don't think there's a simple rule that could make everything perfect.
way more readable and it looks cleaner
And how is that ^ not subjective?
There is CB:51673 now. If that got integrated into Jenkins, it would kill my main argument against alphabetical ordering. I still don't see an argument for it, though. Maybe I just miss something, e.g. is everybody using editors where searching is hard?
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/49062?usp=email )
Change subject: mb/kontron/bsl6: Reorder selects alphabetically ......................................................................
Abandoned
-
Felix Singer (Code Review) -
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)