Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39178 )
Change subject: sb/amd: Move smbus to common place ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/agesa/h... File src/southbridge/amd/agesa/hudson/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/agesa/h... PS7, Line 5: romstage-y += smbus_spd.c ../../common/smbus.c I would avoid using `..` here. Instead, how about making a Kconfig symbol under sb/amd/common/ that, when selected, enables this file to be built?
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/agesa/h... File src/southbridge/amd/agesa/hudson/hudson.c:
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/agesa/h... PS7, Line 26: #include "southbridge/amd/common/smbus.h" Shouldn't this be using <> ?
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/cimx/sb... File src/southbridge/amd/cimx/sb800/smbus.c:
PS7: Is this file the exact same thing as sb/amd/common/smbus.c ?
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/common/... File src/southbridge/amd/common/smbus.h:
https://review.coreboot.org/c/coreboot/+/39178/7/src/southbridge/amd/common/... PS7, Line 16: _ I would reorder the elements so that it matches the path. Also, why only one underscore?
I suggest using either `SB_AMD_COMMON_SMBUS_H` or `_SB_AMD_COMMON_SMBUS_H_`