Re: [PATCH RFC/RFT 1/3] i2c: omap24xx_i2c: Use new function __omap24_i2c_xfer_msg()

[Aniket Limaye]

Hello Heiko,

On 06/03/25 11:21, Heiko Schocher wrote:
Hello Aniket,On 04.03.25 23:04, Aniket Limaye wrote:
Remove __omap24_i2c_read/write() usage from omap_i2c_xfer() in favour of
the more flexible __omap24_i2c_xfer_msg().
Consequently, these are also no longer needed when DM_I2C is enabled.

New function __omap24_i2c_xfer_msg() will take care of individual read
OR write transfers with a target device. It goes through below sequence:
- Program the provided Target Chip address (OMAP_I2C_SA_REG)
- Program the provided Data len (OMAP_I2C_CNT_REG)
- Program the provided Control register flags (OMAP_I2C_CON_REG)
- Read from or Write to the provided Data buffer (OMAP_I2C_DATA_REG)

For a detailed programming guide, refer to the TRM[0] (12.1.3.4 I2C
Programming Guide).

This patch by itself should be a transparent change. However this is
needed for implementing a proper Repeated Start (Sr) functionality for
i2c_msgs.

Previous implementation for omap_i2c_xfer called __omap24_i2c_read/write
functions, with hardcoded addr=0 and alen=0 for each i2c_msg. Each of
these calls would program the registers always with a Stop bit set, not
allowing for a repeated start between i2c_msgs in the same xfer().

[0]: https://www.ti.com/lit/zip/spruj28 (TRM)

Signed-off-by: Aniket Limaye <a-lim...@ti.com>
---
  drivers/i2c/omap24xx_i2c.c | 139 ++++++++++++++++++++++++++++++++-----
  1 file changed, 121 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
index ebe472e20cd..0d044121d27 100644
--- a/drivers/i2c/omap24xx_i2c.c
+++ b/drivers/i2c/omap24xx_i2c.c
@@ -535,6 +535,107 @@ pr_exit:
      return res;
  }
