Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40278 )
Change subject: mb/purism/librem_whl: Add new board Librem Mini (WHL-U) ......................................................................
Patch Set 4: Code-Review+1
(13 comments)
Reviewing board ports for shiny new hardware makes me jealous
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... File src/mainboard/purism/librem_whl/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 2: /* This file is part of the coreboot project. */ We dropped these lines (apply to all files)
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 16: Zero 0, please
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 21: One 1, please
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... File src/mainboard/purism/librem_whl/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 17: Method (_STA, 0, NotSerialized) : { : Return (0x0F) : } Not needed: https://review.coreboot.org/c/coreboot/+/35523/122/src/mainboard/acer/aspire...
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 24: #include "thermal.asl" I'd include this from "dsdt.asl" and rename the file to "ac.asl"
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... File src/mainboard/purism/librem_whl/acpi/thermal.asl:
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 42: nit: Enable ACPI debugging in Linux, and add these two lines to every fan's _STA method right before returning:
Debug = "FN0x._STA" Debug = Local1
Then drop all fan devices where its value is zero, and let me know if there are any fans where _STA is non-zero
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 115: Device (FAN1) Two fans?
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 159: Device (FAN2) THREE fans?
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 203: Device (FAN3) FOUR???
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 247: Device (FAN4) Are you sure this isn't a drone?
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... File src/mainboard/purism/librem_whl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 166: Enable I don't think so
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 167: register "deep_s3_enable_ac" = "0" : register "deep_s3_enable_dc" = "0" : register "deep_s5_enable_ac" = "0" : register "deep_s5_enable_dc" = "0" Those default to zero already and make no sense with "deep_sx_config" = "0"
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... File src/mainboard/purism/librem_whl/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40278/4/src/mainboard/purism/librem... PS4, Line 26: Scope (_SB.PCI0.LPCB) Move this scope inside "acpi/ec.asl" for clarity?