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 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35726/1/src/drivers/aspeed/common/a... File src/drivers/aspeed/common/ast_i2c.c:
https://review.coreboot.org/c/coreboot/+/35726/1/src/drivers/aspeed/common/a... PS1, Line 126: software_i2c[bus] = &ast_ops; You should probably have a check or assert for the bus number here, since your code is clearly only working for a single bus.
https://review.coreboot.org/c/coreboot/+/35726/1/src/drivers/aspeed/common/a... PS1, Line 128: i2c_recover_bus(bus); Why are you doing this here? Do you have reason to believe the bus would be wedged here? I'd keep this separate from just enabling/disabling software I2C unless there's really a reason the bus is always wedged on this platform.
https://review.coreboot.org/c/coreboot/+/35726/1/src/drivers/aspeed/common/a... PS1, Line 133: ast = ast_priv; I don't want to look too closely into the platform code here, but... what is this? If you need to pass platform information in from the calling code, why don't you pass it once into ast_software_i2c_init() rather than for every read?