[PATCH 0/8] I2C patches for v3.8 merge window

[PATCH 0/8] I2C patches for v3.8 merge window

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

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(-)

-- 
1.8.0.rc0

[PATCH 1/8] i2c: omap: no need to access platform_device

[Felipe Balbi]

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 <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..c07d9c4 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1239,8 +1239,7 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_RUNTIME
 static int omap_i2c_runtime_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
 	u16 iv;
 
 	_dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
@@ -1261,8 +1260,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 
 static int omap_i2c_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
 
 	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
-- 
1.8.0.rc0

[PATCH 2/8] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Felipe Balbi]

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

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c07d9c4..bea0277 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
+
 		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;
 	}
-	return -EIO;
+
+	return 0;
+
+err_i2c_init:
+	omap_i2c_init(dev);
+
+err:
+	return ret;
 }
 
 
-- 
1.8.0.rc0

Re: [PATCH 2/8] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Shubhrajyoti Datta]

On Mon, Oct 22, 2012 at 3:16 PM, Felipe Balbi <balbi@ti.com> wrote:
> just a cleanup patch trying to make exit path
> more straightforward. No changes otherwise.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index c07d9c4..bea0277 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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;
> +
>                 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.


>         }
> -       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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/8] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

Hi,

On Thu, Oct 25, 2012 at 05:10:10PM +0530, Shubhrajyoti Datta wrote:
> On Mon, Oct 22, 2012 at 3:16 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> > just a cleanup patch trying to make exit path
> > more straightforward. No changes otherwise.
> >
> > Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index c07d9c4..bea0277 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -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;
> > +
> >                 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.

great catch. I will respin this series.

thanks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 5/8] i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3

[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 <b-cousson@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 arch/arm/mach-omap2/i2c.c                  |  3 ++-
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  2 +-
 drivers/i2c/busses/i2c-omap.c              | 26 ++++++++++++++++++++++----
 include/linux/i2c-omap.h                   |  1 +
 4 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/i2c.c b/arch/arm/mach-omap2/i2c.c
index fc57e67..e871928 100644
--- a/arch/arm/mach-omap2/i2c.c
+++ b/arch/arm/mach-omap2/i2c.c
@@ -66,7 +66,8 @@ int omap_i2c_reset(struct omap_hwmod *oh)
 	u16 i2c_con;
 	int c = 0;
 
-	if (oh->class->rev == OMAP_I2C_IP_VERSION_2) {
+	if ((oh->class->rev == OMAP_I2C_IP_VERSION_2) ||
+			(oh->class->rev == OMAP_I2C_IP_VERSION_3)) {
 		i2c_con = OMAP4_I2C_CON_OFFSET;
 	} else if (oh->class->rev == OMAP_I2C_IP_VERSION_1) {
 		i2c_con = OMAP2_I2C_CON_OFFSET;
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 652d028..ae9c023 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -1521,7 +1521,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_i2c_sysc = {
 static struct omap_hwmod_class omap44xx_i2c_hwmod_class = {
 	.name	= "i2c",
 	.sysc	= &omap44xx_i2c_sysc,
-	.rev	= OMAP_I2C_IP_VERSION_2,
+	.rev	= OMAP_I2C_IP_VERSION_3,
 	.reset	= &omap_i2c_reset,
 };
 
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 6219522..c65d526 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
 
 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_3, we need to read from
+	 * IRQSTATUS_RAW, but write to IRQSTATUS
+	 */
+	if ((i2c_dev->dtrev == OMAP_I2C_IP_VERSION_3) &&
+			(reg == OMAP_I2C_STAT_REG)) {
+		return __raw_readw(i2c_dev->base +
+				((i2c_dev->regs[reg] - 0x04)
+				 << i2c_dev->reg_shift));
+	} else {
+		return __raw_readw(i2c_dev->base +
 				(i2c_dev->regs[reg] << i2c_dev->reg_shift));
+	}
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -1124,10 +1134,18 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->reg_shift = (dev->flags >> OMAP_I2C_FLAG_BUS_SHIFT__SHIFT) & 3;
 
-	if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		dev->regs = (u8 *)reg_map_ip_v2;
-	else
+	switch (dev->dtrev) {
+	case OMAP_I2C_IP_VERSION_1:
 		dev->regs = (u8 *)reg_map_ip_v1;
+		break;
+	case OMAP_I2C_IP_VERSION_2:
+		/* FALLTHROUGH */
+	case OMAP_I2C_IP_VERSION_3:
+		/* FALLTHROUGH */
+	default:
+		dev->regs = (u8 *)reg_map_ip_v2;
+		break;
+	}
 
 	pm_runtime_enable(dev->dev);
 	pm_runtime_set_autosuspend_delay(dev->dev, OMAP_I2C_PM_TIMEOUT);
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
index df804ba..84f7355 100644
--- a/include/linux/i2c-omap.h
+++ b/include/linux/i2c-omap.h
@@ -14,6 +14,7 @@
 
 #define OMAP_I2C_IP_VERSION_1 1
 #define OMAP_I2C_IP_VERSION_2 2
+#define OMAP_I2C_IP_VERSION_3 3
 
 /* struct omap_i2c_bus_platform_data .flags meanings */
 
-- 
1.8.0.rc0

Re: [PATCH 5/8] i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3

[Benoit Cousson]

Hi Felipe,

On 10/22/2012 11:46 AM, Felipe Balbi wrote:
> 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.

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.

Regards,
Benoit

Re: [PATCH 5/8] i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2564 bytes --]

Hi,

On Mon, Oct 22, 2012 at 02:27:20PM +0200, Benoit Cousson wrote:
> Hi Felipe,
> 
> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
> > 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.
> 
> 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.

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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 5/8] i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3

[Benoit Cousson]

On 10/22/2012 02:28 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Oct 22, 2012 at 02:27:20PM +0200, Benoit Cousson wrote:
>> Hi Felipe,
>>
>> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
>>> 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.
>>
>> 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.
> 
> 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.

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


> -ECONFUSED

Well, it is confusing...

Benoit

Re: [PATCH 5/8] i2c: omap: introduce and use OMAP_I2C_IP_VERSION_3

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4051 bytes --]

Hi,

On Mon, Oct 22, 2012 at 03:05:20PM +0200, Benoit Cousson wrote:
> On 10/22/2012 02:28 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Oct 22, 2012 at 02:27:20PM +0200, Benoit Cousson wrote:
> >> Hi Felipe,
> >>
> >> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
> >>> 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.
> >>
> >> 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.
> > 
> > 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.
> 
> No that should be valid as well for OMAP2&3 thanks to the
> OMAP_I2C_FLAG_BUS_SHIFT_2.

aha.. I see. What a hack!

> 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
> 
> 
> > -ECONFUSED
> 
> Well, it is confusing...

Tell me about it... in that case, $SUBJECT can be dropped silently, I
will respin next patch so that it checks for VERSION_2 instead of
VERSION_3.

cheers.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 5/8] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS

