[PATCH 00/17] Big OMAP I2C Cleanup

[PATCH 00/17] Big OMAP I2C Cleanup

[Felipe Balbi]

Hi guys,

I have dropped a few patches from the series and also
tested every single patch on my pandaboard. I2C messages
are still working fine with panda after each patch.

There's still lots of work to be done on the i2c-omap.c
driver but it's now easier to read, IMO.

Felipe Balbi (17):
  i2c: omap: simplify num_bytes handling
  i2c: omap: decrease indentation level on data handling
  i2c: omap: add blank lines
  i2c: omap: simplify omap_i2c_ack_stat()
  i2c: omap: split out [XR]DR and [XR]RDY
  i2c: omap: improve 1p153 errata handling
  i2c: omap: re-factor receive/transmit data loop
  i2c: omap: switch over to do {} while loop
  i2c: omap: ack IRQ in parts
  i2c: omap: get rid of the "complete" label
  i2c: omap: switch to devm_* API
  i2c: omap: switch to platform_get_irq()
  i2c: omap: bus: add a receiver flag
  i2c: omap: simplify errata check
  i2c: omap: always return IRQ_HANDLED
  i2c: omap: simplify IRQ exit path
  i2c: omap: resize fifos before each message

 drivers/i2c/busses/i2c-omap.c |  388 +++++++++++++++++++++++------------------
 1 file changed, 222 insertions(+), 166 deletions(-)

-- 
1.7.10.4

[PATCH 01/17] i2c: omap: simplify num_bytes handling

[Felipe Balbi]

trivial patch, no functional changes

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 801df60..9b532cd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -855,8 +855,7 @@ complete:
 							OMAP_I2C_BUFSTAT_REG)
 							>> 8) & 0x3F;
 			}
