| Message ID | 20240613025412.3848629-2-aryan.srivastava@alliedtelesis.co.nz |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/2] i2c: octeon: refactor hlc r/w operations | expand |
> Refactor the current implementation of the high-level composite read and > write operations in preparation of the addition of block-mode read/write > operations. … * I find that a cover letter can be helpful also for the presented small patch series. * How do you think about to replace any abbreviations in summary phrases? - HLC - r/w Regards, Markus
Hi Aryan, > +/* Construct and send i2c transaction core cmd for read ops */ > +static int octeon_i2c_hlc_read_cmd(struct octeon_i2c *i2c, struct i2c_msg msg, u64 cmd) > +{ > + u64 ext = 0; > + > + if (octeon_i2c_hlc_ext(i2c, msg, &cmd, &ext)) > + octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c)); I think this check here is the only logical change I see. Right? If so, can you please describe in the log why you made this change? Thanks, Andi > + return octeon_i2c_hlc_cmd_send(i2c, cmd); > +}
Hi Andi, On Fri, 2024-06-14 at 00:55 +0100, Andi Shyti wrote: > Hi Aryan, > > > +/* Construct and send i2c transaction core cmd for read ops */ > > +static int octeon_i2c_hlc_read_cmd(struct octeon_i2c *i2c, struct > > i2c_msg msg, u64 cmd) > > +{ > > + u64 ext = 0; > > + > > + if (octeon_i2c_hlc_ext(i2c, msg, &cmd, &ext)) > > + octeon_i2c_writeq_flush(ext, i2c->twsi_base + This used to be called within this block previously for the read function only: if (msgs[0].len == 2) { u64 ext = 0; cmd |= SW_TWSI_EIA; ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT; octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c)); } else { cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; } This is the write function equivelant: if (msgs[0].len == 2) { cmd |= SW_TWSI_EIA; ext |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; set_ext = true; cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT; } else { cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; } They are nearly identical blocks of code, bar the one write. So to use this block generically for both write and read, then I have to pull out this line and run it only in the read case. Therefore this is more of a reordering of operations, as the set_ext flag now gets set where the line used to run, and is used to condition the running of the line. > > SW_TWSI_EXT(i2c)); > > I think this check here is the only logical change I see. Right? > > If so, can you please describe in the log why you made this > change? > > Thanks, > Andi > > > + return octeon_i2c_hlc_cmd_send(i2c, cmd); > > +} Thanks, Aryan.
Hi Markus, On Thu, 2024-06-13 at 08:32 +0200, Markus Elfring wrote: > > Refactor the current implementation of the high-level composite > > read and > > write operations in preparation of the addition of block-mode > > read/write > > operations. > … > > * I find that a cover letter can be helpful also for the presented > small patch series. > I did make one, but I am not sure what happened to it. The link on the lore.kernel thread for it is broken, I might have generated it incorrectly? Hopefully the next patch series will have intact. > * How do you think about to replace any abbreviations in summary > phrases? > - HLC > - r/w > Done. > > Regards, > Markus Thanks, Aryan
diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c index 845eda70b8ca..6772359ca6c8 100644 --- a/drivers/i2c/busses/i2c-octeon-core.c +++ b/drivers/i2c/busses/i2c-octeon-core.c @@ -485,6 +485,50 @@ static int octeon_i2c_hlc_write(struct octeon_i2c *i2c, struct i2c_msg *msgs) return ret; } +/* Process hlc transaction */ +static int octeon_i2c_hlc_cmd_send(struct octeon_i2c *i2c, u64 cmd) +{ + octeon_i2c_hlc_int_clear(i2c); + octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c)); + + return octeon_i2c_hlc_wait(i2c); +} + +/* Generic consideration for extended internal addresses in i2c hlc r/w ops */ +static bool octeon_i2c_hlc_ext(struct octeon_i2c *i2c, struct i2c_msg msg, u64 *cmd_in, u64 *ext) +{ + bool set_ext = false; + u64 cmd; + + if (msg.flags & I2C_M_TEN) + cmd |= SW_TWSI_OP_10_IA; + else + cmd |= SW_TWSI_OP_7_IA; + + if (msg.len == 2) { + cmd |= SW_TWSI_EIA; + *ext = (u64)msg.buf[0] << SW_TWSI_IA_SHIFT; + cmd |= (u64)msg.buf[1] << SW_TWSI_IA_SHIFT; + set_ext = true; + } else { + cmd |= (u64)msg.buf[0] << SW_TWSI_IA_SHIFT; + } + + *cmd_in |= cmd; + return set_ext; +} + +/* Construct and send i2c transaction core cmd for read ops */ +static int octeon_i2c_hlc_read_cmd(struct octeon_i2c *i2c, struct i2c_msg msg, u64 cmd) +{ + u64 ext = 0; + + if (octeon_i2c_hlc_ext(i2c, msg, &cmd, &ext)) + octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c)); + + return octeon_i2c_hlc_cmd_send(i2c, cmd); +} + /* high-level-controller composite write+read, msg0=addr, msg1=data */ static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs) { @@ -499,26 +543,8 @@ static int octeon_i2c_hlc_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs /* A */ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT; - if (msgs[0].flags & I2C_M_TEN) - cmd |= SW_TWSI_OP_10_IA; - else - cmd |= SW_TWSI_OP_7_IA; - - if (msgs[0].len == 2) { - u64 ext = 0; - - cmd |= SW_TWSI_EIA; - ext = (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; - cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT; - octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c)); - } else { - cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; - } - - octeon_i2c_hlc_int_clear(i2c); - octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c)); - - ret = octeon_i2c_hlc_wait(i2c); + /* Send core command */ + ret = octeon_i2c_hlc_cmd(i2c, msgs[0], cmd); if (ret) goto err; @@ -554,19 +580,8 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg /* A */ cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT; - if (msgs[0].flags & I2C_M_TEN) - cmd |= SW_TWSI_OP_10_IA; - else - cmd |= SW_TWSI_OP_7_IA; - - if (msgs[0].len == 2) { - cmd |= SW_TWSI_EIA; - ext |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; - set_ext = true; - cmd |= (u64)msgs[0].buf[1] << SW_TWSI_IA_SHIFT; - } else { - cmd |= (u64)msgs[0].buf[0] << SW_TWSI_IA_SHIFT; - } + /* Set parameters for extended message (if required) */ + set_ext = octeon_i2c_hlc_ext(i2c, msgs[0], &cmd, &ext); for (i = 0, j = msgs[1].len - 1; i < msgs[1].len && i < 4; i++, j--) cmd |= (u64)msgs[1].buf[j] << (8 * i); @@ -579,10 +594,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg if (set_ext) octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c)); - octeon_i2c_hlc_int_clear(i2c); - octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c)); - - ret = octeon_i2c_hlc_wait(i2c); + ret = octeon_i2c_hlc_cmd_send(i2c, cmd); if (ret) goto err;
Refactor the current implementation of the high-level composite read and write operations in preparation of the addition of block-mode read/write operations. The sending of the i2c command is generic and will apply for both the block-mode and non-block-mode R/W. Extract this from the current hlc ops, and place into a generic function, octeon_i2c_hlc_cmd_send. The considerations made for extended addresses in the command construction are common for all r/w cases, extract these into octeon_i2c_hlc_ext. There are parts of the commands construction which are common (only in the read case), extract this and place into generic function octeon_i2c_hlc_read_cmd. The write commands cannot be made entirely into common code as there are distinct differences in the block mode and non-block-mode process. Particularly the writing of data into the buffer. Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz> --- drivers/i2c/busses/i2c-octeon-core.c | 86 ++++++++++++++++------------ 1 file changed, 49 insertions(+), 37 deletions(-)