Attention is currently required from: Patrick Rudolph. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49218 )
Change subject: [WIP]device/Kconfig: Introduce separate graphics menu for Aspeed ......................................................................
Patch Set 1:
(5 comments)
Patchset:
PS1: Looks good overall. I didn't expect the VGA_ROM_RUN as part of the per-device choices, but I like it. :)
File src/device/graphics/aspeed/Kconfig:
https://review.coreboot.org/c/coreboot/+/49218/comment/6dabe521_46e2bece PS1, Line 2: Put an `if DRIVERS_ASPEED_AST_COMMON` around everything or the `source` line?
https://review.coreboot.org/c/coreboot/+/49218/comment/60f5021d_d56a9f73 PS1, Line 10: config MAINBOARD_FORCE_NATIVE_ASPEED_VGA_INIT : def_bool n : depends on MAINBOARD_HAS_NATIVE_ASPEED_VGA_INIT : help : Selected by mainboards / chipsets whose graphics driver can't or : shouldn't be disabled. I don't think this is needed. Do you have a specific use-case in mind?
IIRC, the original MAINBOARD_FORCE_NATIVE_VGA_INIT was only introduced for cases where the code is written such that we don't have a way to disable it. And it was only selected by the ASpeed driver by accident for bad advertisement.
https://review.coreboot.org/c/coreboot/+/49218/comment/9906157c_d52f4c96 PS1, Line 34: depends on VGA_ROM_RUN I assume this would be changed to `select` later when VGA_ROM_RUN is not part of a choice anymore?
https://review.coreboot.org/c/coreboot/+/49218/comment/03d25d76_adf2b572 PS1, Line 36: FIXME: This won't work unless VGA_ROM_RUN is selected as well The `depends on` already makes sure that it won't show up without VGA_ROM_RUN.