-			while (num_bytes) {
-				num_bytes--;
+			while (num_bytes--) {
 				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
 				if (dev->buf_len) {
 					*dev->buf++ = w;
@@ -898,8 +897,7 @@ complete:
 							OMAP_I2C_BUFSTAT_REG)
 							& 0x3F;
 			}
-			while (num_bytes) {
-				num_bytes--;
+			while (num_bytes--) {
 				w = 0;
 				if (dev->buf_len) {
 					w = *dev->buf++;
-- 
1.7.10.4

Re: [PATCH 01/17] i2c: omap: simplify num_bytes handling

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> trivial patch, no functional changes
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
Looks good.
Reviewed-by : Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>

[PATCH 02/17] i2c: omap: decrease indentation level on data handling

[Felipe Balbi]

trivial patch, no functional changes.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 9b532cd..39d5583 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -856,22 +856,7 @@ complete:
 							>> 8) & 0x3F;
 			}
 			while (num_bytes--) {
-				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
-				if (dev->buf_len) {
-					*dev->buf++ = w;
-					dev->buf_len--;
-					/*
-					 * Data reg in 2430, omap3 and
-					 * omap4 is 8 bit wide
-					 */
-					if (dev->flags &
-						 OMAP_I2C_FLAG_16BIT_DATA_REG) {
-						if (dev->buf_len) {
-							*dev->buf++ = w >> 8;
-							dev->buf_len--;
-						}
-					}
-				} else {
+				if (!dev->buf_len) {
 					if (stat & OMAP_I2C_STAT_RRDY)
 						dev_err(dev->dev,
 							"RRDY IRQ while no data"
@@ -882,6 +867,21 @@ complete:
 								" requested\n");
 					break;
 				}
+
+				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
+				*dev->buf++ = w;
+				dev->buf_len--;
+				/*
+				 * Data reg in 2430, omap3 and
+				 * omap4 is 8 bit wide
+				 */
+				if (dev->flags &
+						OMAP_I2C_FLAG_16BIT_DATA_REG) {
+					if (dev->buf_len) {
+						*dev->buf++ = w >> 8;
+						dev->buf_len--;
+					}
+				}
 			}
 			omap_i2c_ack_stat(dev,
 				stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
@@ -898,22 +898,7 @@ complete:
 							& 0x3F;
 			}
 			while (num_bytes--) {
-				w = 0;
-				if (dev->buf_len) {
-					w = *dev->buf++;
-					dev->buf_len--;
-					/*
-					 * Data reg in 2430, omap3 and
-					 * omap4 is 8 bit wide
-					 */
-					if (dev->flags &
-						 OMAP_I2C_FLAG_16BIT_DATA_REG) {
-						if (dev->buf_len) {
-							w |= *dev->buf++ << 8;
-							dev->buf_len--;
-						}
-					}
-				} else {
+				if (!dev->buf_len) {
 					if (stat & OMAP_I2C_STAT_XRDY)
 						dev_err(dev->dev,
 							"XRDY IRQ while no "
@@ -925,6 +910,20 @@ complete:
 					break;
 				}
 
+				w = *dev->buf++;
+				dev->buf_len--;
+				/*
+				 * Data reg in 2430, omap3 and
+				 * omap4 is 8 bit wide
+				 */
+				if (dev->flags &
+						OMAP_I2C_FLAG_16BIT_DATA_REG) {
+					if (dev->buf_len) {
+						w |= *dev->buf++ << 8;
+						dev->buf_len--;
+					}
+				}
+
 				if ((dev->errata & I2C_OMAP3_1P153) &&
 				    errata_omap3_1p153(dev, &stat, &err))
 					goto complete;
-- 
1.7.10.4

Re: [PATCH 02/17] i2c: omap: decrease indentation level on data handling

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> trivial patch, no functional changes.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   63 ++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 9b532cd..39d5583 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -856,22 +856,7 @@ complete:
>                                                        >> 8) & 0x3F;
>                        }
>                        while (num_bytes--) {
> -                               w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> -                               if (dev->buf_len) {
> -                                       *dev->buf++ = w;
> -                                       dev->buf_len--;
> -                                       /*
> -                                        * Data reg in 2430, omap3 and
> -                                        * omap4 is 8 bit wide
> -                                        */
> -                                       if (dev->flags &
> -                                                OMAP_I2C_FLAG_16BIT_DATA_REG) {
> -                                               if (dev->buf_len) {
> -                                                       *dev->buf++ = w >> 8;
> -                                                       dev->buf_len--;
> -                                               }
> -                                       }
> -                               } else {
> +                               if (!dev->buf_len) {
>                                        if (stat & OMAP_I2C_STAT_RRDY)
>                                                dev_err(dev->dev,
>                                                        "RRDY IRQ while no data"
> @@ -882,6 +867,21 @@ complete:
>                                                                " requested\n");
>                                        break;
>                                }
> +
> +                               w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> +                               *dev->buf++ = w;
> +                               dev->buf_len--;
> +                               /*
> +                                * Data reg in 2430, omap3 and
> +                                * omap4 is 8 bit wide
> +                                */
> +                               if (dev->flags &
> +                                               OMAP_I2C_FLAG_16BIT_DATA_REG) {
> +                                       if (dev->buf_len) {
> +                                               *dev->buf++ = w >> 8;
> +                                               dev->buf_len--;
> +                                       }
> +                               }
>                        }
>                        omap_i2c_ack_stat(dev,
>                                stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
> @@ -898,22 +898,7 @@ complete:
>                                                        & 0x3F;
>                        }
>                        while (num_bytes--) {
> -                               w = 0;
> -                               if (dev->buf_len) {
> -                                       w = *dev->buf++;
> -                                       dev->buf_len--;
> -                                       /*
> -                                        * Data reg in 2430, omap3 and
> -                                        * omap4 is 8 bit wide
> -                                        */
> -                                       if (dev->flags &
> -                                                OMAP_I2C_FLAG_16BIT_DATA_REG) {
> -                                               if (dev->buf_len) {
> -                                                       w |= *dev->buf++ << 8;
> -                                                       dev->buf_len--;
> -                                               }
> -                                       }
> -                               } else {
> +                               if (!dev->buf_len) {
>                                        if (stat & OMAP_I2C_STAT_XRDY)
>                                                dev_err(dev->dev,
>                                                        "XRDY IRQ while no "
> @@ -925,6 +910,20 @@ complete:
>                                        break;
>                                }
>
> +                               w = *dev->buf++;
> +                               dev->buf_len--;
> +                               /*
> +                                * Data reg in 2430, omap3 and
> +                                * omap4 is 8 bit wide
> +                                */
> +                               if (dev->flags &
> +                                               OMAP_I2C_FLAG_16BIT_DATA_REG) {
> +                                       if (dev->buf_len) {
> +                                               w |= *dev->buf++ << 8;
> +                                               dev->buf_len--;
> +                                       }
> +                               }
> +
>                                if ((dev->errata & I2C_OMAP3_1P153) &&
>                                    errata_omap3_1p153(dev, &stat, &err))
>                                        goto complete;

Avoids couple of if check and kills the un-necessary if/else inside the loop.
Did I miss anything else in the patch ?

Looks good to me
Reviewed-by : Santosh Shilimkar <santosh.shilimkar@ti.com>

Re: [PATCH 02/17] i2c: omap: decrease indentation level on data handling

[Felipe Balbi]

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

On Thu, Jun 14, 2012 at 04:11:14PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> > trivial patch, no functional changes.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |   63 ++++++++++++++++++++---------------------
> >  1 file changed, 31 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index 9b532cd..39d5583 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -856,22 +856,7 @@ complete:
> >                                                        >> 8) & 0x3F;
> >                        }
> >                        while (num_bytes--) {
> > -                               w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> > -                               if (dev->buf_len) {
> > -                                       *dev->buf++ = w;
> > -                                       dev->buf_len--;
> > -                                       /*
> > -                                        * Data reg in 2430, omap3 and
> > -                                        * omap4 is 8 bit wide
> > -                                        */
> > -                                       if (dev->flags &
> > -                                                OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > -                                               if (dev->buf_len) {
> > -                                                       *dev->buf++ = w >> 8;
> > -                                                       dev->buf_len--;
> > -                                               }
> > -                                       }
> > -                               } else {
> > +                               if (!dev->buf_len) {
> >                                        if (stat & OMAP_I2C_STAT_RRDY)
> >                                                dev_err(dev->dev,
> >                                                        "RRDY IRQ while no data"
> > @@ -882,6 +867,21 @@ complete:
> >                                                                " requested\n");
> >                                        break;
> >                                }
> > +
> > +                               w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
> > +                               *dev->buf++ = w;
> > +                               dev->buf_len--;
> > +                               /*
> > +                                * Data reg in 2430, omap3 and
> > +                                * omap4 is 8 bit wide
> > +                                */
> > +                               if (dev->flags &
> > +                                               OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > +                                       if (dev->buf_len) {
> > +                                               *dev->buf++ = w >> 8;
> > +                                               dev->buf_len--;
> > +                                       }
> > +                               }
> >                        }
> >                        omap_i2c_ack_stat(dev,
> >                                stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
> > @@ -898,22 +898,7 @@ complete:
> >                                                        & 0x3F;
> >                        }
> >                        while (num_bytes--) {
> > -                               w = 0;
> > -                               if (dev->buf_len) {
> > -                                       w = *dev->buf++;
> > -                                       dev->buf_len--;
> > -                                       /*
> > -                                        * Data reg in 2430, omap3 and
> > -                                        * omap4 is 8 bit wide
> > -                                        */
> > -                                       if (dev->flags &
> > -                                                OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > -                                               if (dev->buf_len) {
> > -                                                       w |= *dev->buf++ << 8;
> > -                                                       dev->buf_len--;
> > -                                               }
> > -                                       }
> > -                               } else {
> > +                               if (!dev->buf_len) {
> >                                        if (stat & OMAP_I2C_STAT_XRDY)
> >                                                dev_err(dev->dev,
> >                                                        "XRDY IRQ while no "
> > @@ -925,6 +910,20 @@ complete:
> >                                        break;
> >                                }
> >
> > +                               w = *dev->buf++;
> > +                               dev->buf_len--;
> > +                               /*
> > +                                * Data reg in 2430, omap3 and
> > +                                * omap4 is 8 bit wide
> > +                                */
> > +                               if (dev->flags &
> > +                                               OMAP_I2C_FLAG_16BIT_DATA_REG) {
> > +                                       if (dev->buf_len) {
> > +                                               w |= *dev->buf++ << 8;
> > +                                               dev->buf_len--;
> > +                                       }
> > +                               }
> > +
> >                                if ((dev->errata & I2C_OMAP3_1P153) &&
> >                                    errata_omap3_1p153(dev, &stat, &err))
> >                                        goto complete;
> 
> Avoids couple of if check and kills the un-necessary if/else inside the loop.
> Did I miss anything else in the patch ?

nope, that's about it ;-)

-- 
balbi

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

[PATCH 03/17] i2c: omap: add blank lines

[Felipe Balbi]

trivial patch to aid readability. No functional
changes.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 39d5583..07ae391 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -829,6 +829,7 @@ complete:
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 		}
+
 		/*
 		 * ProDB0017052: Clear ARDY bit twice
 		 */
@@ -841,6 +842,7 @@ complete:
 			omap_i2c_complete_cmd(dev, err);
 			return IRQ_HANDLED;
 		}
+
 		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
 			u8 num_bytes = 1;
 
@@ -887,6 +889,7 @@ complete:
 				stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
 			continue;
 		}
+
 		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
 			u8 num_bytes = 1;
 			if (dev->fifo_size) {
@@ -934,10 +937,12 @@ complete:
 				stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
 			continue;
 		}
+
 		if (stat & OMAP_I2C_STAT_ROVR) {
 			dev_err(dev->dev, "Receive overrun\n");
 			dev->cmd_err |= OMAP_I2C_STAT_ROVR;
 		}
+
 		if (stat & OMAP_I2C_STAT_XUDF) {
 			dev_err(dev->dev, "Transmit underflow\n");
 			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
-- 
1.7.10.4

Re: [PATCH 03/17] i2c: omap: add blank lines

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> trivial patch to aid readability. No functional
> changes.
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
Not sure if it is needed but may be spliting the series into clean
up and functional update like logic changes etc may be better
for merge as well as testing.

Regards
santosh

[PATCH 05/17] i2c: omap: split out [XR]DR and [XR]RDY

[Felipe Balbi]

While they do pretty much the same thing, there
are a few peculiarities. Specially WRT erratas,
it's best to split those out and re-factor the
read/write loop to another function which both
cases call.

This last part will be done on another patch.

While at that, also avoid an unncessary register
read since dev->fifo_len will always contain the
correct amount of data to be transferred.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fa9ddb6..0661ca1 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -844,36 +844,62 @@ complete:
 			return IRQ_HANDLED;
 		}
 
-		if (stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR)) {
+		if (stat & OMAP_I2C_STAT_RDR) {
 			u8 num_bytes = 1;
 
+			if (dev->fifo_size)
+				num_bytes = dev->fifo_size;
+
+			while (num_bytes--) {
+				if (!dev->buf_len) {
+					dev_err(dev->dev,
+							"RDR IRQ while no data"
+							" requested\n");
+					break;
+				}
+
+				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
+				*dev->buf++ = w;
+				dev->buf_len--;
+
+				/*
+				 * Data reg in 2430, omap3 and
+				 * omap4 is 8 bit wide
+				 */
+				if (dev->flags &
+						OMAP_I2C_FLAG_16BIT_DATA_REG) {
+					if (dev->buf_len) {
+						*dev->buf++ = w >> 8;
+						dev->buf_len--;
+					}
+				}
+			}
+
 			if (dev->errata & I2C_OMAP_ERRATA_I207)
 				i2c_omap_errata_i207(dev, stat);
 
-			if (dev->fifo_size) {
-				if (stat & OMAP_I2C_STAT_RRDY)
-					num_bytes = dev->fifo_size;
-				else    /* read RXSTAT on RDR interrupt */
-					num_bytes = (omap_i2c_read_reg(dev,
-							OMAP_I2C_BUFSTAT_REG)
-							>> 8) & 0x3F;
-			}
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RDR);
+			continue;
+		}
+
+		if (stat & OMAP_I2C_STAT_RRDY) {
+			u8 num_bytes = 1;
+
+			if (dev->fifo_size)
+				num_bytes = dev->fifo_size;
+
 			while (num_bytes--) {
 				if (!dev->buf_len) {
-					if (stat & OMAP_I2C_STAT_RRDY)
-						dev_err(dev->dev,
+					dev_err(dev->dev,
 							"RRDY IRQ while no data"
-								" requested\n");
-					if (stat & OMAP_I2C_STAT_RDR)
-						dev_err(dev->dev,
-							"RDR IRQ while no data"
-								" requested\n");
+							" requested\n");
 					break;
 				}
 
 				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
 				*dev->buf++ = w;
 				dev->buf_len--;
+
 				/*
 				 * Data reg in 2430, omap3 and
 				 * omap4 is 8 bit wide
@@ -886,36 +912,68 @@ complete:
 					}
 				}
 			}
-			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
-						OMAP_I2C_STAT_RDR));
+
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RRDY);
 			continue;
 		}
 
-		if (stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR)) {
+		if (stat & OMAP_I2C_STAT_XDR) {
 			u8 num_bytes = 1;
-			if (dev->fifo_size) {
-				if (stat & OMAP_I2C_STAT_XRDY)
-					num_bytes = dev->fifo_size;
-				else    /* read TXSTAT on XDR interrupt */
-					num_bytes = omap_i2c_read_reg(dev,
-							OMAP_I2C_BUFSTAT_REG)
-							& 0x3F;
+
+			if (dev->fifo_size)
+				num_bytes = dev->fifo_size;
+
+			while (num_bytes--) {
+				if (!dev->buf_len) {
+					dev_err(dev->dev,
+							"XDR IRQ while no "
+							"data to send\n");
+					break;
+				}
+
+				w = *dev->buf++;
+				dev->buf_len--;
+
+				/*
+				 * Data reg in 2430, omap3 and
+				 * omap4 is 8 bit wide
+				 */
+				if (dev->flags &
+						OMAP_I2C_FLAG_16BIT_DATA_REG) {
+					if (dev->buf_len) {
+						w |= *dev->buf++ << 8;
+						dev->buf_len--;
+					}
+				}
+
+				if ((dev->errata & I2C_OMAP3_1P153) &&
+				    errata_omap3_1p153(dev, &stat, &err))
+					goto complete;
+
+				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
+
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
+			continue;
+		}
+
+		if (stat & OMAP_I2C_STAT_XRDY) {
+			u8 num_bytes = 1;
+
+			if (dev->fifo_size)
+				num_bytes = dev->fifo_size;
+
 			while (num_bytes--) {
 				if (!dev->buf_len) {
-					if (stat & OMAP_I2C_STAT_XRDY)
-						dev_err(dev->dev,
+					dev_err(dev->dev,
 							"XRDY IRQ while no "
 							"data to send\n");
-					if (stat & OMAP_I2C_STAT_XDR)
-						dev_err(dev->dev,
-							"XDR IRQ while no "
-							"data to send\n");
 					break;
 				}
 
 				w = *dev->buf++;
 				dev->buf_len--;
+
 				/*
 				 * Data reg in 2430, omap3 and
 				 * omap4 is 8 bit wide
@@ -934,8 +992,8 @@ complete:
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
-						OMAP_I2C_STAT_XDR));
+
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
 			continue;
 		}
 
-- 
1.7.10.4

[PATCH 07/17] i2c: omap: re-factor receive/transmit data loop

[Felipe Balbi]

re-factor the common parts to a separate function,
so that code is easier to read and understand.

No functional changes.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 52861c2..381bb36 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -795,12 +795,81 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev)
 	return 0;
 }
 
+static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes,
+		bool is_rdr)
+{
+	u16		w;
+
+	while (num_bytes--) {
+		if (!dev->buf_len) {
+			dev_err(dev->dev, "%s without data",
+					is_rdr ? "RDR" : "RRDY");
+			break;
+		}
+
+		w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
+		*dev->buf++ = w;
+		dev->buf_len--;
+
+		/*
+		 * Data reg in 2430, omap3 and
+		 * omap4 is 8 bit wide
+		 */
+		if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
+			if (dev->buf_len) {
+				*dev->buf++ = w >> 8;
+				dev->buf_len--;
+			}
+		}
+	}
+}
+
+static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
+		bool is_xdr)
+{
+	u16		w;
+
+	while (num_bytes--) {
+		if (!dev->buf_len) {
+			dev_err(dev->dev, "%s without data",
+					is_xdr ? "XDR" : "XRDY");
+			break;
+		}
+
+		w = *dev->buf++;
+		dev->buf_len--;
+
+		/*
+		 * Data reg in 2430, omap3 and
+		 * omap4 is 8 bit wide
+		 */
+		if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
+			if (dev->buf_len) {
+				w |= *dev->buf++ << 8;
+				dev->buf_len--;
+			}
+		}
+
+		if (dev->errata & I2C_OMAP3_1P153) {
+			int ret;
+
+			ret = errata_omap3_1p153(dev);
+			if (ret < 0)
+				return ret;
+		}
+
+		omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
+	}
+
+	return 0;
+}
+
 static irqreturn_t
 omap_i2c_isr(int this_irq, void *dev_id)
 {
 	struct omap_i2c_dev *dev = dev_id;
 	u16 bits;
-	u16 stat, w;
+	u16 stat;
 	int err, count = 0;
 
 	if (pm_runtime_suspended(dev->dev))
@@ -853,30 +922,7 @@ complete:
 			if (dev->fifo_size)
 				num_bytes = dev->fifo_size;
 
-			while (num_bytes--) {
-				if (!dev->buf_len) {
-					dev_err(dev->dev,
-							"RDR IRQ while no data"
-							" requested\n");
-					break;
-				}
-
-				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
-				*dev->buf++ = w;
-				dev->buf_len--;
-
-				/*
-				 * Data reg in 2430, omap3 and
-				 * omap4 is 8 bit wide
-				 */
-				if (dev->flags &
-						OMAP_I2C_FLAG_16BIT_DATA_REG) {
-					if (dev->buf_len) {
-						*dev->buf++ = w >> 8;
-						dev->buf_len--;
-					}
-				}
-			}
+			omap_i2c_receive_data(dev, num_bytes, true);
 
 			if (dev->errata & I2C_OMAP_ERRATA_I207)
 				i2c_omap_errata_i207(dev, stat);
@@ -891,78 +937,23 @@ complete:
 			if (dev->fifo_size)
 				num_bytes = dev->fifo_size;
 
-			while (num_bytes--) {
-				if (!dev->buf_len) {
-					dev_err(dev->dev,
-							"RRDY IRQ while no data"
-							" requested\n");
-					break;
-				}
-
-				w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
-				*dev->buf++ = w;
-				dev->buf_len--;
-
-				/*
-				 * Data reg in 2430, omap3 and
-				 * omap4 is 8 bit wide
-				 */
-				if (dev->flags &
-						OMAP_I2C_FLAG_16BIT_DATA_REG) {
-					if (dev->buf_len) {
-						*dev->buf++ = w >> 8;
-						dev->buf_len--;
-					}
-				}
-			}
-
+			omap_i2c_receive_data(dev, num_bytes, false);
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RRDY);
 			continue;
 		}
 
 		if (stat & OMAP_I2C_STAT_XDR) {
+			int ret;
 			u8 num_bytes = 1;
 
 			if (dev->fifo_size)
 				num_bytes = dev->fifo_size;
 
-			while (num_bytes--) {
-				if (!dev->buf_len) {
-					dev_err(dev->dev,
-							"XDR IRQ while no "
-							"data to send\n");
-					break;
-				}
-
-				w = *dev->buf++;
-				dev->buf_len--;
-
-				/*
-				 * Data reg in 2430, omap3 and
-				 * omap4 is 8 bit wide
-				 */
-				if (dev->flags &
-						OMAP_I2C_FLAG_16BIT_DATA_REG) {
-					if (dev->buf_len) {
-						w |= *dev->buf++ << 8;
-						dev->buf_len--;
-					}
-				}
-
-				if (dev->errata & I2C_OMAP3_1P153) {
-					int ret;
-
-					ret = errata_omap3_1p153(dev);
-					stat = omap_i2c_read_reg(dev,
-							OMAP_I2C_STAT_REG);
-
-					if (ret < 0) {
-						err |= OMAP_I2C_STAT_XUDF;
-						goto complete;
-					}
-				}
-
-				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
+			ret = omap_i2c_transmit_data(dev, num_bytes, true);
+			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+			if (ret < 0) {
+				err |= OMAP_I2C_STAT_XUDF;
+				goto complete;
 			}
 
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
@@ -970,48 +961,17 @@ complete:
 		}
 
 		if (stat & OMAP_I2C_STAT_XRDY) {
+			int ret;
 			u8 num_bytes = 1;
 
 			if (dev->fifo_size)
 				num_bytes = dev->fifo_size;
 
-			while (num_bytes--) {
-				if (!dev->buf_len) {
-					dev_err(dev->dev,
-							"XRDY IRQ while no "
-							"data to send\n");
-					break;
-				}
-
-				w = *dev->buf++;
-				dev->buf_len--;
-
-				/*
-				 * Data reg in 2430, omap3 and
-				 * omap4 is 8 bit wide
-				 */
-				if (dev->flags &
-						OMAP_I2C_FLAG_16BIT_DATA_REG) {
-					if (dev->buf_len) {
-						w |= *dev->buf++ << 8;
-						dev->buf_len--;
-					}
-				}
-
-				if (dev->errata & I2C_OMAP3_1P153) {
-					int ret;
-
-					ret = errata_omap3_1p153(dev);
-					stat = omap_i2c_read_reg(dev,
-							OMAP_I2C_STAT_REG);
-
-					if (ret < 0) {
-						err |= OMAP_I2C_STAT_XUDF;
-						goto complete;
-					}
-				}
-
-				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
+			ret = omap_i2c_transmit_data(dev, num_bytes, false);
+			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+			if (ret < 0) {
+				err |= OMAP_I2C_STAT_XUDF;
+				goto complete;
 			}
 
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
-- 
1.7.10.4

[PATCH 09/17] i2c: omap: ack IRQ in parts

[Felipe Balbi]

According to flow diagrams on OMAP TRMs,
we should ACK the IRQ as they happen.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index f978b14..c00ba7d 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
 		}
 
 complete:
-		/*
-		 * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
-		 * acked after the data operation is complete.
-		 * Ref: TRM SWPU114Q Figure 18-31
-		 */
-		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
-				~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
-				OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
-
-		if (stat & OMAP_I2C_STAT_NACK)
+		if (stat & OMAP_I2C_STAT_NACK) {
+			dev_err(dev->dev, "No Acknowledge\n");
 			err |= OMAP_I2C_STAT_NACK;
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
+			omap_i2c_complete_cmd(dev, err);
+			return IRQ_HANDLED;
+		}
 
 		if (stat & OMAP_I2C_STAT_AL) {
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
+			omap_i2c_complete_cmd(dev, err);
+			return IRQ_HANDLED;
 		}
 
 		/*
@@ -989,12 +988,18 @@ complete:
 
 		if (stat & OMAP_I2C_STAT_ROVR) {
 			dev_err(dev->dev, "Receive overrun\n");
-			dev->cmd_err |= OMAP_I2C_STAT_ROVR;
+			err |= OMAP_I2C_STAT_ROVR;
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
+			omap_i2c_complete_cmd(dev, err);
+			return IRQ_HANDLED;
 		}
 
 		if (stat & OMAP_I2C_STAT_XUDF) {
 			dev_err(dev->dev, "Transmit underflow\n");
-			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
+			err |= OMAP_I2C_STAT_XUDF;
+			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
+			omap_i2c_complete_cmd(dev, err);
+			return IRQ_HANDLED;
 		}
 	} while (stat);
 
-- 
1.7.10.4

Re: [PATCH 09/17] i2c: omap: ack IRQ in parts

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> According to flow diagrams on OMAP TRMs,
> we should ACK the IRQ as they happen.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index f978b14..c00ba7d 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
>                }
>
>  complete:
> -               /*
> -                * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
> -                * acked after the data operation is complete.
> -                * Ref: TRM SWPU114Q Figure 18-31
> -                */
> -               omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
> -                               ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> -                               OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> -
> -               if (stat & OMAP_I2C_STAT_NACK)
> +               if (stat & OMAP_I2C_STAT_NACK) {
> +                       dev_err(dev->dev, "No Acknowledge\n");
>                        err |= OMAP_I2C_STAT_NACK;
> +                       omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> +                       omap_i2c_complete_cmd(dev, err);
> +                       return IRQ_HANDLED;
> +               }
>
Do you think making I2C IRQ ack + complete as one small static inline function
can save clean the code further. I see it has been used in multiple paths.

Re: [PATCH 09/17] i2c: omap: ack IRQ in parts

[Felipe Balbi]

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

On Thu, Jun 14, 2012 at 04:41:45PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> > According to flow diagrams on OMAP TRMs,
> > we should ACK the IRQ as they happen.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |   29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index f978b14..c00ba7d 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >                }
> >
> >  complete:
> > -               /*
> > -                * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
> > -                * acked after the data operation is complete.
> > -                * Ref: TRM SWPU114Q Figure 18-31
> > -                */
> > -               omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
> > -                               ~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> > -                               OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> > -
> > -               if (stat & OMAP_I2C_STAT_NACK)
> > +               if (stat & OMAP_I2C_STAT_NACK) {
> > +                       dev_err(dev->dev, "No Acknowledge\n");
> >                        err |= OMAP_I2C_STAT_NACK;
> > +                       omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> > +                       omap_i2c_complete_cmd(dev, err);
> > +                       return IRQ_HANDLED;
> > +               }
> >
> Do you think making I2C IRQ ack + complete as one small static inline function
> can save clean the code further. I see it has been used in multiple paths.

done in a later patch

-- 
balbi

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

Re: [PATCH 09/17] i2c: omap: ack IRQ in parts

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:20:42PM +0300, Felipe Balbi wrote:
> According to flow diagrams on OMAP TRMs,
> we should ACK the IRQ as they happen.

You don't explain that you're adding a new dev_err() statement into the
driver for a missing acknowledge.

What if you're probing for a device - can this cause spam to the kernel
console?  What if you have protocol mangling enabled with the "ignore
lack of ack bit set" ?

> 
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index f978b14..c00ba7d 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -894,21 +894,20 @@ omap_i2c_isr(int this_irq, void *dev_id)
>  		}
>  
>  complete:
> -		/*
> -		 * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
> -		 * acked after the data operation is complete.
> -		 * Ref: TRM SWPU114Q Figure 18-31
> -		 */
> -		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, stat &
> -				~(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
> -				OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
> -
> -		if (stat & OMAP_I2C_STAT_NACK)
> +		if (stat & OMAP_I2C_STAT_NACK) {
> +			dev_err(dev->dev, "No Acknowledge\n");
>  			err |= OMAP_I2C_STAT_NACK;
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> +			omap_i2c_complete_cmd(dev, err);
> +			return IRQ_HANDLED;
> +		}
>  
>  		if (stat & OMAP_I2C_STAT_AL) {
>  			dev_err(dev->dev, "Arbitration lost\n");
>  			err |= OMAP_I2C_STAT_AL;
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
> +			omap_i2c_complete_cmd(dev, err);
> +			return IRQ_HANDLED;
>  		}
>  
>  		/*
> @@ -989,12 +988,18 @@ complete:
>  
>  		if (stat & OMAP_I2C_STAT_ROVR) {
>  			dev_err(dev->dev, "Receive overrun\n");
> -			dev->cmd_err |= OMAP_I2C_STAT_ROVR;
> +			err |= OMAP_I2C_STAT_ROVR;
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
> +			omap_i2c_complete_cmd(dev, err);
> +			return IRQ_HANDLED;
>  		}
>  
>  		if (stat & OMAP_I2C_STAT_XUDF) {
>  			dev_err(dev->dev, "Transmit underflow\n");
> -			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
> +			err |= OMAP_I2C_STAT_XUDF;
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
> +			omap_i2c_complete_cmd(dev, err);
> +			return IRQ_HANDLED;
>  		}
>  	} while (stat);
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/17] i2c: omap: ack IRQ in parts

[Felipe Balbi]

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

On Thu, Jun 14, 2012 at 12:20:17PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 01:20:42PM +0300, Felipe Balbi wrote:
> > According to flow diagrams on OMAP TRMs,
> > we should ACK the IRQ as they happen.
> 
> You don't explain that you're adding a new dev_err() statement into the
> driver for a missing acknowledge.
> 
> What if you're probing for a device - can this cause spam to the kernel
> console?  What if you have protocol mangling enabled with the "ignore
> lack of ack bit set" ?

indeed... maybe dev_vdbg() would fit better, or just don't add anything.

-- 
balbi

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

[PATCH 10/17] i2c: omap: get rid of the "complete" label

[Felipe Balbi]

we can ack stat and complete the command from
the errata handling itself.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c00ba7d..c20c45f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -893,7 +893,6 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			return IRQ_HANDLED;
 		}
 
-complete:
 		if (stat & OMAP_I2C_STAT_NACK) {
 			dev_err(dev->dev, "No Acknowledge\n");
 			err |= OMAP_I2C_STAT_NACK;
@@ -958,10 +957,12 @@ complete:
 				num_bytes = dev->fifo_size;
 
 			ret = omap_i2c_transmit_data(dev, num_bytes, true);
-			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 			if (ret < 0) {
 				err |= OMAP_I2C_STAT_XUDF;
-				goto complete;
+				omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
+						OMAP_I2C_STAT_XDR);
+				omap_i2c_complete_cmd(dev, err);
+				return IRQ_HANDLED;
 			}
 
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
@@ -976,10 +977,12 @@ complete:
 				num_bytes = dev->fifo_size;
 
 			ret = omap_i2c_transmit_data(dev, num_bytes, false);
-			stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 			if (ret < 0) {
 				err |= OMAP_I2C_STAT_XUDF;
-				goto complete;
+				omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
+						OMAP_I2C_STAT_XRDY);
+				omap_i2c_complete_cmd(dev, err);
+				return IRQ_HANDLED;
 			}
 
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
-- 
1.7.10.4

[PATCH 11/17] i2c: omap: switch to devm_* API

[Felipe Balbi]

that helps deleting some boiler plate code
and lets driver-core manage our resources
for us.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index c20c45f..d805270 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1045,7 +1045,7 @@ omap_i2c_probe(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*dev;
 	struct i2c_adapter	*adap;
-	struct resource		*mem, *irq, *ioarea;
+	struct resource		*mem, *irq;
 	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
 	struct device_node	*node = pdev->dev.of_node;
 	const struct of_device_id *match;
@@ -1058,23 +1058,17 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "no mem resource?\n");
 		return -ENODEV;
 	}
+
 	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!irq) {
 		dev_err(&pdev->dev, "no irq resource?\n");
 		return -ENODEV;
 	}
 
-	ioarea = request_mem_region(mem->start, resource_size(mem),
-			pdev->name);
-	if (!ioarea) {
-		dev_err(&pdev->dev, "I2C region already claimed\n");
-		return -EBUSY;
-	}
-
-	dev = kzalloc(sizeof(struct omap_i2c_dev), GFP_KERNEL);
+	dev = devm_kzalloc(&pdev->dev, sizeof(struct omap_i2c_dev), GFP_KERNEL);
 	if (!dev) {
-		r = -ENOMEM;
-		goto err_release_region;
+		dev_err(&pdev->dev, "not enough memory\n");
+		return -ENOMEM;
 	}
 
 	match = of_match_device(of_match_ptr(omap_i2c_of_match), &pdev->dev);
@@ -1097,10 +1091,10 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->dev = &pdev->dev;
 	dev->irq = irq->start;
-	dev->base = ioremap(mem->start, resource_size(mem));
+	dev->base = devm_request_and_ioremap(&pdev->dev, mem);
 	if (!dev->base) {
-		r = -ENOMEM;
-		goto err_free_mem;
+		dev_err(&pdev->dev, "ioremap failed\n");
+		return -ENOMEM;
 	}
 
 	platform_set_drvdata(pdev, dev);
@@ -1151,7 +1145,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	isr = (dev->rev < OMAP_I2C_OMAP1_REV_2) ? omap_i2c_omap1_isr :
 								   omap_i2c_isr;
-	r = request_irq(dev->irq, isr, 0, pdev->name, dev);
+	r = devm_request_irq(&pdev->dev, dev->irq, isr, 0, pdev->name, dev);
 
 	if (r) {
 		dev_err(dev->dev, "failure requesting irq %i\n", dev->irq);
@@ -1177,24 +1171,16 @@ omap_i2c_probe(struct platform_device *pdev)
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
 		dev_err(dev->dev, "failure adding adapter\n");
-		goto err_free_irq;
+		goto err_unuse_clocks;
 	}
 
 	of_i2c_register_devices(adap);
 
 	return 0;
 
-err_free_irq:
-	free_irq(dev->irq, dev);
 err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 	pm_runtime_put(dev->dev);
-	iounmap(dev->base);
-err_free_mem:
-	platform_set_drvdata(pdev, NULL);
-	kfree(dev);
-err_release_region:
-	release_mem_region(mem->start, resource_size(mem));
 
 	return r;
 }
@@ -1203,17 +1189,10 @@ static int
 omap_i2c_remove(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*dev = platform_get_drvdata(pdev);
-	struct resource		*mem;
 
-	platform_set_drvdata(pdev, NULL);
-
-	free_irq(dev->irq, dev);
 	i2c_del_adapter(&dev->adapter);
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
-	iounmap(dev->base);
-	kfree(dev);
-	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(mem->start, resource_size(mem));
+
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 12/17] i2c: omap: switch to platform_get_irq()

[Felipe Balbi]

that's a nice helper from drivers core which
will give us the exact IRQ number, instead
of a pointer to an IRQ resource.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d805270..43d3289 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1045,11 +1045,12 @@ omap_i2c_probe(struct platform_device *pdev)
 {
 	struct omap_i2c_dev	*dev;
 	struct i2c_adapter	*adap;
-	struct resource		*mem, *irq;
+	struct resource		*mem;
 	struct omap_i2c_bus_platform_data *pdata = pdev->dev.platform_data;
 	struct device_node	*node = pdev->dev.of_node;
 	const struct of_device_id *match;
 	irq_handler_t isr;
+	int irq;
 	int r;
 
 	/* NOTE: driver uses the static register mapping */
@@ -1059,10 +1060,10 @@ omap_i2c_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!irq) {
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
 		dev_err(&pdev->dev, "no irq resource?\n");
-		return -ENODEV;
+		return irq;
 	}
 
 	dev = devm_kzalloc(&pdev->dev, sizeof(struct omap_i2c_dev), GFP_KERNEL);
@@ -1090,7 +1091,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	}
 
 	dev->dev = &pdev->dev;
-	dev->irq = irq->start;
+	dev->irq = irq;
 	dev->base = devm_request_and_ioremap(&pdev->dev, mem);
 	if (!dev->base) {
 		dev_err(&pdev->dev, "ioremap failed\n");
-- 
1.7.10.4

[PATCH 14/17] i2c: omap: simplify errata check

[Felipe Balbi]

omap_i2c_dev is allocated with kzalloc(),
so we need not initialize b_hw to zero.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fad5f6f..fc5b8bc 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1139,9 +1139,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 		dev->fifo_size = (dev->fifo_size / 2);
 
-		if (dev->rev >= OMAP_I2C_REV_ON_3530_4430)
-			dev->b_hw = 0; /* Disable hardware fixes */
-		else
+		if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
 			dev->b_hw = 1; /* Enable hardware fixes */
 
 		/* calculate wakeup latency constraint for MPU */
-- 
1.7.10.4

[PATCH 15/17] i2c: omap: always return IRQ_HANDLED

[Felipe Balbi]

otherwise we could get our IRQ line disabled due
to many spurious IRQs.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index fc5b8bc..5b78a73 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 		}
 	} while (stat);
 
-	return count ? IRQ_HANDLED : IRQ_NONE;
+	return IRQ_HANDLED;
 }
 
 static const struct i2c_algorithm omap_i2c_algo = {
-- 
1.7.10.4

Re: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> otherwise we could get our IRQ line disabled due
> to many spurious IRQs.
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index fc5b8bc..5b78a73 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
>                }
>        } while (stat);
>
> -       return count ? IRQ_HANDLED : IRQ_NONE;
> +       return IRQ_HANDLED;

no sure if this is correct. if you have IRQ flood and instead of _actually_
handling it, if you return handled, you still have interrupt pending, right?

 if (count++ == 100) {
                        dev_warn(dev->dev, "Too much work in one IRQ\n");
                        break;
                }
ofcourse we do get warning message already, so as such the change
should be fine.

Just want to understand the change bit more.

Regards
Santosh



Regards
santosh

Re: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 04:48:56PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> > otherwise we could get our IRQ line disabled due
> > to many spurious IRQs.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> >  drivers/i2c/busses/i2c-omap.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> > index fc5b8bc..5b78a73 100644
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
> >                }
> >        } while (stat);
> >
> > -       return count ? IRQ_HANDLED : IRQ_NONE;
> > +       return IRQ_HANDLED;
> 
> no sure if this is correct. if you have IRQ flood and instead of _actually_
> handling it, if you return handled, you still have interrupt pending, right?

