[PATCH v3 0/5] i2c: davinci improvements and fixes

[PATCH v3 0/5] i2c: davinci improvements and fixes

[Grygorii Strashko]

This series contains some fixes and improvements for Davinci I2C:
patch 1 - small improvement
patch 2 - fixes "Bus busy" state for some case (was reported long time ago:()

patch 3-5 - converts driver to use I2C bus recovery infrastructure and
adds Davinci I2C bus recovery mechanizm based on using ICPFUNC registers.
These patches are combination of two patches from Ben Gardiner [1] and
Mike Looijmans [2] which i've reworked to use I2C bus recovery infrastructure

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2011-April/047305.html
"[PATCH 0/6] i2c-davinci gpio pulsed SCL recovery with ICPFUNC"
[2] https://lkml.org/lkml/2014/2/28/133
"[PATCH] i2c-davinci: Implement a bus recovery that actually works"

Changes in v3:
- minor comments applied

Changes in v2:
- patch 1: error handling improved
- patch 2: commit message updated
- patch 4: minor comments applied
- patch 5: added new optional DT property "ti,has-pfunc"

Link on v2:
 https://lkml.org/lkml/2014/11/26/252

Link on v1:
 http://www.spinics.net/lists/linux-i2c/msg17601.html

CC: Sekhar Nori <nsekhar@ti.com>
CC: Kevin Hilman <khilman@deeprootsystems.com>
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Murali Karicheri <m-karicheri2@ti.com>

Grygorii Strashko (5):
  i2c: i2c-davinci: switch to use platform_get_irq
  i2c: davinci: generate STP always when NACK is received
  i2c: recovery: change input parameter to i2c_adapter for
    prepare/unprepare_recovery
  i2c: davinci: use bus recovery infrastructure
  i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

 .../devicetree/bindings/i2c/i2c-davinci.txt        |   3 +
 drivers/i2c/busses/i2c-davinci.c                   | 205 +++++++++++++++------
 drivers/i2c/i2c-core.c                             |   4 +-
 include/linux/i2c.h                                |   4 +-
 include/linux/platform_data/i2c-davinci.h          |   1 +
 5 files changed, 159 insertions(+), 58 deletions(-)

-- 
1.9.1

[PATCH v3 1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Grygorii Strashko]

Switch Davinci I2C driver to use platform_get_irq(), because
it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
for requesting IRQ resources any more, as they can be not ready yet
in case of DT-boot.

CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 4d96147..25e8e25 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -640,13 +640,17 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 {
 	struct davinci_i2c_dev *dev;
 	struct i2c_adapter *adap;
-	struct resource *mem, *irq;
-	int r;
-
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!irq) {
-		dev_err(&pdev->dev, "no irq resource?\n");
-		return -ENODEV;
+	struct resource *mem;
+	int r, irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		if (!irq)
+			irq = -ENXIO;
+		if (irq != -EPROBE_DEFER)
+			dev_err(&pdev->dev,
+				"can't get irq resource ret=%d\n", irq);
+		return irq;
 	}
 
 	dev = devm_kzalloc(&pdev->dev, sizeof(struct davinci_i2c_dev),
@@ -661,7 +665,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	init_completion(&dev->xfr_complete);
 #endif
 	dev->dev = &pdev->dev;
-	dev->irq = irq->start;
+	dev->irq = irq;
 	dev->pdata = dev_get_platdata(&pdev->dev);
 	platform_set_drvdata(pdev, dev);
 
-- 
1.9.1

Re: [PATCH v3 1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Wolfram Sang]

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

On Mon, Dec 01, 2014 at 05:34:03PM +0200, Grygorii Strashko wrote:
> Switch Davinci I2C driver to use platform_get_irq(), because
> it is not recommened to use platform_get_resource(pdev, IORESOURCE_IRQ, ..)
> for requesting IRQ resources any more, as they can be not ready yet
> in case of DT-boot.
> 
> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

Applied to for-next, thanks!


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

[PATCH v3 2/5] i2c: davinci: generate STP always when NACK is received

[Grygorii Strashko]

According to I2C specification the NACK should be handled as follows:
"When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
Acknowledge signal. The master can then generate either a STOP condition to
abort the transfer, or a repeated START condition to start a new transfer."
[I2C spec Rev. 6, 3.1.6: http://www.nxp.com/documents/user_manual/UM10204.pdf]

Currently the Davinci i2c driver interrupts the transfer on receipt of a
NACK but fails to send a STOP in some situations and so makes the bus
stuck until next I2C IP reset (idle/enable).

For example, the issue will happen during SMBus read transfer which
consists from two i2c messages write command/address and read data:

S Slave Address Wr A Command Code A Sr Slave Address Rd A D1..Dn A P
<--- write -----------------------> <--- read --------------------->

The I2C client device will send NACK if it can't recognize "Command Code"
and it's expected from I2C master to generate STP in this case.
But now, Davinci i2C driver will just exit with -EREMOTEIO and STP will
not be generated.

Hence, fix it by generating Stop condition (STP) always when NACK is received.

This patch fixes Davinci I2C in the same way it was done for OMAP I2C
commit cda2109a26eb ("i2c: omap: query STP always when NACK is received").

CC: Sekhar Nori <nsekhar@ti.com>
CC: Kevin Hilman <khilman@deeprootsystems.com>
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Murali Karicheri <m-karicheri2@ti.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reported-by: Hein Tibosch <hein_tibosch@yahoo.es>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/i2c/busses/i2c-davinci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 25e8e25..17e1203 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -411,11 +411,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	if (dev->cmd_err & DAVINCI_I2C_STR_NACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return msg->len;
-		if (stop) {
-			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-			w |= DAVINCI_I2C_MDR_STP;
-			davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
-		}
+		w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+		w |= DAVINCI_I2C_MDR_STP;
+		davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 		return -EREMOTEIO;
 	}
 	return -EIO;
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/5] i2c: davinci: generate STP always when NACK is received

[Wolfram Sang]

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

On Mon, Dec 01, 2014 at 05:34:04PM +0200, Grygorii Strashko wrote:
> According to I2C specification the NACK should be handled as follows:
> "When SDA remains HIGH during this ninth clock pulse, this is defined as the Not
> Acknowledge signal. The master can then generate either a STOP condition to
> abort the transfer, or a repeated START condition to start a new transfer."
> [I2C spec Rev. 6, 3.1.6: http://www.nxp.com/documents/user_manual/UM10204.pdf]
> 
> Currently the Davinci i2c driver interrupts the transfer on receipt of a
> NACK but fails to send a STOP in some situations and so makes the bus
> stuck until next I2C IP reset (idle/enable).
> 
> For example, the issue will happen during SMBus read transfer which
> consists from two i2c messages write command/address and read data:
> 
> S Slave Address Wr A Command Code A Sr Slave Address Rd A D1..Dn A P

Applied to for-current, thanks!


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

[PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Grygorii Strashko]

This patch changes type of input parameter for .prepare/unprepare_recovery()
callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
This allows to simplify implementation of these callbacks and avoid
type conversations from i2c_bus_recovery_info to i2c_adapter.
The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
which contains pointer on it.

CC: Sekhar Nori <nsekhar@ti.com>
CC: Kevin Hilman <khilman@deeprootsystems.com>
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Murali Karicheri <m-karicheri2@ti.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/i2c/i2c-core.c | 4 ++--
 include/linux/i2c.h    | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..72b6e34 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
 	int i = 0, val = 1, ret = 0;
 
 	if (bri->prepare_recovery)
-		bri->prepare_recovery(bri);
+		bri->prepare_recovery(adap);
 
 	/*
 	 * By this time SCL is high, as we need to give 9 falling-rising edges
@@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
 	}
 
 	if (bri->unprepare_recovery)
-		bri->unprepare_recovery(bri);
+		bri->unprepare_recovery(adap);
 
 	return ret;
 }
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..cf9380f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
 	void (*set_scl)(struct i2c_adapter *, int val);
 	int (*get_sda)(struct i2c_adapter *);
 
-	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
-	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
+	void (*prepare_recovery)(struct i2c_adapter *);
+	void (*unprepare_recovery)(struct i2c_adapter *);
 
 	/* gpio recovery */
 	int scl_gpio;
-- 
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Wolfram Sang]

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

On Mon, Dec 01, 2014 at 05:34:05PM +0200, Grygorii Strashko wrote:
> This patch changes type of input parameter for .prepare/unprepare_recovery()
> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
> This allows to simplify implementation of these callbacks and avoid
> type conversations from i2c_bus_recovery_info to i2c_adapter.
> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
> which contains pointer on it.
> 
> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

Ah, bus recovery, I need to have a closer look on this one. Scheduled
for 3.20.


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

Re: [PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Grygorii Strashko]

Hi Wolfram,

On 12/04/2014 08:29 PM, Wolfram Sang wrote:
> On Mon, Dec 01, 2014 at 05:34:05PM +0200, Grygorii Strashko wrote:
>> This patch changes type of input parameter for .prepare/unprepare_recovery()
>> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
>> This allows to simplify implementation of these callbacks and avoid
>> type conversations from i2c_bus_recovery_info to i2c_adapter.
>> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
>> which contains pointer on it.
>>
>> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
>> Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>
> Ah, bus recovery, I need to have a closer look on this one. Scheduled
> for 3.20.
>

Unfortunately, it looks like you've missed these patches :(

I'm going to resend them if you agree.

regards,
-grygorii

Re: [PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Alexander Sverdlin]

Hi!

On 01/12/14 16:34, Grygorii Strashko wrote:
> This patch changes type of input parameter for .prepare/unprepare_recovery()
> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
> This allows to simplify implementation of these callbacks and avoid
> type conversations from i2c_bus_recovery_info to i2c_adapter.
> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
> which contains pointer on it.
> 
> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/i2c-core.c | 4 ++--
>  include/linux/i2c.h    | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 2f90ac6..72b6e34 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>  	int i = 0, val = 1, ret = 0;
>  
>  	if (bri->prepare_recovery)
> -		bri->prepare_recovery(bri);
> +		bri->prepare_recovery(adap);
>  
>  	/*
>  	 * By this time SCL is high, as we need to give 9 falling-rising edges
> @@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>  	}
>  
>  	if (bri->unprepare_recovery)
> -		bri->unprepare_recovery(bri);
> +		bri->unprepare_recovery(adap);
>  
>  	return ret;
>  }
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b556e0a..cf9380f 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
>  	void (*set_scl)(struct i2c_adapter *, int val);
>  	int (*get_sda)(struct i2c_adapter *);
>  
> -	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
> -	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
> +	void (*prepare_recovery)(struct i2c_adapter *);
> +	void (*unprepare_recovery)(struct i2c_adapter *);
>  
>  	/* gpio recovery */
>  	int scl_gpio;
> 

Acked-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

-- 
Best regards,
Alexander Sverdlin.
Sent from my pdp-11

Re: [PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]

Hi Wolfram,

On 03/12/2015 01:32 PM, Alexander Sverdlin wrote:
> On 01/12/14 16:34, Grygorii Strashko wrote:
>> This patch changes type of input parameter for .prepare/unprepare_recovery()
>> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
>> This allows to simplify implementation of these callbacks and avoid
>> type conversations from i2c_bus_recovery_info to i2c_adapter.
>> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
>> which contains pointer on it.
>>
>> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
>> Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
>> ---
>>   drivers/i2c/i2c-core.c | 4 ++--
>>   include/linux/i2c.h    | 4 ++--
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 2f90ac6..72b6e34 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>>   	int i = 0, val = 1, ret = 0;
>>   
>>   	if (bri->prepare_recovery)
>> -		bri->prepare_recovery(bri);
>> +		bri->prepare_recovery(adap);
>>   
>>   	/*
>>   	 * By this time SCL is high, as we need to give 9 falling-rising edges
>> @@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
>>   	}
>>   
>>   	if (bri->unprepare_recovery)
>> -		bri->unprepare_recovery(bri);
>> +		bri->unprepare_recovery(adap);
>>   
>>   	return ret;
>>   }
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index b556e0a..cf9380f 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
>>   	void (*set_scl)(struct i2c_adapter *, int val);
>>   	int (*get_sda)(struct i2c_adapter *);
>>   
>> -	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
>> -	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
>> +	void (*prepare_recovery)(struct i2c_adapter *);
>> +	void (*unprepare_recovery)(struct i2c_adapter *);
>>   
>>   	/* gpio recovery */
>>   	int scl_gpio;
>>
> 
> Acked-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> 

Could I ask you to consider this patch for 4.1?
Now my I2C Davinci series is based on it and probably I2C OMAP changes
will need to be based on it too - if Felipe would agree with this of course.

regards,
-grygorii


-- 
regards,
-grygorii

Re: [PATCH v3 3/5] i2c: recovery: change input parameter to i2c_adapter for prepare/unprepare_recovery

[Felipe Balbi]

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

Hi,

On Fri, Mar 13, 2015 at 12:15:54PM +0200, Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org wrote:
> Hi Wolfram,
> 
> On 03/12/2015 01:32 PM, Alexander Sverdlin wrote:
> > On 01/12/14 16:34, Grygorii Strashko wrote:
> >> This patch changes type of input parameter for .prepare/unprepare_recovery()
> >> callbacks from struct i2c_bus_recovery_info * to struct i2c_adapter *.
> >> This allows to simplify implementation of these callbacks and avoid
> >> type conversations from i2c_bus_recovery_info to i2c_adapter.
> >> The i2c_bus_recovery_info can be simply retrieved from struct i2c_adapter
> >> which contains pointer on it.
> >>
> >> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> >> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> >> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> >> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> >> Acked-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> >> ---
> >>   drivers/i2c/i2c-core.c | 4 ++--
> >>   include/linux/i2c.h    | 4 ++--
> >>   2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> >> index 2f90ac6..72b6e34 100644
> >> --- a/drivers/i2c/i2c-core.c
> >> +++ b/drivers/i2c/i2c-core.c
> >> @@ -563,7 +563,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
> >>   	int i = 0, val = 1, ret = 0;
> >>   
> >>   	if (bri->prepare_recovery)
> >> -		bri->prepare_recovery(bri);
> >> +		bri->prepare_recovery(adap);
> >>   
> >>   	/*
> >>   	 * By this time SCL is high, as we need to give 9 falling-rising edges
> >> @@ -588,7 +588,7 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
> >>   	}
> >>   
> >>   	if (bri->unprepare_recovery)
> >> -		bri->unprepare_recovery(bri);
> >> +		bri->unprepare_recovery(adap);
> >>   
> >>   	return ret;
> >>   }
> >> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> >> index b556e0a..cf9380f 100644
> >> --- a/include/linux/i2c.h
> >> +++ b/include/linux/i2c.h
> >> @@ -404,8 +404,8 @@ struct i2c_bus_recovery_info {
> >>   	void (*set_scl)(struct i2c_adapter *, int val);
> >>   	int (*get_sda)(struct i2c_adapter *);
> >>   
> >> -	void (*prepare_recovery)(struct i2c_bus_recovery_info *bri);
> >> -	void (*unprepare_recovery)(struct i2c_bus_recovery_info *bri);
> >> +	void (*prepare_recovery)(struct i2c_adapter *);
> >> +	void (*unprepare_recovery)(struct i2c_adapter *);
> >>   
> >>   	/* gpio recovery */
> >>   	int scl_gpio;
> >>
> > 
> > Acked-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
> > 
> 
> Could I ask you to consider this patch for 4.1?
> Now my I2C Davinci series is based on it and probably I2C OMAP changes
> will need to be based on it too - if Felipe would agree with this of course.

I can easily fix my i2c-omap series for this, but I need confirmation
that this is what's going to get merged.

-- 
balbi

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

[PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii Strashko]

This patch converts Davinci I2C driver to use I2C bus recovery
infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
bus recovery infrastructure").

The i2c_bus_recovery_info is configured for Davinci I2C adapter
only in case scl_pin is provided in platform data.

As the controller must be held in reset while doing so, the
recovery routine must re-init the controller. Since this was already
being done after each call to i2c_recover_bus, move those calls into
the recovery_prepare/unprepare routines and as well.

CC: Sekhar Nori <nsekhar@ti.com>
CC: Kevin Hilman <khilman@deeprootsystems.com>
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Murali Karicheri <m-karicheri2@ti.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/i2c/busses/i2c-davinci.c | 77 +++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 17e1203..00aed63 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
 	return readw_relaxed(i2c_dev->base + reg);
 }
 
-/* Generate a pulse on the i2c clock pin. */
-static void davinci_i2c_clock_pulse(unsigned int scl_pin)
-{
-	u16 i;
-
-	if (scl_pin) {
-		/* Send high and low on the SCL line */
-		for (i = 0; i < 9; i++) {
-			gpio_set_value(scl_pin, 0);
-			udelay(20);
-			gpio_set_value(scl_pin, 1);
-			udelay(20);
-		}
-	}
-}
-
-/* This routine does i2c bus recovery as specified in the
- * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
- */
-static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
-{
-	u32 flag = 0;
-	struct davinci_i2c_platform_data *pdata = dev->pdata;
-
-	dev_err(dev->dev, "initiating i2c bus recovery\n");
-	/* Send NACK to the slave */
-	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	flag |=  DAVINCI_I2C_MDR_NACK;
-	/* write the data into mode register */
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-	davinci_i2c_clock_pulse(pdata->scl_pin);
-	/* Send STOP */
-	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	flag |= DAVINCI_I2C_MDR_STP;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
-}
-
 static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
 								int val)
 {
@@ -267,6 +230,34 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 }
 
 /*
+ * This routine does i2c bus recovery by using i2c_generic_gpio_recovery
+ * which is provided by I2C Bus recovery infrastructure.
+ */
+static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	/* Disable interrupts */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);
+
+	/* put I2C into reset */
+	davinci_i2c_reset_ctrl(dev, 0);
+}
+
+static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	i2c_davinci_init(dev);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
+	.recover_bus = i2c_generic_gpio_recovery,
+	.prepare_recovery = davinci_i2c_prepare_recovery,
+	.unprepare_recovery = davinci_i2c_unprepare_recovery,
+};
+
+/*
  * Waiting for bus not busy
  */
 static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
