Hello Wisley Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/34653
to review the following change.
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
mb/google/octopus: Add custom SAR value for Vortininja
Vortininja needs different SAR values than meep. Use sku-id to load SAR values.
BUG=b:138261454 BRANCH=octopus TEST=build and verified SAR values by sku id
Change-Id: I7b3ab51e1d6cada4faaba1b9d72bd9eacf6b04dd Signed-off-by: Wisley Chen wisley.chen@quanta.corp-partner.google.com --- M src/mainboard/google/octopus/variants/meep/include/variant/sku.h M src/mainboard/google/octopus/variants/meep/variant.c 2 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/34653/1
diff --git a/src/mainboard/google/octopus/variants/meep/include/variant/sku.h b/src/mainboard/google/octopus/variants/meep/include/variant/sku.h index 1fd17ef..e0865ed 100644 --- a/src/mainboard/google/octopus/variants/meep/include/variant/sku.h +++ b/src/mainboard/google/octopus/variants/meep/include/variant/sku.h @@ -17,7 +17,10 @@ #define __MAINBOARD_SKU_H__
enum { - + SKU_4_VORTININJA = 4, /* Stylus + rear camera */ + SKU_5_VORTININJA = 5, /* Stylus + no rear camera */ + SKU_6_VORTININJA = 6, /* Stylus + camera */ + SKU_7_VORTININJA = 7, /* no Stylus + no rear camera */ SKU_33_DORP = 33, /* HDMI */ SKU_34_DORP = 34, /* HDMI+Kblight */ SKU_35_DORP = 35, /* HDMI+TS */ diff --git a/src/mainboard/google/octopus/variants/meep/variant.c b/src/mainboard/google/octopus/variants/meep/variant.c index 71e6eb4..4296d62 100644 --- a/src/mainboard/google/octopus/variants/meep/variant.c +++ b/src/mainboard/google/octopus/variants/meep/variant.c @@ -17,6 +17,23 @@ #include <drivers/intel/gma/opregion.h> #include <baseboard/variants.h> #include <variant/sku.h> +#include <sar.h> + +const char *get_wifi_sar_cbfs_filename(void) +{ + const char *filename = NULL; + uint32_t sku_id = get_board_sku(); + + switch (sku_id) { + case SKU_4_VORTININJA: + case SKU_5_VORTININJA: + case SKU_6_VORTININJA: + case SKU_7_VORTININJA: + filename = "wifi_sar-vortininja.hex"; + return filename; + } + return filename; +}
const char *mainboard_vbt_filename(void) {
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/variant.c:
https://review.coreboot.org/c/coreboot/+/34653/1/src/mainboard/google/octopu... PS1, Line 33: return filename; Why is this separate return statement needed? The next statement would be used.
Hello Karthikeyan Ramasubramanian, Marco Chen, Wisley Chen, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34653
to look at the new patch set (#2).
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
mb/google/octopus: Add custom SAR value for Vortininja
Vortininja needs different SAR values than meep. Use sku-id to load SAR values.
BUG=b:138261454 BRANCH=octopus TEST=build and verified SAR values by sku id
Change-Id: I7b3ab51e1d6cada4faaba1b9d72bd9eacf6b04dd Signed-off-by: Wisley Chen wisley.chen@quanta.corp-partner.google.com --- M src/mainboard/google/octopus/variants/meep/include/variant/sku.h M src/mainboard/google/octopus/variants/meep/variant.c 2 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/34653/2
Chen Wisley has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/variant.c:
https://review.coreboot.org/c/coreboot/+/34653/1/src/mainboard/google/octopu... PS1, Line 33: return filename;
Why is this separate return statement needed? The next statement would be used.
I removed the return statement, and used break.
Chen Wisley has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 2:
Please help to review again.
Thanks.
Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/include/variant/sku.h:
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... PS2, Line 22: SKU_6_VORTININJA = 6, /* Stylus + camera */ Presumably this is no stylus + rear camera? Otherwise this reads the same as sku 4 to me.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/include/variant/sku.h:
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... PS2, Line 22: SKU_6_VORTININJA = 6, /* Stylus + camera */
Presumably this is no stylus + rear camera? […]
Yes, and for Googlers as the reviewer you can always refer to go/octopus-skus where list the info of SKU IDs. And SKU ID 6 is with "no stylus + rear camera"
Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/include/variant/sku.h:
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... PS2, Line 22: SKU_6_VORTININJA = 6, /* Stylus + camera */
Yes, and for Googlers as the reviewer you can always refer to go/octopus-skus where list the info of […]
I added those entries to that sheet after seeing this change :)
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/include/variant/sku.h:
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... PS2, Line 22: SKU_6_VORTININJA = 6, /* Stylus + camera */
I added those entries to that sheet after seeing this change :)
Ah~ My fault and thanks for the update. Then the source of truth would be in b/138177049#comment5 .
Hello Karthikeyan Ramasubramanian, Marco Chen, Wisley Chen, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34653
to look at the new patch set (#3).
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
mb/google/octopus: Add custom SAR value for Vortininja
Vortininja needs different SAR values than meep. Use sku-id to load SAR values.
BUG=b:138261454 BRANCH=octopus TEST=build and verified SAR values by sku id
Change-Id: I7b3ab51e1d6cada4faaba1b9d72bd9eacf6b04dd Signed-off-by: Wisley Chen wisley.chen@quanta.corp-partner.google.com --- M src/mainboard/google/octopus/variants/meep/include/variant/sku.h M src/mainboard/google/octopus/variants/meep/variant.c 2 files changed, 21 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/53/34653/3
Chen Wisley has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/include/variant/sku.h:
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... PS2, Line 22: SKU_6_VORTININJA = 6, /* Stylus + camera */
Ah~ My fault and thanks for the update. Then the source of truth would be in b/138177049#comment5 .
I fixed the description. Please help review again
Chen Wisley has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/include/variant/sku.h:
https://review.coreboot.org/c/coreboot/+/34653/2/src/mainboard/google/octopu... PS2, Line 22: SKU_6_VORTININJA = 6, /* Stylus + camera */
I fixed the description. […]
Done
Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 3: Code-Review+2
Chen Wisley has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34653/1/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/meep/variant.c:
https://review.coreboot.org/c/coreboot/+/34653/1/src/mainboard/google/octopu... PS1, Line 33: return filename;
I removed the return statement, and used break.
Done
Furquan Shaikh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34653 )
Change subject: mb/google/octopus: Add custom SAR value for Vortininja ......................................................................
mb/google/octopus: Add custom SAR value for Vortininja
Vortininja needs different SAR values than meep. Use sku-id to load SAR values.
BUG=b:138261454 BRANCH=octopus TEST=build and verified SAR values by sku id
Change-Id: I7b3ab51e1d6cada4faaba1b9d72bd9eacf6b04dd Signed-off-by: Wisley Chen wisley.chen@quanta.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34653 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Justin TerAvest teravest@chromium.org --- M src/mainboard/google/octopus/variants/meep/include/variant/sku.h M src/mainboard/google/octopus/variants/meep/variant.c 2 files changed, 21 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Justin TerAvest: Looks good to me, approved
diff --git a/src/mainboard/google/octopus/variants/meep/include/variant/sku.h b/src/mainboard/google/octopus/variants/meep/include/variant/sku.h index 1fd17ef..16a4eab 100644 --- a/src/mainboard/google/octopus/variants/meep/include/variant/sku.h +++ b/src/mainboard/google/octopus/variants/meep/include/variant/sku.h @@ -17,7 +17,10 @@ #define __MAINBOARD_SKU_H__
enum { - + SKU_4_VORTININJA = 4, /* Stylus + rear camera */ + SKU_5_VORTININJA = 5, /* Stylus + no rear camera */ + SKU_6_VORTININJA = 6, /* no Stylus + rear camera */ + SKU_7_VORTININJA = 7, /* no Stylus + no rear camera */ SKU_33_DORP = 33, /* HDMI */ SKU_34_DORP = 34, /* HDMI+Kblight */ SKU_35_DORP = 35, /* HDMI+TS */ diff --git a/src/mainboard/google/octopus/variants/meep/variant.c b/src/mainboard/google/octopus/variants/meep/variant.c index 71e6eb4..ece8ff9 100644 --- a/src/mainboard/google/octopus/variants/meep/variant.c +++ b/src/mainboard/google/octopus/variants/meep/variant.c @@ -17,6 +17,23 @@ #include <drivers/intel/gma/opregion.h> #include <baseboard/variants.h> #include <variant/sku.h> +#include <sar.h> + +const char *get_wifi_sar_cbfs_filename(void) +{ + const char *filename = NULL; + uint32_t sku_id = get_board_sku(); + + switch (sku_id) { + case SKU_4_VORTININJA: + case SKU_5_VORTININJA: + case SKU_6_VORTININJA: + case SKU_7_VORTININJA: + filename = "wifi_sar-vortininja.hex"; + break; + } + return filename; +}
const char *mainboard_vbt_filename(void) {
-
Chen Wisley (Code Review) -
Furquan Shaikh (Code Review)
-
Justin TerAvest (Code Review)
-
Marco Chen (Code Review)
-
Paul Menzel (Code Review)