David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37444 )
Change subject: drivers/ipmi: Add IPMI Read FRU function ......................................................................
Patch Set 6: Code-Review-1
(13 comments)
Mostly looks good to me, I just have a few (mostly cosmetic) suggestions.
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c File src/drivers/ipmi/ipmi_fru.c:
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 58: 30 This looks arbitrary. To make the rationale for 30ms clear, how about making a #define such as: #define READ_FRU_DATA_RETRY_INTERVAL_MS 30 /* From IPMI spec v2.0 rev 1.1 */
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 67: + nit: space on each side of '+' operator
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 98: + nit: space around '+' operator
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 103: void Would it make sense to add a return type, e.g. cb_err, instead of just printing a serial console message when something fails?
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 104: nit: spacing (this looks like it became a tab?)
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 110: offset = offset * 8; I wonder if it's worth adding a comment about this... It's not immediately obvious why we need to multiply length and offset by 8.
Maybe something like: /* FRU info area offset and length are described in multiples of 8 bytes */
Or a #define like: #define OFFSET_LENGTH_MULTIPLIER 8 /* offsets/lengths are multiples of 8 */
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 115: + nit: spacing around '+' operator (there are a few occurrences in this functions)
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 121: * 8 same comment as above w.r.t. area and length multiplier
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 139: bit[5:0] is the string length. Since this is used several times, maybe a macro will make it easier to follow: #define NUM_DATA_BYTES(t) (t & 0x3f) /* encoded in type/length byte */
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 199: void does it make sense to add a return type?
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 205: Same comments apply to this function (spaces around '+' operator, suggested macros for 8x multiplier and 0x3f bitmask)
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 337: t*/ missing space at the end of the comment
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_ops.h File src/drivers/ipmi/ipmi_ops.h:
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_ops.h... PS6, Line 56: uint8_t count; /* count to read. */ It might be useful to note in the comment that it's 1-based, i.e.: uint8_t count; /* count to read, 1-based */
-
David Hendricks (Code Review)