[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 <b-cousson-l0cyMroinI0@public.gmane.org>
Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 6219522..ba4e910 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
 
 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)) {
+		return __raw_readw(i2c_dev->base +
+				((i2c_dev->regs[reg] - 0x04)
+				 << i2c_dev->reg_shift));
+	} else {
+		return __raw_readw(i2c_dev->base +
 				(i2c_dev->regs[reg] << i2c_dev->reg_shift));
+	}
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
-- 
1.8.0.rc0

[PATCH 3/8] i2c: omap: fix error checking

[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 <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index bea0277..e0eab38 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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)) {
 		ret = -EIO;
 		goto err_i2c_init;
 	}
-- 
1.8.0.rc0

Re: [PATCH 3/8] i2c: omap: fix error checking

[Michael Trimarchi]

Hi

On 10/22/2012 11:46 AM, Felipe Balbi wrote:
> 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 <balbi-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index bea0277..e0eab38 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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?

Michael

>  		ret = -EIO;
>  		goto err_i2c_init;
>  	}
> 

Re: [PATCH 3/8] i2c: omap: fix error checking

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1488 bytes --]

Hi,

On Wed, Oct 24, 2012 at 04:41:11PM +0200, Michael Trimarchi wrote:
> Hi
> 
> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
> > 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 <balbi-l0cyMroinI0@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-omap.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index bea0277..e0eab38 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -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?

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/8] i2c: omap: fix error checking

[Michael Trimarchi]

Hi

On 10/25/2012 12:10 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Oct 24, 2012 at 04:41:11PM +0200, Michael Trimarchi wrote:
>> Hi
>>
>> On 10/22/2012 11:46 AM, Felipe Balbi wrote:
>>> 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 <balbi-l0cyMroinI0@public.gmane.org>
>>> ---
>>>  drivers/i2c/busses/i2c-omap.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>>> index bea0277..e0eab38 100644
>>> --- a/drivers/i2c/busses/i2c-omap.c
>>> +++ b/drivers/i2c/busses/i2c-omap.c
>>> @@ -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?
> 
> 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. 

Michael 

Re: [PATCH 3/8] i2c: omap: fix error checking

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1256 bytes --]

Hi,

On Thu, Oct 25, 2012 at 12:33:18PM +0200, Michael Trimarchi wrote:
> >>> @@ -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?
> > 
> > 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

I never claimed the contrary. I said *semantically* they're not the
same.

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

We still have a single *if* and I'm sure compiler will optimize that
expression as much as it likes.

> can use undefined states to simplify the logic. 

don't-care is not the same as undefined states.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/8] i2c: omap: also complete() when stat becomes zero

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

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

Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e0eab38..6219522 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1021,9 +1021,8 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		}
 	} while (stat);
 
-	omap_i2c_complete_cmd(dev, err);
-
 out:
+	omap_i2c_complete_cmd(dev, err);
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	return IRQ_HANDLED;
-- 
1.8.0.rc0

[PATCH 6/8] i2c: omap: wait for transfer completion before sending STP bit

[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

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 <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 101 ++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 62 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c65d526..8104aa2 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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 +476,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 +537,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 +578,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 +590,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;
 	}
 
 	if ((dev->cmd_err & OMAP_I2C_STAT_AL)
 			|| (dev->cmd_err & OMAP_I2C_STAT_ROVR)
 			|| (dev->cmd_err & 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;
 
-		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);
-		}
-
 		ret = -EREMOTEIO;
-		goto err;
+		omap_i2c_init(dev);
 	}
 
-	return 0;
+out:
 
