[PATCH 0/4] i2c: core: add generic GPIO bus recovery

[PATCH 0/4] i2c: core: add generic GPIO bus recovery

[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.

This patch series was previously sent as a RFC. Significant changes
since RFC:
- "recovery" pinctrl state marked as deprecared in bindings;
- move to "gpio" pinctrl state done after the call to prepare_recovery()
  callback;
- glitch protection when SDA gpio is taken at initialization;

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/busses/i2c-at91.h                 |   3 -
 drivers/i2c/i2c-core-base.c                   | 150 ++++++++++++++++--
 include/linux/i2c.h                           |  11 ++
 5 files changed, 163 insertions(+), 80 deletions(-)

-- 
2.25.1

[PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery

[Codrin Ciubotariu]

The I2C GPIO bus recovery properties consist of two GPIOS and one extra
pinctrl state ("gpio" or "recovery"). "recovery" pinctrl state is
considered deprecated and "gpio" should be used instead.
Not all are mandatory for recovery.

Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
---

Changes from RFC:
 - "recovery" pinctrl state marked as deprecated; updated description to
   reflect this;

 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..150a67da633d 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"(deprecated) state
+
 Required properties (per child device)
 --------------------------------------
 
-- 
2.25.1

[PATCH 2/4] i2c: core: add generic I2C GPIO recovery

[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";
	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>
---

Changes from RFC:
 - removed comment with "recovery" pinctrl state from description;
 - pinctrl state is changed to gpio after calling prepare_recovery()
   callback;
 - addressed requested changes on comments;
 - removed cleanup_pinctrl label from i2c_gpio_init_pinctrl_recovery()
 - added glitch protection for SDA line when its corresponding gpio is
   taken at initialization;

 drivers/i2c/i2c-core-base.c | 126 ++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  11 ++++
 2 files changed, 137 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 69217d2193da..cf0c5eb152e1 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>
@@ -181,6 +182,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 
 	if (bri->prepare_recovery)
 		bri->prepare_recovery(adap);
+	if (bri->pinctrl)
+		pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
 
 	/*
 	 * If we can set SDA, we will always create a STOP to ensure additional
@@ -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,125 @@ 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
+	 * 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;
+		}
+	}
+	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;
+		}
+	}
+
+	/* 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 should 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) {
+		/*
+		 * We have SCL. Pull SCL low and wait a bit so that SDA glitches
+		 * have no effect.
+		 */
+		gpiod_direction_output(bri->scl_gpiod, 0);
+		udelay(10);
+		gpiod = devm_gpiod_get(dev, "sda", GPIOD_IN);
+
+		/* Wait a bit in case of a SDA glitch, and then release SCL. */
+		udelay(10);
+		gpiod_direction_output(bri->scl_gpiod, 1);
+
+		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 +383,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 176bf971cbe0..93b6b698f150 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -606,6 +606,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);
@@ -622,6 +630,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

[PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs

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

Changes from RFC:
 - return -EINVAL if i2c_init_recovery() doesn't have the complete
   information;
 - 'else if' added when checking if i2c_generic_scl_recovery() is used;
 - moved i2c_init_recovery() before class-link creation; class-link
   cleanup removed;
 - moved debug print when the adapter is probed after call to
   i2c_init_recovery();

 drivers/i2c/i2c-core-base.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cf0c5eb152e1..99dbaead269e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -375,15 +375,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";
@@ -399,10 +400,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;
-	}
-
-	if (bri->recover_bus == i2c_generic_scl_recovery) {
+	} else if (bri->recover_bus == i2c_generic_scl_recovery) {
 		/* Generic SCL recovery */
 		if (!bri->set_scl || !bri->get_scl) {
 			err_str = "no {get|set}_scl() found";
@@ -414,10 +412,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 -EINVAL;
 }
 
 static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
@@ -1444,12 +1444,16 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res)
 		goto out_reg;
 
-	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
-
 	pm_runtime_no_callbacks(&adap->dev);
 	pm_suspend_ignore_children(&adap->dev, true);
 	pm_runtime_enable(&adap->dev);
 
+	res = i2c_init_recovery(adap);
+	if (res == -EPROBE_DEFER)
+		goto out_reg;
+
+	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
+
 #ifdef CONFIG_I2C_COMPAT
 	res = class_compat_create_link(i2c_adapter_compat_class, &adap->dev,
 				       adap->dev.parent);
@@ -1458,8 +1462,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 			 "Failed to create compatibility class link\n");
 #endif
 
-	i2c_init_recovery(adap);
-
 	/* create pre-declared device nodes */
 	of_i2c_register_devices(adap);
 	i2c_acpi_register_devices(adap);
-- 
2.25.1

[PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery

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

Changes from RFC:
 - none;

 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

Re: [PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery

[wsa]

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

On Tue, Aug 04, 2020 at 12:59:23PM +0300, Codrin Ciubotariu wrote:
> The I2C GPIO bus recovery properties consist of two GPIOS and one extra
> pinctrl state ("gpio" or "recovery"). "recovery" pinctrl state is
> considered deprecated and "gpio" should be used instead.
> Not all are mandatory for recovery.
> 
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Kept the alphabetical sorting, added a space before '(', took the
liberty to add Rob's review from last version (no significant change
IMO) and applied to for-next, thanks!


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

Re: [PATCH 2/4] i2c: core: add generic I2C GPIO recovery

[wsa]

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

On Tue, Aug 04, 2020 at 12:59:24PM +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";
> 	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>
> ---
Applied to for-next, thanks! Two minor change:

> +	/* 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");
> +	}

I inverted the logic here:

 294         /* for pinctrl state changes, we need all the information */
 295         if (bri->pins_default && bri->pins_gpio) {
 296                 dev_info(dev, "using pinctrl states for GPIO recovery");
 297         } else {
 298                 bri->pinctrl = NULL;
 299                 bri->pins_default = NULL;
 300                 bri->pins_gpio = NULL;
 301         }

I think it is a bit easier to read if the desired path is not in the
else case.


> + * @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.

Added "pinctrl" to "state of SCL/SDA" to make it clear.


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

Re: [PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs

[wsa]

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

On Tue, Aug 04, 2020 at 12:59:25PM +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>

Applied to for-next, thanks!


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

Re: [PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery

[wsa]

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

On Tue, Aug 04, 2020 at 12:59:26PM +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>

Applied to for-next, thanks!


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

Re: [PATCH 0/4] i2c: core: add generic GPIO bus recovery

[wsa]

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

Codrin, everyone

> This patch series was previously sent as a RFC. Significant changes
> since RFC:
> - "recovery" pinctrl state marked as deprecared in bindings;
> - move to "gpio" pinctrl state done after the call to prepare_recovery()
>   callback;
> - glitch protection when SDA gpio is taken at initialization;

Thanks for the fast update, now all merged for inclusion into 5.9. I
think it is really good, but to verify and double check, I think two
things would be even better..

One thing, I'll definately be doing is to add this feature to
i2c-sh_mobile.c and scope the results.

The other thing would be to convert the PXA driver and see if our
generic support can help their advanced use case or if we are missing
something. Codrin, do you have maybe time and interest to do that? That
would be awesome!

Happy hacking and kind regards,

   Wolfram


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

Re: Re: [PATCH 0/4] i2c: core: add generic GPIO bus recovery

[Codrin.Ciubotariu]

On 05.08.2020 11:52, wsa@kernel.org wrote:
> Codrin, everyone
> 
>> This patch series was previously sent as a RFC. Significant changes
>> since RFC:
>> - "recovery" pinctrl state marked as deprecared in bindings;
>> - move to "gpio" pinctrl state done after the call to prepare_recovery()
>>    callback;
>> - glitch protection when SDA gpio is taken at initialization;
> 
> Thanks for the fast update, now all merged for inclusion into 5.9. I
> think it is really good, but to verify and double check, I think two
> things would be even better..

Happy to help.

> 
> One thing, I'll definately be doing is to add this feature to
> i2c-sh_mobile.c and scope the results.
> 
> The other thing would be to convert the PXA driver and see if our
> generic support can help their advanced use case or if we are missing
> something. Codrin, do you have maybe time and interest to do that? That
> would be awesome!

Naturally, these would be the next steps. I can do this, sure, but I 
don't have the HW. I'll look for some development kits. If you have any 
recommendations, please let me know.

Best regards,
Codrin

Re: Re: [PATCH 0/4] i2c: core: add generic GPIO bus recovery

[wsa]

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


> > The other thing would be to convert the PXA driver and see if our
> > generic support can help their advanced use case or if we are missing
> > something. Codrin, do you have maybe time and interest to do that? That
> > would be awesome!
> 
> Naturally, these would be the next steps. I can do this, sure, but I 
> don't have the HW. I'll look for some development kits. If you have any 
> recommendations, please let me know.

No need for HW, I think. Just do a best effort conversion, double or
triple check it, and CC the patches also to Russell King. If he accepts
them, we are good.

Thanks for doing it!


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