The point of returning IRQ_NONE is to indicate to the interrupt layer that
the interrupt you received was not processed by any interrupt handler, and
therefore to provide a way of preventing the system being brought to a halt
though a stuck interrupt line.

So, if you do process an interrupt, you should always return IRQ_HANDLED
even if you couldn't complete its processing (eg, because you've serviced
it 100 times.)

Re: [PATCH 15/17] i2c: omap: always return IRQ_HANDLED

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 4:53 PM, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Thu, Jun 14, 2012 at 04:48:56PM +0530, Shilimkar, Santosh wrote:
>> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
>> > otherwise we could get our IRQ line disabled due
>> > to many spurious IRQs.
>> >
>> > Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
>> > ---
>> >  drivers/i2c/busses/i2c-omap.c |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> > index fc5b8bc..5b78a73 100644
>> > --- a/drivers/i2c/busses/i2c-omap.c
>> > +++ b/drivers/i2c/busses/i2c-omap.c
>> > @@ -1015,7 +1015,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
>> >                }
>> >        } while (stat);
>> >
>> > -       return count ? IRQ_HANDLED : IRQ_NONE;
>> > +       return IRQ_HANDLED;
>>
>> no sure if this is correct. if you have IRQ flood and instead of _actually_
>> handling it, if you return handled, you still have interrupt pending, right?
>
> The point of returning IRQ_NONE is to indicate to the interrupt layer that
> the interrupt you received was not processed by any interrupt handler, and
> therefore to provide a way of preventing the system being brought to a halt
> though a stuck interrupt line.
>
> So, if you do process an interrupt, you should always return IRQ_HANDLED
> even if you couldn't complete its processing (eg, because you've serviced
> it 100 times.)
That make sense. Thanks for explanation Russell.

