Linux-i2c Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery
@ 2020-06-19 14:19 Codrin Ciubotariu
  2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Codrin Ciubotariu @ 2020-06-19 14:19 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-kernel, linux-arm-kernel
  Cc: wsa, robh+dt, ludovic.desroches, nicolas.ferre,
	alexandre.belloni, linux, kamel.bouhara, Codrin Ciubotariu

GPIO recovery has been added already for some I2C bus drivers, such as
imx, pxa and at91. These drivers use similar bindings and have more or
less the same code for recovery. For this reason, we aim to move the
GPIO bus recovery implementation to the I2C core so that other drivers
can benefit from it, with small modifications.
This implementation initializes the pinctrl states and the SDA/SCL
GPIOs based on common bindings. The I2C bus drivers can still use
different bindings or other particular recovery steps if needed.
The ugly part with this patch series is the handle of PROBE_DEFER
which could be returned by devm_gpiod_get(). This changes things a
little for i2c_register_adapter() and for this reason this step is
implemented in a sperate patch.
The at91 Microchip driver is the first to use this implementation,
with an AI to move the rest of the drivers in the following steps.

Codrin Ciubotariu (4):
  dt-binding: i2c: add generic properties for GPIO bus recovery
  i2c: core: add generic I2C GPIO recovery
  i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
  i2c: at91: Move to generic GPIO bus recovery

 Documentation/devicetree/bindings/i2c/i2c.txt |  10 ++
 drivers/i2c/busses/i2c-at91-master.c          |  69 +--------
 drivers/i2c/i2c-core-base.c                   | 139 +++++++++++++++++-
 include/linux/i2c.h                           |  11 ++
 4 files changed, 158 insertions(+), 71 deletions(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu
@ 2020-06-19 14:19 ` Codrin Ciubotariu
  2020-07-05 21:19   ` Wolfram Sang
  2020-07-15 19:21   ` Rob Herring
  2020-06-19 14:19 ` [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Codrin Ciubotariu
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 24+ messages in thread
From: Codrin Ciubotariu @ 2020-06-19 14:19 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-kernel, linux-arm-kernel
  Cc: wsa, robh+dt, ludovic.desroches, nicolas.ferre,
	alexandre.belloni, linux, kamel.bouhara, Codrin Ciubotariu

The I2C GPIO bus recovery properties consist of two GPIOS and one extra
pinctrl state ("gpio" or "recovery"). Not all are mandatory for recovery.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 438ae123107e..6a644a24fc1c 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -77,6 +77,16 @@ wants to support one of the below features, it should adapt these bindings.
 	this information to detect a stalled bus more reliably, for example.
 	Can not be combined with 'multi-master'.
 
+- scl-gpios
+	specify the gpio related to SCL pin. Used for GPIO bus recovery.
+
+- sda-gpios
+	specify the gpio related to SDA pin. Optional for GPIO bus recovery.
+
+- pinctrl
+	add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
+	recovery, call it "gpio" or "recovery" state
+
 Required properties (per child device)
 --------------------------------------
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
  2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu
  2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
@ 2020-06-19 14:19 ` Codrin Ciubotariu
  2020-08-02 16:54   ` Wolfram Sang
  2020-06-19 14:19 ` [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Codrin Ciubotariu
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Codrin Ciubotariu @ 2020-06-19 14:19 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-kernel, linux-arm-kernel
  Cc: wsa, robh+dt, ludovic.desroches, nicolas.ferre,
	alexandre.belloni, linux, kamel.bouhara, Codrin Ciubotariu

Multiple I2C bus drivers use similar bindings to obtain information needed
for I2C recovery. For example, for platforms using device-tree, the
properties look something like this:

&i2c {
	...
	pinctrl-names = "default", "gpio";
	// or pinctrl-names = "default", "recovery";
	pinctrl-0 = <&pinctrl_i2c_default>;
	pinctrl-1 = <&pinctrl_i2c_gpio>;
	sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
	scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
	...
}

For this reason, we can add this common initialization in the core. This
way, other I2C bus drivers will be able to support GPIO recovery just by
providing a pointer to platform's pinctrl and calling i2c_recover_bus()
when SDA is stuck low.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  11 ++++
 2 files changed, 130 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index d1f278f73011..4ee29fec4e93 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -32,6 +32,7 @@
 #include <linux/of_device.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/pm_wakeirq.h>
@@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 	int i = 0, scl = 1, ret = 0;
 
+	if (bri->pinctrl)
+		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
 	if (bri->prepare_recovery)
 		bri->prepare_recovery(adap);
 
@@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 
 	if (bri->unprepare_recovery)
 		bri->unprepare_recovery(adap);
+	if (bri->pinctrl)
+		pinctrl_select_state(bri->pinctrl, bri->pins_default);
 
 	return ret;
 }
@@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
 }
 EXPORT_SYMBOL_GPL(i2c_recover_bus);
 
+static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	struct device *dev = &adap->dev;
+	struct pinctrl *p = bri->pinctrl;
+
+	/*
+	 * we can't change states without pinctrl, so remove the states if
+	 * available
+	 */
+	if (!p) {
+		bri->pins_default = NULL;
+		bri->pins_gpio = NULL;
+		return;
+	}
+
+	if (!bri->pins_default) {
+		bri->pins_default = pinctrl_lookup_state(p,
+							 PINCTRL_STATE_DEFAULT);
+		if (IS_ERR(bri->pins_default)) {
+			dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
+			bri->pins_default = NULL;
+
+			goto cleanup_pinctrl;
+		}
+	}
+	if (!bri->pins_gpio) {
+		bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
+		if (IS_ERR(bri->pins_gpio))
+			bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
+
+		if (IS_ERR(bri->pins_gpio)) {
+			dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
+			bri->pins_gpio = NULL;
+
+			goto cleanup_pinctrl;
+		}
+	}
+
+cleanup_pinctrl:
+	/* for pinctrl state changes, we need all the information */
+	if (!bri->pins_default || !bri->pins_gpio) {
+		bri->pinctrl = NULL;
+		bri->pins_default = NULL;
+		bri->pins_gpio = NULL;
+	} else {
+		dev_info(dev, "using pinctrl states for GPIO recovery");
+	}
+}
+
+static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
+{
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	struct device *dev = &adap->dev;
+	struct gpio_desc *gpiod;
+	int ret = 0;
+
+	/* don't touch the recovery information if the driver is not using
+	 * generic SCL recovery
+	 */
+	if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
+		return 0;
+
+	/*
+	 * pins might be taken as GPIO, so we might as well inform pinctrl about
+	 * this and move the state to GPIO
+	 */
+	if (bri->pinctrl)
+		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
+
+	/*
+	 * if there is incomplete or no recovery information, see if generic
+	 * GPIO recovery is available
+	 */
+	if (!bri->scl_gpiod) {
+		gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
+		if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
+			ret  = -EPROBE_DEFER;
+			goto cleanup_pinctrl_state;
+		}
+		if (!IS_ERR(gpiod)) {
+			bri->scl_gpiod = gpiod;
+			bri->recover_bus = i2c_generic_scl_recovery;
+			dev_info(dev, "using generic GPIOs for recovery\n");
+		}
+	}
+
+	/* SDA GPIOD line is optional, so we care about DEFER only */
+	if (!bri->sda_gpiod) {
+		gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
+		if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
+			ret = -EPROBE_DEFER;
+			goto cleanup_pinctrl_state;
+		}
+		if (!IS_ERR(gpiod))
+			bri->sda_gpiod = gpiod;
+	}
+
+cleanup_pinctrl_state:
+	/* change the state of the pins back to their default state */
+	if (bri->pinctrl)
+		pinctrl_select_state(bri->pinctrl, bri->pins_default);
+
+	return ret;
+}
+
+static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
+{
+	i2c_gpio_init_pinctrl_recovery(adap);
+	return i2c_gpio_init_generic_recovery(adap);
+}
+
 static void i2c_init_recovery(struct i2c_adapter *adap)
 {
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
@@ -259,6 +376,8 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 	if (!bri)
 		return;
 
+	i2c_gpio_init_recovery(adap);
+
 	if (!bri->recover_bus) {
 		err_str = "no recover_bus() found";
 		goto err;
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index c10617bb980a..c62c9b48f719 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -611,6 +611,14 @@ struct i2c_timings {
  *	may configure padmux here for SDA/SCL line or something else they want.
  * @scl_gpiod: gpiod of the SCL line. Only required for GPIO recovery.
  * @sda_gpiod: gpiod of the SDA line. Only required for GPIO recovery.
+ * @pinctrl: pinctrl used by GPIO recovery to change the state of the I2C pins.
+ *      Optional.
+ * @pins_default: default state of SCL/SDA lines, when they are assigned to the
+ *      I2C bus. Optional. Populated internally for GPIO recovery, if a state with
+ *      the name PINCTRL_STATE_DEFAULT is found and pinctrl is valid.
+ * @pins_gpio: recovery state of SCL/SDA lines, when they are used as GPIOs.
+ *      Optional. Populated internally for GPIO recovery, if this state is called
+ *      "gpio" or "recovery" and pinctrl is valid.
  */
 struct i2c_bus_recovery_info {
 	int (*recover_bus)(struct i2c_adapter *adap);
@@ -627,6 +635,9 @@ struct i2c_bus_recovery_info {
 	/* gpio recovery */
 	struct gpio_desc *scl_gpiod;
 	struct gpio_desc *sda_gpiod;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_gpio;
 };
 
 int i2c_recover_bus(struct i2c_adapter *adap);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
  2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu
  2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
  2020-06-19 14:19 ` [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Codrin Ciubotariu
@ 2020-06-19 14:19 ` Codrin Ciubotariu
  2020-08-02 17:05   ` Wolfram Sang
  2020-06-19 14:19 ` [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery Codrin Ciubotariu
  2020-07-05 21:09 ` [RFC PATCH 0/4] i2c: core: add " Wolfram Sang
  4 siblings, 1 reply; 24+ messages in thread
From: Codrin Ciubotariu @ 2020-06-19 14:19 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-kernel, linux-arm-kernel
  Cc: wsa, robh+dt, ludovic.desroches, nicolas.ferre,
	alexandre.belloni, linux, kamel.bouhara, Codrin Ciubotariu

Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
-EPROBE_DEFER, so we should at least treat that. This ends up with
i2c_register_adapter() to be able to return -EPROBE_DEFER.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 4ee29fec4e93..f8d9f2048ca8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
 	return i2c_gpio_init_generic_recovery(adap);
 }
 
-static void i2c_init_recovery(struct i2c_adapter *adap)
+static int i2c_init_recovery(struct i2c_adapter *adap)
 {
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 	char *err_str;
 
 	if (!bri)
-		return;
+		return 0;
 
-	i2c_gpio_init_recovery(adap);
+	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
 
 	if (!bri->recover_bus) {
 		err_str = "no recover_bus() found";
@@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 			if (gpiod_get_direction(bri->sda_gpiod) == 0)
 				bri->set_sda = set_sda_gpio_value;
 		}
-		return;
+		return 0;
 	}
 
 	if (bri->recover_bus == i2c_generic_scl_recovery) {
@@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
 		}
 	}
 
-	return;
+	return 0;
  err:
 	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
 	adap->bus_recovery_info = NULL;
+
+	return 0;
 }
 
 static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