-err_i2c_init:
-	omap_i2c_init(dev);
+	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);
+
+		ret = omap_i2c_wait_for_ardy(dev);
+	}
 
-err:
 	return ret;
 }
 
@@ -947,19 +937,6 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 			break;
 		}
 
-		/*
-		 * ProDB0017052: Clear ARDY bit twice
-		 */
-		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
-					OMAP_I2C_STAT_AL)) {
-			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
-						OMAP_I2C_STAT_RDR |
-						OMAP_I2C_STAT_XRDY |
-						OMAP_I2C_STAT_XDR |
-						OMAP_I2C_STAT_ARDY));
-			break;
-		}
-
 		if (stat & OMAP_I2C_STAT_RDR) {
 			u8 num_bytes = 1;
 
-- 
1.8.0.rc0

[PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Felipe Balbi]

From: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>

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 <shubhrajyoti-l0cyMroinI0@public.gmane.org>
Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 0e949cb..4b35c9b 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -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			*/
 };
 
-- 
1.8.0.rc0

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Jean Delvare]

Hi Felipe, Shubhrajyoti,

On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> From: Shubhrajyoti D <shubhrajyoti@ti.com>
> 
> 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 <shubhrajyoti@ti.com>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  include/uapi/linux/i2c.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> index 0e949cb..4b35c9b 100644
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
> @@ -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.

Not all i2c bus drivers will implement this functionality at first.
Some may even be unable to ever implement it. As soon as i2c chip
drivers start checking this value, you will hit combinations of chip
driver checking the value and bus driver not setting it. And it has to
work.

We have to decide whether it is enough to initialize "transferred" to
0, or if we need a more reliable way to distinguish between "0 bytes
transferred" and "the bus driver can't tell". What's your take on this?
If we need to distinguish, this could be done using a new I2C_FUNC_*
flag, or by initializing "transferred" to (__u16)(-1) instead of 0 for
example.

Then we have to decide whether initialization is done by the individual
drivers, or by i2c-core. By the drivers might be faster, but this may
also mean more code (and bugs more likely) than letting i2c-core handle
it. The exact balance probably depends on the answer to the previous
question (initializing a field to 0 is free in many cases.)

-- 
Jean Delvare

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> Hi Felipe, Shubhrajyoti,
> 
> On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > From: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> > 
> > 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 <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> > Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > ---
> >  include/uapi/linux/i2c.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > index 0e949cb..4b35c9b 100644
> > --- a/include/uapi/linux/i2c.h
> > +++ b/include/uapi/linux/i2c.h
> > @@ -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.

Now that we have the userspace API headers separated, this is now much
easier to detect: a patch which touches a uapi header needs much more
careful consideration than one which doesn't.

So no, strong NAK.  This is not how we treat userspace.

If we want to change userspace API then we do it in a sane manner, where
we provide the new functionality in a way that it doesn't break existing
users.  There's two ways to do this:

1. Leave the existing struct alone, introduce a new one with new ioctls.
   Schedule the removal of the old interfaces for maybe 10 years time.

2. Rename the existing struct (eg struct old_i2c_msg), and create a new
   struct called i2c_msg.  Rename the existing ioctls to have OLD_ in
   their names.  Provide the existing ioctls under those names, and
   make them print a warning once that userspace programs need updating.
   Schedule the removal of the old interfaces for a shorter number of
   years than (1);

Remember all those "old" syscalls we have... this is no different from
those.

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Jean Delvare]

On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> > Hi Felipe, Shubhrajyoti,
> > 
> > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > > From: Shubhrajyoti D <shubhrajyoti@ti.com>
> > > 
> > > 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 <shubhrajyoti@ti.com>
> > > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > > ---
> > >  include/uapi/linux/i2c.h | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > > index 0e949cb..4b35c9b 100644
> > > --- a/include/uapi/linux/i2c.h
> > > +++ b/include/uapi/linux/i2c.h
> > > @@ -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.

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?

-- 
Jean Delvare

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
> On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 25, 2012 at 02:57:48PM +0200, Jean Delvare wrote:
> > > Hi Felipe, Shubhrajyoti,
> > > 
> > > On Mon, 22 Oct 2012 12:46:57 +0300, Felipe Balbi wrote:
> > > > From: Shubhrajyoti D <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> > > > 
> > > > 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 <shubhrajyoti-l0cyMroinI0@public.gmane.org>
> > > > Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> > > > ---
> > > >  include/uapi/linux/i2c.h | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
> > > > index 0e949cb..4b35c9b 100644
> > > > --- a/include/uapi/linux/i2c.h
> > > > +++ b/include/uapi/linux/i2c.h
> > > > @@ -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.
> 
> 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?

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Jean Delvare]

On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
> > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> > > 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?
> 
> 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.

-- 
Jean Delvare

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote:
> On Thu, 25 Oct 2012 14:46:09 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 25, 2012 at 03:42:02PM +0200, Jean Delvare wrote:
> > > On Thu, 25 Oct 2012 14:14:59 +0100, Russell King - ARM Linux wrote:
> > > > 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?
> > 
> > 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.

According to Al Viro, that would be m68k.

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Jean Delvare]

On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote:
> > 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:

--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
 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?

Thanks,
-- 
Jean Delvare

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Al Viro]

