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

[Andi Shyti <andi.shyti@kernel.org>]

> 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.

> > > +       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.

> > > +       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?

Andi