Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42566 )
Change subject: util/amdfwtool: Add support for EFS SPI values for F17h and F15h ......................................................................
Patch Set 7: Code-Review+1
(4 comments)
Some minor nits.
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 222: speed nit: fast speed
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1245: strcmp nit: Use strcasecmp here and below?
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1254: 0 It would be good to have enums for these values. Just easier to read.
https://review.coreboot.org/c/coreboot/+/42566/7/util/amdfwtool/amdfwtool.c@... PS7, Line 1511: soc_name = optarg; Just a thought. Probably, we should do strcasecmp right away and set platform_id to some enum
enum platform { PLATFORM_STONEYRIDGE, PLATFORM_RAVEN, PLATFORM_PICASSO, PLATFORM_RENOIR, PLATFORM_LUCIENNE, };
so that rest of the code just needs to check enum and not perform string comparison again. This will allow the tool to use platform/SoC comparison in other places as well.
It's okay if you don't want to do it right now. We can address that later as well.