Regards
santosh

[PATCH 17/17] i2c: omap: resize fifos before each message

[Felipe Balbi]

This patch will try to avoid the usage of
draining feature by reconfiguring the FIFO
the start condition of each message based
on the message's size.

By doing that, we will be better utilizing
the FIFO when doing big transfers.

While at that also drop the now uneeded
check for dev->buf_len as we always know
the amount of data to be transmitted.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 6e75d03..759fdbd 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -193,6 +193,7 @@ struct omap_i2c_dev {
 	u8			*regs;
 	size_t			buf_len;
 	struct i2c_adapter	adapter;
+	u8			threshold;
 	u8			fifo_size;	/* use as flag and value
 						 * fifo_size==0 implies no fifo
 						 * if set, should be trsh+1
@@ -459,13 +460,6 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, scll);
 	omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, sclh);
 
-	if (dev->fifo_size) {
-		/* Note: setup required fifo size - 1. RTRSH and XTRSH */
-		buf = (dev->fifo_size - 1) << 8 | OMAP_I2C_BUF_RXFIF_CLR |
-			(dev->fifo_size - 1) | OMAP_I2C_BUF_TXFIF_CLR;
-		omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
-	}
-
 	/* Take the I2C module out of reset: */
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 
@@ -508,6 +502,45 @@ static int omap_i2c_wait_for_bb(struct omap_i2c_dev *dev)
 	return 0;
 }
 
