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

[Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>]

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