Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33839 )
Change subject: mb/protectli/vault_kbl: Add FW6 support ......................................................................
Patch Set 10: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... File Documentation/mainboard/protectli/fw6.jpg:
PS10: Where's the flash chip? I wouldn't want to hear about people reflashing their MOSFETs
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... File Documentation/mainboard/protectli/fw6.md:
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... PS10, Line 25: by coreboot by *the* coreboot
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... PS10, Line 49: Use a clip Is the flash chip voltage rail isolated from the rest of the system with a diode?
https://review.coreboot.org/c/coreboot/+/33839/10/Documentation/mainboard/pr... PS10, Line 76: with VGA Option ROM : - HDMI port with libgfxinit This can go on a single line:
... with both libgfxinit and the VGA Option ROM
(yes, I put libgfxinit first on purpose)
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... File src/mainboard/protectli/vault_kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 292: q Trailing `q` ?
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... File src/mainboard/protectli/vault_kbl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/33839/10/src/mainboard/protectli/va... PS10, Line 30: #include "acpi/mainboard.asl" I don't think we want to include an empty file