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

[Francesco Lavra <flavra@baylibre.com>]

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

On Thu, 2026-01-15 at 15:20 +0100, Andi Shyti wrote:
> > 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.
> 
> All constants need to have a comment or defined or both. A good
> comment would definitely help. As for the number of retries it
> looks a bit too much to me, but you definitely know better.

OK, I added a comment and changed the constant to 70.

> > > > +       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.
> 
> It looks to me more like a spam message or a spurious printout
> after a debugging session, rather than an informative message.
> 
> I'd remove it, but if you think that's really useful, then you
> can keep it.

Removed.


> > > > +       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.
> 
> What prevents us from removing conf12 at all?

OK, removed

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