Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38463 )
Change subject: mainboard/system76: Add System76 Lemur Pro (lemp9) ......................................................................
Patch Set 6:
(18 comments)
https://review.coreboot.org/c/coreboot/+/38463/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38463/6//COMMIT_MSG@17 PS6, Line 17: - CPU Note to self: These are CML, not ICL
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/Kconfig:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 24: Fix failure to boot GRUB Is this comment accurate?
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi/ac.asl:
PS6: I think we also use tabs to indent stuff in .asl files
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi/mainboard.asl:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 18: Scope (_SB) { This is redundant. This file is already inside the `_SB` scope when included by `dsdt.asl`
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/acpi_tables.c:
PS6: Is this blank file needed? I think it is not a requirement anymore
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 11: Enable
Disable
Or just drop the comment altogether, so that it can't rot away :)
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 20: # CPU (soc/intel/cannonlake/cpu.c) Are these comments not indented on purpose? it looks a bit odd
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 98: Used by card reader Is it, actually?
Or, do you mean that the HSIO lanes for this xHCI are muxed to PCIe?
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 28: // Some generic macros Please drop this comment. I removed all instances in a recent change.
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 34: // CPU I don't think this comment adds much value. I'd drop it.
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 36: : Scope (_SB) { : Device (PCI0) : { The distinct placement of the open braces is odd. In any case, this can be simplified:
Device (_SB.PCI0)
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 45: // Chipset specific sleep states This was also dropped, as the sleepstates.asl is now a common file
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 49: // PS/2 bus Mixed tabs and spaces?
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 16: #ifndef HDA_VERB_H : #define HDA_VERB_H Really? Header guards in a .c file?
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 24: 0x15581401, /* Subsystem ID */ Nit: align the comments here with tabs
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 38: Kabylake But is it Kaby Lake?
While you're at it, add a blank line to separate the Azalia devices
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/romstage.c:
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... PS6, Line 90: Enabled But is it actually enabled?
You can just removed the "Enabled" part, and the comment will not rot :)
https://review.coreboot.org/c/coreboot/+/38463/6/src/mainboard/system76/lemp... File src/mainboard/system76/lemp9/spd/Makefile.inc:
PS6: Note: there's a common implementation of the "add SPD files to CBFS" code.