+static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
+{
+	u16		buf;
+
+	if (dev->flags & OMAP_I2C_FLAG_NO_FIFO)
+		return;
+
+	/*
+	 * Set up notification threshold based on message size. We're doing
+	 * this to try and avoid draining feature as much as possible. Whenever
+	 * we have big messages to transfer (bigger than our total fifo size)
+	 * then we might use draining feature to transfer the remaining bytes.
+	 */
+
+	dev->threshold = clamp(size, (u8) 1, dev->fifo_size);
+
+	buf = omap_i2c_read_reg(dev, OMAP_I2C_BUF_REG);
+
+	if (is_rx) {
+		/* Clear RX Threshold */
+		buf &= ~(0x3f << 8);
+		buf |= ((dev->threshold - 1) << 8) | OMAP_I2C_BUF_RXFIF_CLR;
+	} else {
+		/* Clear TX Threshold */
+		buf &= ~0x3f;
+		buf |= (dev->threshold - 1) | OMAP_I2C_BUF_TXFIF_CLR;
+	}
+
+	omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
+
+	if (dev->rev < OMAP_I2C_REV_ON_3530_4430)
+		dev->b_hw = 1; /* Enable hardware fixes */
+
+	/* calculate wakeup latency constraint for MPU */
+	if (dev->set_mpu_wkup_lat != NULL)
+		dev->latency = (1000000 * dev->threshold) /
+			(1000 * dev->speed / 8);
+}
+
 /*
  * Low level master read/write transaction.
  */
