Hi Aryan,
[...]
> I have re-ran the patch through checkpatch --strict and changed the
> comments
Thanks!
[...]
> +static void octeon_i2c_block_disable(struct octeon_i2c *i2c)
> +{
> + if (!i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
> + return;
> +
> + i2c->block_enabled = false;
> + octeon_i2c_writeq_flush(TWSI_MODE_STRETCH, i2c->twsi_base + TWSI_MODE(i2c));
> +}
can you leave a blank line here?
> /**
> * octeon_i2c_hlc_wait - wait for an HLC operation to complete
> * @i2c: The struct octeon_i2c
> @@ -268,6 +286,7 @@ static int octeon_i2c_start(struct octeon_i2c *i2c)
> u8 stat;
>
> octeon_i2c_hlc_disable(i2c);
> + octeon_i2c_block_disable(i2c);
>
> octeon_i2c_ctl_write(i2c, TWSI_CTL_ENAB | TWSI_CTL_STA);
> ret = octeon_i2c_wait(i2c);
> @@ -594,6 +613,128 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
> return ret;
> }
>
> +/* high-level-controller composite block write+read, m[0]len<=2, m[1]len<=1024 */
can you please improve the comment?
> +static int octeon_i2c_hlc_block_comp_read(struct octeon_i2c *i2c, struct i2c_msg *msgs)
> +{
> + int i, j, len, ret = 0;
> + u64 cmd, rd;
> +
> + octeon_i2c_hlc_enable(i2c);
> + octeon_i2c_block_enable(i2c);
> +
> + len = msgs[1].len - 1;
why -1?
> + /* Prepare core command */
> + cmd = SW_TWSI_V | SW_TWSI_R | SW_TWSI_SOVR;
cmd needs to be initialized to '0'.
> + /* SIZE */
> + octeon_i2c_writeq_flush((u64)(len), i2c->twsi_base + TWSI_BLOCK_CTL(i2c));
> + /* ADDR */
> + cmd |= (u64)(msgs[0].addr & 0x7full) << SW_TWSI_ADDR_SHIFT;
can you please write some more meaningful comments?
[...]
> + /* Wait for transaction to complete */
> + ret = octeon_i2c_hlc_wait(i2c);
> + if (ret)
> + goto err;
just return err, no point having a goto label here.
[...]
> +err:
> + return ret;
> +}
> +
> +/* high-level-controller composite block write+write, m[0]len<=2, m[1]len<=1024 */
> +static int octeon_i2c_hlc_block_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msgs)
more or less same comments apply here as _comp_read()
[...]
> @@ -720,6 +869,7 @@ int octeon_i2c_init_lowlevel(struct octeon_i2c *i2c)
> /* toggle twice to force both teardowns */
> octeon_i2c_hlc_enable(i2c);
> octeon_i2c_hlc_disable(i2c);
> +
this change does not belong here
> return 0;
> }
>
[...]
> #define TWSI_INT_SDA BIT_ULL(10)
> #define TWSI_INT_SCL BIT_ULL(11)
>
> +#define TWSI_MODE_STRETCH BIT_ULL(1)
> +#define TWSI_MODE_BLOCK_MODE BIT_ULL(2)
> +
> +#define TWSI_BLOCK_STS_RESET_PTR BIT_ULL(0)
> +#define TWSI_BLOCK_STS_BUSY BIT_ULL(1)
The alignment here is a bit off.
Andi
> #define I2C_OCTEON_EVENT_WAIT 80 /* microseconds */
