Change in coreboot[master]: mb/google/zork: Fix get board version and get fw config issue
EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
mb/google/zork: Fix get board version and get fw config issue
google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_board_version return 0 as sucess. Let's correct the logical for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/1
diff --git a/src/mainboard/google/zork/variants/baseboard/helpers.c b/src/mainboard/google/zork/variants/baseboard/helpers.c index d95ab82..9b824e1 100644 --- a/src/mainboard/google/zork/variants/baseboard/helpers.c +++ b/src/mainboard/google/zork/variants/baseboard/helpers.c @@ -57,7 +57,7 @@ return 0; }
- if (google_chromeec_cbi_get_fw_config(&known_value)) { + if (google_chromeec_cbi_get_fw_config(&known_value) !=0) { printk(BIOS_ERR, "FW_CONFIG not set in CBI\n"); return -1; } @@ -95,7 +95,7 @@ if (!CONFIG(VARIANT_SUPPORTS_PRE_V3_SCHEMATICS)) return true;
- if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) return false;
if ((int)board_version < CONFIG_VARIANT_MIN_BOARD_ID_V3_SCHEMATICS) @@ -111,7 +111,7 @@ if (!CONFIG(VARIANT_SUPPORTS_PRE_V3_6_SCHEMATICS)) return true;
- if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) return false;
if ((int)board_version < CONFIG_VARIANT_MIN_BOARD_ID_V3_6_SCHEMATICS) @@ -136,7 +136,7 @@ if (!CONFIG(VARIANT_SUPPORTS_WIFI_POWER_ACTIVE_HIGH)) return true;
- if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) return false;
if ((int)board_version < CONFIG_VARIANT_MIN_BOARD_ID_WIFI_POWER_ACTIVE_LOW) diff --git a/src/mainboard/google/zork/variants/berknip/gpio.c b/src/mainboard/google/zork/variants/berknip/gpio.c index c8cb10f..53ae71c 100644 --- a/src/mainboard/google/zork/variants/berknip/gpio.c +++ b/src/mainboard/google/zork/variants/berknip/gpio.c @@ -61,7 +61,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version <= 1) { diff --git a/src/mainboard/google/zork/variants/dalboz/gpio.c b/src/mainboard/google/zork/variants/dalboz/gpio.c index 7600d4c..ef04341 100644 --- a/src/mainboard/google/zork/variants/dalboz/gpio.c +++ b/src/mainboard/google/zork/variants/dalboz/gpio.c @@ -47,7 +47,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version < 2) { diff --git a/src/mainboard/google/zork/variants/dirinboz/gpio.c b/src/mainboard/google/zork/variants/dirinboz/gpio.c index 8da85b6..0a3c0fc 100644 --- a/src/mainboard/google/zork/variants/dirinboz/gpio.c +++ b/src/mainboard/google/zork/variants/dirinboz/gpio.c @@ -36,7 +36,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version < 2) { diff --git a/src/mainboard/google/zork/variants/ezkinil/gpio.c b/src/mainboard/google/zork/variants/ezkinil/gpio.c index 0ec1ab23..047c3e6 100644 --- a/src/mainboard/google/zork/variants/ezkinil/gpio.c +++ b/src/mainboard/google/zork/variants/ezkinil/gpio.c @@ -81,7 +81,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version <= 1) { diff --git a/src/mainboard/google/zork/variants/morphius/gpio.c b/src/mainboard/google/zork/variants/morphius/gpio.c index 8da0de4..bbfde2a 100644 --- a/src/mainboard/google/zork/variants/morphius/gpio.c +++ b/src/mainboard/google/zork/variants/morphius/gpio.c @@ -69,7 +69,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version <= 1) { diff --git a/src/mainboard/google/zork/variants/trembyle/gpio.c b/src/mainboard/google/zork/variants/trembyle/gpio.c index 9931479..eb5c28e 100644 --- a/src/mainboard/google/zork/variants/trembyle/gpio.c +++ b/src/mainboard/google/zork/variants/trembyle/gpio.c @@ -60,7 +60,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version <= 2) { diff --git a/src/mainboard/google/zork/variants/vilboz/gpio.c b/src/mainboard/google/zork/variants/vilboz/gpio.c index 12b303a..2ed878c 100644 --- a/src/mainboard/google/zork/variants/vilboz/gpio.c +++ b/src/mainboard/google/zork/variants/vilboz/gpio.c @@ -25,7 +25,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 1;
if (board_version < 2) { diff --git a/src/mainboard/google/zork/variants/woomax/gpio.c b/src/mainboard/google/zork/variants/woomax/gpio.c index ebbcbea..8da0742 100644 --- a/src/mainboard/google/zork/variants/woomax/gpio.c +++ b/src/mainboard/google/zork/variants/woomax/gpio.c @@ -60,7 +60,7 @@ uint32_t board_version;
/* If board version cannot be read, assume it is board_version 0. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) !=0) board_version = 0;
if (board_version == 0) {
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
Patch Set 1:
(12 comments)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 60: if (google_chromeec_cbi_get_fw_config(&known_value) !=0) { spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 98: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 114: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 139: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/berknip/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 64: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dalboz/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 50: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/dirinboz/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 39: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/ezkinil/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 84: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/morphius/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 72: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/trembyle/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 63: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/vilboz/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 28: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/woomax/gpio.c:
https://review.coreboot.org/c/coreboot/+/46566/1/src/mainboard/google/zork/v... PS1, Line 63: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
Patch Set 1:
I think we need fix this for all boards.
Hello Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#2).
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
mb/google/zork: Fix get board version and get fw config issue
google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_board_version return 0 as sucess. Let's correct the logical for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/2/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/2/src/mainboard/google/zork/v... PS2, Line 98: if (google_chromeec_cbi_get_board_version(&board_version) !=0) spaces required around that '!=' (ctx:WxV)
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#3).
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
mb/google/zork: Fix get board version and get fw config issue
google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_board_version return 0 as sucess. Let's correct the logical for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/3
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Fix get board version and get fw config issue ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46566/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/1//COMMIT_MSG@9 PS1, Line 9: google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_board_version Aren't these the same function?
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... PS3, Line 60: google_chromeec_cbi_get_fw_config(&known_value) Sorry, I think I am missing something here. Isn't this really checking the same thing just without an explicit `!= 0`?
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#4).
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
mb/google/zork: Correct get board version and get fw config condition
google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_board_version return 0 as success. Let's correct the logic for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/4
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#5).
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
mb/google/zork: Correct get board version and get fw config condition
google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config return 0 as success. Let's correct the logic for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/5
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... PS3, Line 60: google_chromeec_cbi_get_fw_config(&known_value)
Sorry, I think I am missing something here. […]
This actual called cbi_get_unit32 like get_board_id so the same logic.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... PS3, Line 60: google_chromeec_cbi_get_fw_config(&known_value)
This actual called cbi_get_unit32 like get_board_id so the same logic.
Oh, it is the same but it can help the reader be more clear for the return. Because I saw dalboz/variant.c did this as well.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... PS3, Line 60: google_chromeec_cbi_get_fw_config(&known_value)
Oh, it is the same but it can help the reader be more clear for the return. […]
This CL reviewed by you as well. https://review.coreboot.org/c/coreboot/+/44421 😊
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46566/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/1//COMMIT_MSG@9 PS1, Line 9: google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_board_version
Aren't these the same function?
Done
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... File src/mainboard/google/zork/variants/baseboard/helpers.c:
https://review.coreboot.org/c/coreboot/+/46566/3/src/mainboard/google/zork/v... PS3, Line 60: google_chromeec_cbi_get_fw_config(&known_value)
This CL reviewed by you as well. https://review.coreboot. […]
If you don't like this, I can remove...
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
Patch Set 5:
This based on if we know the return value 0 as success and otherwise failed. It's pretty common to use the below statement: if (rv != 0) return rv; But I checked the chromeec/ec.c. Looks like there are no consistent rule on this coding style. Some people check '!= 0' some didn't. What's you prefer?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Correct get board version and get fw config condition ......................................................................
Patch Set 5:
Patch Set 5:
This based on if we know the return value 0 as success and otherwise failed. It's pretty common to use the below statement: if (rv != 0) return rv; But I checked the chromeec/ec.c. Looks like there are no consistent rule on this coding style. Some people check '!= 0' some didn't. What's you prefer?
I don't think there is a consistent rule about it. I have seen both usages within coreboot. I am not against this change, but I think the commit message would need some work. This change doesn't really "correct get board version". It is only a change for readability. So, the commit message should reflect that correctly.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#6).
Change subject: mb/google/zork: Refactor cbi relative functions ......................................................................
mb/google/zork: Refactor cbi relative functions
Since google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config both called cbi_get_unit32 and return 0 as success. Let's add more readability for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/6
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Refactor cbi relative functions ......................................................................
Patch Set 6:
Patch Set 5:
Patch Set 5:
This based on if we know the return value 0 as success and otherwise failed. It's pretty common to use the below statement: if (rv != 0) return rv; But I checked the chromeec/ec.c. Looks like there are no consistent rule on this coding style. Some people check '!= 0' some didn't. What's you prefer?
I don't think there is a consistent rule about it. I have seen both usages within coreboot. I am not against this change, but I think the commit message would need some work. This change doesn't really "correct get board version". It is only a change for readability. So, the commit message should reflect that correctly.
How about this? Yes, I think if we could consistent in this project, it won't confuse the people who not familiar with those functions. Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Refactor cbi relative functions ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/6//COMMIT_MSG@7 PS6, Line 7: Refactor cbi relative functions Probably say "Add explicit check for non-zero return value of cbi functions"?
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#7).
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
mb/google/zork: Add explicit check for non-zero return value of cbi functions
Since google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config both called cbi_get_unit32 and return 0 as success. Let's add more readability for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/7
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/6//COMMIT_MSG@7 PS6, Line 7: Refactor cbi relative functions
Probably say "Add explicit check for non-zero return value of cbi functions"?
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG@10 PS7, Line 10: called Present tense: call
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG@9 PS7, Line 9: Since google_chromeec_cbi_get_board_version and : google_chromeec_cbi_get_fw_config both called cbi_get_unit32 and : return 0 as success. The sentence should continue somehow?
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#8).
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
mb/google/zork: Add explicit check for non-zero return value of cbi functions
Since google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config both call cbi_get_unit32 and return 0 as success. Let's add more readability for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/8
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
Patch Set 8:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG@10 PS7, Line 10: called
Present tense: call
Done
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG@9 PS7, Line 9: Since google_chromeec_cbi_get_board_version and : google_chromeec_cbi_get_fw_config both called cbi_get_unit32 and : return 0 as success.
The sentence should continue somehow?
I didn't get you.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#9).
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
mb/google/zork: Add explicit check for non-zero return value of cbi functions
Since google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config both call cbi_get_unit32 and return 0 as success, non-zero as failure. Let's add more readability for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/9
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/7//COMMIT_MSG@9 PS7, Line 9: Since google_chromeec_cbi_get_board_version and : google_chromeec_cbi_get_fw_config both called cbi_get_unit32 and : return 0 as success.
I didn't get you.
Done
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Add explicit check for non-zero return value of cbi functions ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/9//COMMIT_MSG@7 PS9, Line 7: mb/google/zork: Add explicit check for non-zero return value of cbi functions Could you shorten this line?
The subject line of the commit message is preferably fewer than 65 characters. The absolute maximum is 75.
Maybe:
mb/google/zork: Update style of check on cbi return values
or something like that.
Hello build bot (Jenkins), Martin Roth, Furquan Shaikh, Tim Wawrzynczak,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46566
to look at the new patch set (#10).
Change subject: mb/google/zork: Update style of check on cbi return values ......................................................................
mb/google/zork: Update style of check on cbi return values
Since google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config both call cbi_get_unit32 and return 0 as success, non-zero as failure. Let's add more readability for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/46566/10
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Update style of check on cbi return values ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46566/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46566/9//COMMIT_MSG@7 PS9, Line 7: mb/google/zork: Add explicit check for non-zero return value of cbi functions
Could you shorten this line? […]
Thank you!
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Update style of check on cbi return values ......................................................................
Patch Set 10: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46566 )
Change subject: mb/google/zork: Update style of check on cbi return values ......................................................................
mb/google/zork: Update style of check on cbi return values
Since google_chromeec_cbi_get_board_version and google_chromeec_cbi_get_fw_config both call cbi_get_unit32 and return 0 as success, non-zero as failure. Let's add more readability for the false condition.
BUG=None TEST=check with empty CBI value
Signed-off-by: Eric Lai ericr_lai@compal.corp-partner.google.com Change-Id: Ia49ac1ee35302f8f6afe8c0eb8e13afdf36c5b2b Reviewed-on: https://review.coreboot.org/c/coreboot/+/46566 Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/zork/variants/baseboard/helpers.c M src/mainboard/google/zork/variants/berknip/gpio.c M src/mainboard/google/zork/variants/dalboz/gpio.c M src/mainboard/google/zork/variants/dirinboz/gpio.c M src/mainboard/google/zork/variants/ezkinil/gpio.c M src/mainboard/google/zork/variants/morphius/gpio.c M src/mainboard/google/zork/variants/trembyle/gpio.c M src/mainboard/google/zork/variants/vilboz/gpio.c M src/mainboard/google/zork/variants/woomax/gpio.c 9 files changed, 12 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/mainboard/google/zork/variants/baseboard/helpers.c b/src/mainboard/google/zork/variants/baseboard/helpers.c index d95ab82..cc07fe1 100644 --- a/src/mainboard/google/zork/variants/baseboard/helpers.c +++ b/src/mainboard/google/zork/variants/baseboard/helpers.c @@ -57,7 +57,7 @@ return 0; }
- if (google_chromeec_cbi_get_fw_config(&known_value)) { + if (google_chromeec_cbi_get_fw_config(&known_value) != 0) { printk(BIOS_ERR, "FW_CONFIG not set in CBI\n"); return -1; } @@ -95,7 +95,7 @@ if (!CONFIG(VARIANT_SUPPORTS_PRE_V3_SCHEMATICS)) return true;
- if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) return false;
if ((int)board_version < CONFIG_VARIANT_MIN_BOARD_ID_V3_SCHEMATICS) @@ -111,7 +111,7 @@ if (!CONFIG(VARIANT_SUPPORTS_PRE_V3_6_SCHEMATICS)) return true;
- if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) return false;
if ((int)board_version < CONFIG_VARIANT_MIN_BOARD_ID_V3_6_SCHEMATICS) @@ -136,7 +136,7 @@ if (!CONFIG(VARIANT_SUPPORTS_WIFI_POWER_ACTIVE_HIGH)) return true;
- if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) return false;
if ((int)board_version < CONFIG_VARIANT_MIN_BOARD_ID_WIFI_POWER_ACTIVE_LOW) diff --git a/src/mainboard/google/zork/variants/berknip/gpio.c b/src/mainboard/google/zork/variants/berknip/gpio.c index c8cb10f..ae2d02a 100644 --- a/src/mainboard/google/zork/variants/berknip/gpio.c +++ b/src/mainboard/google/zork/variants/berknip/gpio.c @@ -61,7 +61,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version <= 1) { diff --git a/src/mainboard/google/zork/variants/dalboz/gpio.c b/src/mainboard/google/zork/variants/dalboz/gpio.c index 7600d4c..2b46938 100644 --- a/src/mainboard/google/zork/variants/dalboz/gpio.c +++ b/src/mainboard/google/zork/variants/dalboz/gpio.c @@ -47,7 +47,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version < 2) { diff --git a/src/mainboard/google/zork/variants/dirinboz/gpio.c b/src/mainboard/google/zork/variants/dirinboz/gpio.c index 8da85b6..7f9582c 100644 --- a/src/mainboard/google/zork/variants/dirinboz/gpio.c +++ b/src/mainboard/google/zork/variants/dirinboz/gpio.c @@ -36,7 +36,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version < 2) { diff --git a/src/mainboard/google/zork/variants/ezkinil/gpio.c b/src/mainboard/google/zork/variants/ezkinil/gpio.c index 0ec1ab23..d2dd104 100644 --- a/src/mainboard/google/zork/variants/ezkinil/gpio.c +++ b/src/mainboard/google/zork/variants/ezkinil/gpio.c @@ -81,7 +81,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version <= 1) { diff --git a/src/mainboard/google/zork/variants/morphius/gpio.c b/src/mainboard/google/zork/variants/morphius/gpio.c index 8da0de4..9b36e37 100644 --- a/src/mainboard/google/zork/variants/morphius/gpio.c +++ b/src/mainboard/google/zork/variants/morphius/gpio.c @@ -69,7 +69,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version <= 1) { diff --git a/src/mainboard/google/zork/variants/trembyle/gpio.c b/src/mainboard/google/zork/variants/trembyle/gpio.c index 9931479..4d73ea0 100644 --- a/src/mainboard/google/zork/variants/trembyle/gpio.c +++ b/src/mainboard/google/zork/variants/trembyle/gpio.c @@ -60,7 +60,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version <= 2) { diff --git a/src/mainboard/google/zork/variants/vilboz/gpio.c b/src/mainboard/google/zork/variants/vilboz/gpio.c index 12b303a..91ed61a 100644 --- a/src/mainboard/google/zork/variants/vilboz/gpio.c +++ b/src/mainboard/google/zork/variants/vilboz/gpio.c @@ -25,7 +25,7 @@ * and so apply overrides. If board version is provided by the EC, then apply overrides * if version < 2. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 1;
if (board_version < 2) { diff --git a/src/mainboard/google/zork/variants/woomax/gpio.c b/src/mainboard/google/zork/variants/woomax/gpio.c index ebbcbea..cb98df7 100644 --- a/src/mainboard/google/zork/variants/woomax/gpio.c +++ b/src/mainboard/google/zork/variants/woomax/gpio.c @@ -60,7 +60,7 @@ uint32_t board_version;
/* If board version cannot be read, assume it is board_version 0. */ - if (google_chromeec_cbi_get_board_version(&board_version)) + if (google_chromeec_cbi_get_board_version(&board_version) != 0) board_version = 0;
if (board_version == 0) {
-
build bot (Jenkins) (Code Review) -
EricR Lai (Code Review)
-
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)