Attention is currently required from: Felix Held.
Ana Carolina Cabral has posted comments on this change by Ana Carolina Cabral. ( https://review.coreboot.org/c/coreboot/+/84776?usp=email )
Change subject: drivers/amd/nova: Add Nova Card common driver ......................................................................
Patch Set 8:
(5 comments)
File Makefile.mk:
https://review.coreboot.org/c/coreboot/+/84776/comment/6d956734_be44a5c4?usp... : PS7, Line 121: subdirs-y += src/mainboard/$(MAINBOARDDIR) $(wildcard src/mainboard/*/common)
probably no longer needed after the code was moved to the driver directory
Done
File src/drivers/amd/nova/Kconfig:
https://review.coreboot.org/c/coreboot/+/84776/comment/9a60e35d_dbe5a3af?usp... : PS5, Line 4: def_bool n
the AMD mainboard with a slot for a nova card should select this kconfig directly and not via the de […]
Acknowledged
File src/drivers/amd/nova/chip.h:
https://review.coreboot.org/c/coreboot/+/84776/comment/136ee753_3ae6cde6?usp... : PS7, Line 9: #define NOVA_CARD_EEPROM_CONNECTOR_TYPE_OFFSET 2
i'd move this define into the c file that uses this, since it's not needed outside of that c file
Done
https://review.coreboot.org/c/coreboot/+/84776/comment/1792a405_11a63467?usp... : PS7, Line 10: #define NOVA_CARD_EEPROM_I2C_BUS 2 : #define NOVA_CARD_EEPROM_I2C_ADDRESS 0x55
those two are mainboard-dependend. […]
I put it there as a "default" value, but each board can use a different setup when calling the function. Is it better to move this to each mb/port_descriptors.c file?
File src/drivers/amd/nova/nova_card.c:
https://review.coreboot.org/c/coreboot/+/84776/comment/0b5b1f26_767fb0ec?usp... : PS7, Line 9: #include <soc/amd/phoenix/chip_opensil.h>
i'd like to avoid soc-specific code in a common driver
I can move ddi_connector_type enum to nova/chip.h, since they are only being used in this function, what do you think?
-
Ana Carolina Cabral (Code Review)