Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44037 )
Change subject: sconfig: Allow chipset to provide a base devicetree ......................................................................
Patch Set 13: Code-Review+1
(4 comments)
We are now already loading 3 device trees for one board. Do we need a more generic implementation of this? Will there be more than 3 device trees?
I thought about this too. A loop would probably also make the code clearer (no confusion why something is named `chipset_` but takes the mainboard tree). There might be more trees in the future, indeed. We're talking sometimes about how to properly support COM-Express modules and their carrier boards. We might end up with something like chipset -> module -> module variant -> carrier (-> carrier variant?)
Would be another +2 if there wasn't this odd `devnum` field ;)
https://review.coreboot.org/c/coreboot/+/44037/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44037/13//COMMIT_MSG@12 PS13, Line 12: for both the baseboard devicetree.cb as well as variant overridetree.cb. Please break at 72 chars.
https://review.coreboot.org/c/coreboot/+/44037/13/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/44037/13/util/sconfig/main.c@755 PS13, Line 755: new_d->devnum = devnum; I would very much appreciate if we'd avoid the redundancy. Either reconstruct `devnum` from `path_a.path_b` or refactor `new_device()` to take already separated `path_a` and `path_b` as parameters. It's just hard to grasp why we store both and more comments explaining it might be more work than fixing it...
https://review.coreboot.org/c/coreboot/+/44037/13/util/sconfig/main.c@1761 PS13, Line 1761: * Nit, no asterisk here.
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/sconfig.h File util/sconfig/sconfig.h:
https://review.coreboot.org/c/coreboot/+/44037/9/util/sconfig/sconfig.h@136 PS9, Line 136: devnum
Done
Would be helpful to have a comment _why_ it is ;)