Attention is currently required from: Paul Menzel. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/63944 )
Change subject: commonlib/mem_chip_info: Add clarifying documentation comments ......................................................................
Patch Set 2:
(3 comments)
File src/commonlib/bsd/include/commonlib/bsd/mem_chip_info.h:
https://review.coreboot.org/c/coreboot/+/63944/comment/24634abe_145b7654 PS2, Line 18: uint8_t num_channels;
Out of curiosity, is it required to specify the size of the members, or coud `unsigned int` be used […]
Yes, this structure is meant to be serialized in the coreboot table, fixed width is important.
https://review.coreboot.org/c/coreboot/+/63944/comment/e3971982_0ca66f49 PS2, Line 21: uint64_t density; /* number in _bytes_, not Megabytes! */
Should the unit be appended to the name?
The name is already being used in payload code so a bit too much of a pain to change right now... also matches existing LPDDR spec usage.
https://review.coreboot.org/c/coreboot/+/63944/comment/641d8fdc_0cc49b77 PS2, Line 23: uint8_t manufacturer_id; /* raw value from MR5 */
Should a tab be used here too?
No, because then it's even further to the right. I want the comments to all start on the same column but I also didn't want to add yet another tab everywhere because then it gets too tight, so this was the compromise.