Attention is currently required from: Felix Singer, Hung-Te Lin, Yidi Lin, Yu-Ping Wu.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/79063?usp=email )
Change subject: google/*: Clean up Kconfg board selection for Google MTK boards ......................................................................
Patch Set 1:
(3 comments)
File src/mainboard/google/corsola/Kconfig:
https://review.coreboot.org/c/coreboot/+/79063/comment/da3df88a_ced26a31 : PS1, Line 22: select DRIVER_ANALOGIX_ANX7625 if BOARD_GOOGLE_CHINCHOU Hmm... I think I find my version a bit clearer to be honest, but it's your board so happy to switch this to your proposal if you prefer. I think the way I wrote it makes it pretty straight-forward to read:
- All the Kinglers have ANX, except for Steelix which also has Parade in addition.
- All the Krabbies have Parade, except Chinchou which has ANX instead.
- All the Staryus have MIPI.
I think it would be well set-up to be extended from there too, except if we get more boards like Chinchou which are Krabby but need the Parade, then it might be worth adding a `KRABBY_HAS_PS8640` intermediary option to make that easier to write.
I do like the `CORSOLA_HAS_xxx` options in general to organize things, but if you already have your boards split into subvariants anyway then I don't think you need to make extra options for things that already apply only to exactly one of those. So as long as all the Staryus have MIPI panels and all the other ones don't, you probably don't need a `CORSOLA_HAS_MIPI_PANEL`, you can just use `BOARD_GOOGLE_STARYU_COMMON` there (unless you anticipate that you're gonna add more non-Staryus with MIPI in the future).
Also, I think we don't usually use default-overrides for `bool` options because that's just equivalent to selecting that option from `BOARD_SPECIFIC_OPTIONS` (at least if the default normally is `n`). So if you want to organize it the way you proposed, I think rather than overriding the defaults you could just add ``` select DRIVER_ANALOGIX_ANX7625 if BOARD_GOOGLE_KINGLER_COMMON || BOARD_GOOGLE_CHINCHOU select DRIVER_PARADE_PS8640 if !(DRIVER_ANALOGIX_ANX7625 || BOARD_GOOGLE_STARYU_COMMON) || BOARD_GOOGLE_STEELIX ```
File src/mainboard/google/kukui/Kconfig:
https://review.coreboot.org/c/coreboot/+/79063/comment/ab7aab54_7a2b245d : PS1, Line 27: BOARD_GOOGLE_WILLOW
This needs to select `BOARD_GOOGLE_KUKUI_COMMON`. Here kukui is considered as the parent of jacuzzi.
I did that by adding JACUZZI_COMMON to the def_bool list for KUKUI_COMMON above. I went back and forth a bit about whether it's better that way or with a select here... I settled on that because I think it's a bit more consistent, that way the KUKUI_COMMON option alone tells you about everything it applies to and there are no surprise boards added later from other configs. But happy to switch to the select version if you prefer.
https://review.coreboot.org/c/coreboot/+/79063/comment/f90e2fff_2c0e02a4 : PS1, Line 36: select VBOOT_VBNV_FLASH
Not related to the patch, but I've always been wondering how repeated definitions of a Kconfig optio […]
In effect, yeah, but the mechanism is a little different. This basically just "extends" the original definition, and the encompassing `if` block here is equivalent to just writing `if ...` behind every line. So basically, this is equivalent to adding a `select VBOOT_VBNV_FLASH if BOARD_GOOGLE_KUKUI_COMMON` to the `config VBOOT` definition in `src/security/vboot/Kconfig`.