[PATCH V4] i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode

[PATCH V4] i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode

[Aniket Randive]

In GPI mode, the I2C GENI driver programs an extra TX DMA transfer
descriptor (TRE) on the TX channel when handling a single read message.
This results in an unintended write phase being issued on the I2C bus,
even though a read transaction does not require any TX data.

For a single-byte read, the correct hardware sequence consists of the
CONFIG and GO commands followed by a single RX DMA TRE. Programming an
additional TX DMA TRE is redundant, causes unnecessary DMA buffer
mapping on the TX channel, and may lead to incorrect bus behavior.

Update the transfer logic to avoid programming a TX DMA TRE for single
read messages in GPI mode.

Co-developed-by: Maramaina Naresh <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Maramaina Naresh <naresh.maramaina@oss.qualcomm.com>
Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
---

Changes in v4:
  - Added some more description in comment and changed the label name.
Changes in v3:
  - Added comment in the driver for better readability and changed the
    position of 'skip_dma' label to allow dma engine configuration.
Changes in v2:
  - Updated the commit message.

 drivers/i2c/busses/i2c-qcom-geni.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a4acb78fafb6..a482a4c60744 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -625,8 +625,8 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
 {
 	struct gpi_i2c_config *peripheral;
 	unsigned int flags;
-	void *dma_buf;
-	dma_addr_t addr;
+	void *dma_buf = NULL;
+	dma_addr_t addr = 0;
 	enum dma_data_direction map_dirn;
 	enum dma_transfer_direction dma_dirn;
 	struct dma_async_tx_descriptor *desc;
@@ -639,6 +639,16 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
 	gi2c_gpi_xfer = &gi2c->i2c_multi_desc_config;
 	msg_idx = gi2c_gpi_xfer->msg_idx_cnt;
 
+	/*
+	 * Skip TX DMA mapping for a read message (I2C_M_RD) to avoid
+	 * programming an extra TX DMA TRE that would cause an unintended
+	 * write cycle on the I2C bus before the actual read operation.
+	 */
+	if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) {
+		peripheral->multi_msg = true;
+		goto skip_tx_dma_map;
+	}
+
 	dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1);
 	if (!dma_buf) {
 		ret = -ENOMEM;
@@ -658,6 +668,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
 		goto out;
 	}
 
+skip_tx_dma_map:
 	if (gi2c->is_tx_multi_desc_xfer) {
 		flags = DMA_CTRL_ACK;
 
@@ -740,9 +751,12 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
 	return 0;
 
 err_config:
-	dma_unmap_single(gi2c->se.dev->parent, addr,
-			 msgs[msg_idx].len, map_dirn);
-	i2c_put_dma_safe_msg_buf(dma_buf, &msgs[msg_idx], false);
+	/* Avoid DMA unmap as the write operation skipped DMA mapping */
+	if (dma_buf) {
+		dma_unmap_single(gi2c->se.dev->parent, addr,
+				 msgs[msg_idx].len, map_dirn);
+		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[msg_idx], false);
+	}
 
 out:
 	gi2c->err = ret;
-- 
2.34.1

Re: [PATCH V4] i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode

[Mukesh Kumar Savaliya]

On 4/10/2026 3:49 PM, Aniket Randive wrote:
> In GPI mode, the I2C GENI driver programs an extra TX DMA transfer
> descriptor (TRE) on the TX channel when handling a single read message.
> This results in an unintended write phase being issued on the I2C bus,
> even though a read transaction does not require any TX data.
> 
> For a single-byte read, the correct hardware sequence consists of the
> CONFIG and GO commands followed by a single RX DMA TRE. Programming an
> additional TX DMA TRE is redundant, causes unnecessary DMA buffer
> mapping on the TX channel, and may lead to incorrect bus behavior.
> 
> Update the transfer logic to avoid programming a TX DMA TRE for single
> read messages in GPI mode.
> 
> Co-developed-by: Maramaina Naresh <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Maramaina Naresh <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
Reviewed-by: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>
> ---
> 
> Changes in v4:
>    - Added some more description in comment and changed the label name.
> Changes in v3:
>    - Added comment in the driver for better readability and changed the
>      position of 'skip_dma' label to allow dma engine configuration.
> Changes in v2:
>    - Updated the commit message.
> 
>   drivers/i2c/busses/i2c-qcom-geni.c | 24 +++++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a4acb78fafb6..a482a4c60744 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -625,8 +625,8 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],

[...]

Re: [PATCH V4] i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode

[Andi Shyti]

Hi Aniket,

On Fri, Apr 10, 2026 at 03:49:49PM +0530, Aniket Randive wrote:
> In GPI mode, the I2C GENI driver programs an extra TX DMA transfer
> descriptor (TRE) on the TX channel when handling a single read message.
> This results in an unintended write phase being issued on the I2C bus,
> even though a read transaction does not require any TX data.
> 
> For a single-byte read, the correct hardware sequence consists of the
> CONFIG and GO commands followed by a single RX DMA TRE. Programming an
> additional TX DMA TRE is redundant, causes unnecessary DMA buffer
> mapping on the TX channel, and may lead to incorrect bus behavior.
> 
> Update the transfer logic to avoid programming a TX DMA TRE for single
> read messages in GPI mode.
> 
> Co-developed-by: Maramaina Naresh <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Maramaina Naresh <naresh.maramaina@oss.qualcomm.com>
> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>

