Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40621 )
Change subject: drivers/ipmi: Added function read_data_string() to make code cleaner ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40621/2//COMMIT_MSG@7 PS2, Line 7: Added Add
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c File src/drivers/ipmi/ipmi_fru.c:
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 96: length of the string or 0 when it's failed. */ Concise multi-line comments are meant for comments in code blocks. Please use:
/* * … */
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 97: uint8_t *data_ptr Can it be const?
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 103: printk(BIOS_ERR, "%s:%d - failed due to length is zero\n", __func__, __LINE__); In my opinion error log messages should be more elaborate. But maybe the error should be printed by the caller, and here should only be a debug message?
https://review.coreboot.org/c/coreboot/+/40621/2/src/drivers/ipmi/ipmi_fru.c... PS2, Line 159: &info->manufacturer); Shouldn’t you check if length is 0?