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

[PATCH v2 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 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 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: query 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 v2 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@ti.com>
CC: Kevin Hilman <khilman@deeprootsystems.com>
CC: Santosh Shilimkar <ssantosh@kernel.org>
CC: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 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..7f54903 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 == -EPROBE_DEFER)
+			return irq;
+		if (!irq)
+			irq = -ENXIO;
+		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 v2 1/5] i2c: i2c-davinci: switch to use platform_get_irq

[Uwe Kleine-König]

Hello Grygorii,

On Wed, Nov 26, 2014 at 03:59:49PM +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>
> 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..7f54903 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 == -EPROBE_DEFER)
> +			return irq;
> +		if (!irq)
> +			irq = -ENXIO;
> +		dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
> +		return irq;
The simpler and I think also more usual logic is:

	if (!irq)
		irq = -ENXIO;
	if (irq != -EPROBE_DEFER)
		dev_err(...);
	return irq;

Other than that the patch looks fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Grygorii Strashko]

On 11/26/2014 05:54 PM, Uwe Kleine-König wrote:
> Hello Grygorii,
>
> On Wed, Nov 26, 2014 at 03:59:49PM +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>
>> 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..7f54903 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 == -EPROBE_DEFER)
>> +			return irq;
>> +		if (!irq)
>> +			irq = -ENXIO;
>> +		dev_err(&pdev->dev, "can't get irq resource ret=%d\n", irq);
>> +		return irq;
> The simpler and I think also more usual logic is:
>
> 	if (!irq)
> 		irq = -ENXIO;
> 	if (irq != -EPROBE_DEFER)
> 		dev_err(...);
> 	return irq;
>
> Other than that the patch looks fine.

Ok. will change and add your ack.

regards,
-grygorii

[PATCH v2 2/5] i2c: davinci: query STP always when NACK is received

[Grygorii Strashko]

According to I2C specification the NACK should be handled as folows:
"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 query STP in this case.
But now, Davinci i2C driver will just exit with -EREMOTEIO and STP will
not be queried.

Hence, fix it by querying 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>
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 7f54903..1990ae8 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

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

[Uwe Kleine-König]

Hello,

I don't understand your use of "query" in the subject and later in the
commit log. Do you mean "send"?

On Wed, Nov 26, 2014 at 03:59:50PM +0200, Grygorii Strashko wrote:
> According to I2C specification the NACK should be handled as folows:
s/folows/follows/

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Grygorii Strashko]

On 11/26/2014 05:57 PM, Uwe Kleine-König wrote:
> Hello,
>
> I don't understand your use of "query" in the subject and later in the
> commit log. Do you mean "send"?

will change to "generate". Ok?

>
> On Wed, Nov 26, 2014 at 03:59:50PM +0200, Grygorii Strashko wrote:
>> According to I2C specification the NACK should be handled as folows:
> s/folows/follows/
ok.

Can I assume that you are ok with this patch in general?

regards,
-grygorii

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

[Uwe Kleine-König]

Hello Grygorii,

On Wed, Nov 26, 2014 at 06:31:22PM +0200, Grygorii Strashko wrote:
> On 11/26/2014 05:57 PM, Uwe Kleine-König wrote:
> >I don't understand your use of "query" in the subject and later in the
> >commit log. Do you mean "send"?
> 
> will change to "generate". Ok?
yes that would be better understandable

> Can I assume that you are ok with this patch in general?
Yeah, your argument that being able to send a Sr is not sensible made
sense to me. If Wolfram is happy with your patch so am I.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 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

[PATCH v2 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 if scl_pin is provided in platform data at least.

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>
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 1990ae8..b8605b4 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 v2 4/5] i2c: davinci: use bus recovery infrastructure

[Uwe Kleine-König]

Hello,


On Wed, Nov 26, 2014 at 03:59:52PM +0200, 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 if scl_pin is provided in platform data at least.
My German feeling for English tells me:
s/if//; s/at least//;

With that changed you can add my Ack.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 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: <devicetree@vger.kernel.org>
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 b8605b4..f7dae10f 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 v2 5/5] i2c: davinci: use ICPFUNC to toggle I2C as gpio for bus recovery

[Uwe Kleine-König]

Hello,

On Wed, Nov 26, 2014 at 03:59:53PM +0200, 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.
On what does it depend if this pfunc stuff works or not? Only the SoC,
or also on some board specific properties? Given the former using the
compatible string to detect its availability would be better. (In this
case also sorry, didn't consider this case when requesting the property
in the last round.)

The patch looks ok.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Grygorii Strashko]

On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
> On Wed, Nov 26, 2014 at 03:59:53PM +0200, 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.
> On what does it depend if this pfunc stuff works or not? Only the SoC,
> or also on some board specific properties?

SoC / set of SoCs. Also, similar feature is supported by OMAP and AM335x/AM437x SoCs
using I2C_SYSTEST register.

> Given the former using the
> compatible string to detect its availability would be better. (In this
> case also sorry, didn't consider this case when requesting the property
> in the last round.)
> 
> The patch looks ok.

regards,
-grygorii
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jan Lübbe]

