Attention is currently required from: Bao Zheng, Jason Nien, Martin Roth, Matt DeVillier, Matt DeVillier, Zheng Bao.
Jon Murphy has posted comments on this change by Bao Zheng. ( https://review.coreboot.org/c/coreboot/+/83646?usp=email )
Change subject: mb/google/skyrim: Combine the variants function ......................................................................
Patch Set 1:
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/83646/comment/25696771_16eb1c73?usp... : PS1, Line 9: BUG=b:346716300 I don't understand how this change relates to this bug
File src/mainboard/google/skyrim/variants/baseboard/port_descriptors_combo.c:
PS1:
having two port_descriptors files in the baseboard doesn't make much sense to me. […]
Agree that having duplicate code is not a good thing. I am a little hesitant about this change. I agree with Matt that having two files starts to feel confusing, but I also think naming things combo gets confusing since each variant needs to define the dxio descriptors and it's arbitrary what combo means. Unlikely that either of these variants would change, but they do differ from the rest.
The other variants have SD, NVMe, and WLAN, maybe rather than common you could make a WLAN_EMMC config and use a kconfig as Matt suggested.
In general I want to make sure it's clear what's happening, that we avoid escapes in the future, and agree that we should limit redundant copy/paste code.
In addition to the variant function, having the dxio descriptors guarded by a config and/or in a separate file avoids wasted memory allocation for a second unused array.