Change in coreboot[master]: util/inteltool: Split GPIO community switch-case into its own function
Mimoja has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
util/inteltool: Split GPIO community switch-case into its own function
So far priting the GPIO groups chose the community defition. As the list of supported platforms grows the massive switch case gets repetetive and hinders the readers view. It also reduces the ability to reuse the code in an potential libinteltool. To takle these issues the detection logic was split into its own function.
Change-Id: I215c1b7d6ec164b8afd9489ebd54b63d3df50cb9 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M util/inteltool/gpio_groups.c M util/inteltool/inteltool.h 2 files changed, 38 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/38631/1
diff --git a/util/inteltool/gpio_groups.c b/util/inteltool/gpio_groups.c index 8b60c2b..4c32558 100644 --- a/util/inteltool/gpio_groups.c +++ b/util/inteltool/gpio_groups.c @@ -96,11 +96,11 @@ } }
-void print_gpio_groups(struct pci_dev *const sb) +const struct gpio_community *const *get_gpio_communitys(struct pci_dev *const sb, + size_t* community_count, + size_t* pad_stepping) { - size_t community_count; - const struct gpio_community *const *communities; - size_t pad_stepping = 8; + *pad_stepping = 8;
switch (sb->device_id) { case PCI_DEVICE_ID_INTEL_H110: @@ -114,10 +114,8 @@ case PCI_DEVICE_ID_INTEL_QM170: case PCI_DEVICE_ID_INTEL_HM170: case PCI_DEVICE_ID_INTEL_CM236: - community_count = ARRAY_SIZE(sunrise_communities); - communities = sunrise_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(sunrise_communities); + return sunrise_communities; case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_PRE: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_BASE_SKL: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_PREM_SKL: @@ -128,10 +126,8 @@ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_IHDCP_BASE: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_IHDCP_PREM: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_IHDCP_PREM: - community_count = ARRAY_SIZE(sunrise_lp_communities); - communities = sunrise_lp_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(sunrise_lp_communities); + return sunrise_lp_communities; case PCI_DEVICE_ID_INTEL_C621: case PCI_DEVICE_ID_INTEL_C622: case PCI_DEVICE_ID_INTEL_C624: @@ -145,20 +141,14 @@ case PCI_DEVICE_ID_INTEL_C621_SUPER: case PCI_DEVICE_ID_INTEL_C627_SUPER_2: case PCI_DEVICE_ID_INTEL_C628_SUPER: - community_count = ARRAY_SIZE(lewisburg_communities); - communities = lewisburg_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(lewisburg_communities); + return lewisburg_communities; case PCI_DEVICE_ID_INTEL_DNV_LPC: - community_count = ARRAY_SIZE(denverton_communities); - communities = denverton_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(denverton_communities); + return denverton_communities; case PCI_DEVICE_ID_INTEL_APL_LPC: - community_count = ARRAY_SIZE(apl_communities); - communities = apl_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(apl_communities); + return apl_communities; case PCI_DEVICE_ID_INTEL_H310: case PCI_DEVICE_ID_INTEL_H370: case PCI_DEVICE_ID_INTEL_Z390: @@ -169,20 +159,30 @@ case PCI_DEVICE_ID_INTEL_QM370: case PCI_DEVICE_ID_INTEL_HM370: case PCI_DEVICE_ID_INTEL_CM246: - community_count = ARRAY_SIZE(cannonlake_pch_h_communities); - communities = cannonlake_pch_h_communities; - pad_stepping = 16; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(cannonlake_pch_h_communities); + *pad_stepping = 16; + return cannonlake_pch_h_communities; case PCI_DEVICE_ID_INTEL_ICELAKE_LP_U: - community_count = ARRAY_SIZE(icelake_pch_h_communities); - communities = icelake_pch_h_communities; - pad_stepping = 16; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(icelake_pch_h_communities); + *pad_stepping = 16; + return icelake_pch_h_communities; default: - return; + return NULL; } +} + +void print_gpio_groups(struct pci_dev *const sb) +{ + size_t community_count; + const struct gpio_community *const *communities; + size_t pad_stepping; + + communities = get_gpio_communitys(sb, &community_count, &pad_stepping); + + if (!communities) + return; + + pcr_init(sb);
printf("\n============= GPIOS =============\n\n");
diff --git a/util/inteltool/inteltool.h b/util/inteltool/inteltool.h index 1c1841c..5fa98b5 100644 --- a/util/inteltool/inteltool.h +++ b/util/inteltool/inteltool.h @@ -393,6 +393,9 @@ int print_pmbase(struct pci_dev *sb, struct pci_access *pacc); int print_rcba(struct pci_dev *sb); int print_gpios(struct pci_dev *sb, int show_all, int show_diffs); +const struct gpio_community *const *get_gpio_communitys(struct pci_dev *const sb, + size_t* community_count, + size_t* pad_stepping); void print_gpio_groups(struct pci_dev *sb); int print_epbar(struct pci_dev *nb); int print_dmibar(struct pci_dev *nb);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG@9 PS1, Line 9: priting printing
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG@9 PS1, Line 9: defition definition
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG@12 PS1, Line 12: in an potential in a potential
Hello Paul Menzel, Stefan Reinauer, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38631
to look at the new patch set (#2).
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
util/inteltool: Split GPIO community switch-case into its own function
So far printing the GPIO groups chose the community definition. As the list of supported platforms grows the massive switch case gets repetetive and hinders the readers view. It also reduces the ability to reuse the code in a potential libinteltool. To takle these issues the detection logic was split into its own function.
Change-Id: I215c1b7d6ec164b8afd9489ebd54b63d3df50cb9 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M util/inteltool/gpio_groups.c M util/inteltool/inteltool.h 2 files changed, 38 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/38631/2
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG@9 PS1, Line 9: defition
definition
Done
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG@9 PS1, Line 9: priting
printing
Done
https://review.coreboot.org/c/coreboot/+/38631/1//COMMIT_MSG@12 PS1, Line 12: in an potential
in a potential
Done
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... File util/inteltool/gpio_groups.c:
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... PS2, Line 99: get_gpio_communitys communities
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... PS2, Line 180: get_gpio_communitys here too
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/inteltool.h File util/inteltool/inteltool.h:
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/inteltool.h@... PS2, Line 396: get_gpio_communitys communities
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... File util/inteltool/gpio_groups.c:
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... PS2, Line 99: get_gpio_communitys
communities
Don't forget! get_gpio_communities
Matt DeVillier has uploaded a new patch set (#4) to the change originally created by Mimoja. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
util/inteltool: Split GPIO community switch-case into its own function
So far printing the GPIO groups chose the community definition. As the list of supported platforms grows the massive switch case gets repetetive and hinders the readers view. It also reduces the ability to reuse the code in a potential libinteltool. To takle these issues the detection logic was split into its own function.
Change-Id: I215c1b7d6ec164b8afd9489ebd54b63d3df50cb9 Signed-off-by: Johanna Schander coreboot@mimoja.de --- M util/inteltool/gpio_groups.c M util/inteltool/inteltool.h 2 files changed, 38 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/38631/4
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... File util/inteltool/gpio_groups.c:
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... PS2, Line 99: get_gpio_communitys
Don't forget! get_gpio_communities
Done
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/gpio_groups.... PS2, Line 180: get_gpio_communitys
here too
Done
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/inteltool.h File util/inteltool/inteltool.h:
https://review.coreboot.org/c/coreboot/+/38631/2/util/inteltool/inteltool.h@... PS2, Line 396: get_gpio_communitys
communities
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38631 )
Change subject: util/inteltool: Split GPIO community switch-case into its own function ......................................................................
util/inteltool: Split GPIO community switch-case into its own function
So far printing the GPIO groups chose the community definition. As the list of supported platforms grows the massive switch case gets repetetive and hinders the readers view. It also reduces the ability to reuse the code in a potential libinteltool. To takle these issues the detection logic was split into its own function.
Change-Id: I215c1b7d6ec164b8afd9489ebd54b63d3df50cb9 Signed-off-by: Johanna Schander coreboot@mimoja.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/38631 Reviewed-by: Patrick Georgi pgeorgi@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/inteltool/gpio_groups.c M util/inteltool/inteltool.h 2 files changed, 38 insertions(+), 35 deletions(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/util/inteltool/gpio_groups.c b/util/inteltool/gpio_groups.c index 8b60c2b..3d7d708 100644 --- a/util/inteltool/gpio_groups.c +++ b/util/inteltool/gpio_groups.c @@ -96,11 +96,11 @@ } }
-void print_gpio_groups(struct pci_dev *const sb) +const struct gpio_community *const *get_gpio_communities(struct pci_dev *const sb, + size_t* community_count, + size_t* pad_stepping) { - size_t community_count; - const struct gpio_community *const *communities; - size_t pad_stepping = 8; + *pad_stepping = 8;
switch (sb->device_id) { case PCI_DEVICE_ID_INTEL_H110: @@ -114,10 +114,8 @@ case PCI_DEVICE_ID_INTEL_QM170: case PCI_DEVICE_ID_INTEL_HM170: case PCI_DEVICE_ID_INTEL_CM236: - community_count = ARRAY_SIZE(sunrise_communities); - communities = sunrise_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(sunrise_communities); + return sunrise_communities; case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_PRE: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_BASE_SKL: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_PREM_SKL: @@ -128,10 +126,8 @@ case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_IHDCP_BASE: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_U_IHDCP_PREM: case PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_Y_IHDCP_PREM: - community_count = ARRAY_SIZE(sunrise_lp_communities); - communities = sunrise_lp_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(sunrise_lp_communities); + return sunrise_lp_communities; case PCI_DEVICE_ID_INTEL_C621: case PCI_DEVICE_ID_INTEL_C622: case PCI_DEVICE_ID_INTEL_C624: @@ -145,20 +141,14 @@ case PCI_DEVICE_ID_INTEL_C621_SUPER: case PCI_DEVICE_ID_INTEL_C627_SUPER_2: case PCI_DEVICE_ID_INTEL_C628_SUPER: - community_count = ARRAY_SIZE(lewisburg_communities); - communities = lewisburg_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(lewisburg_communities); + return lewisburg_communities; case PCI_DEVICE_ID_INTEL_DNV_LPC: - community_count = ARRAY_SIZE(denverton_communities); - communities = denverton_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(denverton_communities); + return denverton_communities; case PCI_DEVICE_ID_INTEL_APL_LPC: - community_count = ARRAY_SIZE(apl_communities); - communities = apl_communities; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(apl_communities); + return apl_communities; case PCI_DEVICE_ID_INTEL_H310: case PCI_DEVICE_ID_INTEL_H370: case PCI_DEVICE_ID_INTEL_Z390: @@ -169,20 +159,30 @@ case PCI_DEVICE_ID_INTEL_QM370: case PCI_DEVICE_ID_INTEL_HM370: case PCI_DEVICE_ID_INTEL_CM246: - community_count = ARRAY_SIZE(cannonlake_pch_h_communities); - communities = cannonlake_pch_h_communities; - pad_stepping = 16; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(cannonlake_pch_h_communities); + *pad_stepping = 16; + return cannonlake_pch_h_communities; case PCI_DEVICE_ID_INTEL_ICELAKE_LP_U: - community_count = ARRAY_SIZE(icelake_pch_h_communities); - communities = icelake_pch_h_communities; - pad_stepping = 16; - pcr_init(sb); - break; + *community_count = ARRAY_SIZE(icelake_pch_h_communities); + *pad_stepping = 16; + return icelake_pch_h_communities; default: - return; + return NULL; } +} + +void print_gpio_groups(struct pci_dev *const sb) +{ + size_t community_count; + const struct gpio_community *const *communities; + size_t pad_stepping; + + communities = get_gpio_communities(sb, &community_count, &pad_stepping); + + if (!communities) + return; + + pcr_init(sb);
printf("\n============= GPIOS =============\n\n");
diff --git a/util/inteltool/inteltool.h b/util/inteltool/inteltool.h index 950943f..49af276 100644 --- a/util/inteltool/inteltool.h +++ b/util/inteltool/inteltool.h @@ -395,6 +395,9 @@ int print_pmbase(struct pci_dev *sb, struct pci_access *pacc); int print_rcba(struct pci_dev *sb); int print_gpios(struct pci_dev *sb, int show_all, int show_diffs); +const struct gpio_community *const *get_gpio_communities(struct pci_dev *const sb, + size_t* community_count, + size_t* pad_stepping); void print_gpio_groups(struct pci_dev *sb); int print_epbar(struct pci_dev *nb); int print_dmibar(struct pci_dev *nb);
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Mimoja (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)