Hi Martin, Martin Roth via coreboot wrote: > https://review.coreboot.org/c/coreboot/+/63754 .. > It's not a complete solution, but hopefully it's seen as a step in > the right direction.
Thanks a lot for this. I think that this really helps with visibility while guiding people at the same time. Great idea! I have some comments: (apologies for not using gerrit) * Managing expectations "Run the command 'git checkout $(CONFIG_MAINBOARD_BRANCH)' to build this board." The git command doesn't build anything, so maybe ".. to switch branch." or ".. first and then re-run Kconfig to set up your board." or somesuch? * Most of src/Kconfig now seems excluded for branch boards, maybe: if MAINBOARD_SUPPORTED_ON_BRANCH source "src/mainboard/Kconfig" source "site-local/Kconfig" else # everything endif rather than multiple if-endif statements is less confusing? (Yes, the duplicated source lines aren't great, but maybe okay for two lines?) * Branch names Newer git branch names look like "4.11_branch" so src/Kconfig and Kconfig.branch saying "(supported on 4.11 branch)" could really trap people, especially with "4.11" being a valid git tag. But including the branch name also in the bool description is already redundant and could conflict with select _ON_BRANCH_*. I really like the comment idea in src/Kconfig to connect _ON_BRANCH_* configs with the branch name only in a single place! Do you see any way to avoid all the "(supported on * branch)" instances, while still making the information available in the vendor menu? Hmm. * Having both Kconfig.name + Kconfig.branch The idea is for SHOW_BRANCH_SUPPORTED_PLATFORMS to gate board visibility in Kconfig for "forward-movers", right? (Maybe name it _SUPPORTED_BOARDS btw.?) IIUC the "if SHOW_BRANCH_SUPPORTED_PLATFORMS" logic would need to sometimes go into Kconfig.name (all boards on branches) and other times into Kconfig.branch (only some boards on branches), that seems a bit fiddly. Might it be better to only use Kconfig.name and not introduce a new Kconfig.branch? And maybe it would even be better to introduce this SHOW_BRANCH_SUPPORTED_PLATFORMS in a commit of its own? Thanks again and kind regards //Peter _______________________________________________ coreboot mailing list -- [email protected] To unsubscribe send an email to [email protected]
