[PATCH RFC/RFT 1/3] i2c: omap24xx_i2c: Use new function __omap24_i2c_xfer_msg()
Aniket Limaye
a-limaye at ti.com
Thu Mar 6 13:47:10 CET 2025
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-limaye at 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
More information about the U-Boot
mailing list