On Sat, Oct 27, 2012 at 04:32:24PM +0200, Jean Delvare wrote:
> On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote:
> > > 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:
[snip]
> 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.

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Al Viro]

On Sat, Oct 27, 2012 at 05:40:13PM +0100, Al Viro wrote:

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

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Al Viro]

On Sat, Oct 27, 2012 at 06:02:35PM +0100, Al Viro wrote:
> On Sat, Oct 27, 2012 at 05:40:13PM +0100, Al Viro wrote:
>
> > 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...

... and google for i2c coldfire immediately turns up this:
http://mailman.uclinux.org/pipermail/uclinux-dev/2012-May/051874.html
with
+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.

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Jean Delvare]

Hi Al,

On Sat, 27 Oct 2012 18:10:36 +0100, Al Viro wrote:
> On Sat, Oct 27, 2012 at 06:02:35PM +0100, Al Viro wrote:
> > On Sat, Oct 27, 2012 at 05:40:13PM +0100, Al Viro wrote:
> >
> > > 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...

Thanks for the fast and complete answer.

> ... and google for i2c coldfire immediately turns up this:
> http://mailman.uclinux.org/pipermail/uclinux-dev/2012-May/051874.html
> with
> +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.

You are completely right, we will have to find another way to let bus
drivers report how much of each message they managed to transmit or
receive.

Thanks again,
-- 
Jean Delvare

Re: [PATCH 7/8] i2c: add 'transferred' field to struct i2c_msg

[Russell King - ARM Linux]

On Sat, Oct 27, 2012 at 04:32:24PM +0200, Jean Delvare wrote:
> On Thu, 25 Oct 2012 15:18:00 +0100, Russell King - ARM Linux wrote:
> > On Thu, Oct 25, 2012 at 03:56:33PM +0200, Jean Delvare wrote:
> > > 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:
> 
> --- a/include/uapi/linux/i2c.h
> +++ b/include/uapi/linux/i2c.h
>  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?

So, you're attitude here is "I don't believe you, you are lying".  Well,
if you have that level of distrust of your fellow developers, then you
don't deserve to be a Linux developer at all - and given that why should
I take any notice of you in the future over I2C stuff.

Especially as you've just proven that you don't know how to deal properly
with APIs...

Quite frankly I find your attitude towards me here totally disgusting and
insulting.

Not surprisingly, I didn't lie (I don't lie) and so I didn't "make up"
that M68k is one such architecture, and you've now had the confirmation
from Al.

So, I look forward to your apology for _implying_ that I'm a liar over
this issue, or if not, your resignation as I2C maintainer.

Thanks.

[PATCH 8/8] i2c: omap: implement handling for 'transferred' bytes

[Felipe Balbi]

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 <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 8104aa2..f59c8cd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -594,6 +594,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		goto out;
 	}
 
+	msg->transferred = msg->len;
+	wmb();
+
 	if ((dev->cmd_err & OMAP_I2C_STAT_AL)
 			|| (dev->cmd_err & OMAP_I2C_STAT_ROVR)
 			|| (dev->cmd_err & OMAP_I2C_STAT_XUDF)) {
@@ -603,6 +606,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	}
 
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
+		/* In case of a NACK, we need to check how many bytes we
+		 * actually transferred, so we can tell our client driver about
+		 * it.
+		 *
+		 * Let's check it here and overwrite msg->transferred.
+		 */
+		w = omap_i2c_read_reg(dev, OMAP_I2C_CNT_REG);
+		msg->transferred = msg->len - w - 1;
+
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
 
-- 
1.8.0.rc0

Re: [PATCH 0/8] I2C patches for v3.8 merge window

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

Hi,

On Mon, Oct 22, 2012 at 12:46:50PM +0300, Felipe Balbi wrote:
> 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

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

> 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(-)
> 
> -- 
> 1.8.0.rc0
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/8] I2C patches for v3.8 merge window

[Shubhrajyoti Datta]

On Mon, Oct 22, 2012 at 7:00 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> Hi,
>
> On Mon, Oct 22, 2012 at 12:46:50PM +0300, Felipe Balbi wrote:
>> 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 <shubhrajyoti-l0cyMroinI0@public.gmane.org>

>>
>> 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
>
>> 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(-)
>>
>> --
>> 1.8.0.rc0
>>
>
> --
> balbi

Re: [PATCH 0/8] I2C patches for v3.8 merge window

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 2748 bytes --]

Hi,

On Mon, Oct 22, 2012 at 07:36:31PM +0530, Shubhrajyoti Datta wrote:
> On Mon, Oct 22, 2012 at 7:00 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Oct 22, 2012 at 12:46:50PM +0300, Felipe Balbi wrote:
> >> 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 <shubhrajyoti@ti.com>
> 

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.

> >> 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
> >
> >> 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(-)
> >>
> >> --
> >> 1.8.0.rc0
> >>
> >
> > --
> > balbi

-- 
balbi

