i2c: i2c-imx: allow bus recovery on non-muxable pads

Currently the i2c-imx driver assumes that for bus recovery the i2c pins
can be reconfigured as gpio via pinctrl.

But bus recovery can also be done with a gpio electrically connected to
i2c scl.

Modify i2c_imx_init_recovery_info to allow bus recovery on platforms
without pinctrl. In this case only use scl-gpio and
i2c_generic_scl_recovery.

Signed-off-by: Gregor Herburger <gregor.herburger@ew.tq-group.com>
---
 drivers/i2c/busses/i2c-imx.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)


---
base-commit: ceb2fe0d438644e1de06b9a6468a1fb8e2199c70
change-id: 20231218-i2c-imx-recovery-6da66f0381df

Best regards,

Gregor Herburger <gregor.herburger@ew.tq-group.com> writes:

> Currently the i2c-imx driver assumes that for bus recovery the i2c pins
> can be reconfigured as gpio via pinctrl.
>
> But bus recovery can also be done with a gpio electrically connected to
> i2c scl.
>
> Modify i2c_imx_init_recovery_info to allow bus recovery on platforms
> without pinctrl. In this case only use scl-gpio and
> i2c_generic_scl_recovery.

Why not move to use the generic GPIO recovery instead?  Will something
like this be able to cover at least the same scenarios as your change?

From 7e432496bae8c7ac35c21504bc1cd03f1dfef97f Mon Sep 17 00:00:00 2001
Message-ID: <7e432496bae8c7ac35c21504bc1cd03f1dfef97f.1702971634.git.esben@geanix.com>
From: Esben Haabendal <esben@geanix.com>
Date: Tue, 25 May 2021 11:25:44 +0200
Subject: [PATCH] i2c: imx: move to generic GPIO recovery

Starting with
commit 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
GPIO bus recovery is supported by the I2C core, so we can remove the
driver implementation and use that one instead.

