i2c_xfer() should take a params struct, not individual args |
||
Issue description
i2c_xfer() takes a lot of params.
int i2c_xfer(int port, int slave_addr, const uint8_t *out, int out_size,
uint8_t *in, int in_size, int flags);
It then calls i2c_xfer_no_retry() which calls chip_i2c_xfer() with the same params. And we're now looking at adding another layer of indirection.
Instead:
struct i2c_xfer_params {
int port;
int slave_addr;
const uint8_t *out;
int out_size;
uint8_t *in
int in_size;
int flags;
}
And then i2c_xfer(struct i2c_xfer_params *p);
Any sub-functions which act on the params like notification functions can also simply take the pointer to the params struct (with const, if they're not expected to modify it).
,
Mar 20 2018
Based on the discussion and findings on the CL https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/964600, it looks like we are not actually saving anything with the i2c_xfer change to use i2c_xfer_params. Reason being that LTO is already taking care of inlining the calls and hence with the change to add i2x_xfer_params, we end up using more stack space than before. Here is some analysis with the latest version of the patchset: make BOARD=soraka SECTION=RW analyzestack Before: i2c_write16 (40) [common/i2c_master.c:220] 1009ac9c -> i2c_write16[common/i2c_master.c:235] 1009acd8 i2c_xfer (56) [common/i2c_master.c:83] 100925f4 -> i2c_xfer[common/i2c_master.c:92] 1009287e - chip_i2c_xfer[chip/npcx/i2c.c:664] - i2c_recovery[chip/npcx/i2c.c:211] -> i2c_xfer[common/i2c_master.c:92] 10092a00 - chip_i2c_xfer[chip/npcx/i2c.c:672] - i2c_master_transaction[chip/npcx/i2c.c:292] - i2c_recovery[chip/npcx/i2c.c:211] -> i2c_xfer[common/i2c_master.c:92] 10092a18 - chip_i2c_xfer[chip/npcx/i2c.c:672] - i2c_master_transaction[chip/npcx/i2c.c:296] - i2c_recovery[chip/npcx/i2c.c:211] After: i2c_write16 (48) [common/i2c_master.c:233] 1009ace6 -> i2c_write16[common/i2c_master.c:250] 1009ad2e i2c_xfer (64) [common/i2c_master.c:90] 100925f4 -> i2c_xfer[common/i2c_master.c:99] 10092890 - chip_i2c_xfer[chip/npcx/i2c.c:663] - i2c_recovery[chip/npcx/i2c.c:211] -> i2c_xfer[common/i2c_master.c:99] 10092a14 - chip_i2c_xfer[chip/npcx/i2c.c:671] - i2c_master_transaction[chip/npcx/i2c.c:292] - i2c_recovery[chip/npcx/i2c.c:211] -> i2c_xfer[common/i2c_master.c:99] 10092a2c - chip_i2c_xfer[chip/npcx/i2c.c:671] - i2c_master_transaction[chip/npcx/i2c.c:296] - i2c_recovery[chip/npcx/i2c.c:211] Additionally, the changes result in code size increase (~90-150bytes). Considering the above results, I don't think there is any gain in moving to i2c_xfer_params at this point. |
||
►
Sign in to add a comment |
||
Comment 1 by furquan@chromium.org
, Mar 15 2018