Attention is currently required from: Maximilian Brune, David Milosevic.
Angel Pons 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:
(10 comments)
File src/mainboard/prodrive/atlas/emi.h:
https://review.coreboot.org/c/coreboot/+/68137/comment/3b9fd389_6d0c7656 PS6, Line 33: typedef enum {
avoid typedef as per coreboot coding style
Also, use lowercase for enum type names
https://review.coreboot.org/c/coreboot/+/68137/comment/b4d55588_bf9d0f7d PS6, Line 36: EMI_INSTANCE_1 ///< EMI instance 1 (EMI1)
the comment is unnecessary. […]
+1
https://review.coreboot.org/c/coreboot/+/68137/comment/988044f5_8832e1d9 PS6, Line 76: const u16 addr,
do not use tabs for this kind of indentation. […]
We use tabs for indentation of such things in other files anyway. Tabs are 8 characters wide in coreboot.
But we don't align things this way. Instead, we align to the opening parentheses:
``` int emi_read_region(const EMI_INSTANCE instance, const EMI_REGION region, const EMI_ACCESS access, const u16 addr, u8 *buff); ```
File src/mainboard/prodrive/atlas/emi.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/f905035e_a41201c5 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". […]
It might be a good idea to keep the logic to select an address:
``` u16 emi_select_region(const EMI_INSTANCE instance, const EMI_REGION region, const EMI_ACCESS access, const u16 addr) { const u16 base = emi_base(instance); outb(encode_address_lsb(addr, access), base + EC_ADDRESS_LSB); outb(encode_address_msb(addr, region), base + EC_ADDRESS_MSB); return base; } ```
Then do the access with the desired bit width outside the function.
https://review.coreboot.org/c/coreboot/+/68137/comment/729cd573_4538d058 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).
Also, explicitly declaring things inline is generally not a good idea in coreboot.
File src/mainboard/prodrive/atlas/ld_config.c:
PS6: Please use `device/pnp_ops.h`
File src/mainboard/prodrive/atlas/mainboard.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/c0e0c615_ac800a3f PS6, Line 14: static const char* get_smbios_part_number(void);
"foo* bar" should be "foo *bar"
Please fix.
https://review.coreboot.org/c/coreboot/+/68137/comment/eeb96274_861b7417 PS6, Line 41: static const char* get_smbios_part_number(void)
"foo* bar" should be "foo *bar"
Please fix.
File src/mainboard/prodrive/atlas/smbios.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/435cc610_39182108 PS6, Line 8: const char* smbios_mainboard_serial_number(void)
"foo* bar" should be "foo *bar"
Please fix.
File src/mainboard/prodrive/atlas/vpd.c:
https://review.coreboot.org/c/coreboot/+/68137/comment/0bc7606c_fee279c3 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 na […]
Or if the macro were defined immediately before the function, and using an uppercase name.