Attention is currently required from: Angel Pons, Elyes Haouas, Felix Singer, Pratikkumar V Prajapati.
Máté Kukri has posted comments on this change by Elyes Haouas. ( https://review.coreboot.org/c/coreboot/+/83110?usp=email )
Change subject: inteltool/lpc.c: Add PCI_DEVICE_ID_INTEL_H67 ......................................................................
Patch Set 2:
(3 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83110/comment/27e2c6ce_6b050f3a?usp... : PS2, Line 45: 0x00e0: 0x100c0009 (PCCTL) I think it's entirely believable from the description that this actually works, and these values do not seem useful for someone looking through the commit history in the future.
File util/inteltool/lpc.c:
https://review.coreboot.org/c/coreboot/+/83110/comment/5da6e34b_d36774b8?usp... : PS1, Line 90: case PCI_DEVICE_ID_INTEL_H67:
I am not sure if I understand you correctly. […]
I think adding just one device ID is even misleading, the LPC controller is identical on all SKUs of CPT, and this suggests that H67 is somehow special.
Maybe just add all CPT SKUs, and change the commit message to "inteltool/lpc: Add CPT device IDs"
https://review.coreboot.org/c/coreboot/+/83110/comment/e35646f5_fc3eea0a?usp... : PS1, Line 120: bc = pci_read_long(dev, SUNRISE_LPC_BC);
It definitely is in scope of this patch. […]
I don't see how it is out of scope either: - Before the code was correct because it was only reading registers on hardware where it exists. - Adding this device ID makes the code incorrect by reading non-existent registers. - I would think a commit exposing a code path to different hardware should also fix such problems.