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.