[-- Attachment #1.2: rtctest.c --]
[-- Type: text/x-csrc, Size: 5768 bytes --]

/*
 *      Real Time Clock Driver Test/Example Program
 *
 *      Compile with:
 *		     gcc -s -Wall -Wstrict-prototypes rtctest.c -o rtctest
 *
 *      Copyright (C) 1996, Paul Gortmaker.
 *
 *      Released under the GNU General Public License, version 2,
 *      included herein by reference.
 *
 */

#include <stdio.h>
#include <linux/rtc.h>
#include <sys/ioctl.h>
#include <sys/time.h>
#include <sys/types.h>
#include <fcntl.h>
#include <unistd.h>
#include <stdlib.h>
#include <errno.h>


/*
 * This expects the new RTC class driver framework, working with
 * clocks that will often not be clones of what the PC-AT had.
 * Use the command line to specify another RTC if you need one.
 */
static const char default_rtc[] = "/dev/rtc0";


int main(int argc, char **argv)
{
	int i, fd, retval, irqcount = 0;
	unsigned long tmp, data;
	struct rtc_time rtc_tm;
	const char *rtc = default_rtc;

	switch (argc) {
	case 2:
		rtc = argv[1];
		/* FALLTHROUGH */
	case 1:
		break;
	default:
		fprintf(stderr, "usage:  rtctest [rtcdev]\n");
		return 1;
	}

	fd = open(rtc, O_RDONLY);

	if (fd ==  -1) {
		perror(rtc);
		exit(errno);
	}

	fprintf(stderr, "\n\t\t\tRTC Driver Test Example.\n\n");

	/* Turn on update interrupts (one per second) */
	retval = ioctl(fd, RTC_UIE_ON, 0);
	if (retval == -1) {
		if (errno == ENOTTY) {
			fprintf(stderr,
				"\n...Update IRQs not supported.\n");
			goto test_READ;
		}
		perror("RTC_UIE_ON ioctl");
		exit(errno);
	}

	fprintf(stderr, "Counting 5 update (1/sec) interrupts from reading %s:",
			rtc);
	fflush(stderr);
	for (i=1; i<6; i++) {
		/* This read will block */
		retval = read(fd, &data, sizeof(unsigned long));
		if (retval == -1) {
			perror("read");
			exit(errno);
		}
		fprintf(stderr, " %d",i);
		fflush(stderr);
		irqcount++;
	}

	fprintf(stderr, "\nAgain, from using select(2) on /dev/rtc:");
	fflush(stderr);
	for (i=1; i<6; i++) {
		struct timeval tv = {5, 0};     /* 5 second timeout on select */
		fd_set readfds;

		FD_ZERO(&readfds);
		FD_SET(fd, &readfds);
		/* The select will wait until an RTC interrupt happens. */
		retval = select(fd+1, &readfds, NULL, NULL, &tv);
		if (retval == -1) {
		        perror("select");
		        exit(errno);
		}
		/* This read won't block unlike the select-less case above. */
		retval = read(fd, &data, sizeof(unsigned long));
		if (retval == -1) {
		        perror("read");
		        exit(errno);
		}
		fprintf(stderr, " %d",i);
		fflush(stderr);
		irqcount++;
	}

	/* Turn off update interrupts */
	retval = ioctl(fd, RTC_UIE_OFF, 0);
	if (retval == -1) {
		perror("RTC_UIE_OFF ioctl");
		exit(errno);
	}

test_READ:
	/* Read the RTC time/date */
	retval = ioctl(fd, RTC_RD_TIME, &rtc_tm);
	if (retval == -1) {
		perror("RTC_RD_TIME ioctl");
		exit(errno);
	}

	fprintf(stderr, "\n\nCurrent RTC date/time is %d-%d-%d, %02d:%02d:%02d.\n",
		rtc_tm.tm_mday, rtc_tm.tm_mon + 1, rtc_tm.tm_year + 1900,
		rtc_tm.tm_hour, rtc_tm.tm_min, rtc_tm.tm_sec);

	/* Set the alarm to 5 sec in the future, and check for rollover */
	rtc_tm.tm_sec += 5;
	if (rtc_tm.tm_sec >= 60) {
		rtc_tm.tm_sec %= 60;
		rtc_tm.tm_min++;
	}
	if (rtc_tm.tm_min == 60) {
		rtc_tm.tm_min = 0;
		rtc_tm.tm_hour++;
	}
	if (rtc_tm.tm_hour == 24)
		rtc_tm.tm_hour = 0;

	retval = ioctl(fd, RTC_ALM_SET, &rtc_tm);
	if (retval == -1) {
		if (errno == ENOTTY) {
			fprintf(stderr,
				"\n...Alarm IRQs not supported.\n");
			goto test_PIE;
		}
		perror("RTC_ALM_SET ioctl");
		exit(errno);
	}

	/* Read the current alarm settings */
	retval = ioctl(fd, RTC_ALM_READ, &rtc_tm);
	if (retval == -1) {
		perror("RTC_ALM_READ ioctl");
		exit(errno);
	}

	fprintf(stderr, "Alarm time now set to %02d:%02d:%02d.\n",
		rtc_tm.tm_hour, rtc_tm.tm_min, rtc_tm.tm_sec);

	/* Enable alarm interrupts */
	retval = ioctl(fd, RTC_AIE_ON, 0);
	if (retval == -1) {
		perror("RTC_AIE_ON ioctl");
		exit(errno);
	}

	fprintf(stderr, "Waiting 5 seconds for alarm...");
	fflush(stderr);
	/* This blocks until the alarm ring causes an interrupt */
	retval = read(fd, &data, sizeof(unsigned long));
	if (retval == -1) {
		perror("read");
		exit(errno);
	}
	irqcount++;
	fprintf(stderr, " okay. Alarm rang.\n");

	/* Disable alarm interrupts */
	retval = ioctl(fd, RTC_AIE_OFF, 0);
	if (retval == -1) {
		perror("RTC_AIE_OFF ioctl");
		exit(errno);
	}

test_PIE:
	/* Read periodic IRQ rate */
	retval = ioctl(fd, RTC_IRQP_READ, &tmp);
	if (retval == -1) {
		/* not all RTCs support periodic IRQs */
		if (errno == ENOTTY) {
			fprintf(stderr, "\nNo periodic IRQ support\n");
			goto done;
		}
		perror("RTC_IRQP_READ ioctl");
		exit(errno);
	}
	fprintf(stderr, "\nPeriodic IRQ rate is %ldHz.\n", tmp);

	fprintf(stderr, "Counting 20 interrupts at:");
	fflush(stderr);

	/* The frequencies 128Hz, 256Hz, ... 8192Hz are only allowed for root. */
	for (tmp=2; tmp<=64; tmp*=2) {

		retval = ioctl(fd, RTC_IRQP_SET, tmp);
		if (retval == -1) {
			/* not all RTCs can change their periodic IRQ rate */
			if (errno == ENOTTY) {
				fprintf(stderr,
					"\n...Periodic IRQ rate is fixed\n");
				goto done;
			}
			perror("RTC_IRQP_SET ioctl");
			exit(errno);
		}

		fprintf(stderr, "\n%ldHz:\t", tmp);
		fflush(stderr);

		/* Enable periodic interrupts */
		retval = ioctl(fd, RTC_PIE_ON, 0);
		if (retval == -1) {
			perror("RTC_PIE_ON ioctl");
			exit(errno);
		}

		for (i=1; i<21; i++) {
			/* This blocks */
			retval = read(fd, &data, sizeof(unsigned long));
			if (retval == -1) {
				perror("read");
				exit(errno);
			}
			fprintf(stderr, " %d",i);
			fflush(stderr);
			irqcount++;
		}

		/* Disable periodic interrupts */
		retval = ioctl(fd, RTC_PIE_OFF, 0);
		if (retval == -1) {
			perror("RTC_PIE_OFF ioctl");
			exit(errno);
		}
	}

done:
	fprintf(stderr, "\n\n\t\t\t *** Test complete ***\n");

	close(fd);

	return 0;
}

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/8] I2C patches for v3.8 merge window

