[L] Change in coreboot[master]: [WIP] mb/prodrive/atlas: Populate smbios table with VPD from ECs EMI

[Maximilian Brune (Code Review)]

Attention is currently required from: 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 6:

(12 comments)

File src/mainboard/prodrive/atlas/emi.h:

https://review.coreboot.org/c/coreboot/+/68137/comment/f176343d_1809f136 
PS6, Line 33: typedef enum {
avoid typedef as per coreboot coding style

https://review.coreboot.org/c/coreboot/+/68137/comment/04168093_d79c0636 
PS6, Line 36: 	EMI_INSTANCE_1               ///< EMI instance 1 (EMI1)
the comment is unnecessary. It would probably make more sense to explain in short what an EMI_INSTANCE/EMI_REGION actually is and how they relate to each other.

https://review.coreboot.org/c/coreboot/+/68137/comment/60c1a9ce_5de4d728 
PS6, Line 40: typedef enum {
avoid typedef as per coreboot coding style

https://review.coreboot.org/c/coreboot/+/68137/comment/b9f1d664_35058d6a 
PS6, Line 43: 	EMI_REGION_1                 ///< EMI region 1
the comment is unnecessary

https://review.coreboot.org/c/coreboot/+/68137/comment/8b395072_bc0af328 
PS6, Line 47: typedef enum {
avoid typedef as per coreboot coding style

https://review.coreboot.org/c/coreboot/+/68137/comment/fd5f3490_141e14a5 
PS6, Line 76: 						 const u16 addr,
do not use tabs for this kind of indentation. Since this kind of indentation is supposed to look the same on all editors, spaces are far more appropriate. If jenkins complains about that, ignore it.

File src/mainboard/prodrive/atlas/emi.c:

https://review.coreboot.org/c/coreboot/+/68137/comment/e44d5634_b9f464d3 
PS6, Line 41: int emi_read_region(const EMI_INSTANCE instance, const EMI_REGION region,
as far as I can see this function is only used by "emi_read_region_blocK". You can therefore completely remove this function and put only a very small subset of it into "emi_read_region_block", since you don't need the whole switch case then.

https://review.coreboot.org/c/coreboot/+/68137/comment/ada21463_b7d0a76f 
PS6, Line 83: {
wouldn't it be easier to directly use an u32 value instead of a pointer? It would remove the need to cast (below) and would also get rid of endianness issues. Also it would be easier to call the function. Also it's generally safer to not use pointers if possible.
Even better: remove the function, since it isn't used anyway (only clutters up code).

https://review.coreboot.org/c/coreboot/+/68137/comment/4ca8989d_73c58c17 
PS6, Line 139: inline void emi_write_register(const EMI_INSTANCE instance, const u8 offs, const u8 value)
function can be removed, since it's not used anywhere (only clutters up code).

https://review.coreboot.org/c/coreboot/+/68137/comment/d75db0fa_a2c5bffc 
PS6, Line 144: inline u8 emi_read_register(const EMI_INSTANCE instance, const u8 offs)
function can be removed, since it's not used anywhere (only clutters up code).

File src/mainboard/prodrive/atlas/vpd.h:

https://review.coreboot.org/c/coreboot/+/68137/comment/930cc522_d4c82ef0 
PS6, Line 26: #define vpd(x) offsetof(vpd_section_t, x)
this macro only makes the rest of the code far less obvious.

File src/mainboard/prodrive/atlas/vpd.c:

https://review.coreboot.org/c/coreboot/+/68137/comment/5bc04c02_34fb7de9 
PS6, Line 36: 		case vpd(part_number):   return PN_LENGTH;
as mentioned above, this macro makes it very hard to read (especially considering it has the same name as the parameter). It would be much more readable if you would directly write:
`case offsetof(struct vpd_section, part_number):`