@@ -286,8 +277,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
 				return -ETIMEDOUT;
 			} else {
 				to_cnt = 0;
-				davinci_i2c_recover_bus(dev);
-				i2c_davinci_init(dev);
+				i2c_recover_bus(&dev->adapter);
 			}
 		}
 		if (allow_sleep)
@@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 						      dev->adapter.timeout);
 	if (r == 0) {
 		dev_err(dev->dev, "controller timed out\n");
-		davinci_i2c_recover_bus(dev);
-		i2c_davinci_init(dev);
+		i2c_recover_bus(adap);
 		dev->buf_len = 0;
 		return -ETIMEDOUT;
 	}
@@ -721,6 +710,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	adap->timeout = DAVINCI_I2C_TIMEOUT;
 	adap->dev.of_node = pdev->dev.of_node;
 
+	if (dev->pdata->scl_pin) {
+		adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
+		adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
+		adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
+	}
+
 	adap->nr = pdev->id;
 	r = i2c_add_numbered_adapter(adap);
 	if (r) {
-- 
1.9.1

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Alexander Sverdlin]

Hello Grygorii,

On 01/12/14 16:34, Grygorii Strashko wrote:
> This patch converts Davinci I2C driver to use I2C bus recovery
> infrastructure, introduced by commit 5f9296ba21b3 ("i2c: Add
> bus recovery infrastructure").
> 
> The i2c_bus_recovery_info is configured for Davinci I2C adapter
> only in case scl_pin is provided in platform data.
> 
> As the controller must be held in reset while doing so, the
> recovery routine must re-init the controller. Since this was already
> being done after each call to i2c_recover_bus, move those calls into
> the recovery_prepare/unprepare routines and as well.
> 
> CC: Sekhar Nori <nsekhar@ti.com>
> CC: Kevin Hilman <khilman@deeprootsystems.com>
> CC: Santosh Shilimkar <ssantosh@kernel.org>
> CC: Murali Karicheri <m-karicheri2@ti.com>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
>  drivers/i2c/busses/i2c-davinci.c | 77 +++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 17e1203..00aed63 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -133,43 +133,6 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>  	return readw_relaxed(i2c_dev->base + reg);
>  }
>  
> -/* Generate a pulse on the i2c clock pin. */
> -static void davinci_i2c_clock_pulse(unsigned int scl_pin)
> -{
> -	u16 i;
> -
> -	if (scl_pin) {
> -		/* Send high and low on the SCL line */
> -		for (i = 0; i < 9; i++) {
> -			gpio_set_value(scl_pin, 0);
> -			udelay(20);
> -			gpio_set_value(scl_pin, 1);
> -			udelay(20);
> -		}
> -	}
> -}
> -
> -/* This routine does i2c bus recovery as specified in the
> - * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> - */
> -static void davinci_i2c_recover_bus(struct davinci_i2c_dev *dev)
> -{
> -	u32 flag = 0;
> -	struct davinci_i2c_platform_data *pdata = dev->pdata;
> -
> -	dev_err(dev->dev, "initiating i2c bus recovery\n");
> -	/* Send NACK to the slave */
> -	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	flag |=  DAVINCI_I2C_MDR_NACK;
> -	/* write the data into mode register */
> -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> -	davinci_i2c_clock_pulse(pdata->scl_pin);
> -	/* Send STOP */
> -	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	flag |= DAVINCI_I2C_MDR_STP;
> -	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> -}
> -
>  static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>  								int val)
>  {
> @@ -267,6 +230,34 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  }
>  
>  /*
> + * This routine does i2c bus recovery by using i2c_generic_gpio_recovery
> + * which is provided by I2C Bus recovery infrastructure.
> + */
> +static void davinci_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	/* Disable interrupts */
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, 0);

