[L] Change in coreboot[master]: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI
Attention is currently required from: Angel Pons, David Milosevic.
Maximilian Brune 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 8:
(5 comments)
File src/mainboard/prodrive/atlas/emi.h:
https://review.coreboot.org/c/coreboot/+/68137/comment/81495045_3c981c89 PS6, Line 40: typedef enum {
Coreboot is full of typedefs. I don't see the problem here.
just because other people have done it doesn't mean you have to do it to. It violates coreboot coding style so I don't see a reason to keep the typedef.
https://review.coreboot.org/c/coreboot/+/68137/comment/42a9c760_f56a62a2 PS6, Line 43: EMI_REGION_1 ///< EMI region 1
Agreed.
Done
File src/mainboard/prodrive/atlas/emi.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/5cdecaee_40bfaf47 PS6, Line 83: {
I already considered using a u32 but I did not like this approach so I switched to […]
If the function is not used there is no need to keep it. If there really is a need for that function in the future, it can be added at that time. Implementing functions and functionalitly that is/are never used only worsens readability and blows up code to no end.
https://review.coreboot.org/c/coreboot/+/68137/comment/5b6056aa_c228ab93 PS6, Line 139: inline void emi_write_register(const EMI_INSTANCE instance, const u8 offs, const u8 value)
I wont remove this function for the same reason as above. […]
same as above.
https://review.coreboot.org/c/coreboot/+/68137/comment/d1d5fdd3_2de68753 PS6, Line 144: inline u8 emi_read_register(const EMI_INSTANCE instance, const u8 offs)
See previous replies.
see previous replies
-
Maximilian Brune (Code Review)