Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41630 )
Change subject: soc/amd/picasso: rewrite soc_util ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41630/1/src/soc/amd/picasso/soc_uti... File src/soc/amd/picasso/soc_util.c:
https://review.coreboot.org/c/coreboot/+/41630/1/src/soc/amd/picasso/soc_uti... PS1, Line 108: static enum silicon_type get_silicon_type(void)
Will we ever have a need to switch on the SoC types outside of this file? If so, maybe we could make […]
switch the soc type? this function might be made non-static and added to the header file if some other part of the code wants to consume this information directly. currently the code only uses the soc_is_* functions
https://review.coreboot.org/c/coreboot/+/41630/1/src/soc/amd/picasso/soc_uti... PS1, Line 115: silicon = SILICON_RV2;
I prefer multiple return statements when the returned variable is meant to be changed only once. […]
Hmm, ok, you got a point on the multiple return thing.
I consider if statements without braces an avoidable risk and would vote for disallowing them.
-
Felix Held (Code Review)