Messages in this thread Patch in this message | | | Subject | Re: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO | From | Vladimir Zapolskiy <> | Date | Wed, 28 Sep 2016 15:07:46 +0300 |
| |
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
| |