[Shubhrajyoti]

On Monday 22 October 2012 07:36 PM, Felipe Balbi wrote:
> 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.
On omap3 tested . with off hitting both in suspend and idle path.


Also with rtcwake.

Thanks

Re: [PATCH 0/8] I2C patches for v3.8 merge window

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]


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

The series is a bit confusing mixing V1, V2 and V3 patches. Also, there
are a few comments unaddressed it seems to me (reading in hot path,
barriers). Please make sure these are properly handled and resend as a
seperate series (all patches V4). Bonus point if you rebase it to my
for-next, which I will push out soon.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v2 0/7] I2C patches for v3.8 merge window

[Felipe Balbi]

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.

Changes since v1:
	- remove unnecessary omap_i2c_init() in case of NACK
	- drop "i2c: omap: fix error checking"

Felipe Balbi (6):
  i2c: omap: no need to access platform_device
  i2c: omap: reorder exit path of omap_i2c_xfer_msg()
  i2c: omap: also complete() when stat becomes zero
  i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to
    IRQSTATUS
  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

 drivers/i2c/busses/i2c-omap.c | 124 ++++++++++++++++++++++--------------------
 include/uapi/linux/i2c.h      |   1 +
 2 files changed, 65 insertions(+), 60 deletions(-)

-- 
1.8.0

[PATCH v2 1/7] i2c: omap: no need to access platform_device

[Felipe Balbi]

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 <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..c07d9c4 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1239,8 +1239,7 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev)
 #ifdef CONFIG_PM_RUNTIME
 static int omap_i2c_runtime_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
 	u16 iv;
 
 	_dev->iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
@@ -1261,8 +1260,7 @@ static int omap_i2c_runtime_suspend(struct device *dev)
 
 static int omap_i2c_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+	struct omap_i2c_dev *_dev = dev_get_drvdata(dev);
 
 	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
-- 
1.8.0

Re: [PATCH v2 1/7] i2c: omap: no need to access platform_device

[Santosh Shilimkar]

On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> 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 <balbi-l0cyMroinI0@public.gmane.org>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>

[PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Felipe Balbi]

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

Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c07d9c4..bea0277 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
+
 		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;
 	}
-	return -EIO;
+
+	return 0;
+
+err_i2c_init:
+	omap_i2c_init(dev);
+
+err:
+	return ret;
 }
 
 
-- 
1.8.0

Re: [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Santosh Shilimkar]

On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> just a cleanup patch trying to make exit path
> more straightforward. No changes otherwise.
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
>   1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index c07d9c4..bea0277 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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

>   	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;
> -
Have you have drooped this check completely ?

