Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44022 )
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
arch/x86/smbios.c: Factor out switch-case block
Most of `smbios_fill_dimm_manufacturer_from_id()` is noise. Factor the switch into its own function to improve readability.
Change-Id: Ia0757c01572709d16589a4ed622ca2d2cb69dda2 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/smbios.c 1 file changed, 31 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/44022/1
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 07ccacf..b572624 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -157,56 +157,53 @@ return smbios_add_string(start, str); }
-/* this function will fill the corresponding manufacturer */ -void smbios_fill_dimm_manufacturer_from_id(uint16_t mod_id, struct smbios_type17 *t) +static const char *get_dimm_manufacturer_name(const uint16_t mod_id) { switch (mod_id) { case 0x9b85: - t->manufacturer = smbios_add_string(t->eos, "Crucial"); - break; + return "Crucial"; case 0x4304: - t->manufacturer = smbios_add_string(t->eos, "Ramaxel"); - break; + return "Ramaxel"; case 0x4f01: - t->manufacturer = smbios_add_string(t->eos, "Transcend"); - break; + return "Transcend"; case 0x9801: - t->manufacturer = smbios_add_string(t->eos, "Kingston"); - break; + return "Kingston"; case 0x987f: - t->manufacturer = smbios_add_string(t->eos, "Hynix"); - break; + return "Hynix"; case 0x9e02: - t->manufacturer = smbios_add_string(t->eos, "Corsair"); - break; + return "Corsair"; case 0xb004: - t->manufacturer = smbios_add_string(t->eos, "OCZ"); - break; + return "OCZ"; case 0xad80: - t->manufacturer = smbios_add_string(t->eos, "Hynix/Hyundai"); - break; + return "Hynix/Hyundai"; case 0x3486: - t->manufacturer = smbios_add_string(t->eos, "Super Talent"); - break; + return "Super Talent"; case 0xcd04: - t->manufacturer = smbios_add_string(t->eos, "GSkill"); - break; + return "GSkill"; case 0xce80: - t->manufacturer = smbios_add_string(t->eos, "Samsung"); - break; + return "Samsung"; case 0xfe02: - t->manufacturer = smbios_add_string(t->eos, "Elpida"); - break; + return "Elpida"; case 0x2c80: - t->manufacturer = smbios_add_string(t->eos, "Micron"); - break; - default: { - char string_buffer[256]; + return "Micron"; + default: + return NULL; + } +}
- snprintf(string_buffer, sizeof(string_buffer), "Unknown (%x)", mod_id); - t->manufacturer = smbios_add_string(t->eos, string_buffer); - break; - } +/* this function will fill the corresponding manufacturer */ +void smbios_fill_dimm_manufacturer_from_id(uint16_t mod_id, struct smbios_type17 *t) +{ + const char *const manufacturer = get_dimm_manufacturer_name(mod_id); + + if (manufacturer) { + t->manufacturer = smbios_add_string(t->eos, manufacturer); + + } else { + char string_buffer[256]; + + snprintf(string_buffer, sizeof(string_buffer), "Unknown (%x)", mod_id); + t->manufacturer = smbios_add_string(t->eos, string_buffer); } } /* this function will fill the corresponding locator */
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44022 )
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44022/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/44022/1/src/arch/x86/smbios.c@201 PS1, Line 201: extra blank line
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44022 )
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44022/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/44022/1/src/arch/x86/smbios.c@201 PS1, Line 201:
extra blank line
I usually add it there to add some breathing space. I can remove it if you want, though.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44022
to look at the new patch set (#2).
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
arch/x86/smbios.c: Factor out switch-case block
Most of `smbios_fill_dimm_manufacturer_from_id()` is noise. Factor the switch into its own function to improve readability.
Change-Id: Ia0757c01572709d16589a4ed622ca2d2cb69dda2 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/arch/x86/smbios.c 1 file changed, 30 insertions(+), 34 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/44022/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44022 )
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
Patch Set 2: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44022 )
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44022/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/44022/1/src/arch/x86/smbios.c@201 PS1, Line 201:
I usually add it there to add some breathing space. I can remove it if you want, though.
Ack
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44022 )
Change subject: arch/x86/smbios.c: Factor out switch-case block ......................................................................
arch/x86/smbios.c: Factor out switch-case block
Most of `smbios_fill_dimm_manufacturer_from_id()` is noise. Factor the switch into its own function to improve readability.
Change-Id: Ia0757c01572709d16589a4ed622ca2d2cb69dda2 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44022 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/arch/x86/smbios.c 1 file changed, 30 insertions(+), 34 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/arch/x86/smbios.c b/src/arch/x86/smbios.c index 07ccacf..1b51d90 100644 --- a/src/arch/x86/smbios.c +++ b/src/arch/x86/smbios.c @@ -157,56 +157,52 @@ return smbios_add_string(start, str); }
-/* this function will fill the corresponding manufacturer */ -void smbios_fill_dimm_manufacturer_from_id(uint16_t mod_id, struct smbios_type17 *t) +static const char *get_dimm_manufacturer_name(const uint16_t mod_id) { switch (mod_id) { case 0x9b85: - t->manufacturer = smbios_add_string(t->eos, "Crucial"); - break; + return "Crucial"; case 0x4304: - t->manufacturer = smbios_add_string(t->eos, "Ramaxel"); - break; + return "Ramaxel"; case 0x4f01: - t->manufacturer = smbios_add_string(t->eos, "Transcend"); - break; + return "Transcend"; case 0x9801: - t->manufacturer = smbios_add_string(t->eos, "Kingston"); - break; + return "Kingston"; case 0x987f: - t->manufacturer = smbios_add_string(t->eos, "Hynix"); - break; + return "Hynix"; case 0x9e02: - t->manufacturer = smbios_add_string(t->eos, "Corsair"); - break; + return "Corsair"; case 0xb004: - t->manufacturer = smbios_add_string(t->eos, "OCZ"); - break; + return "OCZ"; case 0xad80: - t->manufacturer = smbios_add_string(t->eos, "Hynix/Hyundai"); - break; + return "Hynix/Hyundai"; case 0x3486: - t->manufacturer = smbios_add_string(t->eos, "Super Talent"); - break; + return "Super Talent"; case 0xcd04: - t->manufacturer = smbios_add_string(t->eos, "GSkill"); - break; + return "GSkill"; case 0xce80: - t->manufacturer = smbios_add_string(t->eos, "Samsung"); - break; + return "Samsung"; case 0xfe02: - t->manufacturer = smbios_add_string(t->eos, "Elpida"); - break; + return "Elpida"; case 0x2c80: - t->manufacturer = smbios_add_string(t->eos, "Micron"); - break; - default: { - char string_buffer[256]; + return "Micron"; + default: + return NULL; + } +}
- snprintf(string_buffer, sizeof(string_buffer), "Unknown (%x)", mod_id); - t->manufacturer = smbios_add_string(t->eos, string_buffer); - break; - } +/* this function will fill the corresponding manufacturer */ +void smbios_fill_dimm_manufacturer_from_id(uint16_t mod_id, struct smbios_type17 *t) +{ + const char *const manufacturer = get_dimm_manufacturer_name(mod_id); + + if (manufacturer) { + t->manufacturer = smbios_add_string(t->eos, manufacturer); + } else { + char string_buffer[256]; + + snprintf(string_buffer, sizeof(string_buffer), "Unknown (%x)", mod_id); + t->manufacturer = smbios_add_string(t->eos, string_buffer); } } /* this function will fill the corresponding locator */
-
Angel Pons (Code Review) -
Tim Wawrzynczak (Code Review)