+static int __omap24_i2c_xfer_msg(void __iomem *i2c_base, int ip_rev, 
int waitdelay,
+                 uchar chip, uchar *buffer, int len, u16 i2c_con_reg)
+{
+    int i;
+    u16 status;
+    int i2c_error = 0;
+    int timeout = I2C_TIMEOUT;
+
+    if (len < 0) {
+        puts("I2C xfer: data len < 0\n");
+        return 1;
+    }
+
+    if (!buffer) {
+        puts("I2C xfer: NULL pointer passed\n");
+        return 1;
+    }
+
+    if (!(i2c_con_reg & I2C_CON_EN)) {
+        puts("I2C xfer: I2C_CON_EN not set\n");
+        return 1;
+    }
Can we use here (and above) a negative error code? Later you only
check in omap_i2c_xfer() if returncode is ! 0 and return "-EREMOTEIO"
may, you can return here better codes and simply return than this
error codes from omap_i2c_xfer()  >
And may you change the strings from the puts call to a more driver specifc
string ... so its clear from which i2c driver the error message comes...
please fix this globally in your patchset.

yeah that makes sense, will update the function with proper error codes 
and error prints.
+
+    /* Set slave address */
+    omap_i2c_write_reg(i2c_base, ip_rev, chip, OMAP_I2C_SA_REG);
+    /* Read/Write len bytes data */
+    omap_i2c_write_reg(i2c_base, ip_rev, len, OMAP_I2C_CNT_REG);
+    /* Configure the I2C_CON register */
+    omap_i2c_write_reg(i2c_base, ip_rev, i2c_con_reg, OMAP_I2C_CON_REG);
+
+    /* read/write data bytewise */
+    for (i = 0; i < len; i++) {
+        status = wait_for_event(i2c_base, ip_rev, waitdelay);
+        /* Ignore I2C_STAT_RRDY in transmitter mode */
+        if (i2c_con_reg & I2C_CON_TRX)
+            status &= ~I2C_STAT_RRDY;
+        else
+            status &= ~I2C_STAT_XRDY;
+
+        /* Try to identify bus that is not padconf'd for I2C */
+        if (status == I2C_STAT_XRDY) {
+            i2c_error = 2;
+            printf("I2C xfer: pads on bus probably not configured 
(status=0x%x)\n",
+                   status);
+            goto xfer_exit;
+        }
+        if (status == 0 || (status & I2C_STAT_NACK)) {
+            i2c_error = 1;
+            printf("I2C xfer: error waiting for ACK (status=0x%x)\n",
+                   status);
+            goto xfer_exit;
+        }
+        if (status & I2C_STAT_XRDY) {
+            /* Transmit data */
+            omap_i2c_write_reg(i2c_base, ip_rev,
+                       buffer[i], OMAP_I2C_DATA_REG);
+            omap_i2c_write_reg(i2c_base, ip_rev,
+                       I2C_STAT_XRDY, OMAP_I2C_STAT_REG);
+        }
+        if (status & I2C_STAT_RRDY) {
+            /* Receive data */
+            *buffer++ = omap_i2c_read_reg(i2c_base, ip_rev,
+                            OMAP_I2C_DATA_REG);
+            omap_i2c_write_reg(i2c_base, ip_rev,
+                       I2C_STAT_RRDY, OMAP_I2C_STAT_REG);
+        }
+    }
+
+    /*
+     * poll ARDY bit for making sure that last byte really has been
+     * transferred on the bus.
+     */
+    do {
+        status = wait_for_event(i2c_base, ip_rev, waitdelay);
+    } while (!(status & I2C_STAT_ARDY) && timeout--);
+    if (timeout <= 0) {
+        puts("I2C xfer: timed out on last byte!\n");
+        i2c_error = 1;
+        goto xfer_exit;
+    } else {
+        omap_i2c_write_reg(i2c_base, ip_rev, I2C_STAT_ARDY, 
OMAP_I2C_STAT_REG);
+    }
+
+    /* If Stop bit set, flush FIFO. */
+    if (i2c_con_reg & I2C_CON_STP)
+        goto xfer_exit;
+
+    return 0;
+
+xfer_exit:
+    flush_fifo(i2c_base, ip_rev);
+    omap_i2c_write_reg(i2c_base, ip_rev, 0xFFFF, OMAP_I2C_STAT_REG);
+    return i2c_error;
+}
+
+#if !CONFIG_IS_ENABLED(DM_I2C)
+/*
+ * The legacy I2C functions. These need to get removed once
+ * all users of this driver are converted to DM.
+ */
+
  /*
   * i2c_read: Function now uses a single I2C read transaction with 
bulk transfer
   *           of the requested number of bytes (note that the 'i2c 
md' command
@@ -836,11 +937,6 @@ wr_exit:
      return i2c_error;
  }
-#if !CONFIG_IS_ENABLED(DM_I2C)
-/*
- * The legacy I2C functions. These need to get removed once
- * all users of this driver are converted to DM.
- */
  static void __iomem *omap24_get_base(struct i2c_adapter *adap)
  {
      switch (adap->hwadapnr) {
@@ -975,23 +1071,30 @@ static int omap_i2c_xfer(struct udevice *bus, 
struct i2c_msg *msg, int nmsgs)
  {
      struct omap_i2c *priv = dev_get_priv(bus);
      int ret;
+    u16 i2c_con_reg = 0;
      debug("i2c_xfer: %d messages\n", nmsgs);
      for (; nmsgs > 0; nmsgs--, msg++) {
-        debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
-        if (msg->flags & I2C_M_RD) {
-            ret = __omap24_i2c_read(priv->regs, priv->ip_rev,
-                        priv->waitdelay,
-                        msg->addr, 0, 0, msg->buf,
-                        msg->len);
-        } else {
-            ret = __omap24_i2c_write(priv->regs, priv->ip_rev,
-                         priv->waitdelay,
-                         msg->addr, 0, 0, msg->buf,
-                         msg->len);
-        }
+        debug("i2c_xfer: chip=0x%x, len=0x%x, read=0x%x\n",
+              msg->addr, msg->len, (msg->flags & I2C_M_RD));
+
+        /* Wait until bus not busy */
+        if (wait_for_bb(priv->regs, priv->ip_rev, priv->waitdelay))
+            return -EREMOTEIO;
+
+        /* Set Controller mode with Start and Stop bit */
+        i2c_con_reg = I2C_CON_EN | I2C_CON_MST | I2C_CON_STT | 
I2C_CON_STP;
+        /* Set Transmitter/Receiver mode if it is a write/read msg */
+        if (msg->flags & I2C_M_RD)
+            i2c_con_reg &= ~I2C_CON_TRX;
+        else
+            i2c_con_reg |= I2C_CON_TRX;
+
+        ret = __omap24_i2c_xfer_msg(priv->regs, priv->ip_rev, 
priv->waitdelay,
+                        msg->addr, msg->buf, msg->len,
+                        i2c_con_reg);
          if (ret) {
-            debug("i2c_write: error sending\n");
+            debug("i2c_xfer: error sending\n");
              return -EREMOTEIO;
          }
      }

Thanks!

bye,
Heiko

Thanks,
Aniket