I suppose, you don't need to disable IRQs if you reset the controller as the very next action.

> +
> +	/* put I2C into reset */
> +	davinci_i2c_reset_ctrl(dev, 0);
> +}
> +
> +static void davinci_i2c_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	i2c_davinci_init(dev);
> +}
> +
> +static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
> +	.recover_bus = i2c_generic_gpio_recovery,
> +	.prepare_recovery = davinci_i2c_prepare_recovery,
> +	.unprepare_recovery = davinci_i2c_unprepare_recovery,
> +};
> +
> +/*
>   * Waiting for bus not busy
>   */
>  static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
> @@ -286,8 +277,7 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
>  				return -ETIMEDOUT;
>  			} else {
>  				to_cnt = 0;
> -				davinci_i2c_recover_bus(dev);
> -				i2c_davinci_init(dev);
> +				i2c_recover_bus(&dev->adapter);
>  			}
>  		}
>  		if (allow_sleep)
> @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  						      dev->adapter.timeout);
>  	if (r == 0) {
>  		dev_err(dev->dev, "controller timed out\n");
> -		davinci_i2c_recover_bus(dev);
> -		i2c_davinci_init(dev);
> +		i2c_recover_bus(adap);
>  		dev->buf_len = 0;
>  		return -ETIMEDOUT;
>  	}
> @@ -721,6 +710,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  	adap->timeout = DAVINCI_I2C_TIMEOUT;
>  	adap->dev.of_node = pdev->dev.of_node;
>  
> +	if (dev->pdata->scl_pin) {
> +		adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
> +		adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
> +		adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
> +	}
> +
>  	adap->nr = pdev->id;
>  	r = i2c_add_numbered_adapter(adap);
>  	if (r) {
> 

-- 
Best regards,
Alexander Sverdlin.

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Wolfram Sang]

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