@@ -524,6 +557,9 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 	if (msg->len == 0)
 		return -EINVAL;
 
+	dev->receiver = !!(msg->flags & I2C_M_RD);
+	omap_i2c_resize_fifo(dev, msg->len, dev->receiver);
+
 	omap_i2c_write_reg(dev, OMAP_I2C_SA_REG, msg->addr);
 
 	/* REVISIT: Could the STB bit of I2C_CON be used with probing? */
@@ -539,7 +575,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 
 	init_completion(&dev->cmd_complete);
 	dev->cmd_err = 0;
-	dev->receiver = !!(msg->flags & I2C_M_RD);
 
 	w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
 
@@ -803,12 +838,6 @@ static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes,
 	u16		w;
 
 	while (num_bytes--) {
-		if (!dev->buf_len) {
-			dev_err(dev->dev, "%s without data",
-					is_rdr ? "RDR" : "RRDY");
-			break;
-		}
-
 		w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG);
 		*dev->buf++ = w;
 		dev->buf_len--;
@@ -818,10 +847,8 @@ static void omap_i2c_receive_data(struct omap_i2c_dev *dev, u8 num_bytes,
 		 * omap4 is 8 bit wide
 		 */
 		if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
-			if (dev->buf_len) {
-				*dev->buf++ = w >> 8;
-				dev->buf_len--;
-			}
+			*dev->buf++ = w >> 8;
+			dev->buf_len--;
 		}
 	}
 }
@@ -832,12 +859,6 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
 	u16		w;
 
 	while (num_bytes--) {
-		if (!dev->buf_len) {
-			dev_err(dev->dev, "%s without data",
-					is_xdr ? "XDR" : "XRDY");
-			break;
-		}
-
 		w = *dev->buf++;
 		dev->buf_len--;
 
@@ -846,10 +867,8 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes,
 		 * omap4 is 8 bit wide
 		 */
 		if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) {
-			if (dev->buf_len) {
-				w |= *dev->buf++ << 8;
-				dev->buf_len--;
-			}
+			w |= *dev->buf++ << 8;
+			dev->buf_len--;
 		}
 
 		if (dev->errata & I2C_OMAP3_1P153) {
@@ -943,8 +962,8 @@ omap_i2c_isr(int this_irq, void *dev_id)
 		if (stat & OMAP_I2C_STAT_RRDY) {
 			u8 num_bytes = 1;
 
-			if (dev->fifo_size)
-				num_bytes = dev->fifo_size;
+			if (dev->threshold)
+				num_bytes = dev->threshold;
 
 			omap_i2c_receive_data(dev, num_bytes, false);
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_RRDY);
@@ -974,8 +993,8 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			int ret;
 			u8 num_bytes = 1;
 
-			if (dev->fifo_size)
-				num_bytes = dev->fifo_size;
+			if (dev->threshold)
+				num_bytes = dev->threshold;
 
 			ret = omap_i2c_transmit_data(dev, num_bytes, false);
 			if (ret < 0) {
-- 
1.7.10.4

[PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()

[Felipe Balbi]

stat & BIT(1) is the same as BIT(1), so let's
simplify things a bit by removing "stat &" from
all omap_i2c_ack_stat() calls.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 07ae391..fa9ddb6 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -774,8 +774,8 @@ static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
 
 	while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) {
 		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
-			omap_i2c_ack_stat(dev, *stat & (OMAP_I2C_STAT_XRDY |
-							OMAP_I2C_STAT_XDR));
+			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
+					OMAP_I2C_STAT_XDR));
 			*err |= OMAP_I2C_STAT_XUDF;
 			return -ETIMEDOUT;
 		}
