Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34475 )
Change subject: Add Razer Blade Stealth (2016) H2U ......................................................................
Patch Set 38:
(14 comments)
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG@18 PS35, Line 18: tho
though
Done
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG@18 PS35, Line 18: Even tho it has a 16MB chip equipped (W25Q128.V) only the first 8MB are used and mapped. The rest should be left empty (0xFF)
These lines are still too long
Done
https://review.coreboot.org/c/coreboot/+/34475/35//COMMIT_MSG@19 PS35, Line 19: ports
port's
Done
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?
Changed to "Not part of this commit"
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
Should be tabs everywhere btw
Done
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 70: S
dito
Done
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.
Done
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?
Was a leftover. Lid works.
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.
Done
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 vend […]
Works without it. Discarded.
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 24: //
The empty comment lines on this file could be removed
Done
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 25: //
The indentation is weird here (tab+space)
Done
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... File src/mainboard/razer/blade_stealth_kbl/mainboard.c:
https://review.coreboot.org/c/coreboot/+/34475/35/src/mainboard/razer/blade_... PS35, Line 38: if (rdev_readat(&cbfs_region, serial_number, 0, serial_len)
[bikeshedding-grade] seeing this line being split makes me suffer. […]
Ack
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)
Thanks
-
Mimoja (Code Review)