Hello Jes Klinke,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/47048
to review the following change.
Change subject: mb/google/volteer: clang-format of mainboard.c ......................................................................
mb/google/volteer: clang-format of mainboard.c
This CL is entirely generated by running the autmatic formatter on this one file.
BUG=None TEST=abuild -t GOOGLE_VOLTEER2 -c max -x
Change-Id: Ibdd8cc2222e7af11c11df963b088ca2db07a3214 Signed-off-by: Jes Bodi Klinke jbk@chromium.org --- M src/mainboard/google/volteer/mainboard.c 1 file changed, 7 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47048/1
diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index 03a78fd..b4d6676 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -51,8 +51,7 @@ return; }
- if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50) && - cr50_is_long_interrupt_pulse_enabled()) { + if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50) && cr50_is_long_interrupt_pulse_enabled()) { printk(BIOS_INFO, "Enabling S0i3.4\n"); } else { /* @@ -75,8 +74,7 @@ base_pads = variant_base_gpio_table(&base_num); override_pads = variant_override_gpio_table(&override_num);
- gpio_configure_pads_with_override(base_pads, base_num, - override_pads, override_num); + gpio_configure_pads_with_override(base_pads, base_num, override_pads, override_num); }
void mainboard_silicon_init_params(FSP_S_CONFIG *params) @@ -84,14 +82,13 @@ bool has_usb4;
/* If device doesn't have USB4 hardware, disable tbt */ - has_usb4 = (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) || - fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3))); + has_usb4 = (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) + || fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)));
if (!has_usb4) - memset(params->ITbtPcieRootPortEn, - 0, - ARRAY_SIZE(params->ITbtPcieRootPortEn) * - sizeof(*params->ITbtPcieRootPortEn)); + memset(params->ITbtPcieRootPortEn, 0, + ARRAY_SIZE(params->ITbtPcieRootPortEn) + * sizeof(*params->ITbtPcieRootPortEn)); }
struct chip_operations mainboard_ops = {
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47048 )
Change subject: mb/google/volteer: clang-format of mainboard.c ......................................................................
Patch Set 2: Code-Review+2
(3 comments)
https://review.coreboot.org/c/coreboot/+/47048/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47048/2//COMMIT_MSG@7 PS2, Line 7: of nit: drop `of`
https://review.coreboot.org/c/coreboot/+/47048/2//COMMIT_MSG@9 PS2, Line 9: autmatic nit: aut*o*matic
https://review.coreboot.org/c/coreboot/+/47048/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/47048/2/src/mainboard/google/voltee... PS2, Line 141: ARRAY_SIZE(params->ITbtPcieRootPortEn) : * sizeof(*params->ITbtPcieRootPortEn) shouldn't this just be `sizeof(params->ITbtPcieRootPortEn)`?
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47048 )
Change subject: mb/google/volteer: clang-format of mainboard.c ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47048/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/47048/2/src/mainboard/google/voltee... PS2, Line 141: ARRAY_SIZE(params->ITbtPcieRootPortEn) : * sizeof(*params->ITbtPcieRootPortEn)
shouldn't this just be `sizeof(params->ITbtPcieRootPortEn)`?
ITbtPcieRootPortEn is an array of UINT8, so yes, while the existing code is correctly multiplying the number of elements with the size of one element, you are right that it could be expressed simpler, by directly using sizeof() to determine the size of the entire array.
I created this CL because I was trying to make an unrelated change to this file, but was told by the commit script that I had to reformat a number of existing lines to make them comply with the formatting standard. I did not want to pollute my functional change with pure formatting. So now I am in a dilemma whether I want to include "obviously" correct simplifications together with the pure formatting changes, in this CL.
Personally, I would prefer to keep this one completely auto-generated.
Hello build bot (Jenkins), Patrick Georgi, Furquan Shaikh, Jes Klinke, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47048
to look at the new patch set (#3).
Change subject: mb/google/volteer: clang-format mainboard.c ......................................................................
mb/google/volteer: clang-format mainboard.c
This CL is entirely generated by running the automatic formatter on this one file.
BUG=None TEST=abuild -t GOOGLE_VOLTEER2 -c max -x
Change-Id: Ibdd8cc2222e7af11c11df963b088ca2db07a3214 Signed-off-by: Jes Bodi Klinke jbk@chromium.org --- M src/mainboard/google/volteer/mainboard.c 1 file changed, 7 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/47048/3
Jes Klinke has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47048 )
Change subject: mb/google/volteer: clang-format mainboard.c ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47048/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47048/2//COMMIT_MSG@7 PS2, Line 7: of
nit: drop `of`
Done
https://review.coreboot.org/c/coreboot/+/47048/2//COMMIT_MSG@9 PS2, Line 9: autmatic
nit: aut*o*matic
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47048 )
Change subject: mb/google/volteer: clang-format mainboard.c ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/47048/2/src/mainboard/google/voltee... File src/mainboard/google/volteer/mainboard.c:
https://review.coreboot.org/c/coreboot/+/47048/2/src/mainboard/google/voltee... PS2, Line 141: ARRAY_SIZE(params->ITbtPcieRootPortEn) : * sizeof(*params->ITbtPcieRootPortEn)
ITbtPcieRootPortEn is an array of UINT8, so yes, while the existing code is correctly multiplying th […]
Ah, forgot to mention why I marked this as resolved: this can be fixed in a follow-up commit. I fully agree with your reasoning about not polluting this change, so feel free to simplify this in a follow-up.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47048 )
Change subject: mb/google/volteer: clang-format mainboard.c ......................................................................
mb/google/volteer: clang-format mainboard.c
This CL is entirely generated by running the automatic formatter on this one file.
BUG=None TEST=abuild -t GOOGLE_VOLTEER2 -c max -x
Change-Id: Ibdd8cc2222e7af11c11df963b088ca2db07a3214 Signed-off-by: Jes Bodi Klinke jbk@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/47048 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/google/volteer/mainboard.c 1 file changed, 7 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/mainboard/google/volteer/mainboard.c b/src/mainboard/google/volteer/mainboard.c index 016572a..acba972 100644 --- a/src/mainboard/google/volteer/mainboard.c +++ b/src/mainboard/google/volteer/mainboard.c @@ -102,8 +102,7 @@ return; }
- if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50) && - cr50_is_long_interrupt_pulse_enabled()) { + if (CONFIG(MAINBOARD_HAS_SPI_TPM_CR50) && cr50_is_long_interrupt_pulse_enabled()) { printk(BIOS_INFO, "Enabling S0i3.4\n"); } else { /* @@ -126,8 +125,7 @@ base_pads = variant_base_gpio_table(&base_num); override_pads = variant_override_gpio_table(&override_num);
- gpio_configure_pads_with_override(base_pads, base_num, - override_pads, override_num); + gpio_configure_pads_with_override(base_pads, base_num, override_pads, override_num); }
void mainboard_silicon_init_params(FSP_S_CONFIG *params) @@ -135,14 +133,13 @@ bool has_usb4;
/* If device doesn't have USB4 hardware, disable tbt */ - has_usb4 = (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) || - fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3))); + has_usb4 = (fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN2)) + || fw_config_probe(FW_CONFIG(DB_USB, USB4_GEN3)));
if (!has_usb4) - memset(params->ITbtPcieRootPortEn, - 0, - ARRAY_SIZE(params->ITbtPcieRootPortEn) * - sizeof(*params->ITbtPcieRootPortEn)); + memset(params->ITbtPcieRootPortEn, 0, + ARRAY_SIZE(params->ITbtPcieRootPortEn) + * sizeof(*params->ITbtPcieRootPortEn)); }
struct chip_operations mainboard_ops = {
-
Angel Pons (Code Review) -
Jes Klinke (Code Review)
-
Patrick Georgi (Code Review)