Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/11791 )
Change subject: mainboard/lenovo/t410: Add new port ......................................................................
Patch Set 21: Code-Review+1
(10 comments)
https://review.coreboot.org/c/coreboot/+/11791/21/Documentation/mainboard/in... File Documentation/mainboard/index.md:
https://review.coreboot.org/c/coreboot/+/11791/21/Documentation/mainboard/in... PS21, Line 91: ## Portwell Spurious?
https://review.coreboot.org/c/coreboot/+/11791/21/Documentation/mainboard/le... File Documentation/mainboard/lenovo/t410_chip_location.jpg:
PS21: I think a closer image would be more helpful. I can barely see the chip
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/Kconfig:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 63: default 4 I think this should be 8
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 19: Scope(_SB.PCI0.LPCB.EC) : { : } Maybe put the dock.asl EC code here?
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 21: static void acpi_update_thermal_table(global_nvs_t *gnvs) : { : gnvs->tcrt = CRITICAL_TEMPERATURE; : gnvs->tpsv = PASSIVE_TEMPERATURE; : } Do we really need a function just for that?
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 32: # Maybe remove the empty comments
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 122: end Maybe move to the previous line, and remove the "dummy" comment?
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 25: / I think you can remove all of these comments
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... File src/mainboard/lenovo/t410/romstage.c:
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 51: pci_write_config16(PCH_LPC_DEV, LPC_EN, : CNF2_LPC_EN | CNF1_LPC_EN | MC_LPC_EN | KBC_LPC_EN | : COMA_LPC_EN | GAMEL_LPC_EN); Maybe reflow this so that it fits in two lines.
Also, if you select NO_UART_ON_SUPERIO, then COMA_LPC_EN doesn't make much sense I guess
https://review.coreboot.org/c/coreboot/+/11791/21/src/mainboard/lenovo/t410/... PS21, Line 204: : /* This should probably go away. Until now it is required : * and mainboard specific : */ This fits in one line, and there's two empty lines beforehand