Morgan Jang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40286 )
Change subject: drivers/ipmi: Implement the function for logging system event into BMC ......................................................................
Patch Set 4:
(13 comments)
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*?
Done
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG@9 PS3, Line 9: clear
cleared?
Done
https://review.coreboot.org/c/coreboot/+/40286/3//COMMIT_MSG@9 PS3, Line 9: need
needed?
Done
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?
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.h... PS3, Line 76: uint8_t record_type;
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/40286/1/src/drivers/ipmi/ipmi_ops.h File src/drivers/ipmi/ipmi_ops.h:
https://review.coreboot.org/c/coreboot/+/40286/1/src/drivers/ipmi/ipmi_ops.h... PS1, Line 53: struct standard_spec_sel_rec{
missing space after struct definition
Done
https://review.coreboot.org/c/coreboot/+/40286/1/src/drivers/ipmi/ipmi_ops.h... PS1, Line 63: struct oem_ts_spec_sel_rec{
missing space after struct definition
Done
https://review.coreboot.org/c/coreboot/+/40286/1/src/drivers/ipmi/ipmi_ops.h... PS1, Line 69: struct oem_nots_spec_sel_rec{
missing space after struct definition
Done
https://review.coreboot.org/c/coreboot/+/40286/1/src/drivers/ipmi/ipmi_ops.c File src/drivers/ipmi/ipmi_ops.c:
https://review.coreboot.org/c/coreboot/+/40286/1/src/drivers/ipmi/ipmi_ops.c... PS1, Line 135: if (sel->record_type == 0x2) {
braces {} are not necessary for single statement blocks
Done
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.
Done
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.c... PS3, Line 130: __func__);
Fits into 96 characters per line.
Done
https://review.coreboot.org/c/coreboot/+/40286/3/src/drivers/ipmi/ipmi_ops.c... PS3, Line 138: timestamped
timestamp?
Done