merged to i2c/i2c-host.

Thank you and thanks Mukesh for the review.

Andi

Claude review: i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode

[Claude Code Review Bot]

Overall Series Review

Subject: i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>
Patches: 3
Reviewed: 2026-04-12T09:50:25.095463

---

This is a single-patch series (v4) fixing a real bug in the Qualcomm GENI I2C GPI DMA path. The problem is that when `geni_i2c_gpi_xfer()` handles a read message, it always calls `geni_i2c_gpi()` with `op=I2C_WRITE` first (for the CONFIG+GO TREs on the TX channel), but on the first message (i=0), `peripheral.multi_msg` is initialized to `false`. This causes the GPI DMA driver (`gpi_create_i2c_tre()`) to generate a DMA data TRE alongside the CONFIG+GO TREs, resulting in a spurious write on the I2C bus before the intended read.

The fix is conceptually correct: it sets `multi_msg = true` before `dmaengine_slave_config()` copies the peripheral config, which tells the GPI DMA driver to suppress the DMA data TRE and emit only CONFIG+GO. However, the implementation has some fragility and readability issues worth addressing.

**Verdict:** The fix addresses a real bug but the approach could be cleaner. A few items need attention before merging.

---

---
Generated by Claude Code Patch Reviewer

Claude review: i2c: qcom-geni: Avoid extra TX DMA TRE for single read message in GPI mode

[Claude Code Review Bot]

Patch Review

**Correctness of the core fix:**

The key insight is that `gpi_create_i2c_tre()` in the GPI DMA driver decides whether to emit a DMA TRE based on:
```c
if (i2c->op == I2C_READ || i2c->multi_msg == false)
    /* generate DMA TRE */
```

When `op == I2C_WRITE` and `multi_msg == true`, the DMA TRE is suppressed and only CONFIG+GO TREs are generated on the TX channel — which is exactly the correct sequence for a read transaction. The patch achieves this by setting `multi_msg = true` before the `dmaengine_slave_config()` call copies the config. This is correct.

**Issue 1: `dmaengine_prep_slave_single()` still called with `addr=0`**

After the `goto skip_tx_dma_map`, the function still reaches:
```c
desc = dmaengine_prep_slave_single(dma_chan, addr, msgs[msg_idx].len,
                                   dma_dirn, flags);
```
with `addr = 0` and `dma_dirn = DMA_MEM_TO_DEV`. This works today because the GPI driver doesn't use the scatterlist address when the DMA TRE is suppressed, but it's fragile — passing DMA address 0 through the DMA engine API is not a clean pattern. If the GPI driver or DMA engine core ever adds validation on the DMA address, this will break. Consider adding a comment at the `skip_tx_dma_map` label explaining that `addr=0` is safe because `gpi_create_i2c_tre()` suppresses the DMA TRE when `multi_msg=true && op=I2C_WRITE`.

**Issue 2: The comment is misleading**

```c
/*
 * Skip TX DMA mapping for a read message (I2C_M_RD) to avoid
 * programming an extra TX DMA TRE that would cause an unintended
 * write cycle on the I2C bus before the actual read operation.
 */
```

The comment says "skip TX DMA mapping" but the actual mechanism that suppresses the TRE is setting `multi_msg = true` — the skip of the mapping is a secondary effect. The comment should explain that `multi_msg = true` is what tells the GPI DMA driver to not generate a DMA data TRE, and the DMA buffer allocation is also skipped since no data transfer occurs.

**Issue 3: Operator precedence — minor style nit**

```c
if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) {
```

While this evaluates correctly due to C operator precedence (`&&` binds looser than `&`), adding explicit parentheses is standard kernel style for bitwise-AND used as a boolean:
```c
if (op == I2C_WRITE && (msgs[msg_idx].flags & I2C_M_RD)) {
```

**Issue 4: Title says "single read message" but fix applies to all reads**

The subject line and commit message emphasize "single read message" and "single-byte read," but the condition `msgs[msg_idx].flags & I2C_M_RD` matches any read message regardless of count or length. In practice the bug only manifests for the first message (i=0) because after the first iteration `multi_msg` is already `true`, but the condition itself is general. The title/description should be more precise — perhaps "Avoid extra TX DMA TRE for read messages in GPI mode."

**Issue 5: The error-path guard is correct but the comment could be better**

```c
/* Avoid DMA unmap as the write operation skipped DMA mapping */
if (dma_buf) {
```

The guard is correct — `dma_buf` is NULL when the mapping was skipped. However, the comment says "write operation skipped DMA mapping" which is confusing because we're talking about a read message that had its TX-side DMA mapping skipped. Suggest: "DMA unmap is not needed when TX DMA mapping was skipped for a read message."

**Issue 6: `map_dirn` uninitialized in the skip path — safe but worth noting**

When the `goto skip_tx_dma_map` is taken, `map_dirn` is never assigned. It's only used in the `err_config` path inside the `if (dma_buf)` guard, so it's safe. But future refactors might break this assumption. Consider initializing `map_dirn` alongside the other zero-initializations at the top.

**Overall:** The functional fix is correct and addresses a real hardware issue. The concerns are about code hygiene and maintainability rather than correctness. I'd suggest at minimum fixing the parentheses (issue 3), improving the comments (issues 2 and 5), and adding a note about why `addr=0` is safe (issue 1).

---
Generated by Claude Code Patch Reviewer