Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40300 )
Change subject: [WIP] mainboard: add Dell Latitude E7240 ......................................................................
Patch Set 1:
(15 comments)
Patch Set 1:
Patch Set 1:
(1 comment)
No. It's interesting that I found a Boot Guard profile in it, but MEInfo reports there's no Boot Guard. By the way, the Boot Guard checking feature of intelmetool is wrong.
intelmetool is completely drunk in that regard
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 9: /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0; Already zero
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 17: // the lid is open by default. That looks like old autoport:
/* The lid is open by default */
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 14: 0x0 0
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 22: 0x0 0
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 23: end Pick these up and place them on the previous line?
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 28: 0x0 0
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 33: register "gen4_dec" = "0x00000000" Already zero, can be dropped
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 46: subsystemid 0x1028 0x05ca Inherit this?
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 90: Unsupported PCI device 8086:282a Hmmm
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 101: Host bridge Host bridge one Host bridge too much?
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 104: device pci 01.0 off # PCIe Bridge for discrete graphics : end This does not exist on HSW-LP
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 100: : device pci 00.0 on # Host bridge Host bridge : subsystemid 0x1028 0x05ca : end : device pci 01.0 off # PCIe Bridge for discrete graphics : end : device pci 02.0 on # Internal graphics VGA controller : subsystemid 0x1028 0x05ca : end : device pci 03.0 on # Mini-HD audio Unsupported PCI device 8086:0a0c : subsystemid 0x1028 0x05ca : end Move these above the "chip southbridge/intel/lynxpoint" entry?
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 1: #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB Missing license header, and these are likely not being used
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/ec.c:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 77: static int write_send(u8 *sdata, u8 c, unsigned count)
Prefer 'unsigned int' to bare use of 'unsigned'
Please address
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... File src/mainboard/dell/latitude_e7240/gpio.c:
https://review.coreboot.org/c/coreboot/+/40300/1/src/mainboard/dell/latitude... PS1, Line 8: LP_GPIO_OUT_LOW, /* 0: OUTPUT LOW */ Maybe align with tabs?