Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 34:
(12 comments)
https://review.coreboot.org/c/coreboot/+/32531/33/Documentation/mainboard/51... File Documentation/mainboard/51nb/x210.jpg:
PS33:
Maybe crop this image a bit, so that it is under 50 KiB?
Done
https://review.coreboot.org/c/coreboot/+/32531/33/Documentation/mainboard/51... File Documentation/mainboard/51nb/x210.md:
https://review.coreboot.org/c/coreboot/+/32531/33/Documentation/mainboard/51... PS33, Line 48: Use libgfxinit once Kaby Lake is supported.
Done, I guess?
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ac... File src/mainboard/51nb/x210/acpi_tables.c:
PS33:
This file shouldn't be needed anymore
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/de... File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/de... PS33, Line 167: register "PcieRpHotPlug[3]" = "1"
Hmmmmm... […]
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/de... PS33, Line 179: webcam
Please capitalize: `Webcam`
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... File src/mainboard/51nb/x210/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... PS33, Line 33: // CPU
This comment does not add much value. […]
same as every other SKL/KBL board though
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ds... PS33, Line 46: // Chipset specific sleep states
This comment was dropped not too long ago
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... File src/mainboard/51nb/x210/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 22: 0x0000000c
12
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 34: 0x17aa2155
This value in the comment might rot away.
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 35: 0x0
0 here and on AZALIA_PIN_CFG macros
Done
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 39: /* Pin Complex (NID 0x19) */
I would drop these as they don't add much info.
ideally someone would add which pin is what, as with other boards
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ro... File src/mainboard/51nb/x210/romstage.c:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/ro... PS33, Line 31: sizeof(RcompResistor));
Fits on the previous line
Done