lkml.org 
[lkml]   [2016]   [Sep]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] pinctrl: freescale: avoid overwriting pin config when freeing GPIO
From
Date


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

\
 
 \ /
  Last update: 2016-09-28 14:08    [W:0.100 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site