Nikolai Vyssotski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42859 )
Change subject: amdfwtool: Take a config file instead of command line parameters ......................................................................
Patch Set 65:
(4 comments)
I like you approach on resolving the dependency issues. If DEP_FILES is placed in the correct place it works well. I tested it on my system for Mandolin target.
https://review.coreboot.org/c/coreboot/+/42859/63/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/63/src/soc/amd/picasso/Makefi... PS63, Line 265: $(AMDFWTOOL) \
Adding a option to amdfwtool to make a list of FW files can solve this problem. […]
Looks good to me. You probably want to add DEP_FILES here and not to AMDFW_COMMON_ARGS
https://review.coreboot.org/c/coreboot/+/42859/65/src/soc/amd/picasso/Makefi... File src/soc/amd/picasso/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/42859/65/src/soc/amd/picasso/Makefi... PS65, Line 259: $(DEP_FILES) \ This does not make sense to me. Why would we want to add dependent files to arguments of AMDFWTOOL. Did you mean to add it to the dependency expression below?
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/data_parse.... PS65, Line 401: fprintf(stderr, "Modules name "%s" is not valid\n", oneline);
line over 96 characters
Nicely done. Did you mean "Module name" or "Module's name"?
https://review.coreboot.org/c/coreboot/+/42859/65/util/amdfwtool/data_parse.... PS65, Line 403: } else {
else is not generally useful after a break or return
I'm with bot on this one. Makes it easier on the eyes.