[coreboot-gerrit] Change in coreboot[master]: [WIP] mainboards/hp: Add HP Elitebook 8470p

[Arthur Heymans (Code Review)]

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.asl
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.txt
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.


-- 
To view, visit https://review.coreboot.org/18985
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Icbc051e2272b8ea73627940db15a56901d737472
Gerrit-PatchSet: 6
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Iru Cai <mytbk920423 at gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur at aheymans.xyz>
Gerrit-Reviewer: Iru Cai <mytbk920423 at gmail.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes