FWIW the way the I2C stuff is used here LGTM. Don't want to review the rest of the patch though.
Patch set 5:Code-Review +1
2 comments:
File src/drivers/aspeed/common/ast_i2c.c:
backup = software_i2c[ASPEED_BUS];
software_i2c[ASPEED_BUS] = &ast_ops;
This looks pretty odd. Looking at the few (dead) code examples in the tree, […]
It's not exactly dead? It's just a debug feature that you normally wouldn't enable in committed code (although I don't mind its use here if it helps). The implementation in src/soc/rockchip/rk3288/software_i2c.c should be considered canonical. The idea was that you could hack in
software_i2c_attach(some_bus);
software_i2c_wedge_write(some_bus);
software_i2c_detach(some_bus);
in some place and then see how the hardware I2C controller deals with that afterwards.
In this case, I think the way it's written here is probably the best way to do it. You have the weirdness that all our I2C functions need a bus number, but usually the SoC is the only thing in charge of assigning bus numbers (because the SoC should know how many I2C controllers it has). The problem here is that this is a pseudo-I2C controller that's independent from the SoC, because it's tunneled over another transport interface. So we don't really have a number we can give it that we can guarantee will not interfere with the SoC's assignments. The solution chosen here is to just pick 0 as the bus number and override the software_i2c[] array only for the duration of this function, restoring the previous state afterwards, which makes sure it cannot clash with anything else that may have a different concept of what "bus 0" is.
Patch Set #5, Line 133: ast = ast_priv;
Don't you need to do this before you call set_clock() and set_data() above?
To view, visit change 35726. To unsubscribe, or for help writing mail filters, visit settings.