Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Jean Delvare]

Hi Komal,

Your post ended up in my spam box once again... I really think you
should send your patches as text attachements (or inline) rather than
binary attachements. Using real names in To: and Cc: fields might help
as well.

> I have attached the updated patch, which addresses the most of review
> comments.

I'll review that new version later today, or tomorrow.

> >The comment is confusing, as this address is usually known as the
> >"slave address" in the I2C world. Masters don't need no address on an
> >I2C bus. "own" is not a very explicit parameter name, what about
> >"slave_addr"? Ideally this should be retrieved from platform_data too,
> >else you can't be sure you won't collide with a device on the bus.
> 
> >"0 for default" doesn't make sense, as the default is, by definition,
> >when the user doesn't speficiy anything. That this is internally coded
> >as 0 is an implementation detail user-space doesn't need to know.
> 
> Updated the comment and changed to slave_addr , and default is changed to "3".

Slightly better, though I still don't get why you worry setting an
address that will never be used.

> >> +		if (armxor_rate > 16000000)
> >> +			psc = (armxor_rate + 8000000) / 12000000;
> >> +		else
> >> +			psc = 0;
> 
> >Can you please explain this formula?
> 
> The OMAP core uses 8-bit value to divide the system clock (SCLK) and
> generates its own sampling clock (ICLK), and the core logic is sampled
> at clock rate of the system clock for the module, divided by (prescaler value + 1)

I should have been more precise, I guess. What surprises me are the
numbers themselves. It's frequent to see forumlae of the form
"a = (b + c/2) / c" to divide with proper rounding, but here you have
2c/3 instread of c/2. My question was more like: is it intentional, or a
typo? Also, with the code above, psc will never have value 1. The "if"
part will always compute to at least 2, and the "else" part to 0. Is
this OK?

> I think it is better to remove those lines and return error if length is zero.
> Is that ok?

Yes. This can be revisited later when/if someone finds a hack to work
around the problem.

> >> +	/* We have an error */
> >> +	if (dev->cmd_err & OMAP_I2C_STAT_NACK) {
> >> +		if (msg->flags & I2C_M_IGNORE_NAK)
> >> +			return 0;
> 
> >Couldn't you have other error bits set as well? I2C_M_IGNORE_NAK means
> >you can ignore OMAP_I2C_STAT_NACK, not other errors.
> 
> This is now being handled by first checking remaining errors first and then
> NACK. Is that ok?

Yes.

> >> +	r = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
> >> +	dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n",
> >> +		 pdev->id - 1, r >> 4, r & 0xf, clock);
> 
> >This "- 1" is error prone IMHO.
> 
> Only if omap devices.c maintainer pushes the values less than one in device
> structure ;)

No, what I meant was rather that printing a bus number which differs
from the internal numbering might confuse the user at some point.

-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Tony Lindgren]

Hi,

* Jean Delvare <khali@linux-fr.org> [060805 11:31]:
> 
> > >> +		if (armxor_rate > 16000000)
> > >> +			psc = (armxor_rate + 8000000) / 12000000;
> > >> +		else
> > >> +			psc = 0;
> > 
> > >Can you please explain this formula?
> > 
> > The OMAP core uses 8-bit value to divide the system clock (SCLK) and
> > generates its own sampling clock (ICLK), and the core logic is sampled
> > at clock rate of the system clock for the module, divided by (prescaler value + 1)
> 
> I should have been more precise, I guess. What surprises me are the
> numbers themselves. It's frequent to see forumlae of the form
> "a = (b + c/2) / c" to divide with proper rounding, but here you have
> 2c/3 instread of c/2. My question was more like: is it intentional, or a
> typo? Also, with the code above, psc will never have value 1. The "if"
> part will always compute to at least 2, and the "else" part to 0. Is
> this OK?
> 

Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C clock
must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock is
typically 12, 13 or 19.2 MHz. So we should have code that produces:

XOR Mhz	Divider	Prescaler
12	1	0
13	2	1
19.2	2	1

Then again the original old code produces something different too [2]...

I suspect the original code had some hw workarounds and and later code
may have a conversion bug somewhere :)

I suggest we keep the code as is for now since it's known to work on
all omaps, and then submit a follow-up patch later once we have
verified that that code based on the TRM works on all omaps.

Regards,

Tony

[1] http://focus.ti.com/general/docs/lit/getliterature.tsp?baseLiteratureNumber=spru681
[2] http://linux-omap.bkbits.net:8080/main/diffs/drivers/i2c/busses/i2c-omap.c@1.12?nav=index.html|src/|src/drivers|src/drivers/i2c|src/drivers/i2c/busses|hist/drivers/i2c/busses/i2c-omap.c

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Komal Shah]

