Re: [PATCH v2] i2c: Add FTDI FT4222H USB I2C adapter

[Francesco Lavra <flavra@baylibre.com>]

Hi Andi,

On Thu, 2026-01-22 at 11:37 +0100, Andi Shyti wrote:
> Hi Francesco,
> 
> ...
> 
> > +static int ft4222_i2c_get_status(struct ft4222_i2c *ftdi)
> > +{
> > +       /*
> > +        * Multiple retries are needed mostly when retrieving the
> > controller
> > +        * status after doing a write with the I2C bus operating at a
> > low speed.
> > +        * Empirical tests conducted at 100 kHz showed that after a
> > +        * maximum-sized (512-byte) write, up to 11 retries are needed
> > before
> > +        * the chip clears its CTRL_BUSY flag. But under certain
> > conditions more
> > +        * retries may be needed: for example, when trying to do a
> > write after
> > +        * disconnecting the SCL line from the I2C slave, tests showed
> > that up
> > +        * to 64 retries are needed.
> > +        */
> > +       const int max_retries = 70;
> > +       int retry;
> > +       u8 status;
> > +
> > +       for (retry = 0; retry < max_retries; retry++) {
> > +               int ret = ft4222_cmd_get(ftdi, FT4222_CMD_I2C_STATUS,
> > &status);
> > +
> > +               if (ret < 0)
> > +                       return ret;
> > +               if (!(status & FT4222_I2C_CTRL_BUSY))
> > +                       break;
> 
> How about adding a small delay here. Does it also help to reduce
> the maximum number of retries?

Yes, I tried adding a call to msleep(10) here, and with that delay I have
never seen the retry variable higher than 4 at the end of the loop.
So I will add msleep(10), and change the max_retries constant to something
like 5.

> > +       }
> 
> ...
> 
> > +       dev_dbg(&adapter->dev, "transfer with %d message(s)", num);
> > +       for (i = 0; i < num; ++i) {
> > +               const u8 addr = msg[i].addr;
> > +               const u16 len = msg[i].len;
> > +               u8 *buf = msg[i].buf;
> > +               u8 flags;
> > +
> > +               flags = ((i == 0) ? FT4222_FLAG_START :
> > FT4222_FLAG_RESTART);
> > +               if (i == num - 1)
> > +                       flags |= FT4222_FLAG_STOP;
> 
> Are we setting here both STOP and RESTART at the same time.

Yes, for each I2C message the driver must set the correct flags in order
for the adapter to create the appropriate I2C bus conditions.
So for example in a single-message transfer, we set (FT4222_FLAG_START |
FT4222_FLAG_STOP); in a 2-message transfer, we set (FT4222_FLAG_START) for
the first message and (FT4222_FLAG_RESTART | FT4222_FLAG_STOP) for the
second message.

> The rest looks good to me.