Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36087 )
Change subject: soc/intel/tigerlake: Do initial SoC commit ......................................................................
Patch Set 8: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36087/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36087/8//COMMIT_MSG@9 PS8, Line 9: Clone entirely from Icelake No. Please don't do this.
Either not much differs between ICL and TGL so most of the copied code becomes a duplicate, or so much differs between ICL and TGL that the final result is completely different from the copied code, so why copy it in the first place?
Moreover, it is impossible to perform an effective code-review of such a large commit. Most of the time, such commits get "rubber-stamped": it gets a positive code-review score just because it works, or the reviewer trusts the author; but it does not get a proper review. And then, it gets merged.
After some years, nobody knows what the code from that large commit is supposed to do, nor why it does things that don't seem to make any sense, nor whether the code is actually correct. This ends up with the code becoming an unmaintainable nightmare: code rot. This has happened several times before.
So, since large commits result in large technical debt, what could be done instead? For example: how about breaking down the introduction of a new SoC more like the SoC itself is structured? For example, a commit could add TGL SATA support, another one could add TGL SMBus, another one, TGL UART support... These commits would be of a reasonable size to be reviewed effectively, which would make mistakes easier to spot.
I know, the structure for a new SoC is more or less the same so it makes sense to copy an older one to start off, but this doesn't mean that a commit for the copy should be created.