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 v4] i2c: octeon: Add block-mode r/w
Date: Mon, 15 Apr 2024 23:59:35 +0200 [thread overview]
Message-ID: <o6xuyvunkceihtx4aifryfwviedx26scmlahygw5blijodmtge@c5cyhfoez5qq> (raw)
In-Reply-To: <20240415005213.3477671-1-aryan.srivastava@alliedtelesis.co.nz>
Hi Aryan,
On Mon, Apr 15, 2024 at 12:52:13PM +1200, Aryan Srivastava wrote:
> Add support for block mode read/write operations on
> Thunderx chips.
>
> When attempting r/w operations of greater then 8 bytes
> block mode is used, instead of performing a series of
> 8 byte reads.
Can you please add some more description of your patch here.
How did you do it? Which modes have you added? What are these
modes doing and how they work?
The patch is not the easiest itself and with little description
is very challenging to review. Please make my life easier :-)
> Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>
...
> +static void octeon_i2c_block_enable(struct octeon_i2c *i2c)
> +{
> + if (i2c->block_enabled || !TWSI_BLOCK_CTL(i2c))
does the block_ctl register stores the length of the message?
If it goes '0' does it mean that it's ready for a block transfer?
(same question for the disable function).
> + return;
> +
> + i2c->block_enabled = true;
> + octeon_i2c_writeq_flush(TWSI_MODE_STRETCH
> + | TWSI_MODE_BLOCK_MODE, i2c->twsi_base + TWSI_MODE(i2c));
> +}
...
> @@ -579,10 +612,7 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
> if (set_ext)
> octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
>
> - octeon_i2c_hlc_int_clear(i2c);
> - octeon_i2c_writeq_flush(cmd, i2c->twsi_base + SW_TWSI(i2c));
> -
> - ret = octeon_i2c_hlc_wait(i2c);
> + ret = octeon_i2c_hlc_cmd_send(i2c, cmd);
> if (ret)
> goto err;
Can you put the octeon_i2c_hlc_comp_read/octeon_i2c_hlc_comp_write
refactoring in a different patch as a preparatory to this one?
It's easier to review.
Please, remember to keep patches logically separated in smaller
chunks.
>
> @@ -594,6 +624,106 @@ static int octeon_i2c_hlc_comp_write(struct octeon_i2c *i2c, struct i2c_msg *msg
> return ret;
> }
>
> +/**
> + * high-level-controller composite block write+read, msg0=addr, msg1=data
This message doesn't mean much. Please check the DOC formatting
and the other functions, as well.
Remember good comments are highly appreciated.
> + * Used in the case where the i2c xfer is for greater than 8 bytes of read data.
> + */
...
> + /* read data in FIFO */
> + octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
> + for (int i = 0; i < len; i += 8) {
> + u64 rd = __raw_readq(i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
> + for (int j = 7; j >= 0; j--)
> + msgs[1].buf[i + (7 - j)] = (rd >> (8 * j)) & 0xff;
Looks good, but do you mind a comment here?
> + }
> +
> + octeon_i2c_block_disable(i2c);
> + return ret;
> +}
...
> + /* Write msg into FIFO buffer */
> + octeon_i2c_writeq_flush(TWSI_BLOCK_STS_RESET_PTR, i2c->twsi_base + TWSI_BLOCK_STS(i2c));
> + for (int i = 0; i < len; i += 8) {
> + u64 buf = 0;
> + for (int j = 7; j >= 0; j--)
> + buf |= (msgs[1].buf[i + (7 - j)] << (8 * j));
a comment here?
> + octeon_i2c_writeq_flush(buf, i2c->twsi_base + TWSI_BLOCK_FIFO(i2c));
> + }
> + if (set_ext)
> + octeon_i2c_writeq_flush(ext, i2c->twsi_base + SW_TWSI_EXT(i2c));
> +
> + /* Send command to core (send data in FIFO) */
> + ret = octeon_i2c_hlc_cmd_send(i2c, cmd);
> + if (ret)
> + return ret;
do we need to disable anything here?
Thanks for your patch,
Andi
> +
> + cmd = __raw_readq(i2c->twsi_base + SW_TWSI(i2c));
> + if ((cmd & SW_TWSI_R) == 0)
> + return octeon_i2c_check_status(i2c, false);
> +
> + octeon_i2c_block_disable(i2c);
> + return ret;
> +}
...
prev parent reply other threads:[~2024-04-15 21:59 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-05 23:45 [PATCH] i2c:octeon:Add block-mode r/w Aryan Srivastava
2023-07-25 22:33 ` Andi Shyti
2023-08-16 23:07 ` Aryan Srivastava
2023-09-03 12:34 ` Andi Shyti
2023-09-04 23:14 ` Aryan Srivastava
2023-09-05 6:27 ` kernel test robot
2023-09-05 7:52 ` kernel test robot
2023-09-05 10:22 ` Andi Shyti
2023-09-12 0:27 ` [PATCH] THUNDERX_I2C_BLOCK_MODE Aryan Srivastava
2023-09-12 3:38 ` kernel test robot
2023-09-12 0:28 ` [PATCH] i2c:octeon:Add block-mode r/w Aryan Srivastava
2023-09-12 1:16 ` [PATCH] i2c: octeon: Add " Aryan Srivastava
2024-01-16 1:38 ` Aryan Srivastava
[not found] ` <9882daa4945914886b21642837816c2d99c027ac.camel@alliedtelesis.co.nz>
2023-10-25 21:20 ` [PATCH] i2c:octeon:Add " Aryan Srivastava
2024-04-15 0:52 ` [PATCH v4] i2c: octeon: Add " Aryan Srivastava
2024-04-15 21:59 ` Andi Shyti [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=o6xuyvunkceihtx4aifryfwviedx26scmlahygw5blijodmtge@c5cyhfoez5qq \
--to=andi.shyti@kernel.org \
--cc=aryan.srivastava@alliedtelesis.co.nz \
--cc=linux-i2c@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).