Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 24 March 2013 19:37, Viresh Kumar wrote: > On 21 March 2013 15:06, Wolfram Sang wrote: >> I applied V11 of the core changes with minor modifications. I do wonder > > I can't find it linux-next or here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git I can see it now in your for-next branch, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 21 March 2013 15:06, Wolfram Sang wrote: > I applied V11 of the core changes with minor modifications. I do wonder I can't find it linux-next or here: git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On Thu, Mar 21, 2013 at 03:45:29PM +0530, Viresh Kumar wrote: > On 21 March 2013 15:06, Wolfram Sang wrote: > > I applied V11 of the core changes with minor modifications. > > Wow!! Thanks. > > > I do wonder > > about the hook in the designware driver. You apply the recovery on > > transfer timeout. I think this should go into the timeout of > > i2c_dw_wait_bus_not_busy()? > > Hmm.. Rajeev/Shiraz were the guys who tested this code earlier and i am > sure we were failing in this piece of code, which i just fixed and so > i2c_dw_wait_bus_not_busy() didn't fail for us. > > Below is a conversation that we had with Uwe some time back on the exact > problem we faced and it was a bit different from the traditional problem. > > So, maybe we need bus recovery at both places: i2c_dw_wait_bus_not_busy() > (for the traditional hang) and during transfer (for our case). DW_IC_STATUS_ACTIVITY check in i2c_dw_wait_bus_not_busy actually only represents controller activity status and has nothing to do with real bus status. -- regards Shiraz -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 21 March 2013 15:06, Wolfram Sang wrote: > I applied V11 of the core changes with minor modifications. Wow!! Thanks. > I do wonder > about the hook in the designware driver. You apply the recovery on > transfer timeout. I think this should go into the timeout of > i2c_dw_wait_bus_not_busy()? Hmm.. Rajeev/Shiraz were the guys who tested this code earlier and i am sure we were failing in this piece of code, which i just fixed and so i2c_dw_wait_bus_not_busy() didn't fail for us. Below is a conversation that we had with Uwe some time back on the exact problem we faced and it was a bit different from the traditional problem. So, maybe we need bus recovery at both places: i2c_dw_wait_bus_not_busy() (for the traditional hang) and during transfer (for our case). -- viresh > On 7 November 2012 18:53, Uwe Kleine-König > wrote: > > Hello Viresh, > > > > in our irc conversation you told that the SDA signal being stuck low > > happened with an sta529 audio codec. I quickly scanned over its > > datasheet (Doc ID 13095 Rev 3; March 2012). > > > > Some questions that come to my mind regarding your issue: > > - What is the situation the stall occurs? How do you reproduce? I2C data lines are permanently held low by the slave device (sta529 codec in this case). There is a fixed way in which we can always reproduce it. The codec can support different sample rate. As soon as we switch from one sample rate to another (and accordingly re-configure sta529), this happens. > > - I didn't find a specification of the allowed i2c clock frequency. Do > >you have a different document that specifies this? (Or maybe I just > >missed it?!) I assume you're using the device in the right range? sta529 can work in fast mode at 400 KHz without any issues. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
Viresh, I applied V11 of the core changes with minor modifications. I do wonder about the hook in the designware driver. You apply the recovery on transfer timeout. I think this should go into the timeout of i2c_dw_wait_bus_not_busy()? Thanks, Wolfram -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
Fixing Wolfram's email id. On 15 March 2013 14:21, Viresh Kumar wrote: > On 31 January 2013 07:58, Viresh Kumar wrote: >> On 26 January 2013 10:43, Viresh Kumar wrote: >>> I have looked them again and attached are the final patches. >>> Following are the improvements i have: >>> >>> Author: Viresh Kumar >>> Date: Sat Jan 26 10:31:42 2013 +0530 >>> >>> fixup! i2c/adapter: Add bus recovery infrastructure >> >> Hi Wolfram, >> >> Any inputs ? > > Hi Wolfram, > > We have already spent lot of time reviewing this patchset (more than a year), > its better we take it now and patch then for any improvements. > > What do you say? > > -- > viresh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 31 January 2013 07:58, Viresh Kumar wrote: > On 26 January 2013 10:43, Viresh Kumar wrote: >> I have looked them again and attached are the final patches. >> Following are the improvements i have: >> >> Author: Viresh Kumar >> Date: Sat Jan 26 10:31:42 2013 +0530 >> >> fixup! i2c/adapter: Add bus recovery infrastructure > > Hi Wolfram, > > Any inputs ? Hi Wolfram, We have already spent lot of time reviewing this patchset (more than a year), its better we take it now and patch then for any improvements. What do you say? -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 26 January 2013 10:43, Viresh Kumar wrote: > I have looked them again and attached are the final patches. > Following are the improvements i have: > > Author: Viresh Kumar > Date: Sat Jan 26 10:31:42 2013 +0530 > > fixup! i2c/adapter: Add bus recovery infrastructure Hi Wolfram, Any inputs ? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 25 January 2013 16:43, Wolfram Sang wrote:
> Code-wise I am mostly satisfied, except that I need to think about
> clock-stretching a little more. We could add that later, too.
Great!!
> However, while I understand you are keen to get rid of this series ASAP,
> it seems that this eagerness results in some sloppyness which is a bit
> cumbersome (most lines changed since last time have something to comment
> on).
:(
> So, I'll try a different review style this time and let you do the
> detective work :)
>
> 1) Please be strict and consistent with spaces around operators.
Sorry, but i couldn't find any such issues in code (but were there
in a comment)
> 2) Make sure the warnings and printouts match the code
> 3) Take the printouts serious and invest a thought if they can be
> further improved.
I have looked them again and attached are the final patches.
Following are the improvements i have:
Author: Viresh Kumar
Date: Sat Jan 26 10:31:42 2013 +0530
fixup! i2c/adapter: Add bus recovery infrastructure
---
drivers/i2c/i2c-core.c | 26 +++---
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 24c8a1e..3ec040e 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -136,14 +136,14 @@ static int i2c_get_gpios_for_recovery(struct
i2c_adapter *adap)
ret = gpio_request_one(bri->scl_gpio, GPIOF_OPEN_DRAIN |
GPIOF_OUT_INIT_HIGH, "i2c-scl");
if (ret) {
- dev_warn(dev, "gpio request fail: %d\n", bri->scl_gpio);
+ dev_warn(dev, "Can't get SCL gpio: %d\n", bri->scl_gpio);
return ret;
}
if (bri->get_sda) {
if (gpio_request_one(bri->sda_gpio, GPIOF_IN, "i2c-sda")) {
/* work without SDA polling */
- dev_warn(dev, "can't get SDA: %d. not using
SDA polling\n",
+ dev_warn(dev, "Can't get SDA gpio: %d. Not
using SDA polling\n",
bri->sda_gpio);
bri->get_sda = NULL;
}
@@ -163,8 +163,11 @@ static void i2c_put_gpios_for_recovery(struct
i2c_adapter *adap)
}
/*
- * We need to provide delay after every half clock pulse,
- * delay in ns = (10^6/ 100 KHz)/2
+ * We are generating clock pulse. ndelay() would decide durating of clk pulse.
+ * We will generate clock with rate 100 KHz and so duration of both
clock levels
+ * would be:
+ *
+ * delay in ns = (10^6 / 100) / 2
*/
#define RECOVERY_CLK_CNT 9
#define RECOVERY_NDELAY5000
@@ -181,13 +184,13 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
*/
while (i++ < RECOVERY_CLK_CNT * 2) {
if (val) {
- /* break if SDA got high */
+ /* Break if SDA is high */
if (bri->get_sda && bri->get_sda(adap))
break;
/* SCL shouldn't be low here */
if (!bri->get_scl(adap)) {
dev_err(&adap->dev,
- "SCL is stuck low exit recovery\n");
+ "SCL is stuck low, exit recovery\n");
ret = -EBUSY;
break;
}
@@ -229,7 +232,7 @@ int i2c_recover_bus(struct i2c_adapter *adap)
if (!adap->bus_recovery_info)
return -EOPNOTSUPP;
- dev_dbg(&adap->dev, "trying i2c bus recovery\n");
+ dev_dbg(&adap->dev, "Trying i2c bus recovery\n");
return adap->bus_recovery_info->recover_bus(adap);
}
@@ -1031,12 +1034,12 @@ static int i2c_register_adapter(struct
i2c_adapter *adap)
struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
if (!bri->recover_bus) {
- dev_err(&adap->dev, "No recover_bus(), not
using recovery\n");
+ dev_err(&adap->dev, "No recover_bus() found,
not using recovery\n");
adap->bus_recovery_info = NULL;
goto exit_recovery;
}
- /* GPIO recovery */
+ /* Generic GPIO recovery */
if (bri->recover_bus == i2c_generic_gpio_recovery) {
if (!gpio_is_valid(bri->scl_gpio)) {
dev_err(&adap->dev, "Invalid SCL gpio,
not using recovery\n");
@@ -1052,8 +1055,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
bri->get_scl = get_scl_gpio_value;
bri->set_scl = set_scl_gpio_value;
} else if (!bri->set_scl || !bri->get_scl) {
- dev_warn(&adap->dev,
- "set_scl() is must for SCL recovery\n");
+ /* Generic SCL rec
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On Fri, Jan 25, 2013 at 03:17:48PM +0530, Viresh Kumar wrote: > Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c > protocol Rev. 03 section 3.1.16 titled "Bus clear". > > http://www.nxp.com/documents/user_manual/UM10204.pdf > > Sometimes during operation i2c bus hangs and we need to give dummy clocks to > slave device to start the transfer again. Now we may have capability in the > bus > controller to generate these clocks or platform may have gpio pins which can > be > toggled to generate dummy clocks. This patch supports both. > > This patch also adds in generic bus recovery routines gpio or SCL line based > which can be used by bus controller. In addition controller driver may provide > its own version of the bus recovery routine. > > This doesn't support multi-master recovery for now. > > Reviewed-by: Paul Carpenter > Signed-off-by: Viresh Kumar Code-wise I am mostly satisfied, except that I need to think about clock-stretching a little more. We could add that later, too. However, while I understand you are keen to get rid of this series ASAP, it seems that this eagerness results in some sloppyness which is a bit cumbersome (most lines changed since last time have something to comment on). So, I'll try a different review style this time and let you do the detective work :) 1) Please be strict and consistent with spaces around operators. 2) Make sure the warnings and printouts match the code 3) Take the printouts serious and invest a thought if they can be further improved. Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang| Industrial Linux Solutions | http://www.pengutronix.de/ | signature.asc Description: Digital signature
Re: [PATCH V10 1/2] i2c/adapter: Add bus recovery infrastructure
On 25 January 2013 15:17, Viresh Kumar wrote: > Add i2c bus recovery infrastructure to i2c adapters as specified in the i2c > protocol Rev. 03 section 3.1.16 titled "Bus clear". > > http://www.nxp.com/documents/user_manual/UM10204.pdf > > Sometimes during operation i2c bus hangs and we need to give dummy clocks to > slave device to start the transfer again. Now we may have capability in the > bus > controller to generate these clocks or platform may have gpio pins which can > be > toggled to generate dummy clocks. This patch supports both. > > This patch also adds in generic bus recovery routines gpio or SCL line based > which can be used by bus controller. In addition controller driver may provide > its own version of the bus recovery routine. ARM mail servers are still broken. Please apply attached patches. 0001-i2c-adapter-Add-bus-recovery-infrastructure.patch Description: Binary data 0002-i2c-designware-Provide-i2c-bus-recovery-support.patch Description: Binary data
