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

[Aniket RANDIVE]

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)