--- Tony Lindgren <tony@atomide.com> wrote:

> Hi,
> 
> * Jean Delvare <khali@linux-fr.org> [060805 11:31]:
> > 
> > > >> +		if (armxor_rate > 16000000)
> > > >> +			psc = (armxor_rate + 8000000) / 12000000;
> > > >> +		else
> > > >> +			psc = 0;
> > > 
> > > >Can you please explain this formula?
> > > 
> > > The OMAP core uses 8-bit value to divide the system clock (SCLK)
> and
> > > generates its own sampling clock (ICLK), and the core logic is
> sampled
> > > at clock rate of the system clock for the module, divided by
> (prescaler value + 1)
> > 
> > I should have been more precise, I guess. What surprises me are the
> > numbers themselves. It's frequent to see forumlae of the form
> > "a = (b + c/2) / c" to divide with proper rounding, but here you
> have
> > 2c/3 instread of c/2. My question was more like: is it intentional,
> or a
> > typo? Also, with the code above, psc will never have value 1. The
> "if"
> > part will always compute to at least 2, and the "else" part to 0.
> Is
> > this OK?
> > 
> 
> Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C
> clock
> must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock
> is
> typically 12, 13 or 19.2 MHz. So we should have code that produces:
> 
> XOR Mhz	Divider	Prescaler
> 12	1	0
> 13	2	1
> 19.2	2	1
> 
> Then again the original old code produces something different too
> [2]...
> 
> I suspect the original code had some hw workarounds and and later
> code
> may have a conversion bug somewhere :)
> 
> I suggest we keep the code as is for now since it's known to work on
> all omaps, and then submit a follow-up patch later once we have
> verified that that code based on the TRM works on all omaps.

I have updated the driver with all other review comments except comment
on formula above by Jean. I can some part of description from Tony's
explanation along with removing the following code line

/* Set Own Address */
omap_i2c_write_reg(dev, OMAP_I2C_OA_REG, dev->own_address);
 
As it is not needed. Please confirm so that I can send the revised
patch  soon.

---Komal Shah
http://komalshah.blogspot.com/

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Jean Delvare]

Hi Komal,

> > Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C
> > clock must be prescaled to be between 7 - 12 MHz [1]. The XOR input
> > clock is typically 12, 13 or 19.2 MHz. So we should have code that
> > produces:
> > 
> > XOR Mhz	Divider	Prescaler
> > 12		1	0
> > 13		2	1
> > 19.2	2	1
> > 
> > Then again the original old code produces something different too
> > [2]...
> > 
> > I suspect the original code had some hw workarounds and and later
> > code
> > may have a conversion bug somewhere :)
> > 
> > I suggest we keep the code as is for now since it's known to work on
> > all omaps, and then submit a follow-up patch later once we have
> > verified that that code based on the TRM works on all omaps.
> 
> I have updated the driver with all other review comments except comment
> on formula above by Jean. I can some part of description from Tony's
> explanation along with removing the following code line
> 
> /* Set Own Address */
> omap_i2c_write_reg(dev, OMAP_I2C_OA_REG, dev->own_address);
>  
> As it is not needed. Please confirm so that I can send the revised
> patch  soon.

Fine with me. If you remove that line, there are a few others you
should be able to remove as well (declaration of slave_add module
parameter, definition of DEFAULT_OWN, etc.)

Waiting for your updated patch :) Thanks for all the changes you
accepted to make, BTW. I believe the driver is in a better shape now.

-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Jean Delvare]

Hi Tony, Komal,

> Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C clock
> must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock is
> typically 12, 13 or 19.2 MHz. So we should have code that produces:
> 
> XOR Mhz	Divider	Prescaler
> 12		1	0
> 13		2	1
> 19.2		2	1

Not that 13 MHz cannot actually be prescaled between 7 and 12 MHz, no
matter how you look at it.

> Then again the original old code produces something different too [2]...
> 
> I suspect the original code had some hw workarounds and and later code
> may have a conversion bug somewhere :)
> 
> I suggest we keep the code as is for now since it's known to work on
> all omaps, and then submit a follow-up patch later once we have
> verified that that code based on the TRM works on all omaps.

