Mostly looks good to me, I just have a few (mostly cosmetic) suggestions.
Patch set 6:Code-Review -1
13 comments:
File src/drivers/ipmi/ipmi_fru.c:
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 */
nit: space on each side of '+' operator
nit: space around '+' operator
Would it make sense to add a return type, e.g. cb_err, instead of just printing a serial console message when something fails?
nit: spacing (this looks like it became a tab?)
Patch Set #6, 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 */
nit: spacing around '+' operator (there are a few occurrences in this functions)
same comment as above w.r.t. area and length multiplier
Patch Set #6, 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 */
does it make sense to add a return type?
Same comments apply to this function (spaces around '+' operator, suggested macros for 8x multiplier and 0x3f bitmask)
missing space at the end of the comment
File src/drivers/ipmi/ipmi_ops.h:
Patch Set #6, 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 */
To view, visit change 37444. To unsubscribe, or for help writing mail filters, visit settings.