Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36871 )
Change subject: mb/lenovo/{t410,x201}: Make T410 a variant board of X201 ......................................................................
Patch Set 21:
(18 comments)
Over half the things you do in this patch are not related to variants.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/Kconfig:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 21: select DRIVERS_LENOVO_HYBRID_GRAPHICS no good on x201.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 25: select MAINBOARD_HAS_LIBGFXINIT why move?
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 47: DEVICETREE use overridetrees
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 72: ONBOARD_VGA_IS_PRIMARY this override is only needed with DRIVERS_LENOVO_HYBRID_GRAPHICS
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 25: romstage-$(CONFIG_BOARD_LENOVO_X201) += variants/x201/gpio.c : romstage-$(CONFIG_BOARD_LENOVO_T410) += variants/t410/gpio.c are those not identical?
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/cmos.layout:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 79: hybrid_graphics_mode you don't want bogus options on x201.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 82: 448 128 r 0 vbnv please don't change lines that are not needed.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/dock.c:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 41: Floating on T410 Don't do things that you know are not working. Also dock.asl needs fixing for t410.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 55: const int gpio_num_array[] = {3, 4, 5, -1}; : : return get_gpios(gpio_num_array) != 7; : } only change things needed please.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/early_init.c:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 4: * Copyright (C) 2019 Patrick Rudolph Not much copyrightable here but why change?
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/mainboard.c:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 7: * Copyright (C) 2019 Maciej Matuszczyk you add a copyright line to remove a newline??
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/romstage.c:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 27: #if CONFIG(BOARD_LENOVO_T410) : { 1, IF1_557, 0 }, : { 1, IF1_55F, 1 }, : { 1, IF1_74B, 3 }, : { 1, IF1_14B, 3 }, : { 1, IF1_14B, 3 }, : { 1, IF1_74B, 3 }, : { 1, IF1_74B, 3 }, : { 1, IF1_74B, 3 }, : { 1, IF1_55F, 4 }, : { 1, IF1_55F, 5 }, : { 1, IF1_74B, 7 }, : { 1, IF1_74B, 7 }, : { 1, IF1_557, 7 }, : { 1, IF1_55F, 7 }, no preprocessor please.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 83: #if CONFIG(BOARD_LENOVO_X201) // Needed to make make happy no preprocessor. if you don't want functions you should have the makefile deal with that.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 99: #if CONFIG(BOARD_LENOVO_X201) no preprocessor.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/smihandler.c:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 56: mdelay(250); please only do things related to variants.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 104: /* If EC wake events are enabled, enable wake on EC WAKE GPE. */ : if (ec_wake & 0x14) { : /* Redirect EC WAKE GPE to SCI. */ Not needed?
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/thermal.h:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 3: * : * Copyright (C) 2008-2009 coresystems GmbH : * Copyright (C) 2011 The Chromium OS Authors. All rights reserved. : * Copyright (C) 2014 Vladimir Serbinenko : * Copyright (C) 2016 Patrick Rudolph siro@das-labor.org : * Copyright (C) 2017 James Ye jye836@gmail.com unrelated.
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... File src/mainboard/lenovo/x201/vboot-rwa.fmd:
https://review.coreboot.org/c/coreboot/+/36871/21/src/mainboard/lenovo/x201/... PS21, Line 18: SMMSTORE(PRESERVE) 0x40000 : : WP_RO { : RO_VPD(PRESERVE) 0x1000 : RO_SECTION 0x11e000 { unrelated?