Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35763 )
Change subject: soc/intel/skylake: rework and fix platform detection ......................................................................
Patch Set 7:
(6 comments)
Looks like you are overdoing the original cosmetic change a bit? Please explain the problem you are trying to fix before putting any more work into it.
Patch set #5 looked rather good?
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@17 PS7, Line 17: this problem in general, depends on platform detection. I still don't see how it fails. Can you give an example for what IDs it fails?
https://review.coreboot.org/c/coreboot/+/35763/7//COMMIT_MSG@23 PS7, Line 23: misplaced ones. FSP-S isn't really the place to look for good examples.
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... File src/include/device/pci_ids.h:
https://review.coreboot.org/c/coreboot/+/35763/7/src/include/device/pci_ids.... PS7, Line 2701: #define PCI_DEVICE_ID_INTEL_SPT_LP_LPC_MASK 0x001f Why is the mask inverted?
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 114: }, we generally indent with tabs
`}, {` can stick on one line.
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 126: { This was correctly indented before. Either two tabs or aligned with the `(`. But _never_ aligned with the body.
https://review.coreboot.org/c/coreboot/+/35763/7/src/soc/intel/skylake/chip_... PS7, Line 178: if ((id & platform->lpc_mask) == platform->lpc_base) Doesn't seem to make sense given the masks provided.