@@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 			 "Failed to create compatibility class link\n");
 #endif
 
-	i2c_init_recovery(adap);
+	res = i2c_init_recovery(adap);
+	if (res == -EPROBE_DEFER)
+		goto out_link;
 
 	/* create pre-declared device nodes */
 	of_i2c_register_devices(adap);
@@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 
 	return 0;
 
+out_link:
+#ifdef CONFIG_I2C_COMPAT
+	class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
+				 adap->dev.parent);
+#endif
 out_reg:
 	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery
  2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu
                   ` (2 preceding siblings ...)
  2020-06-19 14:19 ` [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Codrin Ciubotariu
@ 2020-06-19 14:19 ` Codrin Ciubotariu
  2020-08-02 17:08   ` Wolfram Sang
  2020-07-05 21:09 ` [RFC PATCH 0/4] i2c: core: add " Wolfram Sang
  4 siblings, 1 reply; 24+ messages in thread
From: Codrin Ciubotariu @ 2020-06-19 14:19 UTC (permalink / raw)
  To: linux-i2c, devicetree, linux-kernel, linux-arm-kernel
  Cc: wsa, robh+dt, ludovic.desroches, nicolas.ferre,
	alexandre.belloni, linux, kamel.bouhara, Codrin Ciubotariu

Make the Microchip at91 driver the first to use the generic GPIO bus
recovery support from the I2C core and discard the driver implementation.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---
 drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
 drivers/i2c/busses/i2c-at91.h        |  3 --
 2 files changed, 3 insertions(+), 69 deletions(-)

diff --git a/drivers/i2c/busses/i2c-at91-master.c b/drivers/i2c/busses/i2c-at91-master.c
index 363d540a8345..66864f9cf7ac 100644
--- a/drivers/i2c/busses/i2c-at91-master.c
+++ b/drivers/i2c/busses/i2c-at91-master.c
@@ -816,79 +816,16 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
 	return ret;
 }
 
-static void at91_prepare_twi_recovery(struct i2c_adapter *adap)
-{
-	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
-
-	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
-}
-
-static void at91_unprepare_twi_recovery(struct i2c_adapter *adap)
-{
-	struct at91_twi_dev *dev = i2c_get_adapdata(adap);
-
-	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
-}
-
 static int at91_init_twi_recovery_gpio(struct platform_device *pdev,
 				       struct at91_twi_dev *dev)
 {
 	struct i2c_bus_recovery_info *rinfo = &dev->rinfo;
 
-	dev->pinctrl = devm_pinctrl_get(&pdev->dev);
-	if (!dev->pinctrl || IS_ERR(dev->pinctrl)) {
+	rinfo->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (!rinfo->pinctrl || IS_ERR(rinfo->pinctrl)) {
 		dev_info(dev->dev, "can't get pinctrl, bus recovery not supported\n");
-		return PTR_ERR(dev->pinctrl);
+		return PTR_ERR(rinfo->pinctrl);
 	}
-
-	dev->pinctrl_pins_default = pinctrl_lookup_state(dev->pinctrl,
-							 PINCTRL_STATE_DEFAULT);
-	dev->pinctrl_pins_gpio = pinctrl_lookup_state(dev->pinctrl,
-						      "gpio");
-	if (IS_ERR(dev->pinctrl_pins_default) ||
-	    IS_ERR(dev->pinctrl_pins_gpio)) {
-		dev_info(&pdev->dev, "pinctrl states incomplete for recovery\n");
-		return -EINVAL;
-	}
-
-	/*
-	 * pins will be taken as GPIO, so we might as well inform pinctrl about
-	 * this and move the state to GPIO
-	 */
-	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_gpio);
-
-	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
-	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER)
-		return -EPROBE_DEFER;
-
-	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl",
-					  GPIOD_OUT_HIGH_OPEN_DRAIN);
-	if (PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER)
-		return -EPROBE_DEFER;
-
-	if (IS_ERR(rinfo->sda_gpiod) ||
-	    IS_ERR(rinfo->scl_gpiod)) {
-		dev_info(&pdev->dev, "recovery information incomplete\n");
-		if (!IS_ERR(rinfo->sda_gpiod)) {
-			gpiod_put(rinfo->sda_gpiod);
-			rinfo->sda_gpiod = NULL;
-		}
-		if (!IS_ERR(rinfo->scl_gpiod)) {
-			gpiod_put(rinfo->scl_gpiod);
-			rinfo->scl_gpiod = NULL;
-		}
-		pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
-		return -EINVAL;
-	}
-
-	/* change the state of the pins back to their default state */
-	pinctrl_select_state(dev->pinctrl, dev->pinctrl_pins_default);
-
-	dev_info(&pdev->dev, "using scl, sda for recovery\n");
-
-	rinfo->prepare_recovery = at91_prepare_twi_recovery;
-	rinfo->unprepare_recovery = at91_unprepare_twi_recovery;
-	rinfo->recover_bus = i2c_generic_scl_recovery;
 	dev->adapter.bus_recovery_info = rinfo;
 
 	return 0;
diff --git a/drivers/i2c/busses/i2c-at91.h b/drivers/i2c/busses/i2c-at91.h
index 7e7b4955ca7f..eae673ae786c 100644
--- a/drivers/i2c/busses/i2c-at91.h
+++ b/drivers/i2c/busses/i2c-at91.h
@@ -157,9 +157,6 @@ struct at91_twi_dev {
 	struct at91_twi_dma dma;
 	bool slave_detected;
 	struct i2c_bus_recovery_info rinfo;
-	struct pinctrl *pinctrl;
-	struct pinctrl_state *pinctrl_pins_default;
-	struct pinctrl_state *pinctrl_pins_gpio;
 #ifdef CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL
 	unsigned smr;
 	struct i2c_client *slave;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery
  2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu
                   ` (3 preceding siblings ...)
  2020-06-19 14:19 ` [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery Codrin Ciubotariu
@ 2020-07-05 21:09 ` Wolfram Sang
  4 siblings, 0 replies; 24+ messages in thread
From: Wolfram Sang @ 2020-07-05 21:09 UTC (permalink / raw)
  To: Codrin Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	ludovic.desroches, nicolas.ferre, alexandre.belloni, linux,
	kamel.bouhara


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

On Fri, Jun 19, 2020 at 05:19:00PM +0300, Codrin Ciubotariu wrote:
> GPIO recovery has been added already for some I2C bus drivers, such as
> imx, pxa and at91. These drivers use similar bindings and have more or
> less the same code for recovery. For this reason, we aim to move the
> GPIO bus recovery implementation to the I2C core so that other drivers
> can benefit from it, with small modifications.
> This implementation initializes the pinctrl states and the SDA/SCL
> GPIOs based on common bindings. The I2C bus drivers can still use
> different bindings or other particular recovery steps if needed.
> The ugly part with this patch series is the handle of PROBE_DEFER
> which could be returned by devm_gpiod_get(). This changes things a
> little for i2c_register_adapter() and for this reason this step is
> implemented in a sperate patch.
> The at91 Microchip driver is the first to use this implementation,
> with an AI to move the rest of the drivers in the following steps.

Thanks for doing this! On a first high level review, this looks very
good. I have one question in patch 1. From Tuesday on, I'll be
off-the-net for two weeks. Still I think it will be all good for the 5.9
merge window unless we encounter an unexpected problem. But as said, so
far it is looking good!


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
@ 2020-07-05 21:19   ` Wolfram Sang
  2020-07-24 19:39     ` Wolfram Sang
  2020-07-15 19:21   ` Rob Herring
  1 sibling, 1 reply; 24+ messages in thread
From: Wolfram Sang @ 2020-07-05 21:19 UTC (permalink / raw)
  To: Codrin Ciubotariu, Russell King
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	ludovic.desroches, nicolas.ferre, alexandre.belloni, linux,
	kamel.bouhara


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


> +- pinctrl
> +	add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> +	recovery, call it "gpio" or "recovery" state

I think we should stick with "gpio" only. That is what at91 and imx have
in their bindings. pxa uses "recovery" as a pinctrl state name but I
can't find any further use or documentation of that. PXA is not fully
converted to the best of my knowledge, so maybe it is no problem for PXA
to switch to "gpio", too? We should ask Russell King (cced).

Russell, do you object naming the pinctrl state for bus recovery in
the pxa i2c driver from "recovery" to "gpio"?


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
  2020-07-05 21:19   ` Wolfram Sang
@ 2020-07-15 19:21   ` Rob Herring
  1 sibling, 0 replies; 24+ messages in thread
From: Rob Herring @ 2020-07-15 19:21 UTC (permalink / raw)
  To: Codrin Ciubotariu
  Cc: linux-arm-kernel, alexandre.belloni, linux, ludovic.desroches,
	devicetree, linux-kernel, kamel.bouhara, wsa, linux-i2c, robh+dt

On Fri, 19 Jun 2020 17:19:01 +0300, Codrin Ciubotariu wrote:
> The I2C GPIO bus recovery properties consist of two GPIOS and one extra
> pinctrl state ("gpio" or "recovery"). Not all are mandatory for recovery.
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> ---
>  Documentation/devicetree/bindings/i2c/i2c.txt | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-07-05 21:19   ` Wolfram Sang
@ 2020-07-24 19:39     ` Wolfram Sang
  2020-07-24 20:52       ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 24+ messages in thread
From: Wolfram Sang @ 2020-07-24 19:39 UTC (permalink / raw)
  To: Codrin Ciubotariu, Russell King
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	ludovic.desroches, nicolas.ferre, alexandre.belloni,
	kamel.bouhara


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

On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> 
> > +- pinctrl
> > +	add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> > +	recovery, call it "gpio" or "recovery" state
> 
> I think we should stick with "gpio" only. That is what at91 and imx have
> in their bindings. pxa uses "recovery" as a pinctrl state name but I
> can't find any further use or documentation of that. PXA is not fully
> converted to the best of my knowledge, so maybe it is no problem for PXA
> to switch to "gpio", too? We should ask Russell King (cced).
> 
> Russell, do you object naming the pinctrl state for bus recovery in
> the pxa i2c driver from "recovery" to "gpio"?

No response, so far. I suggest now to support the "recovery" naming but
mark it as deprecated. Opinions?


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-07-24 19:39     ` Wolfram Sang
@ 2020-07-24 20:52       ` Russell King - ARM Linux admin
  2020-07-27 10:44         ` Codrin.Ciubotariu
  0 siblings, 1 reply; 24+ messages in thread
From: Russell King - ARM Linux admin @ 2020-07-24 20:52 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Codrin Ciubotariu, linux-i2c, devicetree, linux-kernel,
	linux-arm-kernel, robh+dt, ludovic.desroches, nicolas.ferre,
	alexandre.belloni, kamel.bouhara

On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> > 
> > > +- pinctrl
> > > +	add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> > > +	recovery, call it "gpio" or "recovery" state
> > 
> > I think we should stick with "gpio" only. That is what at91 and imx have
> > in their bindings. pxa uses "recovery" as a pinctrl state name but I
> > can't find any further use or documentation of that. PXA is not fully
> > converted to the best of my knowledge, so maybe it is no problem for PXA
> > to switch to "gpio", too? We should ask Russell King (cced).

Fully converted to what?  The generic handling where the i2c core layer
handles everything to do with recovery, including the switch between
modes?

i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
GPIO mode, and I don't see a generic driver doing that to avoid causing
any additional glitches on the bus.  Given the use case that this recovery
is targetted at, avoiding glitches is very important to keep.

> > Russell, do you object naming the pinctrl state for bus recovery in
> > the pxa i2c driver from "recovery" to "gpio"?
> 
> No response, so far. I suggest now to support the "recovery" naming but
> mark it as deprecated. Opinions?

I don't have a preference on the exact naming.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-07-24 20:52       ` Russell King - ARM Linux admin
@ 2020-07-27 10:44         ` Codrin.Ciubotariu
  2020-07-27 10:50           ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 24+ messages in thread
From: Codrin.Ciubotariu @ 2020-07-27 10:44 UTC (permalink / raw)
  To: linux, wsa
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni,
	kamel.bouhara

On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>
>>>> +- pinctrl
>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>> + recovery, call it "gpio" or "recovery" state
>>>
>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>> can't find any further use or documentation of that. PXA is not fully
>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>> to switch to "gpio", too? We should ask Russell King (cced).
> 
> Fully converted to what?  The generic handling where the i2c core layer
> handles everything to do with recovery, including the switch between
> modes?
> 
> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
> GPIO mode, and I don't see a generic driver doing that to avoid causing
> any additional glitches on the bus.  Given the use case that this recovery
> is targetted at, avoiding glitches is very important to keep.

Why is it not possbile to handle glitches in a generic way? I guess it 
depends on the pinctl, but we could treat a worst-case scenario to 
assure the switch between states is done properly.

> 
>>> Russell, do you object naming the pinctrl state for bus recovery in
>>> the pxa i2c driver from "recovery" to "gpio"?
>>
>> No response, so far. I suggest now to support the "recovery" naming but
>> mark it as deprecated. Opinions?
> 
> I don't have a preference on the exact naming.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-07-27 10:44         ` Codrin.Ciubotariu
@ 2020-07-27 10:50           ` Russell King - ARM Linux admin
  2020-07-30  9:00             ` Codrin.Ciubotariu
  0 siblings, 1 reply; 24+ messages in thread
From: Russell King - ARM Linux admin @ 2020-07-27 10:50 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: wsa, linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	robh+dt, Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni,
	kamel.bouhara

On Mon, Jul 27, 2020 at 10:44:57AM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
> >> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> >>>
> >>>> +- pinctrl
> >>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> >>>> + recovery, call it "gpio" or "recovery" state
> >>>
> >>> I think we should stick with "gpio" only. That is what at91 and imx have
> >>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
> >>> can't find any further use or documentation of that. PXA is not fully
> >>> converted to the best of my knowledge, so maybe it is no problem for PXA
> >>> to switch to "gpio", too? We should ask Russell King (cced).
> > 
> > Fully converted to what?  The generic handling where the i2c core layer
> > handles everything to do with recovery, including the switch between
> > modes?
> > 
> > i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
> > GPIO mode, and I don't see a generic driver doing that to avoid causing
> > any additional glitches on the bus.  Given the use case that this recovery
> > is targetted at, avoiding glitches is very important to keep.
> 
> Why is it not possbile to handle glitches in a generic way? I guess it 
> depends on the pinctl, but we could treat a worst-case scenario to 
> assure the switch between states is done properly.

Please look at how i2c-pxa switches between the two, and decide whether
the generic implementation can do the same.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-07-27 10:50           ` Russell King - ARM Linux admin
@ 2020-07-30  9:00             ` Codrin.Ciubotariu
  2020-08-03 14:16               ` Russell King - ARM Linux admin
  0 siblings, 1 reply; 24+ messages in thread
From: Codrin.Ciubotariu @ 2020-07-30  9:00 UTC (permalink / raw)
  To: linux
  Cc: wsa, linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	robh+dt, Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni,
	kamel.bouhara

On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Jul 27, 2020 at 10:44:57AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>>>
>>>>>> +- pinctrl
>>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>>>> + recovery, call it "gpio" or "recovery" state
>>>>>
>>>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>>>> can't find any further use or documentation of that. PXA is not fully
>>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>>>> to switch to "gpio", too? We should ask Russell King (cced).
>>>
>>> Fully converted to what?  The generic handling where the i2c core layer
>>> handles everything to do with recovery, including the switch between
>>> modes?
>>>
>>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
>>> GPIO mode, and I don't see a generic driver doing that to avoid causing
>>> any additional glitches on the bus.  Given the use case that this recovery
>>> is targetted at, avoiding glitches is very important to keep.
>>
>> Why is it not possbile to handle glitches in a generic way? I guess it
>> depends on the pinctl, but we could treat a worst-case scenario to
>> assure the switch between states is done properly.
> 
> Please look at how i2c-pxa switches between the two, and decide whether
> the generic implementation can do the same.

The handling of glitches from initialization looks generic to me. I see 
that there are specific clear/reset routines that are in the 
(un)prepare_recovery() callbacks, but these callbacks are not replaced 
by the generic i2c recovery and will still be used if given by the 
driver. The only thing the generic recovery does is to switch the pinmux 
state. We can discuss whether we want to change the pinmux state first 
or call the (un)preapre_recovery().
What I had in mind for the generic recovery was to just handle the 
common parts that follow the same bindings, which is getting the gpios 
and changing the pinmux states before recovering.

Best regards,
Codrin

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
  2020-06-19 14:19 ` [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Codrin Ciubotariu
@ 2020-08-02 16:54   ` Wolfram Sang
  2020-08-03 13:27     ` Codrin.Ciubotariu
  0 siblings, 1 reply; 24+ messages in thread
From: Wolfram Sang @ 2020-08-02 16:54 UTC (permalink / raw)
  To: Codrin Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	ludovic.desroches, nicolas.ferre, alexandre.belloni, linux,
	kamel.bouhara


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

On Fri, Jun 19, 2020 at 05:19:02PM +0300, Codrin Ciubotariu wrote:
> Multiple I2C bus drivers use similar bindings to obtain information needed
> for I2C recovery. For example, for platforms using device-tree, the
> properties look something like this:
> 
> &i2c {
> 	...
> 	pinctrl-names = "default", "gpio";
> 	// or pinctrl-names = "default", "recovery";
> 	pinctrl-0 = <&pinctrl_i2c_default>;
> 	pinctrl-1 = <&pinctrl_i2c_gpio>;
> 	sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
> 	scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> 	...
> }
> 
> For this reason, we can add this common initialization in the core. This
> way, other I2C bus drivers will be able to support GPIO recovery just by
> providing a pointer to platform's pinctrl and calling i2c_recover_bus()
> when SDA is stuck low.
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks, it looks a lot like what I had in mind!

> ---
>  drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  11 ++++
>  2 files changed, 130 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index d1f278f73011..4ee29fec4e93 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -32,6 +32,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/pm_wakeirq.h>
> @@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>  	int i = 0, scl = 1, ret = 0;
>  
> +	if (bri->pinctrl)
> +		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);

I think this should come after 'prepare_recovery'. It may be that
'prepare_recovery' already needs to select the pinctrl state to avoid a
glitch. In this version, there would be a glitch then. If we move it
down, the doubled 'pinctrl_select_state' would be a noop then.

>  	if (bri->prepare_recovery)
>  		bri->prepare_recovery(adap);
>  
> @@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>  
>  	if (bri->unprepare_recovery)
>  		bri->unprepare_recovery(adap);
> +	if (bri->pinctrl)
> +		pinctrl_select_state(bri->pinctrl, bri->pins_default);

Here it is OK and will still work with the PXA version which needs to
select the state on its own.

>  
>  	return ret;
>  }
> @@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
>  }
>  EXPORT_SYMBOL_GPL(i2c_recover_bus);
>  
> +static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	struct device *dev = &adap->dev;
> +	struct pinctrl *p = bri->pinctrl;
> +
> +	/*
> +	 * we can't change states without pinctrl, so remove the states if
> +	 * available

s/available/populated/ ?

> +	 */
> +	if (!p) {
> +		bri->pins_default = NULL;
> +		bri->pins_gpio = NULL;
> +		return;
> +	}
> +
> +	if (!bri->pins_default) {
> +		bri->pins_default = pinctrl_lookup_state(p,
> +							 PINCTRL_STATE_DEFAULT);
> +		if (IS_ERR(bri->pins_default)) {
> +			dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
> +			bri->pins_default = NULL;
> +
> +			goto cleanup_pinctrl;

I'd leave out the goto here. It is OK to check both parameters, I think.

> +		}
> +	}
> +	if (!bri->pins_gpio) {
> +		bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
> +		if (IS_ERR(bri->pins_gpio))
> +			bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
> +
> +		if (IS_ERR(bri->pins_gpio)) {
> +			dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
> +			bri->pins_gpio = NULL;
> +
> +			goto cleanup_pinctrl;

This goto is not needed...

> +		}
> +	}
> +
> +cleanup_pinctrl:

... and this label can go then. Also nicer to read, I'd say.

> +	/* for pinctrl state changes, we need all the information */
> +	if (!bri->pins_default || !bri->pins_gpio) {
> +		bri->pinctrl = NULL;
> +		bri->pins_default = NULL;
> +		bri->pins_gpio = NULL;
> +	} else {
> +		dev_info(dev, "using pinctrl states for GPIO recovery");
> +	}
> +}
> +
> +static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
> +{
> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
> +	struct device *dev = &adap->dev;
> +	struct gpio_desc *gpiod;
> +	int ret = 0;
> +
> +	/* don't touch the recovery information if the driver is not using
> +	 * generic SCL recovery
> +	 */

Not kernel comment style.

> +	if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
> +		return 0;

No need for the first condition. 'i2c_generic_scl_recovery' is
definately not NULL :)

> +
> +	/*
> +	 * pins might be taken as GPIO, so we might as well inform pinctrl about

s/might as well/should/

> +	 * this and move the state to GPIO
> +	 */
> +	if (bri->pinctrl)
> +		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> +
> +	/*
> +	 * if there is incomplete or no recovery information, see if generic
> +	 * GPIO recovery is available
> +	 */
> +	if (!bri->scl_gpiod) {
> +		gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> +		if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
> +			ret  = -EPROBE_DEFER;
> +			goto cleanup_pinctrl_state;
> +		}
> +		if (!IS_ERR(gpiod)) {
> +			bri->scl_gpiod = gpiod;
> +			bri->recover_bus = i2c_generic_scl_recovery;
> +			dev_info(dev, "using generic GPIOs for recovery\n");
> +		}
> +	}

