Received-SPF: pass (google.com: domain of linux-kernel+bounces-4991-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 19 Dec 2023 10:58:42 +0100
From: Gregor Herburger <gregor.herburger@ew.tq-group.com>
To: esben@geanix.com
Cc: Oleksij Rempel <o.rempel@pengutronix.de>,
	Pengutronix Kernel Team <kernel@pengutronix.de>,
	Andi Shyti <andi.shyti@kernel.org>, Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	"linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux@ew.tq-group.com" <linux@ew.tq-group.com>
Subject: Re: Re: [PATCH] i2c: i2c-imx: allow bus recovery on non-muxable pads
Message-ID: <ZYFpUpP0Ll5c99py@herburgerg-w2>
References: <20231218-i2c-imx-recovery-v1-1-f69fa85b228c@ew.tq-group.com>
 <87frzyprhq.fsf@geanix.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87frzyprhq.fsf@geanix.com>

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
>