[PATCH] i2c: imx: move to generic GPIO recovery

[PATCH] i2c: imx: move to generic GPIO recovery

[Esben Haabendal]

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.

Note: The previous error messages about bus recovery not being supported is
dropped with this change. Given that it is perfectly possible to have platforms
where bus recovery works without pinctrl support, I happen to work on one such,
both error messages does not really make sense in those cases. And I don't see
how to know if this is the case or not.

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

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 88a053987403..d6ba93fc7fee 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,13 @@ 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;
-
-	i2c_imx->pinctrl = devm_pinctrl_get(&pdev->dev);
-	if (!i2c_imx->pinctrl) {
-		dev_info(&pdev->dev, "pinctrl unavailable, bus recovery not supported\n");
-		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_optional(&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;
-	}
+	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
 
-	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
-		rinfo->sda_gpiod ? ",sda" : "");
+	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
+	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

Re: [PATCH] i2c: imx: move to generic GPIO recovery

[Oleksij Rempel]

Hi,

On Thu, Jan 25, 2024 at 02:56:36PM +0100, Esben Haabendal wrote:
> 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.
> 
> Note: The previous error messages about bus recovery not being supported is
> dropped with this change. Given that it is perfectly possible to have platforms
> where bus recovery works without pinctrl support, I happen to work on one such,
> both error messages does not really make sense in those cases. And I don't see
> how to know if this is the case or not.
> 
> Signed-off-by: Esben Haabendal <esben@geanix.com>

Thank you for your work.

> ---
....
> +	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
>  
> -	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
> -		rinfo->sda_gpiod ? ",sda" : "");
> +	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
> +	if (IS_ERR(bri->pinctrl))
> +		return PTR_ERR(bri->pinctrl);

According to the commit message - "pinctrl becomes optional", but this
code stops probe if pinctrl will fail for one or another reason. I do
not see any place returning NULL on fail. Do I'm missing something?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] i2c: imx: move to generic GPIO recovery

[Esben Haabendal]

Oleksij Rempel <o.rempel@pengutronix.de> writes:

> ....
>> +	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
>>  
>> -	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
>> -		rinfo->sda_gpiod ? ",sda" : "");
>> +	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
>> +	if (IS_ERR(bri->pinctrl))
>> +		return PTR_ERR(bri->pinctrl);
>
> According to the commit message - "pinctrl becomes optional", but this
> code stops probe if pinctrl will fail for one or another reason. I do
> not see any place returning NULL on fail. Do I'm missing something?

The caller, i2c_imx_probe(), does only check for -EPROBE_DEFER, and
simply ignores any other error codes.

I assume it is on purpose, so any problems with initializing i2c
recovery does not cause complete failure of the i2c controller, which
seems sane to me.

/Esben

Re: [PATCH] i2c: imx: move to generic GPIO recovery

[Oleksij Rempel]

On Fri, Jan 26, 2024 at 08:38:08AM +0100, Esben Haabendal wrote:
> Oleksij Rempel <o.rempel@pengutronix.de> writes:
> 
> > ....
> >> +	struct i2c_bus_recovery_info *bri = &i2c_imx->rinfo;
> >>  
> >> -	dev_dbg(&pdev->dev, "using scl%s for recovery\n",
> >> -		rinfo->sda_gpiod ? ",sda" : "");
> >> +	bri->pinctrl = devm_pinctrl_get(&pdev->dev);
> >> +	if (IS_ERR(bri->pinctrl))
> >> +		return PTR_ERR(bri->pinctrl);
> >
> > According to the commit message - "pinctrl becomes optional", but this
> > code stops probe if pinctrl will fail for one or another reason. I do
> > not see any place returning NULL on fail. Do I'm missing something?
> 
> The caller, i2c_imx_probe(), does only check for -EPROBE_DEFER, and
> simply ignores any other error codes.
> 
> I assume it is on purpose, so any problems with initializing i2c
> recovery does not cause complete failure of the i2c controller, which
> seems sane to me.

Good point, this is what I overlooked :)

Acked-by: Oleksij Rempel <o.rempel@pengutronix.de>

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] i2c: imx: move to generic GPIO recovery

[Andi Shyti]

Hi

On Thu, 25 Jan 2024 14:56:36 +0100, Esben Haabendal wrote:
> 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.
> 
> [...]

Applied to i2c/i2c-host on

git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git

Thank you,
Andi

Patches applied
===============
[1/1] i2c: imx: move to generic GPIO recovery
      commit: 11f1357336cde9924da0b455e528f11fbd5011f4