Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44167 )
Change subject: mb/acer/m3800: add Acer Aspire M3800 desktop ......................................................................
Patch Set 1: Code-Review+1
(12 comments)
https://review.coreboot.org/c/coreboot/+/44167/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44167/1//COMMIT_MSG@30 PS1, Line 30: Core2 Core 2
https://review.coreboot.org/c/coreboot/+/44167/1//COMMIT_MSG@52 PS1, Line 52: S3 suspend/resume ("RAM INIT FAILURE!" on resume) try to call `ite_enable_3vsbsw` in early_init.c
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... File src/mainboard/acer/m3800/Kconfig:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... PS1, Line 28: config MMCONF_BASE_ADDRESS : hex : default 0xe0000000 Shouldn't be needed
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... PS1, Line 40: config MAX_CPUS : int : default 4 Was moved to common Kconfig
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... File src/mainboard/acer/m3800/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/Kc... PS1, Line 2: M3800 Also add the mainboard codename?
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... File src/mainboard/acer/m3800/cmos.layout:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... 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 I'd drop this
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... PS1, Line 27: #120 264 r 0 unused Also drop commented-out unused entries please
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/cm... PS1, Line 90: 11 10 160M : 11 11 224M : 11 12 352M Argh! alignment!
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/de... File src/mainboard/acer/m3800/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/de... PS1, Line 144: device pci 1f.1 on end # PATA/IDE Really?
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... File src/mainboard/acer/m3800/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... PS1, Line 17: Scope (_SB) { : Device (PCI0) : { I use `Device (_SB.PCI0)` and a single scope
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/ds... PS1, Line 22: #include <drivers/intel/gma/acpi/default_brightness_levels.asl> Not necessary anymore
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/gp... File src/mainboard/acer/m3800/gpio.c:
https://review.coreboot.org/c/coreboot/+/44167/1/src/mainboard/acer/m3800/gp... PS1, Line 87: static const struct pch_gpio_set3 pch_gpio_set3_mode = { }; : : static const struct pch_gpio_set3 pch_gpio_set3_direction = { }; : : static const struct pch_gpio_set3 pch_gpio_set3_level = { }; not needed for ICH