Hi,

so, the bus recovery patches look fine to me in general.

It is only this one question left which I always had with bus recovery.
Maybe you guys can join me thinking about it.

> @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  						      dev->adapter.timeout);
>  	if (r == 0) {
>  		dev_err(dev->dev, "controller timed out\n");
> -		davinci_i2c_recover_bus(dev);
> -		i2c_davinci_init(dev);
> +		i2c_recover_bus(adap);
>  		dev->buf_len = 0;
>  		return -ETIMEDOUT;

The I2C specs say in 3.1.16 that the recovery procedure should be used
when SDA is stuck low. So, I do wonder if we should apply the recovery
after a timeout. Stuck SDA might be one reason for timeout, but there
may be others...

Thanks,

   Wolfram


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

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii.Strashko@linaro.org]

Hi,
On 03/18/2015 10:31 PM, Wolfram Sang wrote:
> Hi,
> 
> so, the bus recovery patches look fine to me in general.
> 
> It is only this one question left which I always had with bus recovery.
> Maybe you guys can join me thinking about it.

Ok. Thanks and sorry for delayed reply - missed your e-mail :(
I'll resend them next week.

> 
>> @@ -376,8 +366,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>   						      dev->adapter.timeout);
>>   	if (r == 0) {
>>   		dev_err(dev->dev, "controller timed out\n");
>> -		davinci_i2c_recover_bus(dev);
>> -		i2c_davinci_init(dev);
>> +		i2c_recover_bus(adap);
>>   		dev->buf_len = 0;
>>   		return -ETIMEDOUT;
> 
> The I2C specs say in 3.1.16 that the recovery procedure should be used
> when SDA is stuck low. So, I do wonder if we should apply the recovery
> after a timeout. Stuck SDA might be one reason for timeout, but there
> may be others...

This is ancient code. And regarding your question -
Might be it would be reasonable to add call of
i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()?
This way we will wait for Bus Free before performing recovery.

Of course,  i2c_davinci_wait_bus_not_busy() has to be fixed first
as proposed by Alexander Sverdlin here:
 https://patchwork.ozlabs.org/patch/448994/. 

-- 
regards,
-grygorii

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Wolfram Sang]

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