>   	/* 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;
> +
> +		ret = -EREMOTEIO;
> +		goto err;
>   	}
> -	return -EIO;
> +
> +	return 0;
With initialized value you can use
return ret;

Regards
Santosh

Re: [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

Hi,

On Thu, Oct 25, 2012 at 06:12:00PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >just a cleanup patch trying to make exit path
> >more straightforward. No changes otherwise.
> >
> >Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> >---
> >  drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index c07d9c4..bea0277 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -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
> 
> >  	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;
> >-
> Have you have drooped this check completely ?

yes, the idea is to have a single exit point, so all checks below would
be executed.

> >  	/* 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;
> >+
> >+		ret = -EREMOTEIO;
> >+		goto err;
> >  	}
> >-	return -EIO;
> >+
> >+	return 0;
> With initialized value you can use
> return ret;

hmm, I guess I did that on a follow up patch where I grouped the stop
handling.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 2/7] i2c: omap: reorder exit path of omap_i2c_xfer_msg()

[Lothar Waßmann]

Hi,

Santosh Shilimkar writes:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> > just a cleanup patch trying to make exit path
> > more straightforward. No changes otherwise.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >   drivers/i2c/busses/i2c-omap.c | 26 +++++++++++++++++---------
> >   1 file changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index c07d9c4..bea0277 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -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;
[...]
> > +		ret = -EREMOTEIO;
> > +		goto err;
> >   	}
> > -	return -EIO;
> > +
> > +	return 0;
> With initialized value you can use
> return ret;
> 
Doing it this way has the advantage, that if an additional error exit
is added it will generate an 'uninitialized variable' warning, if it
fails to set the return value.



Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/7] i2c: omap: also complete() when stat becomes zero

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

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

Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index bea0277..b004126 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1021,9 +1021,8 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 		}
 	} while (stat);
 
-	omap_i2c_complete_cmd(dev, err);
-
 out:
+	omap_i2c_complete_cmd(dev, err);
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	return IRQ_HANDLED;
-- 
1.8.0

Re: [PATCH v2 3/7] i2c: omap: also complete() when stat becomes zero

[Santosh Shilimkar]

On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> 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 <balbi-l0cyMroinI0@public.gmane.org>
> ---
Looks fine. Have you hit this issue in any corner case ?

Regards
santosh

Re: [PATCH v2 3/7] i2c: omap: also complete() when stat becomes zero

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

Hi,

On Thu, Oct 25, 2012 at 06:13:16PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >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 <balbi-l0cyMroinI0@public.gmane.org>
> >---
> Looks fine. Have you hit this issue in any corner case ?

in fact, yes. With a difficult to reproduce situation with drv2665 (one
of TI's piezo drivers) I saw that I was missing the ack and all
transfers were timing out ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS

[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 <b-cousson-l0cyMroinI0@public.gmane.org>
Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b004126..20f9ad6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
 
 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)) {
+		return __raw_readw(i2c_dev->base +
+				((i2c_dev->regs[reg] - 0x04)
+				 << i2c_dev->reg_shift));
+	} else {
+		return __raw_readw(i2c_dev->base +
 				(i2c_dev->regs[reg] << i2c_dev->reg_shift));
+	}
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
-- 
1.8.0

Re: [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS

[Santosh Shilimkar]

On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> 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 <b-cousson-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
>   drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b004126..20f9ad6 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
>
>   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.

regards
santosh

Re: [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2360 bytes --]

Hi,

On Thu, Oct 25, 2012 at 06:23:57PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >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 <b-cousson-l0cyMroinI0@public.gmane.org>
> >Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> >---
> >  drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >index b004126..20f9ad6 100644
> >--- a/drivers/i2c/busses/i2c-omap.c
> >+++ b/drivers/i2c/busses/i2c-omap.c
> >@@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
> >
> >  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...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 4/7] i2c: omap: in case of VERSION_2 read IRQSTATUS_RAW but write to IRQSTATUS

[Santosh Shilimkar]

On Thursday 25 October 2012 06:22 PM, Felipe Balbi wrote:
> Hi,
>
> On Thu, Oct 25, 2012 at 06:23:57PM +0530, Santosh Shilimkar wrote:
>> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
>>> 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 <b-cousson@ti.com>
>>> Signed-off-by: Felipe Balbi <balbi@ti.com>
>>> ---
>>>   drivers/i2c/busses/i2c-omap.c | 12 +++++++++++-
>>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>>> index b004126..20f9ad6 100644
>>> --- a/drivers/i2c/busses/i2c-omap.c
>>> +++ b/drivers/i2c/busses/i2c-omap.c
>>> @@ -271,8 +271,18 @@ static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
>>>
>>>   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...
>
Its a hot path since you read many I2C register reads, so getting
rid of that additional check will be good.

[PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit

[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

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 <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 89 ++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 56 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 20f9ad6..699fa12 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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;
 
-		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);
-		}
-
 		ret = -EREMOTEIO;
-		goto err;
+		omap_i2c_init(dev);
 	}
 
-	return 0;
+out:
 
-err_i2c_init:
-	omap_i2c_init(dev);
+	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);
+
+		ret = omap_i2c_wait_for_ardy(dev);
+	}
 
-err:
 	return ret;
 }
 
@@ -947,19 +937,6 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 			break;
 		}
 
-		/*
-		 * ProDB0017052: Clear ARDY bit twice
-		 */
-		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
-					OMAP_I2C_STAT_AL)) {
-			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
-						OMAP_I2C_STAT_RDR |
-						OMAP_I2C_STAT_XRDY |
-						OMAP_I2C_STAT_XDR |
-						OMAP_I2C_STAT_ARDY));
-			break;
-		}
-
 		if (stat & OMAP_I2C_STAT_RDR) {
 			u8 num_bytes = 1;
 
-- 
1.8.0

Re: [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5063 bytes --]

Hi,

On Thu, Oct 25, 2012 at 03:25:13PM +0300, Felipe Balbi wrote:
> 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 <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c | 89 ++++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 20f9ad6..699fa12 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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;
>  
> -		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);
> -		}
> -
>  		ret = -EREMOTEIO;
> -		goto err;
> +		omap_i2c_init(dev);

