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

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

On 3/27/2026 3:37 PM, Aniket RANDIVE wrote:
> 
> 
> On 3/27/2026 11:51 AM, Mukesh Kumar Savaliya wrote:
>>
>>
>> On 3/26/2026 10:01 AM, Aniket Randive wrote:
>>> In GPI mode, the I2C GENI driver incorrectly generates an extra TX DMA
>>> TRE on the TX channel during single read message transfer. This results
>> What's the impact of this extra DMA TRE ? do you see failure/timeout, 
>> anything ?
> 
> This write operation is unnecessary. For a 1‑byte read operation, only 
> the CONFIG, GO and RX DMA TRE are required. However, an additional TX 
> DMA TRE is currently being added. In addition to being redundant, this 
> also results in unnecessary DMA buffer mapping for the TX DMA TRE.
> 
Ok, makes sense.
>>> in an unnecessary write operation on the I2C bus, which is not required.
>>>
>>> Update the logic to avoid generating the extra TX DMA TRE for single
>>> read message, ensuring correct behavior and preventing redundant
>>> transfers.
>>>
>> So for read, we do unwanted write too ? if so, please write it 
>> accordingly.  Correct behavior needs to be justified against wrong.
> 
> Yes. Currently, the driver performs an unnecessary write as part of a 
> read transaction. For a single‑byte read operation, the correct behavior 
> is to issue only the CONFIG, GO command, and an RX DMA TRE. This TX DMA 
> TRE does not contribute to the read operation and results in an 
> unintended write and redundant DMA buffer mapping. Hence, the current 
> behavior is incorrect and should be fixed to align with the required 
> hardware transaction sequence.
> 
Makes sense, please add such details in the commit log of next patch to 
make it clear against the problem and solution. Rest of the changes 
looks good.

>>> 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>
>>> ---
>>>   drivers/i2c/busses/i2c-qcom-geni.c | 18 +++++++++++++-----
>>>   1 file changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/ 
>>> i2c-qcom-geni.c
>>> index a4acb78fafb6..2706309bbebb 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,11 @@ 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;
>>> +    if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) {
>>> +        peripheral->multi_msg = true;
>> what's the actual meaning of multi_msg here ? IIUC, this multi_msg is 
>> set to true for single transfer  ? any better name if so ? Yes, need 
>> to change it out of this patch.
> 
> In the GPI driver, a DMA TRE is created only when either the operation 
> is a read or when multi_msg is set to false. This is controlled by the 
> following check during I2C TRE construction,
> 
>      if (i2c->op == I2C_READ || i2c->multi_msg == false) {
>          /* create the DMA TRE */
>          tre = &desc->tre[tre_idx];
> 
> Previously, when dmaengine_prep_slave_single() was invoked for a write 
> operation, this condition evaluated to true and a TX DMA TRE was created 
> on the TX channel. With the recent change, the flag is explicitly set, 
> which correctly prevents creation of a TX DMA TRE. I agree the variable 
> name can be improved for clarity and propose addressing that in a 
> follow‑up patch to keep this change minimal and focused.
> 
Right, that was my intent too. I think the variable name is misleading.

>>> +        goto skip_dma;
>>> +    }
>>> +
>>>       dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1);
>>>       if (!dma_buf) {
>>>           ret = -ENOMEM;
>>> @@ -668,6 +673,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev 
>>> *gi2c, struct i2c_msg msgs[],
>>>           flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>>>       }
>>> +skip_dma:
>>>       /* set the length as message for rx txn */
>>>       peripheral->rx_len = msgs[msg_idx].len;
>>>       peripheral->op = op;
>>> @@ -740,9 +746,11 @@ 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);
>>> +    if (op == I2C_WRITE && (msgs[msg_idx].flags & I2C_M_RD)) {
>>> +        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;
>>>
>>> ---
>>> base-commit: 785f0eb2f85decbe7c1ef9ae922931f0194ffc2e
>>> change-id: 20260325-skip_extra_dma_tre-a3cf22f81d9b
>>>
>>> Best regards,
>>> -- 
>>> Aniket Randive <aniket.randive@oss.qualcomm.com>
>>>
>>>
>>
>