On Mi, 2014-11-26 at 19:05 +0200, Grygorii Strashko wrote:
> On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
> > On Wed, Nov 26, 2014 at 03:59:53PM +0200, 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.
> > On what does it depend if this pfunc stuff works or not? Only the SoC,
> > or also on some board specific properties?
> 
> SoC / set of SoCs. Also, similar feature is supported by OMAP and AM335x/AM437x SoCs
> using I2C_SYSTEST register.
> 
> > Given the former using the
> > compatible string to detect its availability would be better. (In this
> > case also sorry, didn't consider this case when requesting the property
> > in the last round.)

I only stumbled across this after it was merged, with the additional
ti,has-pfunc property instead of using the compatible string (which
would be better for a soc-dependent feature). Can we still fix this?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Wolfram Sang]

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


> I only stumbled across this after it was merged, with the additional
> ti,has-pfunc property instead of using the compatible string (which
> would be better for a soc-dependent feature). Can we still fix this?

Makes sense to me.


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

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

[Uwe Kleine-König]

Hello,

On Tue, Jul 07, 2015 at 03:37:49PM +0200, Jan Lübbe wrote:
> On Mi, 2014-11-26 at 19:05 +0200, Grygorii Strashko wrote:
> > On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
> > > On Wed, Nov 26, 2014 at 03:59:53PM +0200, 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.
> > > On what does it depend if this pfunc stuff works or not? Only the SoC,
> > > or also on some board specific properties?
> > 
> > SoC / set of SoCs. Also, similar feature is supported by OMAP and AM335x/AM437x SoCs
> > using I2C_SYSTEST register.
> > 
> > > Given the former using the
> > > compatible string to detect its availability would be better. (In this
> > > case also sorry, didn't consider this case when requesting the property
> > > in the last round.)
> 
> I only stumbled across this after it was merged, with the additional
I also wonder how it came to the Reviewed-by tag for me. The last thing
that I said about the patch was "On what does it depend if this pfunc
stuff works or not? Only the SoC, or also on some board specific
properties?" (see above) and "the patch looks ok". IMHO this hardly
justifies to add the Reviewed-by tag for the next round. :-(

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Wolfram Sang]

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

On Tue, Jul 07, 2015 at 03:48:52PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jul 07, 2015 at 03:37:49PM +0200, Jan Lübbe wrote:
> > On Mi, 2014-11-26 at 19:05 +0200, Grygorii Strashko wrote:
> > > On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
> > > > On Wed, Nov 26, 2014 at 03:59:53PM +0200, 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.
> > > > On what does it depend if this pfunc stuff works or not? Only the SoC,
> > > > or also on some board specific properties?
> > > 
> > > SoC / set of SoCs. Also, similar feature is supported by OMAP and AM335x/AM437x SoCs
> > > using I2C_SYSTEST register.
> > > 
> > > > Given the former using the
> > > > compatible string to detect its availability would be better. (In this
> > > > case also sorry, didn't consider this case when requesting the property
> > > > in the last round.)
> > 
> > I only stumbled across this after it was merged, with the additional
> I also wonder how it came to the Reviewed-by tag for me. The last thing
> that I said about the patch was "On what does it depend if this pfunc
> stuff works or not? Only the SoC, or also on some board specific
> properties?" (see above) and "the patch looks ok". IMHO this hardly
> justifies to add the Reviewed-by tag for the next round. :-(

That needs to be discussed with Grygorii. I can't verify the correctness
of tags for every patch, although I do try to keep an eye on it...


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

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

[Grygorii Strashko]

On 07/07/2015 05:13 PM, Wolfram Sang wrote:
> On Tue, Jul 07, 2015 at 03:48:52PM +0200, Uwe Kleine-König wrote:

>> On Tue, Jul 07, 2015 at 03:37:49PM +0200, Jan Lübbe wrote:
>>> On Mi, 2014-11-26 at 19:05 +0200, Grygorii Strashko wrote:
>>>> On 11/26/2014 06:04 PM, Uwe Kleine-König wrote:
>>>>> On Wed, Nov 26, 2014 at 03:59:53PM +0200, 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.
>>>>> On what does it depend if this pfunc stuff works or not? Only the SoC,
>>>>> or also on some board specific properties?
>>>>
>>>> SoC / set of SoCs. Also, similar feature is supported by OMAP and AM335x/AM437x SoCs
>>>> using I2C_SYSTEST register.
>>>>
>>>>> Given the former using the
>>>>> compatible string to detect its availability would be better. (In this
>>>>> case also sorry, didn't consider this case when requesting the property
>>>>> in the last round.)
>>>
>>> I only stumbled across this after it was merged, with the additional
>> I also wonder how it came to the Reviewed-by tag for me. The last thing
>> that I said about the patch was "On what does it depend if this pfunc
>> stuff works or not? Only the SoC, or also on some board specific
>> properties?" (see above) and "the patch looks ok". IMHO this hardly
>> justifies to add the Reviewed-by tag for the next round. :-(
> 
> That needs to be discussed with Grygorii. I can't verify the correctness
> of tags for every patch, although I do try to keep an eye on it...
> 

Regarding "the patch looks ok" - sincerely sorry!
This is not the first time I've treated "looks good.." as Reviewed-by and I got no complaints before :(
Will take it into account.


Regarding technical comments:
OK. Seems I missed smth. or understood wrongly.
So, I'll say what's people usually saying here - Sorry for that :(
But, to be honest I don't feel guilty, because:
- v4 of these patches was merged finally
- that v4 missed >2 kernel releases
- you were added in "TO:" for all versions of these patches.

-- 
regards,
-grygorii

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