Attention is currently required from: Nico Huber, Xue Yao, Paul Menzel, Kilian Neuner, Charles Moyes, Alexander Couzens. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/28950 )
Change subject: lenovo/x230: introduce FHD variant ......................................................................
Patch Set 16:
(6 comments)
File src/mainboard/lenovo/x230/Kconfig:
https://review.coreboot.org/c/coreboot/+/28950/comment/99ded5a4_7b0b9eea PS16, Line 57: default "x230_edp" if BOARD_LENOVO_X230_EDP I wouldn't do this. I'd treat `BOARD_LENOVO_X230_EDP` just like `BOARD_LENOVO_X230` and handle the differences in `INTEL_GMA_VBT_FILE` and Makefile.inc (see my other comments).
https://review.coreboot.org/c/coreboot/+/28950/comment/f9cd6e9d_c4738297 PS16, Line 63: default "ThinkPad X230 edp" if BOARD_LENOVO_X230_EDP I wouldn't change this, it's used in the ID section (a rudimentary mechanism for tools to know which board a coreboot image is for). If flashrom detects a mismatch between the flash chip's ID section and the file to flash's ID section, it will complain and bail out.
TL;DR: Please use the same name as the regular X230 for compatibility.
https://review.coreboot.org/c/coreboot/+/28950/comment/21ee407d_ffbdcef0 PS16, Line 85: config INTEL_GMA_VBT_FILE : string : default "variants/$(CONFIG_VARIANT_DIR)/data.vbt" if BOARD_LENOVO_X230S || BOARD_LENOVO_X230_EDP : default "variants/x230/data.vbt" if BOARD_LENOVO_X230 || BOARD_LENOVO_X230T Without using the x230_edp variant, this can be simplified to:
config INTEL_GMA_VBT_FILE default "variants/x230_edp/data.vbt" if BOARD_LENOVO_X230_EDP
File src/mainboard/lenovo/x230/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/28950/comment/14cff301_9da58f9f PS16, Line 13: ramstage-$(CONFIG_MAINBOARD_USE_LIBGFXINIT) += variants/$(VARIANT_DIR)/gma-mainboard.ads Instead of adding Kconfig options for the other three files, how about handling the special case of X230_EDP directly in the Makefile?
ifeq ($(CONFIG_BOARD_LENOVO_X230_EDP),y) ramstage-$(CONFIG_MAINBOARD_USE_LIBGFXINIT) += variants/x230_edp/gma-mainboard.ads endif else ramstage-$(CONFIG_MAINBOARD_USE_LIBGFXINIT) += variants/$(VARIANT_DIR)/gma-mainboard.ads endif
File src/mainboard/lenovo/x230/variants/x230_edp/board_info.txt:
PS16: Shouldn't be needed
File src/mainboard/lenovo/x230/variants/x230_edp/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/28950/comment/485f8edc_0594e4c9 PS16, Line 2: -- This file is part of the coreboot project. Please remove this line
-
Angel Pons (Code Review)