Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35427 )
Change subject: mb/supermicro/x11: add x11ssm-f board ......................................................................
Patch Set 25:
(6 comments)
Is doc/log_dumps.tgz intentional, not linked anywhere?
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... PS25, Line 3: select BOARD_SUPERMICRO_BASEBOARD_X11 Might not be compulsory, but I would prefer if the last part matched the directory name here.
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... PS25, Line 4: config BOARD_SUPERMICRO_X11SSM_F Typically there is an empty line before keyword config. Sometimes the variant name displayed in menuconfig has '-> ' in front.
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/mainboard.c:
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... PS25, Line 27: if (CONFIG(BOARD_SUPERMICRO_X11SSM_F)) { If you ask me, these should be forbidden in the base directory, once you apply variants-directory scheme. Others are likely to accept this, even though we know it gets ugly fast after a couple variant boards.
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/ramstage.c:
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... PS25, Line 25: // probably can be removed here So this code is also variant-dependent?
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/include/variant/gpio.h:
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... PS25, Line 22: #ifndef __ACPI__ Why? This file does not provide anything usable for .asl?
https://review.coreboot.org/c/coreboot/+/35427/25/src/mainboard/supermicro/x... File src/mainboard/supermicro/x11-lga1151-series/variants/x11ssm-f/working_config:
PS25: We have configs/ directory, maybe better choice would be to commit the result of 'make savedefconfig' there after code merge, if necessary. Typically we would like the default config to be bootable. A full config file like this becomes stale as soon as some of these Kconfig variables is removed or renamed, and that happens quite often.