Change in coreboot[master]: soc/amd/common/espi_util: slightly refactor espi_open_io_window
Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: slightly refactor espi_open_io_window ......................................................................
soc/amd/common/espi_util: slightly refactor espi_open_io_window
Change-Id: I3039817afd79c30a2df2f2f54e7848f52dc2c487 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/lpc/espi_util.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44353/1
diff --git a/src/soc/amd/common/block/lpc/espi_util.c b/src/soc/amd/common/block/lpc/espi_util.c index c1145d0..f1afbdb 100644 --- a/src/soc/amd/common/block/lpc/espi_util.c +++ b/src/soc/amd/common/block/lpc/espi_util.c @@ -183,9 +183,9 @@ if (std_io != -1) { espi_enable_decode(std_io); return 0; + } else { + return espi_open_generic_io_window(base, size); } - - return espi_open_generic_io_window(base, size); }
static int espi_find_mmio_window(uint32_t win_base)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: slightly refactor espi_open_io_window ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44353/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/espi_util.c:
https://review.coreboot.org/c/coreboot/+/44353/1/src/soc/amd/common/block/lp... PS1, Line 186: else { else is not required since there is a return in if block.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: slightly refactor espi_open_io_window ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44353/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/espi_util.c:
https://review.coreboot.org/c/coreboot/+/44353/1/src/soc/amd/common/block/lp... PS1, Line 186: else {
else is not required since there is a return in if block.
sure, it's not required and doesn't change behavior, but i find this much clearer. from a quick look it's not obvious that the last return is the else condition of the if before
Hello build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44353
to look at the new patch set (#3).
Change subject: soc/amd/common/espi_util: slightly refactor espi_open_io_window ......................................................................
soc/amd/common/espi_util: slightly refactor espi_open_io_window
If the espi_open_generic_io_window gets run depends on the condition in the if statement before, so move the command into an else block to make it more obvious that this is the case.
TEST=Timeless build results in identical image.
Change-Id: I3039817afd79c30a2df2f2f54e7848f52dc2c487 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/lpc/espi_util.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44353/3
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: slightly refactor espi_open_io_window ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG@7 PS3, Line 7: slightly refactor espi_open_io_window No need to change it, but I might've called it "clarify" vs. "slightly refactor".
Also, unsure whether you would've meant to omit the "generic" portion of the name.
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG@9 PS3, Line 9: If the espi_open_generic_io_window gets run Haha, normally I feel like your written English is better than mine. In this case I had trouble understanding the commit message until I looked at the code.
Here, how about "Executing espi_open_generic_io_window() depends on..."
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG@10 PS3, Line 10: the if statement before Nit, something like "the preceding if statement" might be more clear to the reader. Something about "the if <anything>" initially appears to be a typo for me.
Hello Jason Glenesk, build bot (Jenkins), Raul Rangel, Furquan Shaikh, Marshall Dawson,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44353
to look at the new patch set (#4).
Change subject: soc/amd/common/espi_util: clarify espi_open_io_window ......................................................................
soc/amd/common/espi_util: clarify espi_open_io_window
Calling espi_open_generic_io_window in espi_open_io_window depends on the condition in the preceding if statement, so move the command into an else block to make it more obvious that this is the case.
TEST=Timeless build results in identical image.
Change-Id: I3039817afd79c30a2df2f2f54e7848f52dc2c487 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/soc/amd/common/block/lpc/espi_util.c 1 file changed, 2 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/44353/4
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: clarify espi_open_io_window ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG@7 PS3, Line 7: slightly refactor espi_open_io_window
No need to change it, but I might've called it "clarify" vs. "slightly refactor". […]
The function that is being refactored is espi_open_io_window; espi_open_generic_io_window gets called by the function that is being refactored here. I replaced slightly refactor with clarify
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG@9 PS3, Line 9: If the espi_open_generic_io_window gets run
Haha, normally I feel like your written English is better than mine. […]
i reworded it a bit differently, but i'd still say that it is clearer in the new version
https://review.coreboot.org/c/coreboot/+/44353/3//COMMIT_MSG@10 PS3, Line 10: the if statement before
Nit, something like "the preceding if statement" might be more clear to the reader. […]
Done
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: clarify espi_open_io_window ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44353/1/src/soc/amd/common/block/lp... File src/soc/amd/common/block/lpc/espi_util.c:
https://review.coreboot.org/c/coreboot/+/44353/1/src/soc/amd/common/block/lp... PS1, Line 186: else {
sure, it's not required and doesn't change behavior, but i find this much clearer. […]
Ack
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: clarify espi_open_io_window ......................................................................
Patch Set 4: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: clarify espi_open_io_window ......................................................................
soc/amd/common/espi_util: clarify espi_open_io_window
Calling espi_open_generic_io_window in espi_open_io_window depends on the condition in the preceding if statement, so move the command into an else block to make it more obvious that this is the case.
TEST=Timeless build results in identical image.
Change-Id: I3039817afd79c30a2df2f2f54e7848f52dc2c487 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/44353 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com --- M src/soc/amd/common/block/lpc/espi_util.c 1 file changed, 2 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved
diff --git a/src/soc/amd/common/block/lpc/espi_util.c b/src/soc/amd/common/block/lpc/espi_util.c index 1b617fc..0878fb7 100644 --- a/src/soc/amd/common/block/lpc/espi_util.c +++ b/src/soc/amd/common/block/lpc/espi_util.c @@ -183,9 +183,9 @@ if (std_io != -1) { espi_enable_decode(std_io); return 0; + } else { + return espi_open_generic_io_window(base, size); } - - return espi_open_generic_io_window(base, size); }
static int espi_find_mmio_window(uint32_t win_base)
Jason Glenesk has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44353 )
Change subject: soc/amd/common/espi_util: clarify espi_open_io_window ......................................................................
Patch Set 5: Code-Review+1
-
Felix Held (Code Review) -
Furquan Shaikh (Code Review)
-
Jason Glenesk (Code Review)
-
Marshall Dawson (Code Review)