[PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Wolfram Sang]

When implementing bus recovery for the i2c-rcar driver, two problems were
encountered: 1) When reading the SDA bit, not the SDA status was returned but
the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
consider the bus free again. SCL/SDA high is not enough, and there is no other
way known to reset the internal logic otherwise.

The obvious solution to just send STOP after recovery makes sense for the
generic case, too, IMO. If we made a device release SDA again, and are about
start a new transfer using START, then we should terminate the previous state
properly with STOP. This may help with some devices and shouldn't create any
drawback AFAICS.

For this, we need to introduce a 'set_sda' callback to the recovery
infrastructure. Because of this and some other minor recovery core changes, I
CCed a few people working on I2C bus recovery recently. The first five patches
may be interesting for you, so your input is greatly appreciated. Also, testing
the new features with GPIO based recovery would be awesome to have.

This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
is documented here:

https://elinux.org/Tests:I2C-bus-recovery

A branch is available here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/rcar-i2c-recovery

Please let me know what you think.

Kind regards,

   Wolfram

Wolfram Sang (6):
  i2c: make kerneldoc about bus recovery more precise
  i2c: add identifier in declarations for i2c_bus_recovery
  i2c: add 'set_sda' to bus_recovery_info
  i2c: ensure SDA is released in recovery if SDA is controllable
  i2c: send STOP after successful bus recovery
  i2c: rcar: implement bus recovery

 drivers/i2c/busses/i2c-rcar.c | 54 +++++++++++++++++++++++++++++++++++++++++--
 drivers/i2c/i2c-core-base.c   | 25 +++++++++++++++++++-
 include/linux/i2c.h           | 20 +++++++++-------
 3 files changed, 88 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH 1/6] i2c: make kerneldoc about bus recovery more precise

[Wolfram Sang]