@@ -835,10 +835,11 @@ complete:
 		 */
 		if (stat & (OMAP_I2C_STAT_ARDY | OMAP_I2C_STAT_NACK |
 					OMAP_I2C_STAT_AL)) {
-			omap_i2c_ack_stat(dev, stat &
-				(OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR |
-				OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR |
-				OMAP_I2C_STAT_ARDY));
+			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));
 			omap_i2c_complete_cmd(dev, err);
 			return IRQ_HANDLED;
 		}
@@ -885,8 +886,8 @@ complete:
 					}
 				}
 			}
-			omap_i2c_ack_stat(dev,
-				stat & (OMAP_I2C_STAT_RRDY | OMAP_I2C_STAT_RDR));
+			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_RRDY |
+						OMAP_I2C_STAT_RDR));
 			continue;
 		}
 
@@ -933,8 +934,8 @@ complete:
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-			omap_i2c_ack_stat(dev,
-				stat & (OMAP_I2C_STAT_XRDY | OMAP_I2C_STAT_XDR));
+			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
+						OMAP_I2C_STAT_XDR));
 			continue;
 		}
 
-- 
1.7.10.4

Re: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> stat & BIT(1) is the same as BIT(1), so let's
> simplify things a bit by removing "stat &" from
> all omap_i2c_ack_stat() calls.
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
Indeed. Looks good to me.
Reviewed-by : Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>

Re: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:20:37PM +0300, Felipe Balbi wrote:
> stat & BIT(1) is the same as BIT(1), so let's
> simplify things a bit by removing "stat &" from
> all omap_i2c_ack_stat() calls.

This doesn't feel right, and the explanation is definitely wrong.

"stat & BIT(1)" is not the same as "BIT(1)" _unless_ you're saying that
stat always has BIT(1) already set.  Can you guarantee that in this code?
If so, how?

What happens if you read the status register, and it has bit 1 clear.
immediately after the read, the status register bit 1 becomes set, and
then you write bit 1 set (because you've dropped the stat & BIT(1) from
the code.)

Is it not going to acknowledge that bit-1-set but because you haven't
read it, you're going to miss that event?

This feels like a buggy change to me.

Re: [PATCH 04/17] i2c: omap: simplify omap_i2c_ack_stat()

[Felipe Balbi]

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

Hi,

On Thu, Jun 14, 2012 at 12:13:33PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 01:20:37PM +0300, Felipe Balbi wrote:
> > stat & BIT(1) is the same as BIT(1), so let's
> > simplify things a bit by removing "stat &" from
> > all omap_i2c_ack_stat() calls.
> 
> This doesn't feel right, and the explanation is definitely wrong.
> 
> "stat & BIT(1)" is not the same as "BIT(1)" _unless_ you're saying that
> stat always has BIT(1) already set.  Can you guarantee that in this code?
> If so, how?
> 
> What happens if you read the status register, and it has bit 1 clear.
> immediately after the read, the status register bit 1 becomes set, and
> then you write bit 1 set (because you've dropped the stat & BIT(1) from
> the code.)
> 
> Is it not going to acknowledge that bit-1-set but because you haven't
> read it, you're going to miss that event?
> 
> This feels like a buggy change to me.

I fail to see that situation would happen with this driver. See what it
does (extremely simplified):

if (stat & NACK) {
	...
	omap_i2c_ack_stat(dev, stat & NACK);
}

if (stat & RDR) {
	...
	omap_i2c_ack_stat(dev, stat & RDR);
}

and so on. The tricky place is only WRT errata handling, for example:

if (*stat & (NACK | AL)) {
	omap_i2c_ack_stat(dev, *stat & (XRDY | XDR));
	...
}

but in this case, the errata says we must clear XRDY and XDR if that
errata triggers, so if they just got enabled or not, it doesn't matter.

Another tricky place is RDR | RRDY (likewise for XDR | XRDY):

if (stat & (RDR | RRDY)) {
	...
	omap_i2c_ack_stat(dev, stat & (RDR | RRDY));
}

again here there will be no issues because those IRQs never fire
simultaneously and one will only after after we have handled the
previous, that's because the same FIFO is used anyway and we won't shift
data into FIFO until we tell the IP "hey, I'm done with the FIFO, you
can shift more data". Right ?

-- 
balbi

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

[PATCH 06/17] i2c: omap: improve 1p153 errata handling

[Felipe Balbi]

Make it not depend on ISR's local variables
in order to make it easier to re-factor the
transmit data loop.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 0661ca1..52861c2 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -768,21 +768,24 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
  * data to DATA_REG. Otherwise some data bytes can be lost while transferring
  * them from the memory to the I2C interface.
  */
-static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
+static int errata_omap3_1p153(struct omap_i2c_dev *dev)
 {
-	unsigned long timeout = 10000;
+	unsigned long	timeout = 10000;
+	u16		stat;
 
-	while (--timeout && !(*stat & OMAP_I2C_STAT_XUDF)) {
-		if (*stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
+	do {
+		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+		if (stat & OMAP_I2C_STAT_XUDF)
+			break;
+
+		if (stat & (OMAP_I2C_STAT_NACK | OMAP_I2C_STAT_AL)) {
 			omap_i2c_ack_stat(dev, (OMAP_I2C_STAT_XRDY |
 					OMAP_I2C_STAT_XDR));
-			*err |= OMAP_I2C_STAT_XUDF;
 			return -ETIMEDOUT;
 		}
 
 		cpu_relax();
-		*stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
-	}
+	} while (--timeout);
 
 	if (!timeout) {
 		dev_err(dev->dev, "timeout waiting on XUDF bit\n");
@@ -946,9 +949,18 @@ complete:
 					}
 				}
 
-				if ((dev->errata & I2C_OMAP3_1P153) &&
-				    errata_omap3_1p153(dev, &stat, &err))
-					goto complete;
+				if (dev->errata & I2C_OMAP3_1P153) {
+					int ret;
+
+					ret = errata_omap3_1p153(dev);
+					stat = omap_i2c_read_reg(dev,
+							OMAP_I2C_STAT_REG);
+
+					if (ret < 0) {
+						err |= OMAP_I2C_STAT_XUDF;
+						goto complete;
+					}
+				}
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
@@ -986,9 +998,18 @@ complete:
 					}
 				}
 
-				if ((dev->errata & I2C_OMAP3_1P153) &&
-				    errata_omap3_1p153(dev, &stat, &err))
-					goto complete;
+				if (dev->errata & I2C_OMAP3_1P153) {
+					int ret;
+
+					ret = errata_omap3_1p153(dev);
+					stat = omap_i2c_read_reg(dev,
+							OMAP_I2C_STAT_REG);
+
+					if (ret < 0) {
+						err |= OMAP_I2C_STAT_XUDF;
+						goto complete;
+					}
+				}
 
 				omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
 			}
-- 
1.7.10.4

