Michał Żygowski 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 5:
(12 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. […]
Done
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) */
#pragma cursed
Left a comment and AGESA.h include in the same place where Porting.h was
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
Did some renaming from FoObAr to foo_bar
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?
Still, I'm not used to it.
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.
There is in lib/timer.h. Even the wait_us/ms macro looks pretty nice for this use, however, I'm bad at using conditions in macros.
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: […]
Done
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)
Done
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)
It should be in RAM still land available in cbmem
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/agesa/h... PS4, Line 139: ioBase
This variable should be lowercase
Done
https://review.coreboot.org/c/coreboot/+/38163/3/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/smbus_spd.c:
https://review.coreboot.org/c/coreboot/+/38163/3/src/southbridge/amd/pi/huds... PS3, Line 21: /* warning: Porting.h includes an open #pragma pack(1) */
Keep comment
Done
https://review.coreboot.org/c/coreboot/+/38163/3/src/southbridge/amd/pi/huds... PS3, Line 23: #include <northbridge/amd/agesa/dimmSpd.h>
pi
Done
https://review.coreboot.org/c/coreboot/+/38163/4/src/southbridge/amd/pi/huds... File src/southbridge/amd/pi/hudson/smbus_spd.c:
PS4:
I am saying that src/southbridge/amd/{agesa,pi}/hudson/smbus_spd.c seem to be the same thing.
yes it is (except the dimmSpd.h include)