Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 3 Sep 2023 14:34:46 +0200
From:   Andi Shyti <andi.shyti@kernel.org>
To:     Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] i2c:octeon:Add block-mode r/w
Message-ID: <20230903123446.vjgpplnogejbzneb@zenone.zhora.eu>
References: <20230725223335.mzgdgr7qgeyc6hj7@intel.intel>
 <20230816230708.2724780-2-aryan.srivastava@alliedtelesis.co.nz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230816230708.2724780-2-aryan.srivastava@alliedtelesis.co.nz>
Precedence: bulk

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 */