Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/18241 )
Change subject: [WIP] mainboards/hp: Add HP Elitebook 2760p ......................................................................
Patch Set 14:
(6 comments)
Some nits about autoport generated files + you need to add licence headers.
https://review.coreboot.org/#/c/18241/14/src/mainboard/hp/2760p/devicetree.c... File src/mainboard/hp/2760p/devicetree.cb:
PS14, Line 110: device pci 00.0 on # Host bridge Host bridge : subsystemid 0x103c 0x162a : end : device pci 01.0 off # PCIe Bridge for discrete graphics : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x103c 0x162a Moving this block up so that pci devices are in order is more consistent. (I know this is a minor flaw of autoport)
https://review.coreboot.org/#/c/18241/14/src/mainboard/hp/2760p/gpio.c File src/mainboard/hp/2760p/gpio.c:
PS14, Line 43: .gpio5 = GPIO_DIR_OUTPUT, Is not meaningful on MODE_NATIVE. If you define this as a static, you can leave those out.
PS14, Line 74: .gpio1 = GPIO_LEVEL_HIGH, Input is Read Only, so if you define as static you can leave those out.
PS14, Line 108: .gpio0 = GPIO_RESET_PWROK, Could also be hidden using static to initialise at 0 and only show GPIO_RESET_RSMRST.
PS14, Line 143: .gpio0 = GPIO_NO_INVERT, Only has effect on DIR_INPUT. Could also hide GPIO_NO_INVERT using static.
PS14, Line 178: .gpio0 = GPIO_NO_BLINK, Only meaningful on DIR_OUTPUT. could also hide NO_BLINK using static declaration.
-
Arthur Heymans (Code Review)