I2C patches for v3.8 merge window

just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -7

index c07d9c4..bea0277 100644
@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 {
        struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
        unsigned long timeout;
+       int ret;
        u16 w;
 
        dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
        dev->buf_len = 0;
        if (timeout == 0) {
                dev_err(dev->dev, "controller timed out\n");
-               omap_i2c_init(dev);
-               return -ETIMEDOUT;
+               ret = -ETIMEDOUT;
+               goto err_i2c_init;
        }
 
-       if (likely(!dev->cmd_err))
-               return 0;
-
        /* We have an error */
        if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
                            OMAP_I2C_STAT_XUDF)) {
-               omap_i2c_init(dev);
-               return -EIO;
+               ret = -EIO;
+               goto err_i2c_init;
        }
 
        if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
                if (msg->flags & I2C_M_IGNORE_NAK)
                        return 0;

drivers/i2c/busses/i2c-omap.c +8 -2

        }
-       return -EIO;
+
+       return 0;
+
+err_i2c_init:
+       omap_i2c_init(dev);
+
+err:
+       return ret;
 }
 
 
--

1.8.0.rc0

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 42 lines · Felipe Balbi

just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-omap.c +5 -7

index c07d9c4..bea0277 100644
@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 {
        struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
        unsigned long timeout;
+       int ret;
        u16 w;
 
        dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
        dev->buf_len = 0;
        if (timeout == 0) {
                dev_err(dev->dev, "controller timed out\n");
-               omap_i2c_init(dev);
-               return -ETIMEDOUT;
+               ret = -ETIMEDOUT;
+               goto err_i2c_init;
        }
 
-       if (likely(!dev->cmd_err))
-               return 0;
-
        /* We have an error */
        if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
                            OMAP_I2C_STAT_XUDF)) {
-               omap_i2c_init(dev);
-               return -EIO;
+               ret = -EIO;
+               goto err_i2c_init;
        }
 

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 608

Quoted 13 lines

drivers/i2c/busses/i2c-omap.c +4 -1

        if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
                if (msg->flags & I2C_M_IGNORE_NAK)
                        return 0;
+
                if (stop) {
                        w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
                        w |= OMAP_I2C_CON_STP;
                        omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
                }
-               return -EREMOTEIO;
+
+               ret = -EREMOTEIO;
+               goto err;

This adds reset to nack may be that can be removed.

It's impossible to have Arbitration Lost, Read Overflow, and Tranmist Underflow all asserted at the same time.

Those error conditions are mutually exclusive so what the code should be doing, instead, is check each error flag separataly.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index bea0277..e0eab38 100644

drivers/i2c/busses/i2c-omap.c +0 -0

 		ret = -EIO;
 		goto err_i2c_init;
 	}

Hi

Quoted 21 lines · Felipe Balbi

It's impossible to have Arbitration Lost, Read Overflow, and Tranmist Underflow all asserted at the same time.

Those error conditions are mutually exclusive so what the code should be doing, instead, is check each error flag separataly.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index bea0277..e0eab38 100644
@@ -587,9 +587,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		goto err_i2c_init;
 	}
 

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 592

Quoted 6 lines

drivers/i2c/busses/i2c-omap.c +3 -3

