Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37444 )
Change subject: drivers/ipmi: Add IPMI Read FRU function ......................................................................
Patch Set 8:
(13 comments)
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: […]
Done
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 67: +
nit: space on each side of '+' operator
Done
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 98: +
nit: space around '+' operator
Done
https://review.coreboot.org/c/coreboot/+/37444/6/src/drivers/ipmi/ipmi_fru.c... PS6, Line 103: void
Oh, I see now... […]
Yes current design is the caller needs to check if the string is NULL to know if it's successfully read or not. If you think it's better to return a type please let me know.
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?)
Done
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... […]
Done
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)
Done
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. […]
Done
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: […]
Done
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?
Same as above.
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 […]
Done
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
Done
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.: […]
Done