> > The I2C specs say in 3.1.16 that the recovery procedure should be used
> > when SDA is stuck low. So, I do wonder if we should apply the recovery
> > after a timeout. Stuck SDA might be one reason for timeout, but there
> > may be others...
> 
> This is ancient code. And regarding your question -
> Might be it would be reasonable to add call of
> i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()?
> This way we will wait for Bus Free before performing recovery.

That might be an improvement, but the generic question still remains:
Is a timeout a reason for recovery? SDA stuck low is one reason for a
timeout. I have problems making up my mind here between being pragmatic
and being in accordance with the specs.

> Of course,  i2c_davinci_wait_bus_not_busy() has to be fixed first
> as proposed by Alexander Sverdlin here:
>  https://patchwork.ozlabs.org/patch/448994/. 

Okay, good that you said it. So I'll give his patch series priority over
this one.


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

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]

On 04/03/2015 11:18 PM, Wolfram Sang wrote:
> 
>>> The I2C specs say in 3.1.16 that the recovery procedure should be used
>>> when SDA is stuck low. So, I do wonder if we should apply the recovery
>>> after a timeout. Stuck SDA might be one reason for timeout, but there
>>> may be others...
>>
>> This is ancient code. And regarding your question -
>> Might be it would be reasonable to add call of
>> i2c_davinci_wait_bus_not_busy() at the end of i2c_davinci_xfer()?
>> This way we will wait for Bus Free before performing recovery.
> 
> That might be an improvement, but the generic question still remains:
> Is a timeout a reason for recovery? SDA stuck low is one reason for a
> timeout. I have problems making up my mind here between being pragmatic
> and being in accordance with the specs.

The timeout here means there were no responses from I2C controller within some
reasonable time period (default - 1 sec). Which in turn
means that Bus/HW state is "unknown" and init&recovery seems reasonable here, but
yes - "init&recovery" could be optimized more, but, in my opinion, only
as subsequent patches.

Actually, i2c_generic_recovery() will just exit if SDA is high already.

> 
>> Of course,  i2c_davinci_wait_bus_not_busy() has to be fixed first
>> as proposed by Alexander Sverdlin here:
>>   https://patchwork.ozlabs.org/patch/448994/.
> 
> Okay, good that you said it. So I'll give his patch series priority over
> this one.


Sorry, but this series already mises few merge windows and it has a lot
of revied-by and tested-by, so could we proceed please?

Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html

-- 
regards,
-grygorii

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Wolfram Sang]

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


> >> Of course,  i2c_davinci_wait_bus_not_busy() has to be fixed first
> >> as proposed by Alexander Sverdlin here:
> >>   https://patchwork.ozlabs.org/patch/448994/.
> > 
> > Okay, good that you said it. So I'll give his patch series priority over
> > this one.
> 
> 
> Sorry, but this series already mises few merge windows and it has a lot
> of revied-by and tested-by, so could we proceed please?
> 
> Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html

??? Didn't you say above that Alexaders's patch is needed first?


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

