Jonathan Neuschäfer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34475 )
Change subject: Add Razer Blade Stealth (2016) H2U ......................................................................
Patch Set 35: Code-Review+1
(10 comments)
Some minor comments/questions.
Otherwise, it LGTM but I don't have the expertise to give a +2.
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG@19 PS35, Line 19: ports port's
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG@33 PS35, Line 33: : Not going to implement: Does that mean never, or not in this commit, or something in between?
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/acpi/battery.asl:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 62: St The indentation looks broken here (8 spaces here vs. two tabs in surrounding lines)
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 70: S dito
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/acpi/ec.asl:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 27: Name dito, for most of the file.
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 75: // Store Lids -> \Lids should be working... Can you explain this comment in more detail, please? What works, what doesn't?
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/board_info.txt:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 1: razer This is inconsistent with the string in Kconfig (RAZER vs. razer); not sure if intended.
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 274: subsystemid 0x1a58 0x6752 There is a way to set the subsystem ID once and for all PCI devices, which makes sense *if* the vendor firmware also sets it for all devices (I don't know if it does).
The trick is to write `subsystemid 0x1a58 0x6752 inherit` into the `device domain 0` block. Example: https://review.coreboot.org/cgit/coreboot.git/tree/src/mainboard/msi/ms9652_...
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 25: // The indentation is weird here (tab+space)
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 21: b0001 I think this should rather be index 0 (0b0000) and index 1 (0b0001)