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

[Francesco Lavra <flavra@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]

Hi Andi,

On Wed, 2026-01-07 at 14:38 +0100, Andi Shyti wrote:
> Hi Francesco,
> 
> I did just a quick initial review.

...

> > +static int ft4222_i2c_get_status(struct ft4222_i2c *ftdi)
> > +{
> > +       u8 status;
> > +       int retry;
> > +       const int max_retries = 512;
> 
> why 512?

These retries are needed mostly when retrieving the status after doing a
write with the I2C bus operating at a low speed.
Doing various tests at 100 kHz, I saw 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 we may need more retries, for example if
I disconnect the SCL line and then try to do a write, I see that up to 64
retries are needed.
So I guess 512 max_retries is too much, but we need a value > 64. Does 128
sound OK to you? Perhaps I can add a comment with the above observations.

> > +       for (retry = 0; retry < max_retries; retry++) {
> > +               int ret = ft4222_cmd_get(ftdi, 0xf5b4, &status);
> > +
> > +               if (ret < 0)
> > +                       return ret;
> > +               if (!(status & FT4222_I2C_CTRL_BUSY))
> > +                       break;
> > +       }
> > +       dev_dbg(&ftdi->adapter.dev, "status 0x%02x (%d retries)",
> > status,
> > +               retry);
> 
> whould this debug message be printed after the check below?

Not sure I understood your question. Are you suggesting to move the
dev_dbg() call after the check below? I put it before the check so that I
get a debug message even if the maximum number of retries has been reached.

> > +       if (retry == max_retries) {
> > +               ft4222_i2c_reset(ftdi);
> > +               return -ETIMEDOUT;
> > +       }
> > +       if (!(status & FT4222_I2C_ERROR))
> > +               return 0;
> > +       if (status & FT4222_I2C_ADDR_NACK)
> > +               return -ENXIO;
> > +       else if (status & FT4222_I2C_DATA_NACK)
> > +               return -EIO;
> > +       else
> > +               return -EBUSY;
> > +}

...

> > +static int ft4222_i2c_probe(struct usb_interface *interface,
> > +                           const struct usb_device_id *id)
> > +{
> > +       enum ft4222_conf_mode conf_mode;
> > +       int ret = ft4222_get_conf(interface, &conf_mode);
> > +       int intf = interface->cur_altsetting->desc.bInterfaceNumber;
> > +
> > +       if (ret)
> > +               return ret;
> > +       if (((conf_mode == ft4222_conf0) || (conf_mode ==
> > ft4222_conf3)) &&
> 
> what about conf12?

As mentioned in the commit message, the I2C functionality is available only
when the chip is configured in mode 0 or 3. In modes 1 and 2, the chip
implements other functionalities, e.g. SPI.
I will add a comment in this function.

> > +           (intf == 0))
> > +               return ft4222_i2c_init(interface);
> > +       return -ENODEV;


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 659 bytes --]