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

[Aniket RANDIVE <aniket.randive@oss.qualcomm.com>]

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.

>> 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.

>> 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.

>> +        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>
>>
>>
>