I think this extra code from the PXA driver makes sense in case SDA was
released while we were executing this code:

1383         /*
1384          * We have SCL. Pull SCL low and wait a bit so that SDA glitches
1385          * have no effect.
1386          */
1387         gpiod_direction_output(bri->scl_gpiod, 0);
1388         udelay(10);
1389         bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
1390 
1391         /* Wait a bit in case of a SDA glitch, and then release SCL. */
1392         udelay(10);
1393         gpiod_direction_output(bri->scl_gpiod, 1);

> +
> +	/* SDA GPIOD line is optional, so we care about DEFER only */
> +	if (!bri->sda_gpiod) {
> +		gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
> +		if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
> +			ret = -EPROBE_DEFER;
> +			goto cleanup_pinctrl_state;
> +		}
> +		if (!IS_ERR(gpiod))
> +			bri->sda_gpiod = gpiod;
> +	}
> +
> +cleanup_pinctrl_state:
> +	/* change the state of the pins back to their default state */
> +	if (bri->pinctrl)
> +		pinctrl_select_state(bri->pinctrl, bri->pins_default);
> +
> +	return ret;
> +}
> +

Rest looks good! If you have some time for this now, I will make sure to
get it into 5.9. With these minor things fixed, this is good to go, me
thinks.


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
  2020-06-19 14:19 ` [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Codrin Ciubotariu
@ 2020-08-02 17:05   ` Wolfram Sang
  2020-08-03 15:33     ` Codrin.Ciubotariu
  0 siblings, 1 reply; 24+ messages in thread
From: Wolfram Sang @ 2020-08-02 17:05 UTC (permalink / raw)
  To: Codrin Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	ludovic.desroches, nicolas.ferre, alexandre.belloni, linux,
	kamel.bouhara


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

On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
> -EPROBE_DEFER, so we should at least treat that. This ends up with
> i2c_register_adapter() to be able to return -EPROBE_DEFER.
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> ---
>  drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 4ee29fec4e93..f8d9f2048ca8 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
>  	return i2c_gpio_init_generic_recovery(adap);
>  }
>  
> -static void i2c_init_recovery(struct i2c_adapter *adap)
> +static int i2c_init_recovery(struct i2c_adapter *adap)
>  {
>  	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>  	char *err_str;
>  
>  	if (!bri)
> -		return;
> +		return 0;
>  
> -	i2c_gpio_init_recovery(adap);
> +	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
>  
>  	if (!bri->recover_bus) {
>  		err_str = "no recover_bus() found";
> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>  			if (gpiod_get_direction(bri->sda_gpiod) == 0)
>  				bri->set_sda = set_sda_gpio_value;
>  		}
> -		return;
> +		return 0;

This is correct but I think the code flow is/was confusing. Can you drop
this 'return' and use 'else if' for the next code block? I think this is
more readable.

>  	}
>  
>  	if (bri->recover_bus == i2c_generic_scl_recovery) {
> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>  		}
>  	}
>  
> -	return;
> +	return 0;
>   err:
>  	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
>  	adap->bus_recovery_info = NULL;
> +
> +	return 0;

'return -EINVAL;' I'd suggest.

>  }
>  
>  static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  			 "Failed to create compatibility class link\n");
>  #endif
>  
> -	i2c_init_recovery(adap);
> +	res = i2c_init_recovery(adap);
> +	if (res == -EPROBE_DEFER)
> +		goto out_link;

Please move 'i2c_init_recovery' above the class-link creation. It
shouldn't make a difference but we can skip the extra label and the
ifdeffery.

>  
>  	/* create pre-declared device nodes */
>  	of_i2c_register_devices(adap);
> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  
>  	return 0;
>  
> +out_link:
> +#ifdef CONFIG_I2C_COMPAT
> +	class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
> +				 adap->dev.parent);
> +#endif
>  out_reg:
>  	init_completion(&adap->dev_released);
>  	device_unregister(&adap->dev);
> -- 
> 2.25.1
> 

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery
  2020-06-19 14:19 ` [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery Codrin Ciubotariu
@ 2020-08-02 17:08   ` Wolfram Sang
  2020-08-03 15:42     ` Codrin.Ciubotariu
  0 siblings, 1 reply; 24+ messages in thread
From: Wolfram Sang @ 2020-08-02 17:08 UTC (permalink / raw)
  To: Codrin Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	ludovic.desroches, nicolas.ferre, alexandre.belloni, linux,
	kamel.bouhara


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

On Fri, Jun 19, 2020 at 05:19:04PM +0300, Codrin Ciubotariu wrote:
> Make the Microchip at91 driver the first to use the generic GPIO bus
> recovery support from the I2C core and discard the driver implementation.
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> ---
>  drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
>  drivers/i2c/busses/i2c-at91.h        |  3 --

Nice diffstat! I will try using this new functionality with another
controller next week.


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
  2020-08-02 16:54   ` Wolfram Sang
@ 2020-08-03 13:27     ` Codrin.Ciubotariu
  2020-08-03 16:49       ` wsa
  0 siblings, 1 reply; 24+ messages in thread
From: Codrin.Ciubotariu @ 2020-08-03 13:27 UTC (permalink / raw)
  To: wsa
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni, linux,
	kamel.bouhara

On 02.08.2020 19:54, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:02PM +0300, Codrin Ciubotariu wrote:
>> Multiple I2C bus drivers use similar bindings to obtain information needed
>> for I2C recovery. For example, for platforms using device-tree, the
>> properties look something like this:
>>
>> &i2c {
>> 	...
>> 	pinctrl-names = "default", "gpio";
>> 	// or pinctrl-names = "default", "recovery";
>> 	pinctrl-0 = <&pinctrl_i2c_default>;
>> 	pinctrl-1 = <&pinctrl_i2c_gpio>;
>> 	sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
>> 	scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> 	...
>> }
>>
>> For this reason, we can add this common initialization in the core. This
>> way, other I2C bus drivers will be able to support GPIO recovery just by
>> providing a pointer to platform's pinctrl and calling i2c_recover_bus()
>> when SDA is stuck low.
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
> 
> Thanks, it looks a lot like what I had in mind!
> 
>> ---
>>   drivers/i2c/i2c-core-base.c | 119 ++++++++++++++++++++++++++++++++++++
>>   include/linux/i2c.h         |  11 ++++
>>   2 files changed, 130 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index d1f278f73011..4ee29fec4e93 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -32,6 +32,7 @@
>>   #include <linux/of_device.h>
>>   #include <linux/of.h>
>>   #include <linux/of_irq.h>
>> +#include <linux/pinctrl/consumer.h>
>>   #include <linux/pm_domain.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/pm_wakeirq.h>
>> @@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>   	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>   	int i = 0, scl = 1, ret = 0;
>>   
>> +	if (bri->pinctrl)
>> +		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
> 
> I think this should come after 'prepare_recovery'. It may be that
> 'prepare_recovery' already needs to select the pinctrl state to avoid a
> glitch. In this version, there would be a glitch then. If we move it
> down, the doubled 'pinctrl_select_state' would be a noop then.

Agree

> 
>>   	if (bri->prepare_recovery)
>>   		bri->prepare_recovery(adap);
>>   
>> @@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>   
>>   	if (bri->unprepare_recovery)
>>   		bri->unprepare_recovery(adap);
>> +	if (bri->pinctrl)
>> +		pinctrl_select_state(bri->pinctrl, bri->pins_default);
> 
> Here it is OK and will still work with the PXA version which needs to
> select the state on its own.
> 
>>   
>>   	return ret;
>>   }
>> @@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
>>   }
>>   EXPORT_SYMBOL_GPL(i2c_recover_bus);
>>   
>> +static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
>> +{
>> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +	struct device *dev = &adap->dev;
>> +	struct pinctrl *p = bri->pinctrl;
>> +
>> +	/*
>> +	 * we can't change states without pinctrl, so remove the states if
>> +	 * available
> 
> s/available/populated/ ?

OK

> 
>> +	 */
>> +	if (!p) {
>> +		bri->pins_default = NULL;
>> +		bri->pins_gpio = NULL;
>> +		return;
>> +	}
>> +
>> +	if (!bri->pins_default) {
>> +		bri->pins_default = pinctrl_lookup_state(p,
>> +							 PINCTRL_STATE_DEFAULT);
>> +		if (IS_ERR(bri->pins_default)) {
>> +			dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found for GPIO recovery\n");
>> +			bri->pins_default = NULL;
>> +
>> +			goto cleanup_pinctrl;
> 
> I'd leave out the goto here. It is OK to check both parameters, I think.

since default state is missing, the next check can be skipped, but I 
agree that removing the label makes things easier to read.

> 
>> +		}
>> +	}
>> +	if (!bri->pins_gpio) {
>> +		bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
>> +		if (IS_ERR(bri->pins_gpio))
>> +			bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
>> +
>> +		if (IS_ERR(bri->pins_gpio)) {
>> +			dev_dbg(dev, "no gpio or recovery state found for GPIO recovery\n");
>> +			bri->pins_gpio = NULL;
>> +
>> +			goto cleanup_pinctrl;
> 
> This goto is not needed...

right

> 
>> +		}
>> +	}
>> +
>> +cleanup_pinctrl:
> 
> ... and this label can go then. Also nicer to read, I'd say.

I will remove it.

> 
>> +	/* for pinctrl state changes, we need all the information */
>> +	if (!bri->pins_default || !bri->pins_gpio) {
>> +		bri->pinctrl = NULL;
>> +		bri->pins_default = NULL;
>> +		bri->pins_gpio = NULL;
>> +	} else {
>> +		dev_info(dev, "using pinctrl states for GPIO recovery");
>> +	}
>> +}
>> +
>> +static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
>> +{
>> +	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +	struct device *dev = &adap->dev;
>> +	struct gpio_desc *gpiod;
>> +	int ret = 0;
>> +
>> +	/* don't touch the recovery information if the driver is not using
>> +	 * generic SCL recovery
>> +	 */
> 
> Not kernel comment style.

Right, sorry about this. Will fix.

> 
>> +	if (bri->recover_bus && bri->recover_bus != i2c_generic_scl_recovery)
>> +		return 0;
> 
> No need for the first condition. 'i2c_generic_scl_recovery' is
> definately not NULL :)

It is not the same thing. Without the first check, it will return 0 if
bri->recover_bus is NULL, which is not what we want. If bri->recover_bus 
is NULL (and the pintrl states and gpios are in place) we can set 
recover_bus to i2c_generic_scl_recovery and use the generic recovery.

> 
>> +
>> +	/*
>> +	 * pins might be taken as GPIO, so we might as well inform pinctrl about
> 
> s/might as well/should/

OK

> 
>> +	 * this and move the state to GPIO
>> +	 */
>> +	if (bri->pinctrl)
>> +		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
>> +
>> +	/*
>> +	 * if there is incomplete or no recovery information, see if generic
>> +	 * GPIO recovery is available
>> +	 */
>> +	if (!bri->scl_gpiod) {
>> +		gpiod = devm_gpiod_get(dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
>> +		if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
>> +			ret  = -EPROBE_DEFER;
>> +			goto cleanup_pinctrl_state;
>> +		}
>> +		if (!IS_ERR(gpiod)) {
>> +			bri->scl_gpiod = gpiod;
>> +			bri->recover_bus = i2c_generic_scl_recovery;
>> +			dev_info(dev, "using generic GPIOs for recovery\n");
>> +		}
>> +	}
> 
> I think this extra code from the PXA driver makes sense in case SDA was
> released while we were executing this code:
> 
> 1383         /*
> 1384          * We have SCL. Pull SCL low and wait a bit so that SDA glitches
> 1385          * have no effect.
> 1386          */
> 1387         gpiod_direction_output(bri->scl_gpiod, 0);
> 1388         udelay(10);
> 1389         bri->sda_gpiod = devm_gpiod_get(dev, "sda", GPIOD_OUT_HIGH_OPEN_DRAIN);
> 1390
> 1391         /* Wait a bit in case of a SDA glitch, and then release SCL. */
> 1392         udelay(10);
> 1393         gpiod_direction_output(bri->scl_gpiod, 1);

I agree. I will add it.

> 
>> +
>> +	/* SDA GPIOD line is optional, so we care about DEFER only */
>> +	if (!bri->sda_gpiod) {
>> +		gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
>> +		if (PTR_ERR(gpiod) == -EPROBE_DEFER) {
>> +			ret = -EPROBE_DEFER;
>> +			goto cleanup_pinctrl_state;
>> +		}
>> +		if (!IS_ERR(gpiod))
>> +			bri->sda_gpiod = gpiod;
>> +	}
>> +
>> +cleanup_pinctrl_state:
>> +	/* change the state of the pins back to their default state */
>> +	if (bri->pinctrl)
>> +		pinctrl_select_state(bri->pinctrl, bri->pins_default);
>> +
>> +	return ret;
>> +}
>> +
> 
> Rest looks good! If you have some time for this now, I will make sure to
> get it into 5.9. With these minor things fixed, this is good to go, me
> thinks.
> 

I agree will all your suggestions, except with the removal of if 
(bri->recover_bus) . If you agree with this, I can send the next version 
and remove the RFC.

Thanks and best regards,
Codrin

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-07-30  9:00             ` Codrin.Ciubotariu
@ 2020-08-03 14:16               ` Russell King - ARM Linux admin
  2020-08-03 16:42                 ` Codrin.Ciubotariu
  0 siblings, 1 reply; 24+ messages in thread
From: Russell King - ARM Linux admin @ 2020-08-03 14:16 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: wsa, linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	robh+dt, Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni,
	kamel.bouhara

On Thu, Jul 30, 2020 at 09:00:36AM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Mon, Jul 27, 2020 at 10:44:57AM +0000, Codrin.Ciubotariu@microchip.com wrote:
> >> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
> >>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
> >>>>>
> >>>>>> +- pinctrl
> >>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
> >>>>>> + recovery, call it "gpio" or "recovery" state
> >>>>>
> >>>>> I think we should stick with "gpio" only. That is what at91 and imx have
> >>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
> >>>>> can't find any further use or documentation of that. PXA is not fully
> >>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
> >>>>> to switch to "gpio", too? We should ask Russell King (cced).
> >>>
> >>> Fully converted to what?  The generic handling where the i2c core layer
> >>> handles everything to do with recovery, including the switch between
> >>> modes?
> >>>
> >>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
> >>> GPIO mode, and I don't see a generic driver doing that to avoid causing
> >>> any additional glitches on the bus.  Given the use case that this recovery
> >>> is targetted at, avoiding glitches is very important to keep.
> >>
> >> Why is it not possbile to handle glitches in a generic way? I guess it
> >> depends on the pinctl, but we could treat a worst-case scenario to
> >> assure the switch between states is done properly.
> > 
> > Please look at how i2c-pxa switches between the two, and decide whether
> > the generic implementation can do the same.
> 
> The handling of glitches from initialization looks generic to me. I see 
> that there are specific clear/reset routines that are in the 
> (un)prepare_recovery() callbacks, but these callbacks are not replaced 
> by the generic i2c recovery and will still be used if given by the 
> driver. The only thing the generic recovery does is to switch the pinmux 
> state. We can discuss whether we want to change the pinmux state first 
> or call the (un)preapre_recovery().

Right, the key point i2c-pxa does is that on prepare:
- read the current state of the SCL and SDA lines and set the GPIO to
  reflect those values.
- then switch the pinmux state.

That must be preserved, otherwise if SCL is being held low by the I2C
master, and we switch to GPIO mode, SCL will be released.  So the
driver needs to be involved before the pinmux state is changed.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
  2020-08-02 17:05   ` Wolfram Sang
@ 2020-08-03 15:33     ` Codrin.Ciubotariu
  2020-08-03 16:59       ` wsa
  0 siblings, 1 reply; 24+ messages in thread
From: Codrin.Ciubotariu @ 2020-08-03 15:33 UTC (permalink / raw)
  To: wsa
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni, linux,
	kamel.bouhara

On 02.08.2020 20:05, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
>> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
>> -EPROBE_DEFER, so we should at least treat that. This ends up with
>> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> ---
>>   drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 4ee29fec4e93..f8d9f2048ca8 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter *adap)
>>   	return i2c_gpio_init_generic_recovery(adap);
>>   }
>>   
>> -static void i2c_init_recovery(struct i2c_adapter *adap)
>> +static int i2c_init_recovery(struct i2c_adapter *adap)
>>   {
>>   	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>>   	char *err_str;
>>   
>>   	if (!bri)
>> -		return;
>> +		return 0;
>>   
>> -	i2c_gpio_init_recovery(adap);
>> +	if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
>> +		return -EPROBE_DEFER;
>>   
>>   	if (!bri->recover_bus) {
>>   		err_str = "no recover_bus() found";
>> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>>   			if (gpiod_get_direction(bri->sda_gpiod) == 0)
>>   				bri->set_sda = set_sda_gpio_value;
>>   		}
>> -		return;
>> +		return 0;
> 
> This is correct but I think the code flow is/was confusing. Can you drop
> this 'return' and use 'else if' for the next code block? I think this is
> more readable.

Ok, it makes sense. Should I make a separate patch for this only?
One more question, should we keep:
if (!bri->set_sda && !bri->get_sda) {
	err_str = "either get_sda() or set_sda() needed";
	goto err;
}
?
Without {get/set}_sda we won't be able to generate stop commands and 
possibly check if the bus is free, but we can still generate the SCL 
clock pulses.

> 
>>   	}
>>   
>>   	if (bri->recover_bus == i2c_generic_scl_recovery) {
>> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>>   		}
>>   	}
>>   
>> -	return;
>> +	return 0;
>>    err:
>>   	dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
>>   	adap->bus_recovery_info = NULL;
>> +
>> +	return 0;
> 
> 'return -EINVAL;' I'd suggest.

OK

> 
>>   }
>>   
>>   static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
>> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>   			 "Failed to create compatibility class link\n");
>>   #endif
>>   
>> -	i2c_init_recovery(adap);
>> +	res = i2c_init_recovery(adap);
>> +	if (res == -EPROBE_DEFER)
>> +		goto out_link;
> 
> Please move 'i2c_init_recovery' above the class-link creation. It
> shouldn't make a difference but we can skip the extra label and the
> ifdeffery.

Ok. Perhaps I should also move the debug print with the registered 
adapter after calling i2c_init_recovery().

> 
>>   
>>   	/* create pre-declared device nodes */
>>   	of_i2c_register_devices(adap);
>> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>>   
>>   	return 0;
>>   
>> +out_link:
>> +#ifdef CONFIG_I2C_COMPAT
>> +	class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
>> +				 adap->dev.parent);
>> +#endif
>>   out_reg:
>>   	init_completion(&adap->dev_released);
>>   	device_unregister(&adap->dev);
>> -- 
>> 2.25.1
>>

Do you want me to integrate this patch in the previous one?

Best regards,
Codrin

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery
  2020-08-02 17:08   ` Wolfram Sang
@ 2020-08-03 15:42     ` Codrin.Ciubotariu
  2020-08-03 16:59       ` wsa
  0 siblings, 1 reply; 24+ messages in thread
From: Codrin.Ciubotariu @ 2020-08-03 15:42 UTC (permalink / raw)
  To: wsa
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni, linux,
	kamel.bouhara

On 02.08.2020 20:08, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:04PM +0300, Codrin Ciubotariu wrote:
>> Make the Microchip at91 driver the first to use the generic GPIO bus
>> recovery support from the I2C core and discard the driver implementation.
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> ---
>>   drivers/i2c/busses/i2c-at91-master.c | 69 ++--------------------------
>>   drivers/i2c/busses/i2c-at91.h        |  3 --
> 
> Nice diffstat! I will try using this new functionality with another
> controller next week.
> 

Thanks, this would be great! I tested this on a sam9x60, with the HW 
feature for the 9 pulses disabled, with a picky audio codec as I2C device.
Please let me know of the result.

Best regards,
Codrin

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
  2020-08-03 14:16               ` Russell King - ARM Linux admin
@ 2020-08-03 16:42                 ` Codrin.Ciubotariu
  0 siblings, 0 replies; 24+ messages in thread
From: Codrin.Ciubotariu @ 2020-08-03 16:42 UTC (permalink / raw)
  To: linux
  Cc: wsa, linux-i2c, devicetree, linux-kernel, linux-arm-kernel,
	robh+dt, Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni,
	kamel.bouhara

On 03.08.2020 17:16, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Jul 30, 2020 at 09:00:36AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Jul 27, 2020 at 10:44:57AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>>>> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>
>>>>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote:
>>>>>> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote:
>>>>>>>
>>>>>>>> +- pinctrl
>>>>>>>> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus
>>>>>>>> + recovery, call it "gpio" or "recovery" state
>>>>>>>
>>>>>>> I think we should stick with "gpio" only. That is what at91 and imx have
>>>>>>> in their bindings. pxa uses "recovery" as a pinctrl state name but I
>>>>>>> can't find any further use or documentation of that. PXA is not fully
>>>>>>> converted to the best of my knowledge, so maybe it is no problem for PXA
>>>>>>> to switch to "gpio", too? We should ask Russell King (cced).
>>>>>
>>>>> Fully converted to what?  The generic handling where the i2c core layer
>>>>> handles everything to do with recovery, including the switch between
>>>>> modes?
>>>>>
>>>>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and
>>>>> GPIO mode, and I don't see a generic driver doing that to avoid causing
>>>>> any additional glitches on the bus.  Given the use case that this recovery
>>>>> is targetted at, avoiding glitches is very important to keep.
>>>>
>>>> Why is it not possbile to handle glitches in a generic way? I guess it
>>>> depends on the pinctl, but we could treat a worst-case scenario to
>>>> assure the switch between states is done properly.
>>>
>>> Please look at how i2c-pxa switches between the two, and decide whether
>>> the generic implementation can do the same.
>>
>> The handling of glitches from initialization looks generic to me. I see
>> that there are specific clear/reset routines that are in the
>> (un)prepare_recovery() callbacks, but these callbacks are not replaced
>> by the generic i2c recovery and will still be used if given by the
>> driver. The only thing the generic recovery does is to switch the pinmux
>> state. We can discuss whether we want to change the pinmux state first
>> or call the (un)preapre_recovery().
> 
> Right, the key point i2c-pxa does is that on prepare:
> - read the current state of the SCL and SDA lines and set the GPIO to
>    reflect those values.
> - then switch the pinmux state.
> 
> That must be preserved, otherwise if SCL is being held low by the I2C
> master, and we switch to GPIO mode, SCL will be released.  So the
> driver needs to be involved before the pinmux state is changed.

I understand, and I admit that I didn't see this case. In my mind, the 
master would always be in (almost) a reset state before calling for SDA 
recovery, so it won't hold any lines.
These steps can't be generic, of course. Also, not all I2C masters have 
a way to show the state of its lines. For these masters, one idea would 
be to reset them before calling i2c_recover_bus()

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
> 


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
  2020-08-03 13:27     ` Codrin.Ciubotariu
@ 2020-08-03 16:49       ` wsa
  0 siblings, 0 replies; 24+ messages in thread
From: wsa @ 2020-08-03 16:49 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni, linux,
	kamel.bouhara


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

> I agree will all your suggestions, except with the removal of if 
> (bri->recover_bus) . If you agree with this, I can send the next version 
> and remove the RFC.

Yes, sure. That was a brown paper bag thingie from my side. Please go
ahead!


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
  2020-08-03 15:33     ` Codrin.Ciubotariu
@ 2020-08-03 16:59       ` wsa
  0 siblings, 0 replies; 24+ messages in thread
From: wsa @ 2020-08-03 16:59 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni, linux,
	kamel.bouhara


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


> > This is correct but I think the code flow is/was confusing. Can you drop
> > this 'return' and use 'else if' for the next code block? I think this is
> > more readable.
> 
> Ok, it makes sense. Should I make a separate patch for this only?

I am fine if this is included in this change.

> One more question, should we keep:
> if (!bri->set_sda && !bri->get_sda) {
> 	err_str = "either get_sda() or set_sda() needed";
> 	goto err;
> }
> ?
> Without {get/set}_sda we won't be able to generate stop commands and 
> possibly check if the bus is free, but we can still generate the SCL 
> clock pulses.

My gut feeling says we need to keep it. I can't recall the reason now
and want to send out this answer ASAP. Anyhow, this definately would be
a seperate patch. If you really want to, send a patch, and then I have
to think why we still need it ;)

