Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45138 )
Change subject: arch/x86/smbios: Update SMBIOS type 0 ec version ......................................................................
Patch Set 2:
(3 comments)
Patch Set 2:
Not sure it is ok to use BMC version directly?
I've no idea if it's correct to use the BMC version here. If SMBIOS says this is for the Embedded Controller, then we shouldn't write the BMC information
https://review.coreboot.org/c/coreboot/+/45138/2/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/45138/2/src/arch/x86/smbios.c@428 PS2, Line 428: smbios_ec_revision(&ipmi_bmc_major_revision, &ipmi_bmc_minor_revision); : t->ec_major_release = ipmi_bmc_major_revision; : t->ec_minor_release = ipmi_bmc_minor_revision; If SMBIOS says this is EC, why use `ipmi_bmc` here?
Also, you should be able to eliminate the local variables:
smbios_ec_revision(&t->ec_major_release, &t->ec_minor_release);
https://review.coreboot.org/c/coreboot/+/45138/2/src/arch/x86/smbios_default... File src/arch/x86/smbios_defaults.c:
https://review.coreboot.org/c/coreboot/+/45138/2/src/arch/x86/smbios_default... PS2, Line 66: __weak void smbios_ec_revision(uint8_t *ipmi_bmc_major_revision, : uint8_t *ipmi_bmc_minor_revision) the parameter names should be `ec_major_release` and `ec_minor_release`
https://review.coreboot.org/c/coreboot/+/45138/2/src/drivers/ipmi/ipmi_kcs_o... File src/drivers/ipmi/ipmi_kcs_ops.c:
https://review.coreboot.org/c/coreboot/+/45138/2/src/drivers/ipmi/ipmi_kcs_o... PS2, Line 283: ((bmc_revision_major / 16) * 10) + (bmc_revision_major % 16) Huh? What is this math needed for?