-	/* We have an error */
-	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
-			    OMAP_I2C_STAT_XUDF)) {
+	if ((dev->cmd_err & OMAP_I2C_STAT_AL)
+			|| (dev->cmd_err & OMAP_I2C_STAT_ROVR)
+			|| (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {

Sorry, what is the difference? I didn't understand the optimisation and why now is more clear. Can you just add a comment?

Hi,

Quoted 24 lines · Michael Trimarchi

Hi

Quoted 21 lines · Felipe Balbi

It's impossible to have Arbitration Lost, Read Overflow, and Tranmist Underflow all asserted at the same time.

Those error conditions are mutually exclusive so what the code should be doing, instead, is check each error flag separataly.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index bea0277..e0eab38 100644
@@ -587,9 +587,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		goto err_i2c_init;
 	}
 

Quoted 9 lines

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 592

Quoted 6 lines

drivers/i2c/busses/i2c-omap.c +3 -3

-	/* We have an error */
-	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
-			    OMAP_I2C_STAT_XUDF)) {
+	if ((dev->cmd_err & OMAP_I2C_STAT_AL)
+			|| (dev->cmd_err & OMAP_I2C_STAT_ROVR)
+			|| (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {

Sorry, what is the difference? I didn't understand the optimisation and why now is more clear. Can you just add a comment?

semantically they're not the same, right ? We want to check if each of those bits are set, not if all of them are set together.

my 2 cents.

Quoted 14 lines

Quoted 13 lines

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 592

Quoted 10 lines

drivers/i2c/busses/i2c-omap.c +3 -3

@@ -587,9 +587,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		goto err_i2c_init;
 	}
 
-	/* We have an error */
-	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
-			    OMAP_I2C_STAT_XUDF)) {
+	if ((dev->cmd_err & OMAP_I2C_STAT_AL)
+			|| (dev->cmd_err & OMAP_I2C_STAT_ROVR)
+			|| (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {

Sorry, what is the difference? I didn't understand the optimisation and why now is more clear. Can you just add a comment?

Quoted 4 lines

semantically they're not the same, right ? We want to check if each of those bits are set, not if all of them are set together.

my 2 cents.

You are doing the same thing, but of course is better with just one

*if* as before . A general rule is: when you have logic expression you

can use undefined states to simplify the logic.

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Hi Felipe,

Quoted 13 lines · Felipe Balbi

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Quoted 3 lines

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Do you need OMAP_I2C_IP_VERSION_3 for OMAP4?

OMAP_I2C_IP_VERSION_2 was already introduced to detect OMAP3630 vs omap4430 because they were sharing the same IP version..

/* I2C controller revisions present on specific hardware */

#define OMAP_I2C_REV_ON_2430            0x36
#define OMAP_I2C_REV_ON_3430_3530       0x3C
#define OMAP_I2C_REV_ON_3630_4430       0x40

So in theory you should not need an extra version.

Hi,

Quoted 25 lines · Benoit Cousson

Hi Felipe,

Quoted 13 lines · Felipe Balbi

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Quoted 3 lines

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Do you need OMAP_I2C_IP_VERSION_3 for OMAP4?

OMAP_I2C_IP_VERSION_2 was already introduced to detect OMAP3630 vs omap4430 because they were sharing the same IP version..

Quoted 6 lines

/* I2C controller revisions present on specific hardware */

#define OMAP_I2C_REV_ON_2430            0x36
#define OMAP_I2C_REV_ON_3430_3530       0x3C
#define OMAP_I2C_REV_ON_3630_4430       0x40

So in theory you should not need an extra version.

are you sure that's how they're used ? Looking at the code where we choose reg_map_ip_v1 and reg_map_ip_v2:

1120 if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
1121 dev->regs = (u8 *)reg_map_ip_v2;
1122 else
1123 dev->regs = (u8 *)reg_map_ip_v1;

And looking at reg_map_ip_v1:

218 static const u8 reg_map_ip_v1[] = {

 219         [OMAP_I2C_REV_REG] = 0x00,
 220         [OMAP_I2C_IE_REG] = 0x01,
 221         [OMAP_I2C_STAT_REG] = 0x02,
 222         [OMAP_I2C_IV_REG] = 0x03,
 223         [OMAP_I2C_WE_REG] = 0x03,
 224         [OMAP_I2C_SYSS_REG] = 0x04,
 225         [OMAP_I2C_BUF_REG] = 0x05,
 226         [OMAP_I2C_CNT_REG] = 0x06,
 227         [OMAP_I2C_DATA_REG] = 0x07,
 228         [OMAP_I2C_SYSC_REG] = 0x08,
 229         [OMAP_I2C_CON_REG] = 0x09,
 230         [OMAP_I2C_OA_REG] = 0x0a,
 231         [OMAP_I2C_SA_REG] = 0x0b,
 232         [OMAP_I2C_PSC_REG] = 0x0c,
 233         [OMAP_I2C_SCLL_REG] = 0x0d,
 234         [OMAP_I2C_SCLH_REG] = 0x0e,
 235         [OMAP_I2C_SYSTEST_REG] = 0x0f,
 236         [OMAP_I2C_BUFSTAT_REG] = 0x10,

237 };

that's really not the register map on OMAP3. That only looks valid for OMAP1.

-ECONFUSED

Quoted 66 lines · Felipe Balbi

Hi,

Quoted 25 lines · Benoit Cousson

Hi Felipe,

Quoted 13 lines · Felipe Balbi

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Quoted 3 lines

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Do you need OMAP_I2C_IP_VERSION_3 for OMAP4?

OMAP_I2C_IP_VERSION_2 was already introduced to detect OMAP3630 vs omap4430 because they were sharing the same IP version..

Quoted 6 lines

/* I2C controller revisions present on specific hardware */

#define OMAP_I2C_REV_ON_2430            0x36
#define OMAP_I2C_REV_ON_3430_3530       0x3C
#define OMAP_I2C_REV_ON_3630_4430       0x40

So in theory you should not need an extra version.

are you sure that's how they're used ? Looking at the code where we choose reg_map_ip_v1 and reg_map_ip_v2:

1120 if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
1121 dev->regs = (u8 *)reg_map_ip_v2;
1122 else
1123 dev->regs = (u8 *)reg_map_ip_v1;

And looking at reg_map_ip_v1:

218 static const u8 reg_map_ip_v1[] = {

 219         [OMAP_I2C_REV_REG] = 0x00,
 220         [OMAP_I2C_IE_REG] = 0x01,
 221         [OMAP_I2C_STAT_REG] = 0x02,
 222         [OMAP_I2C_IV_REG] = 0x03,
 223         [OMAP_I2C_WE_REG] = 0x03,
 224         [OMAP_I2C_SYSS_REG] = 0x04,
 225         [OMAP_I2C_BUF_REG] = 0x05,
 226         [OMAP_I2C_CNT_REG] = 0x06,
 227         [OMAP_I2C_DATA_REG] = 0x07,
 228         [OMAP_I2C_SYSC_REG] = 0x08,
 229         [OMAP_I2C_CON_REG] = 0x09,
 230         [OMAP_I2C_OA_REG] = 0x0a,
 231         [OMAP_I2C_SA_REG] = 0x0b,
 232         [OMAP_I2C_PSC_REG] = 0x0c,
 233         [OMAP_I2C_SCLL_REG] = 0x0d,
 234         [OMAP_I2C_SCLH_REG] = 0x0e,
 235         [OMAP_I2C_SYSTEST_REG] = 0x0f,
 236         [OMAP_I2C_BUFSTAT_REG] = 0x10,

237 };

Quoted 2 lines

that's really not the register map on OMAP3. That only looks valid for OMAP1.

No that should be valid as well for OMAP2&3 thanks to the OMAP_I2C_FLAG_BUS_SHIFT_2.

It was introduced during DT adaptation:

#ifdef CONFIG_OF

static struct omap_i2c_bus_platform_data omap3_pdata = {
        .rev = OMAP_I2C_IP_VERSION_1,
        .flags = OMAP_I2C_FLAG_APPLY_ERRATA_I207 |
                 OMAP_I2C_FLAG_RESET_REGS_POSTIDLE |
                 OMAP_I2C_FLAG_BUS_SHIFT_2,
};

static struct omap_i2c_bus_platform_data omap4_pdata = {
        .rev = OMAP_I2C_IP_VERSION_2,
};

static const struct of_device_id omap_i2c_of_match[] = {
        {
                .compatible = "ti,omap4-i2c",
                .data = &omap4_pdata,
        },
        {
                .compatible = "ti,omap3-i2c",
                .data = &omap3_pdata,
        },
        { },
};

MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
#endif

Quoted 1 line

-ECONFUSED

Well, it is confusing...

From: Shubhrajyoti D <[email protected]>

In case of a NACK, it's wise to tell our clients drivers about how many bytes were actually transferred.

Support this by adding an extra field to the struct i2c_msg which gets incremented the amount of bytes actually transferred.

Signed-off-by Shubhrajyoti D <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

include/uapi/linux/i2c.h +0 -0

index 0e949cb..4b35c9b 100644

Hi Felipe, Shubhrajyoti,

Quoted 15 lines · Felipe Balbi

From: Shubhrajyoti D <[email protected]>

In case of a NACK, it's wise to tell our clients drivers about how many bytes were actually transferred.

Support this by adding an extra field to the struct i2c_msg which gets incremented the amount of bytes actually transferred.

Signed-off-by Shubhrajyoti D <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

include/uapi/linux/i2c.h struct i2c_msg { · line 82

Quoted 11 lines

include/uapi/linux/i2c.h +1 -0

index 0e949cb..4b35c9b 100644
@@ -77,6 +77,7 @@ struct i2c_msg {
 #define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
 	__u16 len;		/* msg length				*/
+	__u16 transferred;	/* actual bytes transferred             */
 	__u8 *buf;		/* pointer to msg data			*/
 };

On the principle I am fine with this, however you also need to define who should initialize this field, and to what value.

Quoted 18 lines · Jean Delvare

Hi Felipe, Shubhrajyoti,

Quoted 15 lines · Felipe Balbi

From: Shubhrajyoti D <[email protected]>

In case of a NACK, it's wise to tell our clients drivers about how many bytes were actually transferred.

Support this by adding an extra field to the struct i2c_msg which gets incremented the amount of bytes actually transferred.

Signed-off-by Shubhrajyoti D <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

Quoted 14 lines

include/uapi/linux/i2c.h struct i2c_msg { · line 82

Quoted 11 lines

include/uapi/linux/i2c.h +1 -0

index 0e949cb..4b35c9b 100644
@@ -77,6 +77,7 @@ struct i2c_msg {
 #define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
 	__u16 len;		/* msg length				*/
+	__u16 transferred;	/* actual bytes transferred             */
 	__u8 *buf;		/* pointer to msg data			*/
 };

On the principle I am fine with this, however you also need to define who should initialize this field, and to what value.

You also miss one very very very big point. This will break every I2C using userspace program out there unless it is rebuilt - this change will require the exact right version of those userspace programs for the kernel that they're being used on.

Quoted 34 lines · Russell King - ARM Linux

Quoted 32 lines · Jean Delvare

Hi Felipe, Shubhrajyoti,

Quoted 20 lines · Felipe Balbi

From: Shubhrajyoti D <[email protected]>

In case of a NACK, it's wise to tell our clients drivers about how many bytes were actually transferred.

Support this by adding an extra field to the struct i2c_msg which gets incremented the amount of bytes actually transferred.

Signed-off-by Shubhrajyoti D <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

include/uapi/linux/i2c.h +0 -0

index 0e949cb..4b35c9b 100644

include/uapi/linux/i2c.h struct i2c_msg { · line 82

Quoted 6 lines

include/uapi/linux/i2c.h +1 -0

@@ -77,6 +77,7 @@ struct i2c_msg {
 #define I2C_M_NO_RD_ACK		0x0800	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_RECV_LEN		0x0400	/* length will be first received byte */
 	__u16 len;		/* msg length				*/
+	__u16 transferred;	/* actual bytes transferred             */
 	__u8 *buf;		/* pointer to msg data			*/
 };

On the principle I am fine with this, however you also need to define who should initialize this field, and to what value.

Quoted 4 lines

You also miss one very very very big point. This will break every I2C using userspace program out there unless it is rebuilt - this change will require the exact right version of those userspace programs for the kernel that they're being used on.

How that? The extra field is added in a hole, so we don't change the struct size nor the relative positions of existing fields. Why would user-space care?

Quoted 6 lines · Jean Delvare

Quoted 4 lines · Russell King - ARM Linux

You also miss one very very very big point. This will break every I2C using userspace program out there unless it is rebuilt - this change will require the exact right version of those userspace programs for the kernel that they're being used on.

Quoted 3 lines

How that? The extra field is added in a hole, so we don't change the struct size nor the relative positions of existing fields. Why would user-space care?

You know the layout of that struct for certain across all Linux supported architectures, including some of the more obscure ones which may not require pointers to be aligned?

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Jean Delvare

Quoted 4 lines · Russell King - ARM Linux

You also miss one very very very big point. This will break every I2C using userspace program out there unless it is rebuilt - this change will require the exact right version of those userspace programs for the kernel that they're being used on.

Quoted 7 lines

Quoted 3 lines

How that? The extra field is added in a hole, so we don't change the struct size nor the relative positions of existing fields. Why would user-space care?

You know the layout of that struct for certain across all Linux supported architectures, including some of the more obscure ones which may not require pointers to be aligned?

No I don't, I naively supposed it would be fine. I would expect gcc to always align pointers even when the architecture doesn't need them to be, for performance reasons, even when it doesn't have to, as long as attribute packed isn't used. After all, integers don't _have_ to be aligned on x86, but gcc aligns them.

The original idea of using the hole in the i2c_msg structure is from David Brownell, who was apparently familiar with such practice, so I assumed it was OK. Actually I still assume it is, until someone comes with an supported architecture where it is not.

Quoted 4 lines · Jean Delvare

The original idea of using the hole in the i2c_msg structure is from David Brownell, who was apparently familiar with such practice, so I assumed it was OK. Actually I still assume it is, until someone comes with an supported architecture where it is not.

According to Al Viro, that would be m68k.

Quoted 7 lines · Russell King - ARM Linux

Quoted 4 lines · Jean Delvare

The original idea of using the hole in the i2c_msg structure is from David Brownell, who was apparently familiar with such practice, so I assumed it was OK. Actually I still assume it is, until someone comes with an supported architecture where it is not.

According to Al Viro, that would be m68k.

OK... So to make things clear, let me ask Al directly about it. Al, can you please tell us if:

would break binary compatibility on m68k or any other architecture you are familiar with? Note that struct i2c_msg isn't declared with attribute packed, so my assumption was that pointer "buf" was align on at least 4 bytes, leaving a hole between len and buf. Am I wrong?

You are wrong. Assumption that pointers are aligned to 32bit boundary is simply not true. In particular, on m68k alignment is 16bit, i.e. there

struct foo {
	char x;
	void *p;
}; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes

occupied by p.

Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be surprised by e.g. coldfire-based SoC with i2c on it.

Quoted 10 lines · Al Viro

You are wrong. Assumption that pointers are aligned to 32bit boundary is simply not true. In particular, on m68k alignment is 16bit, i.e. there

struct foo {
	char x;
	void *p;
}; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes

occupied by p.

Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be surprised by e.g. coldfire-based SoC with i2c on it.

BTW, that's easily verified - take a cross-compiler and do this:
; cat >a.c <<'EOF'
struct { char x; void *y; } v;
int z = (char *)&v.y - (char *)&v;
EOF
; m68k-linux-gnu-gcc -S a.c
; grep -A1 'z:' a.s
z:
.long 2
;
and watch what it puts into z. gcc is very liberal about what it considers a constant expression, so it allows that sort of expressions as initializers for global variables. Not a portable C, but convenient for experiments like that; just grab a cross-toolchain and feed it testcases of that kind...

Quoted 13 lines · Al Viro

Quoted 10 lines · Al Viro

You are wrong. Assumption that pointers are aligned to 32bit boundary is simply not true. In particular, on m68k alignment is 16bit, i.e. there

struct foo {
	char x;
	void *p;
}; will have 1 byte occupied by x, followed by 1-byte gap, followed by 4 bytes

occupied by p.

Note, BTW, that m68k includes things like coldfire, etc. and I wouldn't be surprised by e.g. coldfire-based SoC with i2c on it.

Quoted 6 lines

BTW, that's easily verified - take a cross-compiler and do this:
; cat >a.c <<'EOF'
struct { char x; void *y; } v;
int z = (char *)&v.y - (char *)&v;
EOF
; m68k-linux-gnu-gcc -S a.c

Quoted 8 lines

; grep -A1 'z:' a.s
z:
.long 2
;
and watch what it puts into z. gcc is very liberal about what it considers a constant expression, so it allows that sort of expressions as initializers for global variables. Not a portable C, but convenient for experiments like that; just grab a cross-toolchain and feed it testcases of that kind...

... and google for i2c coldfire immediately turns up this:
http://mailman.uclinux.org/pipermail/uclinux-dev/2012-May/051874.html
with

(quoted diff) +9 -0

+config I2C_COLDFIRE
+       tristate "Freescale Coldfire I2C driver"
+       depends on !M5272
+       help
+         This driver supports the I2C interface availible on most Freescale
+         Coldfire processors.
+
+         This driver can be built as a module.  If so, the module
+         will be called i2c-coldfire.

in it, along with addition of drivers/i2c/busses/i2c-coldfire.c... IOW, i2c on m68k is quite real. Sorry, no go - you don't even have an excuse of i2c never existing on the architecture in question.

Quoted 7 lines · Russell King - ARM Linux

Quoted 4 lines · Jean Delvare

The original idea of using the hole in the i2c_msg structure is from David Brownell, who was apparently familiar with such practice, so I assumed it was OK. Actually I still assume it is, until someone comes with an supported architecture where it is not.

According to Al Viro, that would be m68k.

OK... So to make things clear, let me ask Al directly about it. Al, can you please tell us if:

include/uapi/linux/i2c.h +1 -0

 struct i2c_msg {
 	__u16 addr;		/* slave address			*/
 	__u16 flags;
 	__u16 len;		/* msg length				*/
+	__u16 transferred;	/* actual bytes transferred             */
 	__u8 *buf;		/* pointer to msg data			*/
 };

would break binary compatibility on m68k or any other architecture you are familiar with? Note that struct i2c_msg isn't declared with attribute packed, so my assumption was that pointer "buf" was align on at least 4 bytes, leaving a hole between len and buf. Am I wrong?

Hi guys,

here's another series for OMAP I2C driver. There are a few cleanups and one very nice new feature: we can now report how many bytes we transferred until NACK.

Note that the implemementation for OMAP-I2C turned out to be a little more complex then I expected, mainly because of the way I2C_CNT register behaves and because of the very buggy register usage on that driver.

I have boot tested all patches on beagle xM (3630) and pandaboard rev A3 (4430), will send boot-logs if anyone wants to see.

All patches are available at [1] if anyone wants an easy way to test the patches.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git i2c-transferred-bytes-on-NACK

Felipe Balbi (7):
i2c: omap: no need to access platform_device
i2c: omap: reorder exit path of omap_i2c_xfer_msg()
i2c: omap: fix error checking
i2c: omap: also complete() when stat becomes zero
i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3
i2c: omap: wait for transfer completion before sending STP bit
i2c: omap: implement handling for 'transferred' bytes

Shubhrajyoti D (1):
i2c: add 'transferred' field to struct i2c_msg

 arch/arm/mach-omap2/i2c.c                  |   3 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   2 +-
 drivers/i2c/busses/i2c-omap.c              | 156 ++++++++++++++++-------------
 include/linux/i2c-omap.h                   |   1 +
 include/uapi/linux/i2c.h                   |   1 +
 5 files changed, 89 insertions(+), 74 deletions(-)

Signature · 1 line

1.8.0.rc0

Hi,

Quoted 13 lines · Felipe Balbi

Hi guys,

here's another series for OMAP I2C driver. There are a few cleanups and one very nice new feature: we can now report how many bytes we transferred until NACK.

Note that the implemementation for OMAP-I2C turned out to be a little more complex then I expected, mainly because of the way I2C_CNT register behaves and because of the very buggy register usage on that driver.

I have boot tested all patches on beagle xM (3630) and pandaboard rev A3 (4430), will send boot-logs if anyone wants to see.

Quoted 5 lines

All patches are available at [1] if anyone wants an easy way to test the patches.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git i2c-transferred-bytes-on-NACK

forgot to mention, with [1] I could veridy suspend to ram with boards mentioned above.

[1] http://marc.info/?l=linux-arm-kernel&m=135090724817604&w=2

Quoted 19 lines

Felipe Balbi (7):
i2c: omap: no need to access platform_device
i2c: omap: reorder exit path of omap_i2c_xfer_msg()
i2c: omap: fix error checking
i2c: omap: also complete() when stat becomes zero
i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3
i2c: omap: wait for transfer completion before sending STP bit
i2c: omap: implement handling for 'transferred' bytes

Shubhrajyoti D (1):
i2c: add 'transferred' field to struct i2c_msg

 arch/arm/mach-omap2/i2c.c                  |   3 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   2 +-
 drivers/i2c/busses/i2c-omap.c              | 156 ++++++++++++++++-------------
 include/linux/i2c-omap.h                   |   1 +
 include/uapi/linux/i2c.h                   |   1 +
 5 files changed, 89 insertions(+), 74 deletions(-)

Quoted 3 lines

--
1.8.0.rc0

--
balbi

Quoted 16 lines · Felipe Balbi

Hi,

Quoted 13 lines · Felipe Balbi

Hi guys,

here's another series for OMAP I2C driver. There are a few cleanups and one very nice new feature: we can now report how many bytes we transferred until NACK.

Note that the implemementation for OMAP-I2C turned out to be a little more complex then I expected, mainly because of the way I2C_CNT register behaves and because of the very buggy register usage on that driver.

I have boot tested all patches on beagle xM (3630) and pandaboard rev A3 (4430), will send boot-logs if anyone wants to see.

tested the below branch on omap4430sdp , panda , omap3430sdp.

Doing simple i2ctools
.

Tested-by Shubhrajyoti D <[email protected]>

Quoted 35 lines

Quoted 4 lines

All patches are available at [1] if anyone wants an easy way to test the patches.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git i2c-transferred-bytes-on-NACK

forgot to mention, with [1] I could veridy suspend to ram with boards mentioned above.

[1] http://marc.info/?l=linux-arm-kernel&m=135090724817604&w=2

Quoted 19 lines

Felipe Balbi (7):
i2c: omap: no need to access platform_device
i2c: omap: reorder exit path of omap_i2c_xfer_msg()
i2c: omap: fix error checking
i2c: omap: also complete() when stat becomes zero
i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3
i2c: omap: wait for transfer completion before sending STP bit
i2c: omap: implement handling for 'transferred' bytes

Shubhrajyoti D (1):
i2c: add 'transferred' field to struct i2c_msg

 arch/arm/mach-omap2/i2c.c                  |   3 +-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |   2 +-
 drivers/i2c/busses/i2c-omap.c              | 156 ++++++++++++++++-------------
 include/linux/i2c-omap.h                   |   1 +
 include/uapi/linux/i2c.h                   |   1 +
 5 files changed, 89 insertions(+), 74 deletions(-)

Quoted 3 lines

--
1.8.0.rc0

--
balbi

can you also check if echo mem > /sys/power/state works ? Don't forget to enable UART wakeups with:

echo enabled > /sys/devices/platform/omap_uart.2/power/wakeup echo enabled > /sys/devices/platform/omap_uart.2/tty/ttyO2/power/wakeup

before trying to suspend. Another cool test is rtctest (since our RTC is part of TWL). It worked fine on my two platforms. Attached you will find the sourcecode for rtctest.

Quoted 5 lines

Quoted 5 lines

here's another series for OMAP I2C driver. There are a few cleanups and one very nice new feature: we can now report how many bytes we transferred until NACK.

Quoted 7 lines

Quoted 7 lines

Note that the implemementation for OMAP-I2C turned out to be a little more complex then I expected, mainly because of the way I2C_CNT register behaves and because of the very buggy register usage on that driver.

I have boot tested all patches on beagle xM (3630) and pandaboard rev A3 (4430), will send boot-logs if anyone wants to see.

PM callbacks pass our device pointer as argument
and we don't need to access the platform_device
just to dereference that down to dev->drvdata.

instead, just use dev_get_drvdata() directly.

Signed-off-by Felipe Balbi <[email protected]>

---

just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.

Signed-off-by Felipe Balbi <[email protected]>

---
  drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
  1 file changed, 17 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index c07d9c4..bea0277 100644

drivers/i2c/busses/i2c-omap.c +0 -0

  	u16 w;
 
  	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
  	dev->buf_len = 0;
  	if (timeout == 0) {

drivers/i2c/busses/i2c-omap.c +3 -2

  	/* We have an error */
  	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
  			    OMAP_I2C_STAT_XUDF)) {
-		omap_i2c_init(dev);
-		return -EIO;
+		ret = -EIO;
+		goto err_i2c_init;
  	}
 
  	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
  		if (msg->flags & I2C_M_IGNORE_NAK)
  			return 0;
+
  		if (stop) {
  			w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
  			w |= OMAP_I2C_CON_STP;

Quoted 13 lines · Felipe Balbi

just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index c07d9c4..bea0277 100644

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 508

Quoted 4 lines

drivers/i2c/busses/i2c-omap.c +1 -0

@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 {
 	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
 	unsigned long timeout;
+	int ret;

You might want to initialize the error value to avoid return 0. Compiler might be already cribbing for it

Quoted 13 lines

drivers/i2c/busses/i2c-omap.c +2 -2

 	u16 w;
 
 	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -582,31 +583,38 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	dev->buf_len = 0;
 	if (timeout == 0) {
 		dev_err(dev->dev, "controller timed out\n");
-		omap_i2c_init(dev);
-		return -ETIMEDOUT;
+		ret = -ETIMEDOUT;
+		goto err_i2c_init;
 	}
 

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 591

Quoted 3 lines

drivers/i2c/busses/i2c-omap.c +0 -3

-	if (likely(!dev->cmd_err))
-		return 0;
-

Have you have drooped this check completely ?

Quoted 20 lines

drivers/i2c/busses/i2c-omap.c +4 -3

 	/* We have an error */
 	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
-		omap_i2c_init(dev);
-		return -EIO;
+		ret = -EIO;
+		goto err_i2c_init;
 	}
 
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
+
 		if (stop) {
 			w = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
 			w |= OMAP_I2C_CON_STP;
 			omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
 		}
-		return -EREMOTEIO;
+

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 611

Quoted 6 lines

drivers/i2c/busses/i2c-omap.c +4 -1

+		ret = -EREMOTEIO;
+		goto err;
 	}
-	return -EIO;
+
+	return 0;

With initialized value you can use
return ret;

Quoted 17 lines · Felipe Balbi

just a cleanup patch trying to make exit path
more straightforward. No changes otherwise.

Signed-off-by Felipe Balbi <[email protected]>

---
  drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
  1 file changed, 17 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-omap.c +1 -0

index c07d9c4..bea0277 100644
@@ -505,6 +505,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
  {
  	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
  	unsigned long timeout;
+	int ret;

drivers/i2c/busses/i2c-omap.c static int omap_i2c_xfer_msg(struct i2c_adapter *adap, · line 611

Quoted 6 lines

drivers/i2c/busses/i2c-omap.c +4 -1

+		ret = -EREMOTEIO;
+		goto err;
  	}
-	return -EIO;
+
+	return 0;

With initialized value you can use
return ret;

In case we loop on IRQ handler until stat is finally zero, we would end up in a situation where all I2C transfers would misteriously timeout because we were not calling complete() in that situation.

Fix the issue by moving omap_i2c_complete_cmd() call inside the 'out' label.

Signed-off-by Felipe Balbi <[email protected]>

---

Quoted 6 lines · Felipe Balbi

In case we loop on IRQ handler until stat is finally zero, we would end up in a situation where all I2C transfers would misteriously timeout because we were not calling complete() in that situation.

Quoted 5 lines

Fix the issue by moving omap_i2c_complete_cmd() call inside the 'out' label.

Signed-off-by Felipe Balbi <[email protected]>

---

Looks fine. Have you hit this issue in any corner case ?

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Cc Benoit Cousson <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
  drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index b004126..20f9ad6 100644

Quoted 29 lines · Felipe Balbi

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Cc Benoit Cousson <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index b004126..20f9ad6 100644
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
 

drivers/i2c/busses/i2c-omap.c static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev, · line 278

Quoted 8 lines

drivers/i2c/busses/i2c-omap.c +5 -1

 static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 {
-	return __raw_readw(i2c_dev->base +
+	/* if we are OMAP_I2C_IP_VERSION_2, we need to read from
+	 * IRQSTATUS_RAW, but write to IRQSTATUS
+	 */
+	if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
+			(reg == OMAP_I2C_STAT_REG)) {

Doing this check on every I2C register read seems to expensive to me. Can you not sort this in init with some offset which can be 0 or non zero ? Sorry in case this is already dicussed.

Hi,

Quoted 30 lines · Santosh Shilimkar

Quoted 29 lines · Felipe Balbi

on OMAP4+ we want to read IRQSTATUS_RAW register, instead of IRQSTATUS. The reason being that IRQSTATUS will only contain the bits which were enabled on IRQENABLE_SET and that will break when we need to poll for a certain bit which wasn't enabled as an IRQ source.

One such case is after we finish converting to deferred stop bit, we will have to poll for ARDY bit before returning control for the client driver in order to prevent us from trying to start a transfer on a bus which is already busy.

Note, however, that omap-i2c.c needs a big rework on register definition and register access. Such work will be done in a separate series of patches.

Cc Benoit Cousson <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
  drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-omap.c +0 -0

index b004126..20f9ad6 100644
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
 

Quoted 11 lines

drivers/i2c/busses/i2c-omap.c static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev, · line 278

Quoted 8 lines

drivers/i2c/busses/i2c-omap.c +5 -1

  static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
  {
-	return __raw_readw(i2c_dev->base +
+	/* if we are OMAP_I2C_IP_VERSION_2, we need to read from
+	 * IRQSTATUS_RAW, but write to IRQSTATUS
+	 */
+	if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_2) &&
+			(reg == OMAP_I2C_STAT_REG)) {

Doing this check on every I2C register read seems to expensive to me. Can you not sort this in init with some offset which can be 0 or non zero ? Sorry in case this is already dicussed.

could be. I didn't go that route because I'm planning a complete rewrite of all register accesses. The way it's done today is completely broken and already expensive (with reg_shift and different map tables and so on).

If it's really a big of a deal, I can try to find another way, maybe just adding omap_i2c_read_stat() and limit the version check just to I2C_STAT reads would do it for now...

Later patches will come adding support for reporting amount of bytes transferred so that client drivers can count how many bytes are left to transfer.

This is useful mostly in case of NACKs when client driver wants to know exactly which byte got NACKed so it doesn't have to resend all bytes again.

In order to make that work with OMAP's I2C controller, we need to prevent sending STP bit until message is transferred. The reason behind that is because OMAP_I2C_CNT_REG gets reset to original value after STP bit is shifted through I2C_SDA line and that would prevent us from reading the correct number of bytes left to transfer.

The full programming model suggested by IP owner was the following:

- start I2C transfer (without STP bit)
- upon completion or NACK, read I2C_CNT register
- write STP bit to I2C_CON register
- wait for ARDY bit

With this patch we're implementing all steps except step #2 which will come in a later patch adding such support.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 89 ++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 56 deletions(-)

drivers/i2c/busses/i2c-omap.c +24 -32

index 20f9ad6..699fa12 100644
@@ -438,9 +438,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 
 	/* Enable interrupts */
 	dev->iestate = (OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY |
-			OMAP_I2C_IE_ARDY | OMAP_I2C_IE_NACK |
-			OMAP_I2C_IE_AL)  | ((dev->fifo_size) ?
-				(OMAP_I2C_IE_RDR | OMAP_I2C_IE_XDR) : 0);
+			OMAP_I2C_IE_NACK | OMAP_I2C_IE_AL)  |
+			((dev->fifo_size) ? (OMAP_I2C_IE_RDR |
+				OMAP_I2C_IE_XDR) : 0);
 	omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev->iestate);
 	if (dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		dev->pscstate = psc;

@@ -470,6 +470,22 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
 	return 0;
 }
 
+static int omap_i2c_wait_for_ardy(struct omap_i2c_dev *dev)
+{
+	unsigned long timeout;
+
+	timeout = jiffies + OMAP_I2C_TIMEOUT;
+	while (!(omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG) & OMAP_I2C_STAT_ARDY)) {
+		if (time_after(jiffies, timeout)) {
+			dev_warn(dev->dev, "timeout waiting for access ready\n");
+			return -ETIMEDOUT;
+		}
+		usleep_range(800, 1200);
+	}
+
+	return 0;
+}
+
 static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 {
 	u16		buf;

@@ -515,7 +531,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 {
 	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
 	unsigned long timeout;
-	int ret;
+	int ret = 0;
 	u16 w;
 
 	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",

@@ -556,35 +572,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	if (!(msg->flags & I2C_M_RD))
 		w |= OMAP_I2C_CON_TRX;
 
-	if (!dev->b_hw && stop)
-		w |= OMAP_I2C_CON_STP;
-
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
 
 	/*
-	 * Don't write stt and stp together on some hardware.
-	 */
-	if (dev->b_hw && stop) {
-		unsigned long delay = jiffies + OMAP_I2C_TIMEOUT;
-		u16 con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
-		while (con & OMAP_I2C_CON_STT) {
-			con = omap_i2c_read_reg(dev, OMAP_I2C_CON_REG);
-
-			/* Let the user know if i2c is in a bad state */
-			if (time_after(jiffies, delay)) {
-				dev_err(dev->dev, "controller timed out "
-				"waiting for start condition to finish\n");
-				return -ETIMEDOUT;
-			}
-			cpu_relax();
-		}
-
-		w |= OMAP_I2C_CON_STP;
-		w &= ~OMAP_I2C_CON_STT;
-		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, w);
-	}
-
-	/*
 	 * REVISIT: We should abort the transfer on signals, but the bus goes
 	 * into arbitration and we're currently unable to recover from it.
 	 */

@@ -594,36 +584,36 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	if (timeout == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		ret = -ETIMEDOUT;
-		goto err_i2c_init;
+		omap_i2c_init(dev);
+		goto out;
 	}
 
 	/* We have an error */
 	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
 		ret = -EIO;
-		goto err_i2c_init;
+		omap_i2c_init(dev);
+		goto out;
 	}
 
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;

Later patches will come adding support for reporting amount of bytes transferred so that client drivers can count how many bytes are left to transfer.

This is useful mostly in case of NACKs when client driver wants to know exactly which byte got NACKed so it doesn't have to resend all bytes again.

In order to make that work with OMAP's I2C controller, we need to prevent sending STP bit until message is transferred. The reason behind that is because OMAP_I2C_CNT_REG gets reset to original value after STP bit is shifted through I2C_SDA line and that would prevent us from reading the correct number of bytes left to transfer.

The full programming model suggested by IP owner was the following:

- start I2C transfer (without STP bit)
- upon completion or NACK, read I2C_CNT register
- write STP bit to I2C_CON register
- wait for ARDY bit

With this patch we're implementing all steps except step #2 which will come in a later patch adding such support.

Signed-off-by Felipe Balbi <[email protected]>

---

Quoted 27 lines · Felipe Balbi

Later patches will come adding support for reporting amount of bytes transferred so that client drivers can count how many bytes are left to transfer.

This is useful mostly in case of NACKs when client driver wants to know exactly which byte got NACKed so it doesn't have to resend all bytes again.

In order to make that work with OMAP's I2C controller, we need to prevent sending STP bit until message is transferred. The reason behind that is because OMAP_I2C_CNT_REG gets reset to original value after STP bit is shifted through I2C_SDA line and that would prevent us from reading the correct number of bytes left to transfer.

The full programming model suggested by IP owner was the following:

- start I2C transfer (without STP bit)
- upon completion or NACK, read I2C_CNT register
- write STP bit to I2C_CON register
- wait for ARDY bit

Quoted 4 lines

With this patch we're implementing all steps except step #2 which will come in a later patch adding such support.

Will this not break the bisect since CNT and
NACK, completion is added in later patch

this is important in cases where client driver wants to know how many bytes were actually transferred.

There is one trick here: if transfer is completed, meaning I2C_CNT reaches zero, then ARDY will be asserted to let SW know that it can program a new transfer.

When ARDY is asserted, I2C_CNT is reset to the original value (msg->len), which means that for a successful message, msg->transferred = msg->len and we don't need to spend time with a register read.

In case of NACK condition, however, I2C_CNT will remain with the end value which is the amount of data transferred until NACK condition found on the bus inclusive. In this situation, msg->transferred needs to be initialized with:

msg->len - read(I2C_CNT) - 1;

This patch implements exactly that handling.

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/i2c/busses/i2c-omap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/i2c/busses/i2c-omap.c +0 -0

index 699fa12..d268e92 100644

Hi,

here's another series for OMAP I2C driver. There are a few cleanups and one very nice new feature: we can now report how many bytes we transferred until NACK.

Note that the implemementation for OMAP-I2C turned out to be a little more complex then I expected, mainly because of the way I2C_CNT register behaves and because of the very buggy register usage on that driver.

I have boot tested all patches on beagle xM (3630) and pandaboard rev A3 (4430), will send boot-logs if anyone wants to see.

All patches are available at [1] if anyone wants an easy way to test the patches.