> Ok. Perhaps I should also move the debug print with the registered 
> adapter after calling i2c_init_recovery().

Yes, makes sense.

> Do you want me to integrate this patch in the previous one?

Nope, please keep it seperate.


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: Re: [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery
  2020-08-03 15:42     ` Codrin.Ciubotariu
@ 2020-08-03 16:59       ` wsa
  0 siblings, 0 replies; 24+ messages in thread
From: wsa @ 2020-08-03 16:59 UTC (permalink / raw)
  To: Codrin.Ciubotariu
  Cc: linux-i2c, devicetree, linux-kernel, linux-arm-kernel, robh+dt,
	Ludovic.Desroches, Nicolas.Ferre, alexandre.belloni, linux,
	kamel.bouhara


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


> > Nice diffstat! I will try using this new functionality with another
> > controller next week.
> > 
> 
> Thanks, this would be great! I tested this on a sam9x60, with the HW 
> feature for the 9 pulses disabled, with a picky audio codec as I2C device.
> Please let me know of the result.

I'll surely let you know.


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

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, back to index

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 14:19 [RFC PATCH 0/4] i2c: core: add generic GPIO bus recovery Codrin Ciubotariu
2020-06-19 14:19 ` [RFC PATCH 1/4] dt-binding: i2c: add generic properties for " Codrin Ciubotariu
2020-07-05 21:19   ` Wolfram Sang
2020-07-24 19:39     ` Wolfram Sang
2020-07-24 20:52       ` Russell King - ARM Linux admin
2020-07-27 10:44         ` Codrin.Ciubotariu
2020-07-27 10:50           ` Russell King - ARM Linux admin
2020-07-30  9:00             ` Codrin.Ciubotariu
2020-08-03 14:16               ` Russell King - ARM Linux admin
2020-08-03 16:42                 ` Codrin.Ciubotariu
2020-07-15 19:21   ` Rob Herring
2020-06-19 14:19 ` [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery Codrin Ciubotariu
2020-08-02 16:54   ` Wolfram Sang
2020-08-03 13:27     ` Codrin.Ciubotariu
2020-08-03 16:49       ` wsa
2020-06-19 14:19 ` [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs Codrin Ciubotariu
2020-08-02 17:05   ` Wolfram Sang
2020-08-03 15:33     ` Codrin.Ciubotariu
2020-08-03 16:59       ` wsa
2020-06-19 14:19 ` [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery Codrin Ciubotariu
2020-08-02 17:08   ` Wolfram Sang
2020-08-03 15:42     ` Codrin.Ciubotariu
2020-08-03 16:59       ` wsa
2020-07-05 21:09 ` [RFC PATCH 0/4] i2c: core: add " Wolfram Sang

Linux-i2c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i2c/0 linux-i2c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i2c linux-i2c/ https://lore.kernel.org/linux-i2c \
		linux-i2c@vger.kernel.org
	public-inbox-index linux-i2c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-i2c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git