Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO

[Vladimir Zapolskiy]

On 09/28/2016 06:38 AM, Stefan Agner wrote:
> On 2016-09-27 19:00, Viresh Kumar wrote:
> > On 27-09-16, 12:34, Stefan Agner wrote:
> > > Added Viresh Kumar to the discussion, he implemented the I2C recovery
> > > functions.
> > >
> > > Yes, reordering the pinctrl/gpio_free calls would fix the problem too.
> > >
> > > However, I guess there is no explicit rule to that ("request/free GPIOs
> > > only when they are muxed as GPIO"), so I think of it that the issue is
> > > actually in the pinctrl driver.
> > >
> > > On top of that it is not entirely trivial to reorder the calls the way
> > > i2c_generic_gpio_recovery and i2c_generic_recovery are set up right now.
> >
> > AFAICT, these routines don't touch the muxing part at all. Perhaps it is done
> > internally by the GPIO calls. Can you please elaborate the exact change you are
> > hinting towards here ?
>
> The i.MX I2C driver touches the pinctrl in its prepare/unprepare
> callbacks.
>
> So, on a i.MX or Vybrid, the call chain looks like this:
>
> i2c_generic_gpio_recovery
>     -> i2c_get_gpios_for_recovery
>        -> gpio_request_one
>     -> i2c_generic_recovery
>        -> prepare_recovery (i2c_imx_prepare_recovery)
>           -> pinctrl_select_state [gpio]
>        -> unprepare_recovery (i2c_imx_unprepare_recovery)
>           -> pinctrl_select_state [default]
>     -> i2c_put_gpios_for_recovery
>        -> gpio_free

I would expect that the change below improves the situation, but I didn't
perform any tests and here the core change is governed by the accepted
i.MX i2c bus driver specific changes, thus conceptually it may be incorrect:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index da3a02ef4a31..3a4f59c3c3e6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -697,9 +697,6 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
 	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 	int i = 0, val = 1, ret = 0;
 
-	if (bri->prepare_recovery)
-		bri->prepare_recovery(adap);
-
 	bri->set_scl(adap, val);
 	ndelay(RECOVERY_NDELAY);
 
@@ -725,22 +722,34 @@ static int i2c_generic_recovery(struct i2c_adapter *adap)
 		ndelay(RECOVERY_NDELAY);
 	}
 
-	if (bri->unprepare_recovery)
-		bri->unprepare_recovery(adap);
-
 	return ret;
 }
 
 int i2c_generic_scl_recovery(struct i2c_adapter *adap)
 {
-	return i2c_generic_recovery(adap);
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
+	int ret;
+
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
+	ret = i2c_generic_recovery(adap);
+
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(i2c_generic_scl_recovery);
 
 int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
 {
+	struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
 	int ret;
 
+	if (bri->prepare_recovery)
+		bri->prepare_recovery(adap);
+
 	ret = i2c_get_gpios_for_recovery(adap);
 	if (ret)
 		return ret;
@@ -748,6 +757,9 @@ int i2c_generic_gpio_recovery(struct i2c_adapter *adap)
 	ret = i2c_generic_recovery(adap);
 	i2c_put_gpios_for_recovery(adap);
 
+	if (bri->unprepare_recovery)
+		bri->unprepare_recovery(adap);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(i2c_generic_gpio_recovery);


Alternatively you may consider to add contents of i2c_get_gpios_for_recovery()
into i2c_imx_prepare_recovery(), contents of i2c_put_gpios_for_recovery() into
i2c_imx_unprepare_recovery() in the i.MX I2C bus driver and change the recovery
callback .recover_bus to i2c_generic_scl_recovery().

> And for the pinctrl/GPIO driver of Vybrid this is actually a problem
> because gpio_free disables the output driver of the pad, and when that
> happens after the (I2C) default pinctrl state gets selected the pad is
> no longer active.

--
With best wishes,
Vladimir