Attention is currently required from: Maximilian Brune, Angel Pons, Lean Sheng Tan.
David Milosevic has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68137 )
Change subject: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI ......................................................................
Patch Set 10:
(15 comments)
File src/mainboard/prodrive/atlas/emi.h:
https://review.coreboot.org/c/coreboot/+/68137/comment/c8391426_579d8195 PS6, Line 33: typedef enum {
Also, use lowercase for enum type names
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/527e58c0_fda3d9a0 PS6, Line 36: EMI_INSTANCE_1 ///< EMI instance 1 (EMI1)
+1
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/6cdcc049_2bb90a1a PS6, Line 40: typedef enum {
I totally understand why someone might not like the usage of typedefs. […]
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/819992cb_a97026ee PS6, Line 47: typedef enum {
See previous reply.
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/40c69742_de4ec2d4 PS6, Line 76: const u16 addr,
We use tabs for indentation of such things in other files anyway. […]
Done
File src/mainboard/prodrive/atlas/emi.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/a5b20d51_f662985a PS6, Line 41: int emi_read_region(const EMI_INSTANCE instance, const EMI_REGION region,
It might be a good idea to keep the logic to select an address: […]
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/2ff5e1b8_c6caf51b PS6, Line 83: {
personally I think it makes sense to keep the write function, because: […]
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/c0fe72cc_fe89812a PS6, Line 139: inline void emi_write_register(const EMI_INSTANCE instance, const u8 offs, const u8 value)
same as above.
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/1046704f_2b697f43 PS6, Line 144: inline u8 emi_read_register(const EMI_INSTANCE instance, const u8 offs)
see previous replies
Done
File src/mainboard/prodrive/atlas/mainboard.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/16e89680_c691be1b PS6, Line 14: static const char* get_smbios_part_number(void);
"foo* bar" should be "foo *bar" […]
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/e54c1748_217a03e6 PS6, Line 41: static const char* get_smbios_part_number(void)
"foo* bar" should be "foo *bar" […]
Done
https://review.coreboot.org/c/coreboot/+/68137/comment/cb4e0f8a_b5e7b8ba PS6, Line 46: strncpy((char *) pn, prefix, sizeof(prefix));
Regarding
Done
File src/mainboard/prodrive/atlas/smbios.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/4cd07f97_a31960dc PS6, Line 8: const char* smbios_mainboard_serial_number(void)
"foo* bar" should be "foo *bar" […]
Done
File src/mainboard/prodrive/atlas/vpd.h:
https://review.coreboot.org/c/coreboot/+/68137/comment/0e45094e_65823958 PS6, Line 26: #define vpd(x) offsetof(vpd_section_t, x)
this macro only makes the rest of the code far less obvious.
Done
File src/mainboard/prodrive/atlas/vpd.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/0294c8c1_47d102a0 PS6, Line 36: case vpd(part_number): return PN_LENGTH;
Or if the macro were defined immediately before the function, and using an uppercase name.
Done