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