Re: [PATCH 06/17] i2c: omap: improve 1p153 errata handling

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> Make it not depend on ISR's local variables
> in order to make it easier to re-factor the
> transmit data loop.
>
> Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   47 +++++++++++++++++++++++++++++------------
>  1 file changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 0661ca1..52861c2 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -768,21 +768,24 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
>  * data to DATA_REG. Otherwise some data bytes can be lost while transferring
>  * them from the memory to the I2C interface.
>  */
> -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
> +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
>  {
> -       unsigned long timeout = 10000;
> +       unsigned long   timeout = 10000;

Not related to this patch but this 10000 timeout magic
needs to be fixed as well.

The patch looks fine.

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

Re: [PATCH 06/17] i2c: omap: improve 1p153 errata handling

[Russell King - ARM Linux]

On Thu, Jun 14, 2012 at 01:20:39PM +0300, Felipe Balbi wrote:
> -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
> +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
>  {
> -	unsigned long timeout = 10000;
> +	unsigned long	timeout = 10000;
> +	u16		stat;

Eww, no, not more of this "lets add tabs to align auto variable
declarations".  This is detrimental when you add another variable whose
type is longer than the current indentation - you have to reformat the
entire block.

It's really not worth it.  Stick to just using one space, like the rest
of the kernel code.  Like the code which Linus writes.  And thereby help
to avoid future "pointless churn" whinges.

Re: [PATCH 06/17] i2c: omap: improve 1p153 errata handling

[Felipe Balbi]

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

Hi,

On Thu, Jun 14, 2012 at 12:17:46PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 14, 2012 at 01:20:39PM +0300, Felipe Balbi wrote:
> > -static int errata_omap3_1p153(struct omap_i2c_dev *dev, u16 *stat, int *err)
> > +static int errata_omap3_1p153(struct omap_i2c_dev *dev)
> >  {
> > -	unsigned long timeout = 10000;
> > +	unsigned long	timeout = 10000;
> > +	u16		stat;
> 
> Eww, no, not more of this "lets add tabs to align auto variable
> declarations".  This is detrimental when you add another variable whose
> type is longer than the current indentation - you have to reformat the
> entire block.
> 
> It's really not worth it.  Stick to just using one space, like the rest
> of the kernel code.  Like the code which Linus writes.  And thereby help
> to avoid future "pointless churn" whinges.

fair enough, no need to get so over the top like that, it's just a tab.
Will fix it.

-- 
balbi

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

[PATCH 08/17] i2c: omap: switch over to do {} while loop

[Felipe Balbi]

this will make sure that we execute at least once.
No functional changes otherwise.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 381bb36..f978b14 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -870,20 +870,29 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 bits;
 	u16 stat;
-	int err, count = 0;
+	int err = 0, count = 0;
 
 	if (pm_runtime_suspended(dev->dev))
 		return IRQ_NONE;
 
-	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
-	while ((stat = (omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG))) & bits) {
+	do {
+		bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
+		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
+		stat &= bits;
+
+		if (!stat) {
+			/* my work here is done */
+			omap_i2c_complete_cmd(dev, err);
+			return IRQ_HANDLED;
+		}
+
 		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
 		if (count++ == 100) {
 			dev_warn(dev->dev, "Too much work in one IRQ\n");
-			break;
+			omap_i2c_complete_cmd(dev, err);
+			return IRQ_HANDLED;
 		}
 
-		err = 0;
 complete:
 		/*
 		 * Ack the stat in one go, but [R/X]DR and [R/X]RDY should be
@@ -987,7 +996,7 @@ complete:
 			dev_err(dev->dev, "Transmit underflow\n");
 			dev->cmd_err |= OMAP_I2C_STAT_XUDF;
 		}
-	}
+	} while (stat);
 
 	return count ? IRQ_HANDLED : IRQ_NONE;
 }
-- 
1.7.10.4

Re: [PATCH 08/17] i2c: omap: switch over to do {} while loop

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> this will make sure that we execute at least once.
> No functional changes otherwise.
>
> Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
> ---
Executing at least once instead of never is
still a functional change :-)

Regards
Santosh

Re: [PATCH 08/17] i2c: omap: switch over to do {} while loop

[Felipe Balbi]

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

On Thu, Jun 14, 2012 at 04:33:39PM +0530, Shilimkar, Santosh wrote:
> On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi@ti.com> wrote:
> > this will make sure that we execute at least once.
> > No functional changes otherwise.
> >
> > Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> Executing at least once instead of never is
> still a functional change :-)

there's a check for spurious interrupts. The idea was mainly to
initialise stat and bits correctly at the beginning of the handler.

Besides that "otherwise" is telling you that: "except this, there are no
other functional changes" ;)

-- 
balbi

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

[PATCH 13/17] i2c: omap: bus: add a receiver flag

[Felipe Balbi]

that way we can ignore TX IRQs while in receiver
mode and ignore RX IRQs while in transmitter mode.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 43d3289..fad5f6f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -199,6 +199,7 @@ struct omap_i2c_dev {
 						 */
 	u8			rev;
 	unsigned		b_hw:1;		/* bad h/w fixes */
+	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */
 	u16			pscstate;
 	u16			scllstate;
@@ -538,6 +539,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
 
 	init_completion(&dev->cmd_complete);
 	dev->cmd_err = 0;
+	dev->receiver = !!(msg->flags & I2C_M_RD);
 
 	w = OMAP_I2C_CON_EN | OMAP_I2C_CON_MST | OMAP_I2C_CON_STT;
 
@@ -880,6 +882,13 @@ omap_i2c_isr(int this_irq, void *dev_id)
 		stat = omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 		stat &= bits;
 
+		/* If we're in receiver mode, ignore XDR/XRDY */
+		if (dev->receiver) {
+			stat &= ~(OMAP_I2C_STAT_XDR | OMAP_I2C_STAT_XRDY);
+		} else {
+			stat &= ~(OMAP_I2C_STAT_RDR | OMAP_I2C_STAT_RRDY);
+		}
+
 		if (!stat) {
 			/* my work here is done */
 			omap_i2c_complete_cmd(dev, err);
-- 
1.7.10.4

[PATCH 16/17] i2c: omap: simplify IRQ exit path

[Felipe Balbi]

instead of having multiple return points, use
a goto statement to make that clearer.

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

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 5b78a73..6e75d03 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -889,33 +889,27 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			stat &= ~(OMAP_I2C_STAT_RDR | OMAP_I2C_STAT_RRDY);
 		}
 
-		if (!stat) {
-			/* my work here is done */
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
-		}
+		if (!stat)
+			goto out;
 
 		dev_dbg(dev->dev, "IRQ (ISR = 0x%04x)\n", stat);
 		if (count++ == 100) {
 			dev_warn(dev->dev, "Too much work in one IRQ\n");
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
+			goto out;
 		}
 
 		if (stat & OMAP_I2C_STAT_NACK) {
 			dev_err(dev->dev, "No Acknowledge\n");
 			err |= OMAP_I2C_STAT_NACK;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
+			goto out;
 		}
 
 		if (stat & OMAP_I2C_STAT_AL) {
 			dev_err(dev->dev, "Arbitration lost\n");
 			err |= OMAP_I2C_STAT_AL;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_NACK);
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
+			goto out;
 		}
 
 		/*
@@ -928,8 +922,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 						OMAP_I2C_STAT_XRDY |
 						OMAP_I2C_STAT_XDR |
 						OMAP_I2C_STAT_ARDY));
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
+			goto out;
 		}
 
 		if (stat & OMAP_I2C_STAT_RDR) {
@@ -970,8 +963,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				err |= OMAP_I2C_STAT_XUDF;
 				omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
 						OMAP_I2C_STAT_XDR);
-				omap_i2c_complete_cmd(dev, err);
-				return IRQ_HANDLED;
+				goto out;
 			}
 
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XDR);
@@ -990,8 +982,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 				err |= OMAP_I2C_STAT_XUDF;
 				omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF |
 						OMAP_I2C_STAT_XRDY);
-				omap_i2c_complete_cmd(dev, err);
-				return IRQ_HANDLED;
+				goto out;
 			}
 
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
@@ -1002,19 +993,19 @@ omap_i2c_isr(int this_irq, void *dev_id)
 			dev_err(dev->dev, "Receive overrun\n");
 			err |= OMAP_I2C_STAT_ROVR;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_ROVR);
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
+			goto out;
 		}
 
 		if (stat & OMAP_I2C_STAT_XUDF) {
 			dev_err(dev->dev, "Transmit underflow\n");
 			err |= OMAP_I2C_STAT_XUDF;
 			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XUDF);
-			omap_i2c_complete_cmd(dev, err);
-			return IRQ_HANDLED;
+			goto out;
 		}
 	} while (stat);
 
+out:
+	omap_i2c_complete_cmd(dev, err);
 	return IRQ_HANDLED;
 }
 
-- 
1.7.10.4

Re: [PATCH 00/17] Big OMAP I2C Cleanup

[Shilimkar, Santosh]

On Thu, Jun 14, 2012 at 3:50 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> Hi guys,
>
> I have dropped a few patches from the series and also
> tested every single patch on my pandaboard. I2C messages
> are still working fine with panda after each patch.
>
> There's still lots of work to be done on the i2c-omap.c
> driver but it's now easier to read, IMO.
>
Great cleanup. Have reviewed the entire series based on
limited knowledge i have about the OMAP i2c driver.
Apart from those comments, the series looks good to me.

We may to take similar dig at MMC and SPI driver one day :-p

Regards
santosh