"Used internally" is vague. What it actually means is that those fields
are populated by the core if valid GPIOs are provided. Change the
comments to reflect that.

Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/i2c.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 5d7f3c1853ae4b..f8a9d81e911e52 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -545,12 +545,12 @@ struct i2c_timings {
  * @recover_bus: Recover routine. Either pass driver's recover_bus() routine, or
  *	i2c_generic_scl_recovery().
  * @get_scl: This gets current value of SCL line. Mandatory for generic SCL
- *      recovery. Used internally for generic GPIO recovery.
- * @set_scl: This sets/clears SCL line. Mandatory for generic SCL recovery. Used
- *      internally for generic GPIO recovery.
+ *      recovery. Populated internally for generic GPIO recovery.
+ * @set_scl: This sets/clears the SCL line. Mandatory for generic SCL recovery.
+ *      Populated internally for generic GPIO recovery.
  * @get_sda: This gets current value of SDA line. Optional for generic SCL
- *      recovery. Used internally, if sda_gpio is a valid GPIO, for generic GPIO
- *      recovery.
+ *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
+ *      GPIO recovery.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
-- 
2.11.0

[PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery

[Wolfram Sang]

No reason to have them undefined, so let's add them.

Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 include/linux/i2c.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f8a9d81e911e52..8a020617b4e780 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -561,9 +561,9 @@ struct i2c_timings {
 struct i2c_bus_recovery_info {
 	int (*recover_bus)(struct i2c_adapter *);
 
-	int (*get_scl)(struct i2c_adapter *);
-	void (*set_scl)(struct i2c_adapter *, int val);
-	int (*get_sda)(struct i2c_adapter *);
+	int (*get_scl)(struct i2c_adapter *adap);
+	void (*set_scl)(struct i2c_adapter *adap, int val);
+	int (*get_sda)(struct i2c_adapter *adap);
 
 	void (*prepare_recovery)(struct i2c_adapter *);
 	void (*unprepare_recovery)(struct i2c_adapter *);
-- 
2.11.0

[PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

This will be needed when we want to create STOP conditions, too, later.
Create the needed fields and populate them for the GPIO case if the GPIO
is set to output.

Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 11 ++++++++++-
 include/linux/i2c.h         |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index bb34a5d4113331..f4313801a0aaba 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -27,6 +27,7 @@
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/i2c-smbus.h>
@@ -147,6 +148,11 @@ static int get_sda_gpio_value(struct i2c_adapter *adap)
 	return gpiod_get_value_cansleep(adap->bus_recovery_info->sda_gpiod);
 }
 
+static void set_sda_gpio_value(struct i2c_adapter *adap, int val)
+{
+	gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val);
+}
+
 /*
  * We are generating clock pulses. ndelay() determines durating of clk pulses.
  * We will generate clock with rate 100 KHz and so duration of both clock levels
@@ -225,8 +231,11 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 	if (bri->scl_gpiod && bri->recover_bus == i2c_generic_scl_recovery) {
 		bri->get_scl = get_scl_gpio_value;
 		bri->set_scl = set_scl_gpio_value;
-		if (bri->sda_gpiod)
+		if (bri->sda_gpiod) {
 			bri->get_sda = get_sda_gpio_value;
+			if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
+				bri->set_sda = set_sda_gpio_value;
+		}
 		return;
 	}
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8a020617b4e780..c3e402e647791c 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -551,6 +551,9 @@ struct i2c_timings {
  * @get_sda: This gets current value of SDA line. Optional for generic SCL
  *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
  *      GPIO recovery.
+ * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
+ *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
+ *	recovery.
  * @prepare_recovery: This will be called before starting recovery. Platform may
  *	configure padmux here for SDA/SCL line or something else they want.
  * @unprepare_recovery: This will be called after completing recovery. Platform
@@ -564,6 +567,7 @@ struct i2c_bus_recovery_info {
 	int (*get_scl)(struct i2c_adapter *adap);
 	void (*set_scl)(struct i2c_adapter *adap, int val);
 	int (*get_sda)(struct i2c_adapter *adap);
+	void (*set_sda)(struct i2c_adapter *adap, int val);
 
 	void (*prepare_recovery)(struct i2c_adapter *);
 	void (*unprepare_recovery)(struct i2c_adapter *);
-- 
2.11.0

[PATCH 4/6] i2c: ensure SDA is released in recovery if SDA is controllable

[Wolfram Sang]

If we have a function to control SDA, we should ensure that SDA is not
held down by us. So, release the GPIO in this case.

Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index f4313801a0aaba..dae7f4a783dbe4 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -170,6 +170,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 		bri->prepare_recovery(adap);
 
 	bri->set_scl(adap, val);
+	if (bri->set_sda)
+		bri->set_sda(adap, 1);
 	ndelay(RECOVERY_NDELAY);
 
 	/*
-- 
2.11.0

[PATCH 5/6] i2c: send STOP after successful bus recovery

[Wolfram Sang]

If we managed to get a client release SDA again, send a STOP afterwards
to make sure we have a consistent state on the bus again.

Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index dae7f4a783dbe4..29d6606d653229 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -200,6 +200,18 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 	if (bri->get_sda && !bri->get_sda(adap))
 		ret = -EBUSY;
 
+	/* If all went well, send STOP for a sane bus state. */
+	if (ret == 0 && bri->set_sda) {
+		bri->set_scl(adap, 0);
+		ndelay(RECOVERY_NDELAY / 2);
+		bri->set_sda(adap, 0);
+		ndelay(RECOVERY_NDELAY / 2);
+		bri->set_scl(adap, 1);
+		ndelay(RECOVERY_NDELAY / 2);
+		bri->set_sda(adap, 1);
+		ndelay(RECOVERY_NDELAY / 2);
+	}
+
 	if (bri->unprepare_recovery)
 		bri->unprepare_recovery(adap);
 
-- 
2.11.0

[PATCH 6/6] i2c: rcar: implement bus recovery

[Wolfram Sang]

We can force levels of SCL and SDA, so we can use that for bus recovery.
Note that we cannot read SDA back, because we will only get the internal
state of the bus free detection.

Cc: Phil Reid <preid@electromag.com.au>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 54 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 8a2ae3e6c561c4..d4b7b5380c2983 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -132,6 +132,7 @@ struct rcar_i2c_priv {
 	int pos;
 	u32 icccr;
 	u32 flags;
+	u8 recovery_icmcr;	/* protected by adapter lock */
 	enum rcar_i2c_type devtype;
 	struct i2c_client *slave;
 
@@ -158,6 +159,46 @@ static u32 rcar_i2c_read(struct rcar_i2c_priv *priv, int reg)
 	return readl(priv->io + reg);
 }
 
+static int rcar_i2c_get_scl(struct i2c_adapter *adap)
+{
+	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	return !!(rcar_i2c_read(priv, ICMCR) & FSCL);
+
+};
+
+static void rcar_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	if (val)
+		priv->recovery_icmcr |= FSCL;
+	else
+		priv->recovery_icmcr &= ~FSCL;
+
+	rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
+};
+
+/* No get_sda, because the HW only reports its bus free logic, not SDA itself */
+
+static void rcar_i2c_set_sda(struct i2c_adapter *adap, int val)
+{
+	struct rcar_i2c_priv *priv = i2c_get_adapdata(adap);
+
+	if (val)
+		priv->recovery_icmcr |= FSDA;
+	else
+		priv->recovery_icmcr &= ~FSDA;
+
+	rcar_i2c_write(priv, ICMCR, priv->recovery_icmcr);
+};
+
+static struct i2c_bus_recovery_info rcar_i2c_bri = {
+	.get_scl = rcar_i2c_get_scl,
+	.set_scl = rcar_i2c_set_scl,
+	.set_sda = rcar_i2c_set_sda,
+	.recover_bus = i2c_generic_scl_recovery,
+};
 static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 {
 	/* reset master mode */
@@ -170,7 +211,7 @@ static void rcar_i2c_init(struct rcar_i2c_priv *priv)
 
 static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
 {
-	int i;
+	int i, ret;
 
 	for (i = 0; i < LOOP_TIMEOUT; i++) {
 		/* make sure that bus is not busy */
@@ -179,7 +220,15 @@ static int rcar_i2c_bus_barrier(struct rcar_i2c_priv *priv)
 		udelay(1);
 	}
 
-	return -EBUSY;
+	/* Waiting did not help, try to recover */
+	priv->recovery_icmcr = MDBS | OBPC | FSDA | FSCL;
+	ret = i2c_recover_bus(&priv->adap);
+
+	/* No failure when recovering, so check bus busy bit again */
+	if (ret == 0)
+		ret = (rcar_i2c_read(priv, ICMCR) & FSDA) ? -EBUSY : 0;
+
+	return ret;
 }
 
 static int rcar_i2c_clock_calculate(struct rcar_i2c_priv *priv, struct i2c_timings *t)
@@ -851,6 +900,7 @@ static int rcar_i2c_probe(struct platform_device *pdev)
 	adap->retries = 3;
 	adap->dev.parent = dev;
 	adap->dev.of_node = dev->of_node;
+	adap->bus_recovery_info = &rcar_i2c_bri;
 	i2c_set_adapdata(adap, priv);
 	strlcpy(adap->name, pdev->name, sizeof(adap->name));
 
-- 
2.11.0

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Phil Reid]

G'day Wolfram,


On 4/12/2017 20:36, Wolfram Sang wrote:
> This will be needed when we want to create STOP conditions, too, later.
> Create the needed fields and populate them for the GPIO case if the GPIO
> is set to output.
> 
> Cc: Phil Reid <preid@electromag.com.au>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>   drivers/i2c/i2c-core-base.c | 11 ++++++++++-
>   include/linux/i2c.h         |  4 ++++
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index bb34a5d4113331..f4313801a0aaba 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -27,6 +27,7 @@
>   #include <linux/delay.h>
>   #include <linux/err.h>
>   #include <linux/errno.h>
> +#include <linux/gpio.h>

I thought we weren't supposed to use this any more.
But it's the only place were gpiod_get_direction return flags are defined at present.
+cc Linus W.

>   #include <linux/gpio/consumer.h>
>   #include <linux/i2c.h>
>   #include <linux/i2c-smbus.h>
> @@ -147,6 +148,11 @@ static int get_sda_gpio_value(struct i2c_adapter *adap)
>   	return gpiod_get_value_cansleep(adap->bus_recovery_info->sda_gpiod);
>   }
>   
> +static void set_sda_gpio_value(struct i2c_adapter *adap, int val)
> +{
> +	gpiod_set_value_cansleep(adap->bus_recovery_info->sda_gpiod, val);
> +}
> +
>   /*
>    * We are generating clock pulses. ndelay() determines durating of clk pulses.
>    * We will generate clock with rate 100 KHz and so duration of both clock levels
> @@ -225,8 +231,11 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>   	if (bri->scl_gpiod && bri->recover_bus == i2c_generic_scl_recovery) {
>   		bri->get_scl = get_scl_gpio_value;
>   		bri->set_scl = set_scl_gpio_value;
> -		if (bri->sda_gpiod)
> +		if (bri->sda_gpiod) {
>   			bri->get_sda = get_sda_gpio_value;
> +			if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> +				bri->set_sda = set_sda_gpio_value;
> +		}
>   		return;
>   	}
>   
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 8a020617b4e780..c3e402e647791c 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -551,6 +551,9 @@ struct i2c_timings {
>    * @get_sda: This gets current value of SDA line. Optional for generic SCL
>    *      recovery. Populated internally, if sda_gpio is a valid GPIO, for generic
>    *      GPIO recovery.
> + * @set_sda: This sets/clears the SDA line. Optional for generic SCL recovery.
> + *	Populated internally, if sda_gpio is a valid GPIO, for generic GPIO
> + *	recovery.
>    * @prepare_recovery: This will be called before starting recovery. Platform may
>    *	configure padmux here for SDA/SCL line or something else they want.
>    * @unprepare_recovery: This will be called after completing recovery. Platform
> @@ -564,6 +567,7 @@ struct i2c_bus_recovery_info {
>   	int (*get_scl)(struct i2c_adapter *adap);
>   	void (*set_scl)(struct i2c_adapter *adap, int val);
>   	int (*get_sda)(struct i2c_adapter *adap);
> +	void (*set_sda)(struct i2c_adapter *adap, int val);
>   
>   	void (*prepare_recovery)(struct i2c_adapter *);
>   	void (*unprepare_recovery)(struct i2c_adapter *);
> 


-- 
Regards
Phil Reid

Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Phil Reid]

On 4/12/2017 20:36, Wolfram Sang wrote:
> When implementing bus recovery for the i2c-rcar driver, two problems were
> encountered: 1) When reading the SDA bit, not the SDA status was returned but
> the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
> consider the bus free again. SCL/SDA high is not enough, and there is no other
> way known to reset the internal logic otherwise.
> 
> The obvious solution to just send STOP after recovery makes sense for the
> generic case, too, IMO. If we made a device release SDA again, and are about
> start a new transfer using START, then we should terminate the previous state
> properly with STOP. This may help with some devices and shouldn't create any
> drawback AFAICS.
> 
> For this, we need to introduce a 'set_sda' callback to the recovery
> infrastructure. Because of this and some other minor recovery core changes, I
> CCed a few people working on I2C bus recovery recently. The first five patches
> may be interesting for you, so your input is greatly appreciated. Also, testing
> the new features with GPIO based recovery would be awesome to have.
> 
> This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
> is documented here:

Patches 1-5.
Tested-by: Phil Reid <preid@electromag.com.au>


-- 
Regards
Phil Reid

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Linus Walleij]

On Mon, Dec 4, 2017 at 1:36 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:

> This will be needed when we want to create STOP conditions, too, later.
> Create the needed fields and populate them for the GPIO case if the GPIO
> is set to output.
>
> Cc: Phil Reid <preid@electromag.com.au>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

(...)
>  #include <linux/errno.h>
> +#include <linux/gpio.h>

Please no.

> +                       if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> +                               bri->set_sda = set_sda_gpio_value;

Just compare it to zero. if (!gpiod_get_direction())

This flag is only for requesting GPIOs in the old API.
We didn't add a define in the new API, it seemed overengineered.

Yours,
Linus Walleij

Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Andrzej Hajda]

On 04.12.2017 13:36, Wolfram Sang wrote:
> When implementing bus recovery for the i2c-rcar driver, two problems were
> encountered: 1) When reading the SDA bit, not the SDA status was returned but
> the internal state of the "bus_is_busy" logic. 2) This logic needs a STOP to
> consider the bus free again. SCL/SDA high is not enough, and there is no other
> way known to reset the internal logic otherwise.
>
> The obvious solution to just send STOP after recovery makes sense for the
> generic case, too, IMO. If we made a device release SDA again, and are about
> start a new transfer using START, then we should terminate the previous state
> properly with STOP. This may help with some devices and shouldn't create any
> drawback AFAICS.
>
> For this, we need to introduce a 'set_sda' callback to the recovery
> infrastructure. Because of this and some other minor recovery core changes, I
> CCed a few people working on I2C bus recovery recently. The first five patches
> may be interesting for you, so your input is greatly appreciated. Also, testing
> the new features with GPIO based recovery would be awesome to have.
>
> This was tested on a Renesas Lager board (r8a7790/R-Car H2). My test procedure
> is documented here:
>
> https://elinux.org/Tests:I2C-bus-recovery
>
> A branch is available here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/rcar-i2c-recovery
>
> Please let me know what you think.

I do not feel myself experienced I2C developer, so please be merciful if
I write stupid things :)

>From what I see the whole recovery infrastructure partially duplicates
i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates
i2c-gpio driver.
Wouldn't be better to somehow reuse existing code? For example by adding
recovery callback in i2c_algorithm, and call i2c-gpio::recovery or
i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case).

I am not sure how much work it requires, and if it is worth it. Please
consider it as suggestion.

Regards
Andrzej

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Phil Reid]

On 5/12/2017 16:39, Linus Walleij wrote:
> On Mon, Dec 4, 2017 at 1:36 PM, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> 
>> This will be needed when we want to create STOP conditions, too, later.
>> Create the needed fields and populate them for the GPIO case if the GPIO
>> is set to output.
>>
>> Cc: Phil Reid <preid@electromag.com.au>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> (...)
>>   #include <linux/errno.h>
>> +#include <linux/gpio.h>
> 
> Please no.
> 
>> +                       if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
>> +                               bri->set_sda = set_sda_gpio_value;
> 
> Just compare it to zero. if (!gpiod_get_direction())
> 
> This flag is only for requesting GPIOs in the old API.
> We didn't add a define in the new API, it seemed overengineered.
> 

Perhaps the docs need updating?
as of 4.15-rc2 it says
  * Return GPIOF_DIR_IN or GPIOF_DIR_OUT, or an error code in case of error.

I can submit something if you wish.


-- 
Regards
Phil Reid

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

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


> Perhaps the docs need updating?
> as of 4.15-rc2 it says
>  * Return GPIOF_DIR_IN or GPIOF_DIR_OUT, or an error code in case of error.
> 
> I can submit something if you wish.

They surely need updating. I was relying on exactly this information.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Linus Walleij]

On Tue, Dec 5, 2017 at 10:57 AM, Phil Reid <preid@electromag.com.au> wrote:
> On 5/12/2017 16:39, Linus Walleij wrote:

>> Just compare it to zero. if (!gpiod_get_direction())
>>
>> This flag is only for requesting GPIOs in the old API.
>> We didn't add a define in the new API, it seemed overengineered.
>>
>
> Perhaps the docs need updating?
> as of 4.15-rc2 it says
>  * Return GPIOF_DIR_IN or GPIOF_DIR_OUT, or an error code in case of error.
>
> I can submit something if you wish.

Hit me.

Sorry for the incoherent information.

Yours,
Linus Walleij

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

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


> > +                       if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> > +                               bri->set_sda = set_sda_gpio_value;
> 
> Just compare it to zero. if (!gpiod_get_direction())

So, this?

	if (!gpiod_get_direction())
		bri->set_sda = set_sda_gpio_value;

That looks like "if i couldn't the direction, then...", or?

Comparing to plain numbers feels like magic values?

Maybe this should be named 'gpiod_is_direction_input()'?

Or?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Linus Walleij]

On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang <wsa@the-dreams.de> wrote:

>> Just compare it to zero. if (!gpiod_get_direction())
>
> So, this?
>
>         if (!gpiod_get_direction())
>                 bri->set_sda = set_sda_gpio_value;
>
> That looks like "if i couldn't the direction, then...", or?
>
> Comparing to plain numbers feels like magic values?

Hehe :)

> Maybe this should be named 'gpiod_is_direction_input()'?

Two statice inlines in <linux/gpio/consumer.h>
named

int gpiod_is output()
int gpiod_is_input()

should conform to Rusty Russell's API hierarchy.

Interested in fixing it, or should I?
I can almost ACK it before you write the patch.

Yours,
Linus Walleij

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

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


> Two statice inlines in <linux/gpio/consumer.h>
> named
> 
> int gpiod_is output()
> int gpiod_is_input()
> 
> should conform to Rusty Russell's API hierarchy.

Yes, sounds good!

> Interested in fixing it, or should I?

Please do. I don't want to add magic numbers to the kernel ;)
Could you provide an immutable branch then that I could pick up, because
my series will depend on it.

> I can almost ACK it before you write the patch.

:)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

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


> > Two statice inlines in <linux/gpio/consumer.h>
> > named
> > 
> > int gpiod_is output()
> > int gpiod_is_input()
> > 
> > should conform to Rusty Russell's API hierarchy.
> 
> Yes, sounds good!
> 
> > Interested in fixing it, or should I?
> 
> Please do. I don't want to add magic numbers to the kernel ;)

OK?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Linus Walleij]

On Thu, Dec 7, 2017 at 12:25 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> > Two statice inlines in <linux/gpio/consumer.h>
>> > named
>> >
>> > int gpiod_is output()
>> > int gpiod_is_input()
>> >
>> > should conform to Rusty Russell's API hierarchy.
>>
>> Yes, sounds good!
>>
>> > Interested in fixing it, or should I?
>>
>> Please do. I don't want to add magic numbers to the kernel ;)
>
> OK?

Just slow, sorry. Sent a patch now!

Yours,
Linus Walleij

Re: [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery

[Andy Shevchenko]

On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> No reason to have them undefined, so let's add them.
> 

>  	int (*recover_bus)(struct i2c_adapter *);
>  
> -	int (*get_scl)(struct i2c_adapter *);
> -	void (*set_scl)(struct i2c_adapter *, int val);
> -	int (*get_sda)(struct i2c_adapter *);
> +	int (*get_scl)(struct i2c_adapter *adap);
> +	void (*set_scl)(struct i2c_adapter *adap, int val);
> +	int (*get_sda)(struct i2c_adapter *adap);
>  
>  	void (*prepare_recovery)(struct i2c_adapter *);
>  	void (*unprepare_recovery)(struct i2c_adapter *);

It seems inconsistent with the rest of the members even from this
visible piece.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Andy Shevchenko]

On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> This will be needed when we want to create STOP conditions, too,
> later.
> Create the needed fields and populate them for the GPIO case if the
> GPIO
> is set to output.

> +#include <linux/gpio.h>
>  #include <linux/gpio/consumer.h>

I think it should be not done like this. (Yes, I know why you did that)

Perhaps Linus will accept a patch to move direction flags to some header
feasible via consumer.h.

Linus?

Or if we wish to hide:

static inline bool gpiod_is_direction_out() {}
static inline bool gpiod_is_direction_in() {}

> 	if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)

P.S. Yep, I saw some other upstreamed patch doing this kind of
comparison.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Andy Shevchenko]

On Tue, 2017-12-05 at 16:31 +0100, Linus Walleij wrote:
> On Tue, Dec 5, 2017 at 2:38 PM, Wolfram Sang <wsa@the-dreams.de>
> wrote:
> 


> Two statice inlines in <linux/gpio/consumer.h>
> named
> 
> int gpiod_is output()
> int gpiod_is_input()

Ha, just proposed similar.

> 
> should conform to Rusty Russell's API hierarchy.
> 
> Interested in fixing it, or should I?
> I can almost ACK it before you write the patch.

I vote for this type of API, and agree with Wolfram !_get_direction() is
confusing.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 0/6] i2c: send STOP after recovery; use it for i2c-rcar

[Wolfram Sang]

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


> I do not feel myself experienced I2C developer, so please be merciful if
> I write stupid things :)

Don't worry :) Thanks for sharing!

> From what I see the whole recovery infrastructure partially duplicates
> i2c_bit_algo/i2c_algo_bit_data algorithm, and GPIO recovery duplicates
> i2c-gpio driver.
> Wouldn't be better to somehow reuse existing code? For example by adding
> recovery callback in i2c_algorithm, and call i2c-gpio::recovery or
> i2c_bit_algo::recovery from rcar-i2c-recovery (in this particular case).

I understand what you mean but I also don't think it is a good idea in
practice. We can't save much by using i2c-gpio, because GPIO is only one
use-case, having custom set/get functions being another one, for
example. A bigger chance for reuse would, in deed, be i2c-algo-bit.c,
but there are some subtle differences and encoding them in the generic
functions would make them more unreadable IMHO. But the real argument
is: dependency hell. The core might be build-in, the rest is a module,
and things get more complicated. And I don't want to enforce this on
_every_ I2C user out there. So, given the really simple functions, I
think it makes sense to have them in the core.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] i2c: add identifier in declarations for i2c_bus_recovery

[Wolfram Sang]

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

On Wed, Dec 13, 2017 at 05:23:02PM +0200, Andy Shevchenko wrote:
> On Mon, 2017-12-04 at 13:36 +0100, Wolfram Sang wrote:
> > No reason to have them undefined, so let's add them.
> > 
> 
> >  	int (*recover_bus)(struct i2c_adapter *);
> >  
> > -	int (*get_scl)(struct i2c_adapter *);
> > -	void (*set_scl)(struct i2c_adapter *, int val);
> > -	int (*get_sda)(struct i2c_adapter *);
> > +	int (*get_scl)(struct i2c_adapter *adap);
> > +	void (*set_scl)(struct i2c_adapter *adap, int val);
> > +	int (*get_sda)(struct i2c_adapter *adap);
> >  
> >  	void (*prepare_recovery)(struct i2c_adapter *);
> >  	void (*unprepare_recovery)(struct i2c_adapter *);
> 
> It seems inconsistent with the rest of the members even from this
> visible piece.

Agreed. Either overlooked, or lost during rebase. Any way, will fix!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/6] i2c: add 'set_sda' to bus_recovery_info

[Wolfram Sang]

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

On Tue, Dec 05, 2017 at 09:39:48AM +0100, Linus Walleij wrote:
> On Mon, Dec 4, 2017 at 1:36 PM, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
> 
> > This will be needed when we want to create STOP conditions, too, later.
> > Create the needed fields and populate them for the GPIO case if the GPIO
> > is set to output.
> >
> > Cc: Phil Reid <preid@electromag.com.au>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > Cc: Claudio Foellmi <claudio.foellmi@ergon.ch>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> (...)
> >  #include <linux/errno.h>
> > +#include <linux/gpio.h>
> 
> Please no.
> 
> > +                       if (gpiod_get_direction(bri->sda_gpiod) == GPIOF_DIR_OUT)
> > +                               bri->set_sda = set_sda_gpio_value;
> 
> Just compare it to zero. if (!gpiod_get_direction())

Okay, for now, I'll do:

	/* FIXME: add proper flag once provided by GPIO core */
	if (gpiod_get_direction(bri->sda_gpiod) == 0)

Thanks!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]