Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38163 )
Change subject: sb/amd/{pi,agesa}/hudson/smbus_spd.c: use coreboot code style and calls ......................................................................
Patch Set 4:
(10 comments)
https://review.coreboot.org/c/coreboot/+/38163/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38163/4//COMMIT_MSG@9 PS4, Line 9: replace To me, "replace X with Y" is "s/X/Y" in vim-speech. I'd rather say "use coreboot-provided inb/outb/rdtsc"
https://review.coreboot.org/c/coreboot/+/38163/3/src/southbridge/amd/agesa/h... File src/southbridge/amd/agesa/hudson/smbus_spd.c:
https://review.coreboot.org/c/coreboot/+/38163/3/src/southbridge/amd/agesa/h... PS3, Line 20: /* warning: Porting.h includes an open #pragma pack(1) */
Ok.. What this does is nasty. It changes structs to __packed for the remaining of the headers. […]
#pragma cursed
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... File src/southbridge/amd/agesa/hudson/smbus_spd.c:
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 26: readSmbusByteData These things might need a rename
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 27: int offset) But this still fits in 96 chars?
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 43: limit = rdtscll() + 2000000000 / 10; This is reinventing the wheel, there should be a timer API somewhere.
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 60: if (status == 2) : status = 0; : return status; this could even be:
return status == 2 ? 0 : status;
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 80: { Please pick this up (should be on the previous line)
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 112: printk(BIOS_SPEW, "-------------READING SPD-----------\n"); Good thing nobody sees this by default (I hope)
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 139: ioBase This variable should be lowercase
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/smbus_spd.c:
PS4: Am I drunk? I think I see double. Aren't these files basically the same thing?