[v7,1/2] i2c: octeon: refactor common i2c operations

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(-)

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