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