Re: [PATCH v3 4/5] i2c: davinci: use bus recovery infrastructure

[Grygorii.Strashko-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]

On 04/06/2015 07:09 PM, Wolfram Sang wrote:
>
>>>> Of course,  i2c_davinci_wait_bus_not_busy() has to be fixed first
>>>> as proposed by Alexander Sverdlin here:
>>>>    https://patchwork.ozlabs.org/patch/448994/.
>>>
>>> Okay, good that you said it. So I'll give his patch series priority over
>>> this one.
>>
>>
>> Sorry, but this series already mises few merge windows and it has a lot
>> of revied-by and tested-by, so could we proceed please?
>>
>> Re-based & re-sent http://www.spinics.net/lists/arm-kernel/msg410810.html
>
> ??? Didn't you say above that Alexaders's patch is needed first?
>

Sorry for misunderstanding.

I said that if We'd like to continue and optimize more recovery path
then yes - Alexaders's patch will be needed (patch 2 from his series
[PATCH 2/3] i2c: davinci: Refactor i2c_davinci_wait_bus_not_busy(), 
which, in turn need to be rebased as the first one in his series and 
re-send). And in my opinion all such improvements could be done by 
subsequent patches.

-- 
regards,
-grygorii

[PATCH 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Grygorii Strashko]

Having a board where the I2C bus locks up occasionally made it clear
that the bus recovery in the i2c-davinci driver will only work on
some boards, because on regular boards, this will only toggle GPIO
lines that aren't muxed to the actual pins.

The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
built-in capability to bit-bang its lines by using the ICPFUNC registers
of the i2c controller.
Implement the suggested procedure by toggling SCL and checking SDA using
the ICPFUNC registers of the I2C controller when present. Allow platforms
to indicate the presence of the ICPFUNC registers with a has_pfunc platform
data flag and add optional DT property "ti,has-pfunc" to indicate
the same in DT.

CC: Sekhar Nori <nsekhar@ti.com>
CC: Kevin Hilman <khilman@deeprootsystems.com>
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Murali Karicheri <m-karicheri2@ti.com>
CC: Mike Looijmans <info@milosoftware.com>
CC: <devicetree@vger.kernel.org>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Signed-off-by: Mike Looijmans <milo-software@users.sourceforge.net>
[grygorii.strashko@ti.com: combined patches from Ben Gardiner and
Mike Looijmans and reimplemented ICPFUNC bus recovery using I2C
bus recovery infrastructure]
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 .../devicetree/bindings/i2c/i2c-davinci.txt        |   3 +
 drivers/i2c/busses/i2c-davinci.c                   | 102 ++++++++++++++++++++-
 include/linux/platform_data/i2c-davinci.h          |   1 +
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
index 2dc935b..a4e1cbc 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
@@ -10,6 +10,9 @@ Required properties:
 Recommended properties :
 - interrupts : standard interrupt property.
 - clock-frequency : desired I2C bus clock frequency in Hz.
+- ti,has-pfunc: boolean; if defined, it indicates that SoC supports PFUNC
+	registers. PFUNC registers allow to switch I2C pins to function as
+	GPIOs, so they can by toggled manually.
 
 Example (enbw_cmc board):
 	i2c@1c22000 {
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 00aed63..a1bb587 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -64,6 +64,12 @@
 #define DAVINCI_I2C_IVR_REG	0x28
 #define DAVINCI_I2C_EMDR_REG	0x2c
 #define DAVINCI_I2C_PSC_REG	0x30
+#define DAVINCI_I2C_FUNC_REG	0x48
+#define DAVINCI_I2C_DIR_REG	0x4c
+#define DAVINCI_I2C_DIN_REG	0x50
+#define DAVINCI_I2C_DOUT_REG	0x54
+#define DAVINCI_I2C_DSET_REG	0x58
+#define DAVINCI_I2C_DCLR_REG	0x5c
 
 #define DAVINCI_I2C_IVR_AAS	0x07
 #define DAVINCI_I2C_IVR_SCD	0x06
@@ -97,6 +103,29 @@
 #define DAVINCI_I2C_IMR_NACK	BIT(1)
 #define DAVINCI_I2C_IMR_AL	BIT(0)
 
+/* set SDA and SCL as GPIO */
+#define DAVINCI_I2C_FUNC_PFUNC0	BIT(0)
+
+/* set SCL as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR0	BIT(0)
+/* set SDA as output when used as GPIO*/
+#define DAVINCI_I2C_DIR_PDIR1	BIT(1)
+
+/* read SCL GPIO level */
+#define DAVINCI_I2C_DIN_PDIN0 BIT(0)
+/* read SDA GPIO level */
+#define DAVINCI_I2C_DIN_PDIN1 BIT(1)
+
+/*set the SCL GPIO high */
+#define DAVINCI_I2C_DSET_PDSET0	BIT(0)
+/*set the SDA GPIO high */
+#define DAVINCI_I2C_DSET_PDSET1	BIT(1)
+
+/* set the SCL GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR0	BIT(0)
+/* set the SDA GPIO low */
+#define DAVINCI_I2C_DCLR_PDCLR1	BIT(1)
+
 struct davinci_i2c_dev {
 	struct device           *dev;
 	void __iomem		*base;
@@ -257,6 +286,71 @@ static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
 	.unprepare_recovery = davinci_i2c_unprepare_recovery,
 };
 
+static void davinci_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	if (val)
+		davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
+				      DAVINCI_I2C_DSET_PDSET0);
+	else
+		davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
+				      DAVINCI_I2C_DCLR_PDCLR0);
+}
+
+static int davinci_i2c_get_scl(struct i2c_adapter *adap)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+	int val;
+
+	/* read the state of SCL */
+	val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+	return val & DAVINCI_I2C_DIN_PDIN0;
+}
+
+static int davinci_i2c_get_sda(struct i2c_adapter *adap)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+	int val;
+
+	/* read the state of SDA */
+	val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
+	return val & DAVINCI_I2C_DIN_PDIN1;
+}
+
+static void davinci_i2c_scl_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	davinci_i2c_prepare_recovery(adap);
+
+	/* SCL output, SDA input */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_DIR_REG, DAVINCI_I2C_DIR_PDIR0);
+
+	/* change to GPIO mode */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG,
+			      DAVINCI_I2C_FUNC_PFUNC0);
+}
+
+static void davinci_i2c_scl_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
+
+	/* change back to I2C mode */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG, 0);
+
+	davinci_i2c_unprepare_recovery(adap);
+}
+
+static struct i2c_bus_recovery_info davinci_i2c_scl_recovery_info = {
+	.recover_bus = i2c_generic_scl_recovery,
+	.set_scl = davinci_i2c_set_scl,
+	.get_scl = davinci_i2c_get_scl,
+	.get_sda = davinci_i2c_get_sda,
+	.prepare_recovery = davinci_i2c_scl_prepare_recovery,
+	.unprepare_recovery = davinci_i2c_scl_unprepare_recovery,
+};
+
 /*
  * Waiting for bus not busy
  */