As a nice side-effect, pinctrl becomes optional, allowing bus recovery on
LS1021A, which does not have such luxury, but can be wired up to use extra
fixed GPIO pins.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/i2c/busses/i2c-imx.c | 62 ++++--------------------------------
 1 file changed, 7 insertions(+), 55 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 1775a79aeba2..824d8bbb9be5 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -212,10 +212,6 @@ struct imx_i2c_struct {
 	const struct imx_i2c_hwdata	*hwdata;
 	struct i2c_bus_recovery_info rinfo;
 
-	struct pinctrl *pinctrl;
-	struct pinctrl_state *pinctrl_pins_default;
-	struct pinctrl_state *pinctrl_pins_gpio;
-
 	struct imx_i2c_dma	*dma;
 	struct i2c_client	*slave;
 	enum i2c_slave_event last_slave_event;
@@ -1357,24 +1353,6 @@ static int i2c_imx_xfer_atomic(struct i2c_adapter *adapter,
 	return result;
 }
 
-static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
-{
-	struct imx_i2c_struct *i2c_imx;
-
-	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
-
-	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
-}
-
-static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
-{
-	struct imx_i2c_struct *i2c_imx;
-
-	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
-
-	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);
-}
-
 /*
  * We switch SCL and SDA to their GPIO function and do some bitbanging
  * for bus recovery. These alternative pinmux settings can be
@@ -1385,43 +1363,17 @@ static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
 static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
 		struct platform_device *pdev)
 {
-	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
+	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
 
-	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
-	if (!i2c_imx->pinctrl) {
-		dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
+	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
+	if (PTR_ERR(bri->pinctrl) == -ENODEV) {
+		bri->pinctrl = NULL;
 		return 0;
 	}
-	if (IS_ERR(i2c_imx->pinctrl)) {
-		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
-		return PTR_ERR(i2c_imx->pinctrl);
-	}
-
-	i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
-			PINCTRL_STATE_DEFAULT);
-	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
-			"gpio");
-	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
-	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
-
-	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
-	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
-		return -EPROBE_DEFER;
-	} else if (IS_ERR(rinfo->sda_gpiod) ||
-		   IS_ERR(rinfo->scl_gpiod) ||
-		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
-		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
-		dev_dbg(&pdev->dev, "recovery information incomplete\n");
-		return 0;
-	}
-
-	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
-		rinfo->sda_gpiod ? ",sda" : "");
+	if (IS_ERR(bri->pinctrl))
+		return PTR_ERR(bri->pinctrl);
 
-	rinfo->prepare_recovery = i2c_imx_prepare_recovery;
-	rinfo->unprepare_recovery = i2c_imx_unprepare_recovery;
-	rinfo->recover_bus = i2c_generic_scl_recovery;
-	i2c_imx->adapter.bus_recovery_info = rinfo;
+	i2c_imx->adapter.bus_recovery_info = bri;
 
 	return 0;
 }

Hi Esben,

On Tue, Dec 19, 2023 at 07:43:21AM +0000, esben@geanix.com wrote:
> Gregor Herburger <gregor.herburger@ew.tq-group.com> writes:
> 
> > Currently the i2c-imx driver assumes that for bus recovery the i2c pins
> > can be reconfigured as gpio via pinctrl.
> >
> > But bus recovery can also be done with a gpio electrically connected to
> > i2c scl.
> >
> > Modify i2c_imx_init_recovery_info to allow bus recovery on platforms
> > without pinctrl. In this case only use scl-gpio and
> > i2c_generic_scl_recovery.
> 
> Why not move to use the generic GPIO recovery instead?  Will something
> like this be able to cover at least the same scenarios as your change?

I was not aware of the generic GPIO recovery functions. At a first
glance I think your solution should work. I will give it a try and test
it on hardware.

> 
> From 7e432496bae8c7ac35c21504bc1cd03f1dfef97f Mon Sep 17 00:00:00 2001
> Message-ID: <7e432496bae8c7ac35c21504bc1cd03f1dfef97f.1702971634.git.esben@geanix.com>
> From: Esben Haabendal <esben@geanix.com>
> Date: Tue, 25 May 2021 11:25:44 +0200
> Subject: [PATCH] i2c: imx: move to generic GPIO recovery
> 
> Starting with
> commit 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
> GPIO bus recovery is supported by the I2C core, so we can remove the
> driver implementation and use that one instead.
> 
> As a nice side-effect, pinctrl becomes optional, allowing bus recovery on
> LS1021A, which does not have such luxury, but can be wired up to use extra
> fixed GPIO pins.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 62 ++++--------------------------------
>  1 file changed, 7 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 1775a79aeba2..824d8bbb9be5 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -212,10 +212,6 @@ struct imx_i2c_struct {
>  	const struct imx_i2c_hwdata	*hwdata;
>  	struct i2c_bus_recovery_info rinfo;
>  
> -	struct pinctrl *pinctrl;
> -	struct pinctrl_state *pinctrl_pins_default;
> -	struct pinctrl_state *pinctrl_pins_gpio;
> -
>  	struct imx_i2c_dma	*dma;
>  	struct i2c_client	*slave;
>  	enum i2c_slave_event last_slave_event;
> @@ -1357,24 +1353,6 @@ static int i2c_imx_xfer_atomic(struct i2c_adapter *adapter,
>  	return result;
>  }
>  
> -static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
> -{
> -	struct imx_i2c_struct *i2c_imx;
> -
> -	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> -
> -	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
> -}
> -
> -static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
> -{
> -	struct imx_i2c_struct *i2c_imx;
> -
> -	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> -
> -	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);
> -}
> -
>  /*
>   * We switch SCL and SDA to their GPIO function and do some bitbanging
>   * for bus recovery. These alternative pinmux settings can be
> @@ -1385,43 +1363,17 @@ static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
>  static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>  		struct platform_device *pdev)
>  {
> -	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
> +	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
>  
> -	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> -	if (!i2c_imx->pinctrl) {
> -		dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
> +	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (PTR_ERR(bri->pinctrl) == -ENODEV) {
> +		bri->pinctrl = NULL;
>  		return 0;
>  	}
> -	if (IS_ERR(i2c_imx->pinctrl)) {
> -		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> -		return PTR_ERR(i2c_imx->pinctrl);
> -	}
> -
> -	i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
> -			PINCTRL_STATE_DEFAULT);
> -	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> -			"gpio");
> -	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> -	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> -
> -	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
> -	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
> -		return -EPROBE_DEFER;
> -	} else if (IS_ERR(rinfo->sda_gpiod) ||
> -		   IS_ERR(rinfo->scl_gpiod) ||
> -		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
> -		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
> -		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> -		return 0;
> -	}
> -
> -	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
> -		rinfo->sda_gpiod ? ",sda" : "");
> +	if (IS_ERR(bri->pinctrl))
> +		return PTR_ERR(bri->pinctrl);
>  
> -	rinfo->prepare_recovery = i2c_imx_prepare_recovery;
> -	rinfo->unprepare_recovery = i2c_imx_unprepare_recovery;
> -	rinfo->recover_bus = i2c_generic_scl_recovery;
> -	i2c_imx->adapter.bus_recovery_info = rinfo;
> +	i2c_imx->adapter.bus_recovery_info = bri;
>  
>  	return 0;
>  }
> -- 
> 2.43.0
> 
> 
> /Esben
>

Gregor Herburger <gregor.herburger@ew.tq-group.com> writes:

> Hi Esben,
>
>> Why not move to use the generic GPIO recovery instead?  Will something
>> like this be able to cover at least the same scenarios as your change?
>
> I was not aware of the generic GPIO recovery functions. At a first
> glance I think your solution should work. I will give it a try and test
> it on hardware.

Great. Let me know if it works for me. If it does, I guess I should send
the patch to the list in it's own thread.

/Esben

Hi Esben,

I had another look at your patch and tested it on a LX2160a SoC without
pinctrl. I agree that using the generic GPIO recovery you suggested is
the better solution.

To make your solution work I had to make a small change (see below).

On Tue, Dec 19, 2023 at 07:43:21AM +0000, esben@geanix.com wrote:
> Why not move to use the generic GPIO recovery instead?  Will something
> like this be able to cover at least the same scenarios as your change?
> 
> From 7e432496bae8c7ac35c21504bc1cd03f1dfef97f Mon Sep 17 00:00:00 2001
> Message-ID: <7e432496bae8c7ac35c21504bc1cd03f1dfef97f.1702971634.git.esben@geanix.com>
> From: Esben Haabendal <esben@geanix.com>
> Date: Tue, 25 May 2021 11:25:44 +0200
> Subject: [PATCH] i2c: imx: move to generic GPIO recovery
> 
> Starting with
> commit 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
> GPIO bus recovery is supported by the I2C core, so we can remove the
> driver implementation and use that one instead.
> 
> As a nice side-effect, pinctrl becomes optional, allowing bus recovery on
> LS1021A, which does not have such luxury, but can be wired up to use extra
> fixed GPIO pins.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/i2c/busses/i2c-imx.c | 62 ++++--------------------------------
>  1 file changed, 7 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index 1775a79aeba2..824d8bbb9be5 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -212,10 +212,6 @@ struct imx_i2c_struct {
>  	const struct imx_i2c_hwdata	*hwdata;
>  	struct i2c_bus_recovery_info rinfo;
>  
> -	struct pinctrl *pinctrl;
> -	struct pinctrl_state *pinctrl_pins_default;
> -	struct pinctrl_state *pinctrl_pins_gpio;
> -
>  	struct imx_i2c_dma	*dma;
>  	struct i2c_client	*slave;
>  	enum i2c_slave_event last_slave_event;
> @@ -1357,24 +1353,6 @@ static int i2c_imx_xfer_atomic(struct i2c_adapter *adapter,
>  	return result;
>  }
>  
> -static void i2c_imx_prepare_recovery(struct i2c_adapter *adap)
> -{
> -	struct imx_i2c_struct *i2c_imx;
> -
> -	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> -
> -	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_gpio);
> -}
> -
> -static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
> -{
> -	struct imx_i2c_struct *i2c_imx;
> -
> -	i2c_imx = container_of(adap, struct imx_i2c_struct, adapter);
> -
> -	pinctrl_select_state(i2c_imx->pinctrl, i2c_imx->pinctrl_pins_default);
> -}
> -
>  /*
>   * We switch SCL and SDA to their GPIO function and do some bitbanging
>   * for bus recovery. These alternative pinmux settings can be
> @@ -1385,43 +1363,17 @@ static void i2c_imx_unprepare_recovery(struct i2c_adapter *adap)
>  static int i2c_imx_init_recovery_info(struct imx_i2c_struct *i2c_imx,
>  		struct platform_device *pdev)
>  {
> -	struct i2c_bus_recovery_info *rinfo = &i2c_imx->rinfo;
> +	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
>  
> -	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
> -	if (!i2c_imx->pinctrl) {
> -		dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
> +	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (PTR_ERR(bri->pinctrl) == -ENODEV) {
> +		bri->pinctrl = NULL;
>  		return 0;
Should not return here to allow setting of adapter.bus_recovery_info
later.

>  	}
> -	if (IS_ERR(i2c_imx->pinctrl)) {
> -		dev_info(&pdev->dev, "can't get pinctrl, bus recovery not supported\n");
> -		return PTR_ERR(i2c_imx->pinctrl);
> -	}
> -
> -	i2c_imx->pinctrl_pins_default = pinctrl_lookup_state(i2c_imx->pinctrl,
> -			PINCTRL_STATE_DEFAULT);
> -	i2c_imx->pinctrl_pins_gpio = pinctrl_lookup_state(i2c_imx->pinctrl,
> -			"gpio");
> -	rinfo->sda_gpiod = devm_gpiod_get(&pdev->dev, "sda", GPIOD_IN);
> -	rinfo->scl_gpiod = devm_gpiod_get(&pdev->dev, "scl", GPIOD_OUT_HIGH_OPEN_DRAIN);
> -
> -	if (PTR_ERR(rinfo->sda_gpiod) == -EPROBE_DEFER ||
> -	    PTR_ERR(rinfo->scl_gpiod) == -EPROBE_DEFER) {
> -		return -EPROBE_DEFER;
> -	} else if (IS_ERR(rinfo->sda_gpiod) ||
> -		   IS_ERR(rinfo->scl_gpiod) ||
> -		   IS_ERR(i2c_imx->pinctrl_pins_default) ||
> -		   IS_ERR(i2c_imx->pinctrl_pins_gpio)) {
> -		dev_dbg(&pdev->dev, "recovery information incomplete\n");
> -		return 0;
> -	}
> -
> -	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
> -		rinfo->sda_gpiod ? ",sda" : "");
> +	if (IS_ERR(bri->pinctrl))
> +		return PTR_ERR(bri->pinctrl);
>  
> -	rinfo->prepare_recovery = i2c_imx_prepare_recovery;
> -	rinfo->unprepare_recovery = i2c_imx_unprepare_recovery;
> -	rinfo->recover_bus = i2c_generic_scl_recovery;
> -	i2c_imx->adapter.bus_recovery_info = rinfo;
> +	i2c_imx->adapter.bus_recovery_info = bri;
>  
>  	return 0;
>  }

Best regards,
Gregor