Attention is currently required from: Arthur Heymans, Nico Huber, Tim Wawrzynczak, Christian Walter, Julius Werner, EricR Lai. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59768 )
Change subject: drivers/i2c/tpm: Add override function for TPM I2C bus ......................................................................
Patch Set 4:
(2 comments)
File src/drivers/i2c/tpm/tis.c:
https://review.coreboot.org/c/coreboot/+/59768/comment/cc84e768_ded4bcc2 PS4, Line 22: int `unsigned int`, please.
https://review.coreboot.org/c/coreboot/+/59768/comment/15bedb66_574e9aef PS4, Line 22: int __weak get_tpm_i2c_bus(void) : { : return CONFIG_DRIVER_TPM_I2C_BUS; : }
I don't think a weak function is a good solution here. […]
+1, there's absolutely no reason to use weak symbols here.
I'd add a `DRIVER_TPM_I2C_BUS_RUNTIME_DEPENDENT` Kconfig symbol. When selected, the prompt for `DRIVER_TPM_I2C_BUS` is not visible and the `get_tpm_i2c_bus()` function must be defined.
I'd also introduce a helper function to get the I2C bus number according to the Kconfig options:
static unsigned int _get_tpm_i2c_bus(void) { if (CONFIG(DRIVER_TPM_I2C_BUS_RUNTIME_DEPENDENT)) return get_tpm_i2c_bus(); else return CONFIG_DRIVER_TPM_I2C_BUS; }
Note that there are no weak symbols. This way, if `DRIVER_TPM_I2C_BUS_RUNTIME_DEPENDENT` is enabled but `get_tpm_i2c_bus()` is not defined, there will be a build-time error (linker will complain about unresolved symbol `get_tpm_i2c_bus`). However, when `DRIVER_TPM_I2C_BUS_RUNTIME_DEPENDENT` is *not* enabled, the compiler will automatically optimize out the `get_tpm_i2c_bus()` function call, so the linker will never see this symbol and won't cause any build errors.
-
Angel Pons (Code Review)