| Message ID | 20240620041746.3315255-2-aryan.srivastava@alliedtelesis.co.nz |
|---|---|
| State | Superseded |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: octeon: Add block-mode r/w | expand |
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)); > + What I meant last time is that there is still a change here. I understand the common parts you addressed in my previous review, but you're still missing this... > + 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)); ... this! While I don’t know the hardware internals, this is a logical change that requires justification, especially when compared to what you’ve described in the commit message. Andi > - ret = octeon_i2c_hlc_wait(i2c); > + /* Send core command */ > + ret = octeon_i2c_hlc_read_cmd(i2c, msgs[0], cmd); > if (ret) > goto err;
Hi Andi On Wed, 2024-09-11 at 11:19 +0200, 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 + > > SW_TWSI_EXT(i2c)); > > + > > What I meant last time is that there is still a change here. I > understand the common parts you addressed in my previous review, > but you're still missing this... > > > + 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)); > > ... this! While I don’t know the hardware internals, this is a > logical change that requires justification, especially when > compared to what you’ve described in the commit message. > Yes you are right, I've add some info to the commit message to describe exactly what I'm trying to achieve here. > Andi > > > - ret = octeon_i2c_hlc_wait(i2c); > > + /* Send core command */ > > + ret = octeon_i2c_hlc_read_cmd(i2c, msgs[0], cmd); > > if (ret) > > goto err; Aryan
diff --git a/drivers/i2c/busses/i2c-octeon-core.c b/drivers/i2c/busses/i2c-octeon-core.c index 845eda70b8ca..0b4a602dab8b 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_read_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 ops. 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 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(-)