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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a503fea8fdf4156526d473ee2208b42b896b50c1.camel@baylibre.com>
Date: Mon, 12 Jan 2026 17:26:40 +0100
From: Francesco Lavra <flavra@...libre.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH] i2c: Add FTDI FT4222H USB I2C adapter

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;


Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)