Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38698 )
Change subject: vc/amd/fsp/picasso: Add PCIe and DDI helpers ......................................................................
Patch Set 1:
(1 comment)
do you want to update the patch or should i take over from here?
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... File src/vendorcode/amd/fsp/picasso/platform_descriptors.h:
https://review.coreboot.org/c/coreboot/+/38698/1/src/vendorcode/amd/fsp/pica... PS1, Line 37: GEN_MAX, // Maximum supported
It sometimes seems to depend on who's reviewing. I believe I've seen that requested to be omitted. OTOH, maybe there's a better case for clarity here since the info is shared between coreboot and FSP.
When I see enums without at least one explicit value assignemnt, I'd assume that only the symbolic names and not the underlying raw values are used/matter. But yeah, different reviewers might have different opinions on things.
Hmm, the values mixed hex and decimal numbers above. I don't recall now if this was originally copy/pasted out of AGESA.
Oh, missed that one; just making all values hex values might be a good idea. Also aligning the = signs with tabs slightly improves readability; especially since the comments are already tab-aligned