I've now taken Komal's patch (#4). Here is a proposed patch which brings
the prescaler computation formula in line with your comment and table
above. It could be applied on top of Komal's patch unless it causes a
problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
2 to 1.

I don't have any hardware to test it, though. If it happens to be
better to be slightly over 12 MHz than slightly below 7 MHz, the
"> 12000000" condition below can be replaced with "> 14000000".


i2c: Fix OMAP clock prescaler to match the comment

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/busses/i2c-omap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.18-rc4.orig/drivers/i2c/busses/i2c-omap.c	2006-08-10 09:56:54.000000000 +0200
+++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-omap.c	2006-08-10 10:12:03.000000000 +0200
@@ -231,8 +231,8 @@
 		 * 13		2		1
 		 * 19.2		2		1
 		 */
-		if (fclk_rate > 16000000)
-			psc = (fclk_rate + 8000000) / 12000000;
+		if (fclk_rate > 12000000)
+			psc = fclk_rate / 12000000;
 	}
 
 	/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */


-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Tony Lindgren]

* Jean Delvare <khali@linux-fr.org> [060810 11:30]:
> Hi Tony, Komal,
> 
> > Hmmm, this sounds like a bug somewhere. TRM for 5912 says the I2C clock
> > must be prescaled to be between 7 - 12 MHz [1]. The XOR input clock is
> > typically 12, 13 or 19.2 MHz. So we should have code that produces:
> > 
> > XOR Mhz	Divider	Prescaler
> > 12		1	0
> > 13		2	1
> > 19.2		2	1
> 
> Not that 13 MHz cannot actually be prescaled between 7 and 12 MHz, no
> matter how you look at it.

True :) But that's what the docs say..
 
> > Then again the original old code produces something different too [2]...
> > 
> > I suspect the original code had some hw workarounds and and later code
> > may have a conversion bug somewhere :)
> > 
> > I suggest we keep the code as is for now since it's known to work on
> > all omaps, and then submit a follow-up patch later once we have
> > verified that that code based on the TRM works on all omaps.
> 
> I've now taken Komal's patch (#4). Here is a proposed patch which brings
> the prescaler computation formula in line with your comment and table
> above. It could be applied on top of Komal's patch unless it causes a
> problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
> prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
> 2 to 1.

OK cool. As far as I'm concerned, I'm fine with it too:
Signed-off-by: Tony Lindgren <tony@atomide.com>
 
> I don't have any hardware to test it, though. If it happens to be
> better to be slightly over 12 MHz than slightly below 7 MHz, the
> "> 12000000" condition below can be replaced with "> 14000000".

Thanks, we'll test it on various omaps and let you know if it works.

Tony

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Jean Delvare]

Hi Tony, all,

On Thu, 10 Aug 2006 16:19:26 +0300, Tony Lindgren wrote:
> * Jean Delvare <khali@linux-fr.org> [060810 11:30]:
> > I've now taken Komal's patch (#4). Here is a proposed patch which brings
> > the prescaler computation formula in line with your comment and table
> > above. It could be applied on top of Komal's patch unless it causes a
> > problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
> > prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
> > 2 to 1.
> 
> OK cool. As far as I'm concerned, I'm fine with it too:
> Signed-off-by: Tony Lindgren <tony@atomide.com>
>  
> > I don't have any hardware to test it, though. If it happens to be
> > better to be slightly over 12 MHz than slightly below 7 MHz, the
> > "> 12000000" condition below can be replaced with "> 14000000".
> 
> Thanks, we'll test it on various omaps and let you know if it works.

Any news on this? I still have this patch in my local tree. Should I
push it into Linux 2.6.20?

i2c: Fix OMAP clock prescaler to match the comment

Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 drivers/i2c/busses/i2c-omap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- linux-2.6.18-rc4.orig/drivers/i2c/busses/i2c-omap.c	2006-08-10 09:56:54.000000000 +0200
+++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-omap.c	2006-08-10 10:12:03.000000000 +0200
@@ -231,8 +231,8 @@
 		 * 13		2		1
 		 * 19.2		2		1
 		 */
-		if (fclk_rate > 16000000)
-			psc = (fclk_rate + 8000000) / 12000000;
+		if (fclk_rate > 12000000)
+			psc = fclk_rate / 12000000;
 	}
 
 	/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */


-- 
Jean Delvare

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Tony Lindgren]

Hi,

