Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35726 )
Change subject: drivers/aspeed/common: Add support for high resolution framebuffer ......................................................................
Patch Set 14:
Patch Set 14:
based on the inclusion of i2c_simple.h, this would seem to only work on boards which also offer hardware I2C, otherwise platform_i2c_transfer() remains unimplemented and causes a linking error. Can hack around it with preprocessor guards, but not exactly pretty.
Took me a moment to figure out why: The call to platform_i2c_transfer() is not garbage collected because the call to software_i2c_transfer() is runtime conditional (based on the bus number).
i2c_simple is messy and will stay that way unless we define some rules (e.g. let mainboard code assign bus numbers). The problem is that it is designed for a single chip does everything solution (and don't you dare to put another I2C controller on a board). It's just incompatible to this case. For the moment, adding more Kconfig and if's, would do. But to be scalable, it would need function pointers per bus that are registered during runtime; and that would bring it unlikely close to `i2c_bus.h`.
There is only one entity in coreboot that can glue such things properly together: the mainboard code. In the convenient case, this is done via the devicetree.
I don't see how the call to platform_i2c_transfer() is supposed to do something useful in case `bus >= SOFTWARE_I2C_MAX_BUS`. I would expect bus numbers to overlap anyway. Maybe it's the best to never call it in case of CONFIG(SOFTWARE_I2C)?
OTOH, if that is true, we shouldn't select SOFTWARE_I2C from a driver because it disables platform_i2c_tansfer() and who knows when it is needed?
My current hunch would be to redesign software i2c so one can use it to implement i2c_bus. And deprecate i2c_simple anyway for everything that might remotely have a second controller chip on board.
I'll have a look at this next year.