i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()
Subsystems (2)
PATCH 2/3 i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy()
There are several problems in the function:
- "to_cnt" variable does nothing
- schedule_timeout() call without setting current state does nothing
- "allow_sleep" parameter is not really used
Refactor the function so that it really tries to wait. In case of timeout try to recover the bus and finally initialize the controller. We cannot really do more than that, so it makes no sense to check BB bit after init.
--- drivers/i2c/busses/i2c-davinci.c | 53 +++++++++++++++++++++---------------- 1 files changed, 30 insertions(+), 23 deletions(-)
drivers/i2c/busses/i2c-davinci.c +30 -23
@@ -152,6 +152,26 @@ static void davinci_i2c_clock_pulse(unsigned int scl_pin) } } +/* +* Wait until specific bit in status register has particular value +* Function returns 0 if condition was met, +* -ETIMEDOUT in case of timeout. +*/ +static int i2c_davinci_wait_status_change (struct davinci_i2c_dev *dev,u16 mask, +u16 val) +{ +unsigned long timeout = jiffies + dev->adapter.timeout; + +while ((davinci_i2c_read_reg (dev,DAVINCI_I2C_STR_REG ) & mask) != val) { +if (time_after (jiffies, timeout)) +return -ETIMEDOUT ; +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_timeout (1 ); + } + +return 0 ; +} +/* This routine does i2c bus recovery as specified in the * i2c protocol Rev. 03 section 3.16 titled "Bus clear" */
@@ -269,29 +289,16 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)/* * Waiting for bus not busy */ -static int i2c_davinci_wait_bus_not_busy (struct davinci_i2c_dev *dev, -char allow_sleep) +static int i2c_davinci_wait_bus_not_busy (struct davinci_i2c_dev *dev) { -unsigned long timeout; -static u16 to_cnt; - - timeout = jiffies + dev->adapter.timeout; -while (davinci_i2c_read_reg (dev,DAVINCI_I2C_STR_REG ) - &DAVINCI_I2C_STR_BB ) { -if (to_cnt <=DAVINCI_I2C_MAX_TRIES ) { -if (time_after (jiffies, timeout)) { -dev_warn (dev->dev, -"timeout waiting for bus ready\n" ); - to_cnt++; -return -ETIMEDOUT ; - }else { - to_cnt =0 ; -davinci_i2c_recover_bus (dev); -i2c_davinci_init (dev); - } - } -if (allow_sleep) -schedule_timeout (1 ); +if (i2c_davinci_wait_status_change (dev,DAVINCI_I2C_STR_BB ,0 )) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +davinci_i2c_recover_bus (dev); +i2c_davinci_init (dev); +/* +* the bus should not be busy after init, otherwise something +* is badly broken +*/ }return 0 ;
@@ -449,7 +456,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)dev_dbg (dev->dev,"%s: msgs: %d\n" , __func__, num); - ret =i2c_davinci_wait_bus_not_busy (dev,1 ); + ret =i2c_davinci_wait_bus_not_busy (dev);if (ret <0 ) {dev_warn (dev->dev,"timeout waiting for bus ready\n" );return ret;
Hi Alexander,
Quoted 19 lines · Alexander Sverdlin
There are several problems in the function:
- "to_cnt" variable does nothing
-schedule_timeout()call without setting current state does nothing
- "allow_sleep" parameter is not really usedRefactor the function so that it really tries to wait. In case of timeout try to recover the bus and finally initialize the controller. We cannot really do more than that, so it makes no sense to check BB bit after init.
Signed-off-by Alexander Sverdlin <[email protected]>--- drivers/i2c/busses/i2c-davinci.c | 53 +++++++++++++++++++++---------------- 1 files changed, 30 insertions(+), 23 deletions(-)drivers/i2c/busses/i2c-davinci.c +0
-0
drivers/i2c/busses/i2c-davinci.c static void davinci_i2c_clock_pulse(unsigned int scl_pin) · line 161
Quoted 10 lines
drivers/i2c/busses/i2c-davinci.c +7
-0@@ -152,6 +152,26 @@ static void davinci_i2c_clock_pulse(unsigned int scl_pin) } } +/* +* Wait until specific bit in status register has particular value +* Function returns 0 if condition was met, +* -ETIMEDOUT in case of timeout. +*/ +static int i2c_davinci_wait_status_change (struct davinci_i2c_dev *dev,u16 mask, +u16 val)
do we really need it as separate function? (even if it looks good:)
drivers/i2c/busses/i2c-davinci.c static void davinci_i2c_clock_pulse(unsigned int scl_pin) · line 169
Quoted 8 lines
drivers/i2c/busses/i2c-davinci.c +8
-0+{ +unsigned long timeout = jiffies + dev->adapter.timeout; + +while ((davinci_i2c_read_reg (dev,DAVINCI_I2C_STR_REG ) & mask) != val) { +if (time_after (jiffies, timeout)) +return -ETIMEDOUT ; +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_timeout (1 );
schedule_timeout_uninterruptible()
Quoted 34 lines
drivers/i2c/busses/i2c-davinci.c +6
-20+ } + +return 0 ; +} +/* This routine does i2c bus recovery as specified in the * i2c protocol Rev. 03 section 3.16 titled "Bus clear" */ @@ -269,29 +289,16 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)/* * Waiting for bus not busy */ -static int i2c_davinci_wait_bus_not_busy (struct davinci_i2c_dev *dev, -char allow_sleep) +static int i2c_davinci_wait_bus_not_busy (struct davinci_i2c_dev *dev) { -unsigned long timeout; -static u16 to_cnt; - - timeout = jiffies + dev->adapter.timeout; -while (davinci_i2c_read_reg (dev,DAVINCI_I2C_STR_REG ) - &DAVINCI_I2C_STR_BB ) { -if (to_cnt <=DAVINCI_I2C_MAX_TRIES ) { -if (time_after (jiffies, timeout)) { -dev_warn (dev->dev, -"timeout waiting for bus ready\n" ); - to_cnt++; -return -ETIMEDOUT ; - }else { - to_cnt =0 ; -davinci_i2c_recover_bus (dev); -i2c_davinci_init (dev); - } - }
drivers/i2c/busses/i2c-davinci.c static int i2c_davinci_init(struct davinci_i2c_dev *dev) · line 301
Quoted 10 lines
drivers/i2c/busses/i2c-davinci.c +8
-2-if (allow_sleep) -schedule_timeout (1 ); +if (i2c_davinci_wait_status_change (dev,DAVINCI_I2C_STR_BB ,0 )) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +davinci_i2c_recover_bus (dev); +i2c_davinci_init (dev); +/* +* the bus should not be busy after init, otherwise something +* is badly broken +*/
I think you should recheck BB and return error if it's still detected.
drivers/i2c/busses/i2c-davinci.c i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) · line 462
Quoted 13 lines
drivers/i2c/busses/i2c-davinci.c +1
-1}return 0 ;@@ -449,7 +456,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)dev_dbg (dev->dev,"%s: msgs: %d\n" , __func__, num); - ret =i2c_davinci_wait_bus_not_busy (dev,1 ); + ret =i2c_davinci_wait_bus_not_busy (dev);if (ret <0 ) {dev_warn (dev->dev,"timeout waiting for bus ready\n" );return ret;
regards,
-grygorii
Signature · 2 lines
regards, -grygorii
Hi!
Quoted 6 lines · Grygorii.Strashko@... <grygorii.strashko@...> writes:
drivers/i2c/busses/i2c-davinci.c static void davinci_i2c_clock_pulse(unsigned int scl_pin)
Quoted 6 lines
drivers/i2c/busses/i2c-davinci.c +6
-0+/* +* Wait until specific bit in status register has particular value +* Function returns 0 if condition was met, +* -ETIMEDOUT in case of timeout. +*/ +static int i2c_davinci_wait_status_change(struct davinci_i2c_dev *dev,
u16 mask,
Quoted 3 lines
Quoted 1 line
+ u16 val)
do we really need it as separate function? (even if it looks good:)
Initially I had a patch for "recover" function also, but I've dropped it and I'm going to test your series. It might be re-used in case if we want to wait for something in the recover...
Quoted 4 lines
drivers/i2c/busses/i2c-davinci.c static void davinci_i2c_clock_pulse(unsigned int scl_pin)
Quoted 4 lines
drivers/i2c/busses/i2c-davinci.c +4
-0+{ +unsigned long timeout = jiffies + dev->adapter.timeout; + + while ((davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG) & mask) !=
val) {
Quoted 6 lines
drivers/i2c/busses/i2c-davinci.c static void davinci_i2c_clock_pulse(unsigned int scl_pin) · line 169
Quoted 4 lines
drivers/i2c/busses/i2c-davinci.c +4
-0+if (time_after (jiffies, timeout)) +return -ETIMEDOUT ; +set_current_state (TASK_UNINTERRUPTIBLE ); +schedule_timeout (1 );
schedule_timeout_uninterruptible()
Will do in v2...
[...]
Quoted 6 lines
drivers/i2c/busses/i2c-davinci.c static int i2c_davinci_init(struct davinci_i2c_dev *dev)
Quoted 6 lines
drivers/i2c/busses/i2c-davinci.c +6
-0+if (i2c_davinci_wait_status_change (dev,DAVINCI_I2C_STR_BB ,0 )) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +davinci_i2c_recover_bus (dev); +i2c_davinci_init (dev); +/* +* the bus should not be busy after init, otherwise
something
Quoted 4 lines
drivers/i2c/busses/i2c-davinci.c static int i2c_davinci_init(struct davinci_i2c_dev *dev) · line 301
Quoted 2 lines
drivers/i2c/busses/i2c-davinci.c +2
-0+* is badly broken +*/ I think you should recheck BB and return error if it's still detected.
Good idea! Will do in v2...
Quoted 4 lines
Quoted 4 lines
}return 0;
<at> <at> -449,7 +456,7 <at> <at> i2c_davinci_xfer(struct
i2c_adapter *adap, struct i2c_msg msgs[], int num)
Quoted 8 lines
drivers/i2c/busses/i2c-davinci.c i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) · line 462
Quoted 8 lines
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);drivers/i2c/busses/i2c-davinci.c +1
-1- ret =i2c_davinci_wait_bus_not_busy (dev,1 ); + ret =i2c_davinci_wait_bus_not_busy (dev);if (ret <0 ) {dev_warn (dev->dev,"timeout waiting for bus ready\n" );return ret;
Alex.
Hi!
Quoted 10 lines · On 11/03/15 19:35, ext [email protected]…
drivers/i2c/busses/i2c-davinci.c static int i2c_davinci_init(struct davinci_i2c_dev *dev) · line 301
Quoted 8 lines
drivers/i2c/busses/i2c-davinci.c +8
-0+if (i2c_davinci_wait_status_change (dev,DAVINCI_I2C_STR_BB ,0 )) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +davinci_i2c_recover_bus (dev); +i2c_davinci_init (dev); +/* +* the bus should not be busy after init, otherwise something +* is badly broken +*/ I think you should recheck BB and return error if it's still detected.
But after re-reading the datasheet, I must conclude, this is almost not possible. It will be a dead code. Reset will clear BB anyway and the only possibility to see BB set to 1 here is when another master transmits right after our reset. We cannot guarantee that we will always catch this here, so this must be any way handled over AL interrupt.
Signature · 2 lines
Best regards, Alexander Sverdlin.
Quoted 10 lines · Alexander Sverdlin
Quoted 9 lines · On 11/03/15 19:35, ext [email protected]…
Quoted 8 lines
drivers/i2c/busses/i2c-davinci.c +8
-0+if (i2c_davinci_wait_status_change (dev,DAVINCI_I2C_STR_BB ,0 )) { +dev_warn (dev->dev,"timeout waiting for bus ready\n" ); +davinci_i2c_recover_bus (dev); +i2c_davinci_init (dev); +/* +* the bus should not be busy after init, otherwise something +* is badly broken +*/
Quoted 7 lines
Quoted 1 line
I think you should recheck BB and return error if it's still detected.
But after re-reading the datasheet, I must conclude, this is almost not possible. It will be a dead code. Reset will clear BB anyway and the only possibility to see BB set to 1 here is when another master transmits right after our reset. We cannot guarantee that we will always catch this here, so this must be any way handled over AL interrupt.
The question here what is worse:
- silently continue execution with undefined behavior
- or recheck and return error (That's how it was before).
I like option 2 :) - don't believe HW.
Signature · 2 lines
Best regards, -grygorii
Hi!
Quoted 16 lines · ext Grygorii Strashko
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
+ if (
i2c_davinci_wait_status_change(dev, DAVINCI_I2C_STR_BB, 0)) {Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
(quoted diff) +7
-0+ dev_warn(dev->dev, "timeout waiting for bus ready\n"); + davinci_i2c_recover_bus(dev); + i2c_davinci_init(dev); + /* + * the bus should not be busy after init, otherwise something + * is badly broken + */Quoted 7 lines
Quoted 7 lines
Quoted 1 line
Quoted 1 line
I think you should recheck BB and return error if it's still detected.
But after re-reading the datasheet, I must conclude, this is almost not possible. It will be a dead code. Reset will clear BB anyway and the only possibility to see BB set to 1 here is when another master transmits right after our reset. We cannot guarantee that we will always catch this here, so this must be any way handled over AL interrupt.
Quoted 5 lines
The question here what is worse:
- silently continue execution with undefined behavior
- or recheck and return error (That's how it was before).I like option 2 :) - don't believe HW.
OK, it will at least catch the cases when the bus is short-circuited.
Signature · 2 lines
Best regards, Alexander Sverdlin.