Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44696 ) Change subject: mb/intel/dq45ek: Add new mainboard ...................................................................... Patch Set 2: Code-Review+1 (9 comments) https://review.coreboot.org/c/coreboot/+/44696/1//COMMIT_MSG Commit Message: https://review.coreboot.org/c/coreboot/+/44696/1//COMMIT_MSG@13 PS1, Line 13: GPIO33
On the back of the board, underneath the ICH10. […] Ack
https://review.coreboot.org/c/coreboot/+/44696/1//COMMIT_MSG@16 PS1, Line 16: At that point
Instead, I'd say: […] Done
https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... File src/mainboard/intel/dq45ek/cmos.layout: https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... PS1, Line 6: # ----------------------------------------------------------------- : # Status Register A : # ----------------------------------------------------------------- : # Status Register B : # ----------------------------------------------------------------- : # Status Register C : #96 4 r 0 status_c_rsvd : #100 1 r 0 uf_flag : #101 1 r 0 af_flag : #102 1 r 0 pf_flag : #103 1 r 0 irqf_flag : # ----------------------------------------------------------------- : # Status Register D : #104 7 r 0 status_d_rsvd : #111 1 r 0 valid_cmos_ram : # ----------------------------------------------------------------- : # Diagnostic Status Register : #112 8 r 0 diag_rsvd1
IMHO this isn't very useful, and I'd drop it Done
https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... PS1, Line 27: unused
All commented-out `unused` entries can go away Done
https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... PS1, Line 88: 11 9 96M : 11 10 160M : 11 11 224M : 11 12 352M
This seems to be misaligned Done
https://review.coreboot.org/c/coreboot/+/44696/2/src/mainboard/intel/dq45ek/... File src/mainboard/intel/dq45ek/cmos.layout: https://review.coreboot.org/c/coreboot/+/44696/2/src/mainboard/intel/dq45ek/... PS2, Line 8: #120 264 r 0 unused This one, too https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... File src/mainboard/intel/dq45ek/devicetree.cb: https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... PS1, Line 84: IDE
Strictly speaking, this is SATA ports 4 and 5 when operating in IDE mode Done
https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... File src/mainboard/intel/dq45ek/dsdt.asl: https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... PS1, Line 17: {
nit: move this brace to the next line for consistency Done
https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... File src/mainboard/intel/dq45ek/gpio.c: https://review.coreboot.org/c/coreboot/+/44696/1/src/mainboard/intel/dq45ek/... PS1, Line 88: static const struct pch_gpio_set3 pch_gpio_set3_mode = { : .gpio72 = GPIO_MODE_GPIO, : }; : : static const struct pch_gpio_set3 pch_gpio_set3_direction = { : .gpio72 = GPIO_DIR_INPUT, : }; : : static const struct pch_gpio_set3 pch_gpio_set3_level = { };
GPIO72 is described in section 13.10.12 of the datasheet. […] Ack
-- To view, visit https://review.coreboot.org/c/coreboot/+/44696 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I565690c4ed7b4ae60760b8fa6b1e2d783f8e094b Gerrit-Change-Number: 44696 Gerrit-PatchSet: 2 Gerrit-Owner: Samuel Holland <samuel@sholland.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Martin Roth <martinroth@google.com> Gerrit-Reviewer: Patrick Georgi <pgeorgi@google.com> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Wed, 26 Aug 2020 12:47:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Angel Pons <th3fanbus@gmail.com> Comment-In-Reply-To: Samuel Holland <samuel@sholland.org> Gerrit-MessageType: comment