Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48648 )
Change subject: mb/asus: Add ASUS H110T mainboard ......................................................................
Patch Set 1: Code-Review+1
(10 comments)
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/Kc... File src/mainboard/asus/h110t/Kconfig:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/Kc... PS1, Line 4: : select BOARD_ROMSIZE_KB_16384 : select HAVE_ACPI_RESUME : select HAVE_ACPI_TABLES : select HAVE_OPTION_TABLE : select HAVE_CMOS_DEFAULT : select INTEL_GMA_HAVE_VBT : select INTEL_INT15 : select SOC_INTEL_KABYLAKE : select SKYLAKE_SOC_PCH_H : select SUPERIO_NUVOTON_COMMON_COM_A : select SUPERIO_NUVOTON_NCT5539D : select REALTEK_8168_RESET : select RT8168_SET_LED_MODE : select MAINBOARD_USES_IFD_GBE_REGION : select DRIVER_INTEL_I210 : select MAINBOARD_HAS_LPC_TPM Please order alphabetically
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/Kc... PS1, Line 38: config DEVICETREE : string : default "devicetree.cb" Remove, devicetree.cb is the default
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/bo... File src/mainboard/asus/h110t/board_info.txt:
PS1: Board name and Vendor name missing
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... File src/mainboard/asus/h110t/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/de... PS1, Line 5: register "deep_s3_enable_ac" = "0" : register "deep_s3_enable_dc" = "0" : register "deep_s5_enable_ac" = "0" : register "deep_s5_enable_dc" = "0" Remove, 0 is the default
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ds... File src/mainboard/asus/h110t/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ds... PS1, Line 7: 0x02, // DSDT revision: ACPI v2.0 and up Usually, we use the defines here.
ACPI_DSDT_REV_2
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ds... PS1, Line 8: "COREv4", // OEM id OEM_ID
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ds... PS1, Line 9: "COREBOOT", // OEM table id ACPI_TABLE_CREATOR
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ma... File src/mainboard/asus/h110t/mainboard.c:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ma... PS1, Line 14: mainboard_enable enable_mainboard
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ra... File src/mainboard/asus/h110t/ramstage.c:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ra... PS1, Line 10: gpio_configure_pads(gpio_table, ARRAY_SIZE(gpio_table)); Add the following hook to mainboard_ops and do the GPIO configuration there:
.init = init_mainboard
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ro... File src/mainboard/asus/h110t/romstage.c:
https://review.coreboot.org/c/coreboot/+/48648/1/src/mainboard/asus/h110t/ro... PS1, Line 6: include <string.h> Is this used?