Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40286 )
Change subject: drivers/ipmi: Implement "IPMI add SEL" function ......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG@8 PS3, Line 8: What is *IPMI add SEL*?
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG@9 PS3, Line 9: need needed?
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG@9 PS3, Line 9: clear cleared?
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG@11 PS3, Line 11: TEST=Check if SEL is add in BMC How did you check that?
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.h File src/drivers/ipmi/ipmi_ops.h:
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.h... PS3, Line 56: uint8_t evm_rev; Please use a space instead of a tab.
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.h... PS3, Line 76: uint8_t record_type; Ditto.
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.c File src/drivers/ipmi/ipmi_ops.c:
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.c... PS3, Line 129: printk(BIOS_ERR, "%s failed, null pointer parameter\n", Please reword the error message. BIOS_ERROR messages should be user understandable.
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.c... PS3, Line 130: __func__); Fits into 96 characters per line.
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.c... PS3, Line 138: timestamped timestamp?