Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32293 )
Change subject: smbios: Add memory type 9 system slot support ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/#/c/32293/1/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32293/1/src/arch/x86/smbios.c@831 PS1, Line 831: const char *name, u8 type, u8 slot_char1,
Done
please constify all arguments and use enums where possible. That would be: slot_type, slot_data_bus_width, current_usage, slot_length.
https://review.coreboot.org/#/c/32293/1/src/arch/x86/smbios.c@849 PS1, Line 849: t->device_function_number = dev_func;
Done
I don't see segment group number or peer useful at the moment.
https://review.coreboot.org/#/c/32293/4/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/#/c/32293/4/src/arch/x86/smbios.c@847 PS4, Line 847: t->slot_designation = smbios_add_string(t->eos, name); Use a default string if name is NULL? Like SLOT-%d?
https://review.coreboot.org/#/c/32293/4/src/arch/x86/smbios.c@857 PS4, Line 857: t->data_bus_width = SlotDataBusWidthOther; I need data_bus_width in https://review.coreboot.org/c/coreboot/+/32307/1. I haven't implemented slot_length in https://review.coreboot.org/c/coreboot/+/32307/1, but it's easy an probably should be done too. Also current_usage can be detected at runtime. If the PCI device has enabled children, it's 'In use', otherwise 'Available'.
Setting a default for them doesn't make sense.
https://review.coreboot.org/#/c/32293/4/src/include/smbios.h File src/include/smbios.h:
https://review.coreboot.org/#/c/32293/4/src/include/smbios.h@624 PS4, Line 624: struct slot_peer_groups peer[1]; peer[0] if peer_group_count is always zero