Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/18985 )
Change subject: [WIP] mainboards/hp: Add HP Elitebook 8470p ......................................................................
Patch Set 6:
(8 comments)
A few nits but it looks quite alright.
https://review.coreboot.org/#/c/18985/6//COMMIT_MSG Commit Message:
PS6, Line 25: - AC and battery status vendor DSDT might hold the answer to that (in the hope that no SMM is used for that...)?
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/Kconfig File src/mainboard/hp/8470p/Kconfig:
PS6, Line 37: config VGA_BIOS_FILE : string : default "pci8086,0166.rom" should be set automatically by VGA_BIOS_ID
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/acpi/platform... File src/mainboard/hp/8470p/acpi/platform.asl:
Line 1: Method(_WAK,1) licence header.
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/board_info.tx... File src/mainboard/hp/8470p/board_info.txt:
Line 1: Category: laptop maybe add link to vendor website page for this device?
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/devicetree.cb File src/mainboard/hp/8470p/devicetree.cb:
PS6, Line 16: 0x0d9c0d9c just a suggestion: if you don't want to be at full brightness (annyoing in the dark) you could lower duty cycle = lower 16 bits divided by upper 16 bits. so 50% would be 0x0d9c06ce
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/gpio.c File src/mainboard/hp/8470p/gpio.c:
Line 1: /* you could use https://review.coreboot.org/#/c/19508/ to generate this file again so entries that that are very meaningful are omitted.
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/mainboard.c File src/mainboard/hp/8470p/mainboard.c:
PS6, Line 37: nstall_intel_vga_int15_handler(GMA_INT15_ACTIVE_LFP_INT_LVDS, GMA_INT15_PANEL_FIT_DEFAULT, GMA_INT15_BOOT_DISPLAY_DEFAULT, 0); : } split line.
https://review.coreboot.org/#/c/18985/6/src/mainboard/hp/8470p/romstage.c File src/mainboard/hp/8470p/romstage.c:
PS6, Line 16: #include <stdint.h> : #include <string.h> : #include <lib.h> : #include <timestamp.h> : #include <arch/byteorder.h> : #include <arch/io.h> : #include <device/pci_def.h> : #include <device/pnp_def.h> : #include <cpu/x86/lapic.h> : #include <arch/acpi.h> : #include <console/console.h> : #include "northbridge/intel/sandybridge/sandybridge.h" : #include "northbridge/intel/sandybridge/raminit_native.h" : #include "southbridge/intel/bd82x6x/pch.h" : #include <southbridge/intel/common/gpio.h> : #include <arch/cpu.h> : #include <cpu/x86/msr.h> I think the only ones needed would be #include <arch/io.h> and #include <northbridge/intel/sandybridge/raminit_native.h>.
Better also #include <southbridge/intel/bd82x6x/pch.h> and #include <northbridge/intel/sandybridge/sandybridge.h> since that is where the actual stuff you need is.