Re: [PATCH v1 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

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

On 9/7/2026 12:16 PM, Mukesh Savaliya wrote:
> 
> 
> On 9/3/2026 9:44 PM, Jyothi Kumar Seerapu wrote:
> [...]
> 
>>> +static int geni_i2c_gpi_bus_cmd(struct geni_i2c_dev *gi2c, enum 
>>> i2c_op cmd)
>>> +{
>>> +    const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>>> +    struct dma_async_tx_descriptor *desc;
>>> +    struct gpi_i2c_config peripheral = {};
>>> +    struct dma_slave_config config = {};
>>> +    unsigned long time_left;
>>> +    unsigned long flags;
>>> +    dma_cookie_t cookie;
>>> +
>>> +    config.peripheral_config = &peripheral;
>>> +    config.peripheral_size = sizeof(peripheral);
>>> +
>>> +    peripheral.set_config  = 1;
>>> +    peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
>>> +    peripheral.cycle_count = itr->t_cycle_cnt;
>>> +    peripheral.high_count  = itr->t_high_cnt;
>>> +    peripheral.low_count   = itr->t_low_cnt;
>>> +    peripheral.clk_div     = itr->clk_div;
>>> +    peripheral.op          = cmd;
>>> +
>>> +    if (dmaengine_slave_config(gi2c->tx_c, &config)) {
> should capture the return value and print, as it returns specific error 
> code.>> +        dev_err(gi2c->se.dev, "dma config error for bus cmd 
> %u\n", cmd);

Yes we can capture it. I will update it in v2 patch.

>>> +        return -EIO;
>>> +    }
>>> +
> 
> [...]
> 
>>> @@ -1066,14 +1136,18 @@ static int geni_i2c_xfer(struct i2c_adapter 
>>> *adap,
>>>       else
>>>           ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
>>> -    if (!gi2c->gpi_mode &&
>>> -        (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
>>> +    if (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN ||
>>> +        (gi2c->gpi_mode && ret == -EIO)) {
>>>           /*
>>>            * Only attempt recovery if SDA is stuck low. -EPROTO and
>>>            * -ETIMEDOUT indicate bus errors where the target may be
>>>            * holding SDA low. ARB_LOST (-EAGAIN) on a single-controller
>>>            * bus indicates a stuck target, not a real arbitration loss.
>>> -         * GPI DMA mode extends this trigger separately.
>>> +         * In GPI DMA mode the completion callback reports NACK,
>>> +         * BUS_PROTO and ARB_LOST alike as -EIO, so the SE_GENI_IOS
>>> +         * RX_DATA_IN check below is what distinguishes a genuinely
>>> +         * stuck bus from a benign NACK: a NACK leaves SDA released
>>> +         * (high) after the STOP, so recovery is skipped.
> after the STOP, indicating i2c bus is in idle state, so.....>> 

Sure. Will update the comment.

>             */
>>>           if (!(readl_relaxed(gi2c->se.base + SE_GENI_IOS) & 
>>> RX_DATA_IN)) {
>>>               int recovery_ret = i2c_recover_bus(adap);
>> This does not appear to handle scenarios such as an I2C multi-message 
>> transfer where the stretch bit is set during the first message, but an 
>> error occurs while preparing the second message (for example, during 
>> memory allocation or DMA descriptor preparation). In such cases, the 
>> SCL line may remain asserted low and impact subsequent transfers.
>>   > @@ -1292,10 +1366,8 @@ static int geni_i2c_probe(struct 
>> platform_device *pdev)
>>>       if (ret < 0)
>>>           return ret;
>>> -    if (!gi2c->gpi_mode) {
>>> -        gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
>>> -        gi2c->adap.bus_recovery_info = &gi2c->rinfo;
>>> -    }
>>> +    gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
>>> +    gi2c->adap.bus_recovery_info = &gi2c->rinfo;
>>>       ret = i2c_add_adapter(&gi2c->adap);
>>>       if (ret)
>>>
>>
>