* Jean Delvare <khali@linux-fr.org> [061204 09:49]:
> Hi Tony, all,
> 
> On Thu, 10 Aug 2006 16:19:26 +0300, Tony Lindgren wrote:
> > * Jean Delvare <khali@linux-fr.org> [060810 11:30]:
> > > I've now taken Komal's patch (#4). Here is a proposed patch which brings
> > > the prescaler computation formula in line with your comment and table
> > > above. It could be applied on top of Komal's patch unless it causes a
> > > problem on some of the OMAP systems. For XOR = 13 MHz, it changes the
> > > prescaler from 0 to 1. For XOR = 19.2 MHz it changes the prescaler from
> > > 2 to 1.
> > 
> > OK cool. As far as I'm concerned, I'm fine with it too:
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> >  
> > > I don't have any hardware to test it, though. If it happens to be
> > > better to be slightly over 12 MHz than slightly below 7 MHz, the
> > > "> 12000000" condition below can be replaced with "> 14000000".
> > 
> > Thanks, we'll test it on various omaps and let you know if it works.
> 
> Any news on this? I still have this patch in my local tree. Should I
> push it into Linux 2.6.20?
> 
> i2c: Fix OMAP clock prescaler to match the comment
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  drivers/i2c/busses/i2c-omap.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.18-rc4.orig/drivers/i2c/busses/i2c-omap.c	2006-08-10 09:56:54.000000000 +0200
> +++ linux-2.6.18-rc4/drivers/i2c/busses/i2c-omap.c	2006-08-10 10:12:03.000000000 +0200
> @@ -231,8 +231,8 @@
>  		 * 13		2		1
>  		 * 19.2		2		1
>  		 */
> -		if (fclk_rate > 16000000)
> -			psc = (fclk_rate + 8000000) / 12000000;
> +		if (fclk_rate > 12000000)
> +			psc = fclk_rate / 12000000;
>  	}
>  
>  	/* Setup clock prescaler to obtain approx 12MHz I2C module clock: */

Sorry for the delay in replying. Yes, it's safe to push. When the original
code was done, the max limit of 12MHz was ignored.

Regards,

Tony

Re: [PATCH] OMAP: I2C driver for TI OMAP boards #3

[Jean Delvare]

Hi Komal,

> I have attached the updated patch, which addresses the most of review
> comments.

Here we go for another review:

> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -287,6 +287,14 @@ config I2C_OCORES
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-ocores.
>  
> +config I2C_OMAP
> +	tristate "OMAP I2C adapter"
> +	depends on I2C && ARCH_OMAP
> +	default y if MACH_OMAP_H3 || MACH_OMAP_OSK
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  Texas Instruments OMAP(http://www.ti.com/omap) I2C driver.
> +

Missing space before opening parenthesis. And "including support for a
driver" still doesn't make sense, sorry. The driver is what supports a
device.

> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-omap.c

> +/* I2C System Test Register (OMAP_I2C_SYSTEST): */
> +#ifdef DEBUG
> +#define OMAP_I2C_SYSTEST_ST_EN		(1 << 15)	/* System test enable */
> +#define OMAP_I2C_SYSTEST_FREE		(1 << 14)	/* Free running mode */
> +#define OMAP_I2C_SYSTEST_TMODE_MASK	(3 << 12)	/* Test mode select */
> +#define OMAP_I2C_SYSTEST_TMODE_SHIFT	(12)		/* Test mode select */
> +#define OMAP_I2C_SYSTEST_SCL_I		(1 << 3)	/* SCL line sense in */
> +#define OMAP_I2C_SYSTEST_SCL_O		(1 << 2)	/* SCL line drive out */
> +#define OMAP_I2C_SYSTEST_SDA_I		(1 << 1)	/* SDA line sense in */
> +#define OMAP_I2C_SYSTEST_SDA_O		(1 << 0)	/* SDA line drive out */
> +#endif
> +
> +/* I2C System Status register (OMAP_I2C_SYSS): */
> +#define OMAP_I2C_SYSS_RDONE		1		/* Reset Done */

Shouldn't it be (1 << 0) for consistency?

> +
> +/* I2C System Configuration Register (OMAP_I2C_SYSC): */
> +#define OMAP_I2C_SYSC_SRST		(1 << 1)	/* Soft Reset */
> +static int omap_i2c_get_clocks(struct omap_i2c_dev *dev)
> +{
> +	if (cpu_is_omap16xx() || cpu_is_omap24xx()) {
> +		dev->iclk = clk_get(dev->dev, "i2c_ick");
> +		if (IS_ERR(dev->iclk)) {
> +			dev->iclk = NULL;
> +			return -ENODEV;
> +		}
> +	}
> +
> +	dev->fclk = clk_get(dev->dev, "i2c_fck");
> +	if (IS_ERR(dev->fclk)) {
> +		if (dev->iclk != NULL) {
> +			clk_put(dev->iclk);
> +			dev->iclk = NULL;
> +			return -ENODEV;
> +		}
> +	}
> +
> +	return 0;
> +}

You broke the second error path while trying to address my previous
objection... dev->fclk is still not reset to NULL on error, and
additionally you return 0 (instead of -ENODEV) if dev->iclk == NULL and
clk_get(dev->dev, "i2c_fck") fails. Please fix.

> +static int omap_i2c_init(struct omap_i2c_dev *dev)
> +{
> +	u16 psc = 0;
> +	unsigned long fclk_rate = 12000000;
> +	unsigned long timeout;
> +
> +	if (!dev->rev1) {
> +		omap_i2c_write_reg(dev, OMAP_I2C_SYSC_REG, OMAP_I2C_SYSC_SRST);
> +		/* For some reason we need to set the EN bit before the
> +		 * reset done bit gets set. */
> +		timeout = jiffies + OMAP_I2C_TIMEOUT;
> +		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
> +		while (!(omap_i2c_read_reg(dev, OMAP_I2C_SYSS_REG) &
> +			 OMAP_I2C_SYSS_RDONE)) {
> +			if (time_after(jiffies, timeout)) {
> +				dev_warn(dev->dev, "timeout waiting"
> +						"for controller reset\n");
> +				return -ETIMEDOUT;
> +			}
> +			msleep(1);
> +		}
> +	}
> +	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
> +
> +	if (cpu_class_is_omap1()) {
> +		struct clk *armxor_ck;
> +
> +		armxor_ck = clk_get(NULL, "armxor_ck");
> +		if (IS_ERR(armxor_ck)) {
> +			dev_warn(dev->dev, "i2c: Could not get armxor_ck\n");

The "i2c: " should go away now that you use dev_warn() instead of
printk().

> +			fclk_rate = 12000000;

This is already the value of fclk_rate, you initialized it at the top
of the function.

> +		} else {
> +			fclk_rate = clk_get_rate(armxor_ck);
> +			clk_put(armxor_ck);
> +		}
> +
> +		if (fclk_rate > 16000000)
> +			psc = (fclk_rate + 8000000) / 12000000;
> +		else
> +			psc = 0;

Ditto.

> +	}

> +
> +/*
> + * Low level master read/write transaction.
> + */
> +static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
> +			     struct i2c_msg *msg, int stop)
> +{
> +	struct omap_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int r;
> +	u16 w;
> +	u8 zero_byte = 0;
> +
> +	dev_dbg(dev->dev, "addr: 0x%04x, len: %d, flags: 0x%x, stop: %d\n",
> +		msg->addr, msg->len, msg->flags, stop);
> +
> +	omap_i2c_write_reg(dev, OMAP_I2C_SA_REG, msg->addr);
> +
> +	/* Sigh, seems we can't do zero length transactions. Thus, we
> +	 * can't probe for devices w/o actually sending/receiving at least
> +	 * a single byte. So we'll set count to 1 for the zero length
> +	 * transaction case and hope we don't cause grief for some
> +	 * arbitrary device due to random byte write/read during
> +	 * probes.
> +	 */
> +	/* REVISIT: Could the STB bit of I2C_CON be used with probing? */
> +	if (msg->len == 0) {
> +		dev->buf = &zero_byte;
> +		dev->buf_len = 1;
> +	} else {

Hm, I thought we had all agreed that it wasn't acceptable? If msg->len
== 0, you can't handle the message, so return an error. Don't even
write to the address register.

> +		dev->buf = msg->buf;
> +		dev->buf_len = msg->len;
> +	}
> +	omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev->buf_len);

> +static u32
> +omap_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C;
> +}

Most i2c chip drivers will not work then. You can actually achieve most
SMBus transactions, just not Quick Command. So you should return
I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK.

> +#ifdef DEBUG
> +			if (!(stat & OMAP_I2C_STAT_BB)) {
> +				dev_warn(dev->dev, "XRDY while bus not busy\n");
> +				bail_out = 1;
> +			}
> +#endif
> +			omap_i2c_write_reg(dev, OMAP_I2C_DATA_REG, w);
> +			omap_i2c_ack_stat(dev, OMAP_I2C_STAT_XRDY);
> +			if (bail_out)
> +				omap_i2c_complete_cmd(dev, OMAP_I2C_STAT_SBD);
> +			continue;
> +		}

Enabling debug should never change the functional behavior of a driver.

Rest looks OK to me. Please address the few remaining issues and
resubmit your patch for inclusion.

-- 
Jean Delvare