this is wrong. I will resend this patch alone. My bad.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v3 5/7] i2c: omap: wait for transfer completion before sending STP bit

[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

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 <balbi@ti.com>
---

Changes since v2:
	- remove unnecessary omap_i2c_init() which was added during a rebase.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 20f9ad6..726b916 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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,35 @@ 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;
 
-		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);
-		}
-
 		ret = -EREMOTEIO;
-		goto err;
 	}
 
-	return 0;
+out:
 
-err_i2c_init:
-	omap_i2c_init(dev);
+	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);
+
+		ret = omap_i2c_wait_for_ardy(dev);
+	}
 
-err:
 	return ret;
 }
 
@@ -947,19 +936,6 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 			break;
 		}
 
-		/*
-		 * ProDB0017052: Clear ARDY bit twice
-		 */
-		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
-					OMAP_I2C_STAT_AL)) {
-			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
-						OMAP_I2C_STAT_RDR |
-						OMAP_I2C_STAT_XRDY |
-						OMAP_I2C_STAT_XDR |
-						OMAP_I2C_STAT_ARDY));
-			break;
-		}
-
 		if (stat & OMAP_I2C_STAT_RDR) {
 			u8 num_bytes = 1;
 
-- 
1.8.0

Re: [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit

[Santosh Shilimkar]

On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> 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.
>
Will this not break the bisect since CNT and
NACK, completion is added in later patch

> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
Apart from above, rest of the change follow
the change log and looks fine tome. The
change is quite drastic so hopefully it
has gone through wider testing.

Regards
santosh

Re: [PATCH v2 5/7] i2c: omap: wait for transfer completion before sending STP bit

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 1370 bytes --]

HI,

On Thu, Oct 25, 2012 at 06:31:58PM +0530, Santosh Shilimkar wrote:
> On Thursday 25 October 2012 05:55 PM, Felipe Balbi wrote:
> >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.
> >
> Will this not break the bisect since CNT and
> NACK, completion is added in later patch

not really. It keeps current behavior.

-- 
balbi

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] i2c: omap: implement handling for 'transferred' bytes

[Felipe Balbi]

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 <balbi-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-omap.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 699fa12..d268e92 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -588,6 +588,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 		goto out;
 	}
 
+	msg->transferred = msg->len;
+	wmb();
+
 	/* We have an error */
 	if (dev->cmd_err & (OMAP_I2C_STAT_AL | OMAP_I2C_STAT_ROVR |
 			    OMAP_I2C_STAT_XUDF)) {
@@ -597,6 +600,15 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	}
 
 	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
+		/* In case of a NACK, we need to check how many bytes we
+		 * actually transferred, so we can tell our client driver about
+		 * it.
+		 *
+		 * Let's check it here and overwrite msg->transferred.
+		 */
+		w = omap_i2c_read_reg(dev, OMAP_I2C_CNT_REG);
+		msg->transferred = msg->len - w - 1;
+
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
 
-- 
1.8.0

Re: [PATCH v2 7/7] i2c: omap: implement handling for 'transferred' bytes

[Paul Walmsley]

Hi

On Thu, 25 Oct 2012, Felipe Balbi wrote:

> 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 <balbi-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 699fa12..d268e92 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -588,6 +588,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
>  		goto out;
>  	}
>  
> +	msg->transferred = msg->len;
> +	wmb();

If this barrier is being used to ensure that the compiler doesn't move 
this store past the point at which the I2C interrupt handler can be 
entered, then please consider the same comments as on the other 
patch that adds a barrier:

http://marc.info/?l=linux-omap&m=135129247524136&w=2

http://marc.info/?l=linux-omap&m=135135357305794&w=2


- Paul

Re: [PATCH v2 0/7] I2C patches for v3.8 merge window

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

Hi,

On Thu, Oct 25, 2012 at 03:25:08PM +0300, Felipe Balbi wrote:
> 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.

Wolfram, patches 1-5 are ok to apply, we just need to rework patches 6
and 7 so drivers can report how many bytes managed to be transferred.
None of these patches cause any known regressions.

let me know if you want me to resend only the 5 patches which should be
applied.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v2 6/7] i2c: add 'transferred' field to struct i2c_msg

[Felipe Balbi]

From: Shubhrajyoti D <shubhrajyoti@ti.com>

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 <shubhrajyoti@ti.com>
Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 0e949cb..4b35c9b 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -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			*/
 };
 
-- 
1.8.0