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

[Vladimir Zapolskiy]

Hi Stefan,

On 09/27/2016 07:37 PM, Stefan Agner wrote:
> On 2016-09-27 05:12, Vladimir Zapolskiy wrote:
> > Hi Stefan,
> >
> > On 09/27/2016 03:26 AM, Stefan Agner wrote:
> > > If a GPIO gets freed after selecting a new pinctrl configuration
> > > the driver should not change pinctrl anymore. Otherwise this will
> > > likely lead to a unusable pin configuration for > the newly selected
> > > pinctrl.
> > >
> > > Signed-off-by: Stefan Agner <stefan@xxxxxxxx>
> > > ---
> > > This turned out to be problematic when using the I2C GPIO bus recovery
> > > functionality. After muxing back to I2C, the GPIO is being freed, which
> > > cased I2C to stop working completely.
> >
> > IMHO this recent "i.MX I2C GPIO bus recovery" feature is kind of a hack,
> > for example I believe it breaks I2C bus driver initialization on i.MX31
> > boards, where today there is no pinctrl driver at all.
>
> This has been addressed by Li Yang's patch, already in the next branch:
> https://lkml.org/lkml/2016/9/12/1161

Nice to know about it, thank you for the link.

> > IMHO something like I've partially described in the recent "Requesting as
> > a GPIO a pin already used through pinctrl" topic should be done here.
> > Could you consider to add another pinctrl-1 group with alternative GPIO
> > line mux/config settings to an i2c controller device node and apply it,
> > when you need a bus recovery? You may find references how this kind of
> > dynamic pinctrl management is done within mmc/sd subsystem.
>
> I don't quite understand, that is already the case. This is what device
> tree looks like to get the I2C recovery functionality:
>
> &i2c1 {
> 	pinctrl-names = "default", "gpio";
> 	pinctrl-0 = <&pinctrl_i2c1>;
> 	pinctrl-1 = <&pinctrl_i2c1_gpio>;
> 	scl-gpios = <&gpio1 6 GPIO_ACTIVE_HIGH>;
> 	sda-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> 	status = "okay";
> };

Great, then why do you experience a problem you've described?

> > > After muxing back to I2C, the GPIO is being freed, which cased I2C
> > > to stop working completely.

Release GPIO firstly, then mux back to I2C, that's the correct sequence
and I believe it obsoletes this change.

> > By the way did I miss a patch, which falls back to mux settings on
> > .gpio_disable_free call for non-Vybrid platforms?
>
> Currently only Vybrid makes use of the .gpio_request_enable... and so
> should .gpio_disable_free then.

So, I guess this is a change with a runtime difference for Vybrid only.

I find that it was initially done wrong that a number of Vybrid specific
hooks were added to the shared pinctrl-imx.c, in my opinion it is better
to make needed abstractions and move all code around SHARE_MUX_CONF_REG
to pinctrl-vf610.c:

./pinctrl-imx.c:216:		if (info->flags & SHARE_MUX_CONF_REG) {
./pinctrl-imx.c:317:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:357:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:382:	if (!(info->flags & SHARE_MUX_CONF_REG))
./pinctrl-imx.c:425:	if (info->flags & SHARE_MUX_CONF_REG)
./pinctrl-imx.c:450:		if (info->flags & SHARE_MUX_CONF_REG) {
./pinctrl-imx.c:534:	if (info->flags & SHARE_MUX_CONF_REG)
./pinctrl-imx.c:575:		if (info->flags & SHARE_MUX_CONF_REG) {

Nevertheless this is not directly related to the change.

--
With best wishes,
Vladimir