Julius Werner 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:
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)?
I think the problem is just that you never know how many I2C busses a platform may have, and SOFTWARE_I2C_MAX_BUS has to be hardcoded to something. I think I just didn't want to restrict the maximum number of real I2C busses just for this feature when I wrote it. But if it makes things easier now, maybe we can just change SOFTWARE_I2C_MAX_BUS into I2C_MAX_BUS and enforce it everywhere to fix this issue (I believe 10 should be enough for all platforms I've ever seen, but I haven't seen them all).
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?
CONFIG(SOFTWARE_I2C) only says that the software I2C code is compiled in, it doesn't mean you can't still use hardware I2C controllers. The way it's designed right now it will still always use hardware by default unless you explicitly enable software I2C (by setting the software_i2c[bus] pointer to something non-NULL).
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.
i2c_simple is still used for all non-x86 devices, and we'll probably want to keep it that way since i2c_bus is specific to devicetree, which is only used on x86. Making software I2C a possible backend for i2c_bus sounds reasonable, but please don't kill i2c_simple.