Hi

On 10/25/2012 12:10 PM, Felipe Balbi wrote:

Hi,

On Wed, Oct 24, 2012 at 04:41:11PM +0200, Michael Trimarchi wrote:

quoted

Hi

On 10/22/2012 11:46 AM, Felipe Balbi wrote:

quoted

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

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

Signed-off-by: Felipe Balbi <redacted>
---
 drivers/i2c/busses/i2c-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index bea0277..e0eab38 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c

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

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

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

my 2 cents.

You are doing the same thing, but of course is better with just one *if* as before . A general rule is: when you have logic expression you can use undefined states to simplify the logic. 

Michael