@@ -669,6 +763,10 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 		if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
 			&prop))
 			dev->pdata->bus_freq = prop / 1000;
+
+		dev->pdata->has_pfunc =
+			of_property_read_bool(pdev->dev.of_node,
+					      "ti,has-pfunc");
 	} else if (!dev->pdata) {
 		dev->pdata = &davinci_i2c_platform_data_default;
 	}
@@ -710,7 +808,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	adap->timeout = DAVINCI_I2C_TIMEOUT;
 	adap->dev.of_node = pdev->dev.of_node;
 
-	if (dev->pdata->scl_pin) {
+	if (dev->pdata->has_pfunc)
+		adap->bus_recovery_info = &davinci_i2c_scl_recovery_info;
+	else if (dev->pdata->scl_pin) {
 		adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
 		adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
 		adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
diff --git a/include/linux/platform_data/i2c-davinci.h b/include/linux/platform_data/i2c-davinci.h
index 2312d19..89fd347 100644
--- a/include/linux/platform_data/i2c-davinci.h
+++ b/include/linux/platform_data/i2c-davinci.h
@@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
 	unsigned int	bus_delay;	/* post-transaction delay (usec) */
 	unsigned int    sda_pin;        /* GPIO pin ID to use for SDA */
 	unsigned int    scl_pin;        /* GPIO pin ID to use for SCL */
+	bool		has_pfunc;	/*chip has a ICPFUNC register */
 };
 
 /* for board setup code */
-- 
1.9.1

Re: [PATCH 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Alexander Sverdlin]

Hello Grygorii,

On 01/12/14 16:34, Grygorii Strashko wrote:
> Having a board where the I2C bus locks up occasionally made it clear
> that the bus recovery in the i2c-davinci driver will only work on
> some boards, because on regular boards, this will only toggle GPIO
> lines that aren't muxed to the actual pins.
> 
> The I2C controller on SoCs like da850 (and da830), Keystone 2 has the
> built-in capability to bit-bang its lines by using the ICPFUNC registers
> of the i2c controller.
> Implement the suggested procedure by toggling SCL and checking SDA using
> the ICPFUNC registers of the I2C controller when present. Allow platforms
> to indicate the presence of the ICPFUNC registers with a has_pfunc platform
> data flag and add optional DT property "ti,has-pfunc" to indicate
> the same in DT.
> 
> CC: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> CC: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> CC: Santosh Shilimkar <ssantosh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Murali Karicheri <m-karicheri2-l0cyMroinI0@public.gmane.org>
> CC: Mike Looijmans <info-KQxf8TFvCoAi8rCdYzckzA@public.gmane.org>
> CC: <devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Ben Gardiner <bengardiner-ScDXFp4xN3PN+rMO2ozGnw@public.gmane.org>
> Signed-off-by: Mike Looijmans <milo-software-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> [grygorii.strashko-l0cyMroinI0@public.gmane.org: combined patches from Ben Gardiner and
> Mike Looijmans and reimplemented ICPFUNC bus recovery using I2C
> bus recovery infrastructure]
> Signed-off-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>

We have tested it on a custom Keystone2-based board, recovery seems to work
when SDA is held low externally.

Acked-by: Alexander Sverdlin <alexander.sverdlin-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Tested-by: Michael Lawnick <michael.lawnick-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

> ---
>  .../devicetree/bindings/i2c/i2c-davinci.txt        |   3 +
>  drivers/i2c/busses/i2c-davinci.c                   | 102 ++++++++++++++++++++-
>  include/linux/platform_data/i2c-davinci.h          |   1 +
>  3 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
> index 2dc935b..a4e1cbc 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
> @@ -10,6 +10,9 @@ Required properties:
>  Recommended properties :
>  - interrupts : standard interrupt property.
>  - clock-frequency : desired I2C bus clock frequency in Hz.
> +- ti,has-pfunc: boolean; if defined, it indicates that SoC supports PFUNC
> +	registers. PFUNC registers allow to switch I2C pins to function as
> +	GPIOs, so they can by toggled manually.
>  
>  Example (enbw_cmc board):
>  	i2c@1c22000 {
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 00aed63..a1bb587 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -64,6 +64,12 @@
>  #define DAVINCI_I2C_IVR_REG	0x28
>  #define DAVINCI_I2C_EMDR_REG	0x2c
>  #define DAVINCI_I2C_PSC_REG	0x30
> +#define DAVINCI_I2C_FUNC_REG	0x48
> +#define DAVINCI_I2C_DIR_REG	0x4c
> +#define DAVINCI_I2C_DIN_REG	0x50
> +#define DAVINCI_I2C_DOUT_REG	0x54
> +#define DAVINCI_I2C_DSET_REG	0x58
> +#define DAVINCI_I2C_DCLR_REG	0x5c
>  
>  #define DAVINCI_I2C_IVR_AAS	0x07
>  #define DAVINCI_I2C_IVR_SCD	0x06
> @@ -97,6 +103,29 @@
>  #define DAVINCI_I2C_IMR_NACK	BIT(1)
>  #define DAVINCI_I2C_IMR_AL	BIT(0)
>  
> +/* set SDA and SCL as GPIO */
> +#define DAVINCI_I2C_FUNC_PFUNC0	BIT(0)
> +
> +/* set SCL as output when used as GPIO*/
> +#define DAVINCI_I2C_DIR_PDIR0	BIT(0)
> +/* set SDA as output when used as GPIO*/
> +#define DAVINCI_I2C_DIR_PDIR1	BIT(1)
> +
> +/* read SCL GPIO level */
> +#define DAVINCI_I2C_DIN_PDIN0 BIT(0)
> +/* read SDA GPIO level */
> +#define DAVINCI_I2C_DIN_PDIN1 BIT(1)
> +
> +/*set the SCL GPIO high */
> +#define DAVINCI_I2C_DSET_PDSET0	BIT(0)
> +/*set the SDA GPIO high */
> +#define DAVINCI_I2C_DSET_PDSET1	BIT(1)
> +
> +/* set the SCL GPIO low */
> +#define DAVINCI_I2C_DCLR_PDCLR0	BIT(0)
> +/* set the SDA GPIO low */
> +#define DAVINCI_I2C_DCLR_PDCLR1	BIT(1)
> +
>  struct davinci_i2c_dev {
>  	struct device           *dev;
>  	void __iomem		*base;
> @@ -257,6 +286,71 @@ static struct i2c_bus_recovery_info davinci_i2c_gpio_recovery_info = {
>  	.unprepare_recovery = davinci_i2c_unprepare_recovery,
>  };
>  
> +static void davinci_i2c_set_scl(struct i2c_adapter *adap, int val)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	if (val)
> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DSET_REG,
> +				      DAVINCI_I2C_DSET_PDSET0);
> +	else
> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DCLR_REG,
> +				      DAVINCI_I2C_DCLR_PDCLR0);
> +}
> +
> +static int davinci_i2c_get_scl(struct i2c_adapter *adap)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int val;
> +
> +	/* read the state of SCL */
> +	val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
> +	return val & DAVINCI_I2C_DIN_PDIN0;
> +}
> +
> +static int davinci_i2c_get_sda(struct i2c_adapter *adap)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +	int val;
> +
> +	/* read the state of SDA */
> +	val = davinci_i2c_read_reg(dev, DAVINCI_I2C_DIN_REG);
> +	return val & DAVINCI_I2C_DIN_PDIN1;
> +}
> +
> +static void davinci_i2c_scl_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	davinci_i2c_prepare_recovery(adap);
> +
> +	/* SCL output, SDA input */
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_DIR_REG, DAVINCI_I2C_DIR_PDIR0);
> +
> +	/* change to GPIO mode */
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG,
> +			      DAVINCI_I2C_FUNC_PFUNC0);
> +}
> +
> +static void davinci_i2c_scl_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> +
> +	/* change back to I2C mode */
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_FUNC_REG, 0);
> +
> +	davinci_i2c_unprepare_recovery(adap);
> +}
> +
> +static struct i2c_bus_recovery_info davinci_i2c_scl_recovery_info = {
> +	.recover_bus = i2c_generic_scl_recovery,
> +	.set_scl = davinci_i2c_set_scl,
> +	.get_scl = davinci_i2c_get_scl,
> +	.get_sda = davinci_i2c_get_sda,
> +	.prepare_recovery = davinci_i2c_scl_prepare_recovery,
> +	.unprepare_recovery = davinci_i2c_scl_unprepare_recovery,
> +};
> +
>  /*
>   * Waiting for bus not busy
>   */
> @@ -669,6 +763,10 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  		if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>  			&prop))
>  			dev->pdata->bus_freq = prop / 1000;
> +
> +		dev->pdata->has_pfunc =
> +			of_property_read_bool(pdev->dev.of_node,
> +					      "ti,has-pfunc");
>  	} else if (!dev->pdata) {
>  		dev->pdata = &davinci_i2c_platform_data_default;
>  	}
> @@ -710,7 +808,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  	adap->timeout = DAVINCI_I2C_TIMEOUT;
>  	adap->dev.of_node = pdev->dev.of_node;
>  
> -	if (dev->pdata->scl_pin) {
> +	if (dev->pdata->has_pfunc)
> +		adap->bus_recovery_info = &davinci_i2c_scl_recovery_info;
> +	else if (dev->pdata->scl_pin) {
>  		adap->bus_recovery_info = &davinci_i2c_gpio_recovery_info;
>  		adap->bus_recovery_info->scl_gpio = dev->pdata->scl_pin;
>  		adap->bus_recovery_info->sda_gpio = dev->pdata->sda_pin;
> diff --git a/include/linux/platform_data/i2c-davinci.h b/include/linux/platform_data/i2c-davinci.h
> index 2312d19..89fd347 100644
> --- a/include/linux/platform_data/i2c-davinci.h
> +++ b/include/linux/platform_data/i2c-davinci.h
> @@ -18,6 +18,7 @@ struct davinci_i2c_platform_data {
>  	unsigned int	bus_delay;	/* post-transaction delay (usec) */
>  	unsigned int    sda_pin;        /* GPIO pin ID to use for SDA */
>  	unsigned int    scl_pin;        /* GPIO pin ID to use for SCL */
> +	bool		has_pfunc;	/*chip has a ICPFUNC register */
>  };
>  
>  /* for board setup code */

-- 
Best regards,
Alexander Sverdlin.