Re: [PATCH 5/5] pinctrl: cherryview: restore padctrl1 reg when gpio is disabled

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

On Thu, Jun 09, 2016 at 05:41:04PM +0100, Dan O'Donovan wrote:
> On 06/06/2016 11:40 AM, Mika Westerberg wrote:
> > On Thu, Jun 02, 2016 at 10:55:43PM +0100, Dan O'Donovan wrote:
> >> chv_gpio_request_enable() clears some bits in the padctrl1 register
> >> when GPIO mode is selected, but these bits are not restored by
> >> chv_gpio_disable_free() when GPIO mode is unselected and this can
> >> prevent other pin modes (e.g. I2C) from functioning correctly
> >> thereafter on that pin.  This patch adds saving/restoring of those
> >> bits.
> > Not sure how useful this is. If you want to mux I2C out of pins (even if
> > they were previosly configured as GPIO), you should call pinctrl to do
> > that (or let the core to do that automatically). Expecting that certain
> > (possibly unknown state) is restored does seem fragile to me.
> Perhaps my description of the change was misleading.  Consider this scenario:
>  * chv_pinmux_set_mux() is invoked at start-up (triggered by registering a pin map), and this sets a pin to an alternate mode (e.g. I2C, PWM, whatever).
>    - For some pins/modes, this may set the INVRXTX bits in the PADCTRL1 register.  These bits may have also been set early by the BIOS.
>  * some time later, the user exports the pin via sysfs for use as a GPIO, which triggers chv_gpio_request_enable()
>    - chv_gpio_request_enable() clears some bits in the PADCTRL1 register (including INVRXTX)
>  * later again, the user unexports the GPIO pin, which triggers chv_gpio_disable_free().
>    - this returns the pin to its previously-selected alternate mode (GPIO is disabled).  However, the INVRXTX bits are not restored here, so the alternate mode no longer works.

I think it is not quaranteed anywhere that the pin returns to its
previous state. Better to explicitly request muxing of the pin as
needed.

> >From the way the driver is written (possibly influenced by the hardware design), it appears that this can be a valid usage scenario - i.e. the pin can optionally be set to an alternate mode initially, but the user can bring it in/out of GPIO mode thereafter.  But, because the entry and exit from GPIO mode is not "symmetrical", it loses some configuration that was previously set for the alternate mode.
> 
> Admittedly, I've only encountered this scenario in validation testing - I'm not sure if there is a real use-case that would require this - so I can certainly drop this patch if you feel that a fix isn't warranted here.
> >
> > Also what happens if the pin was originally muxed as GPIO?
> This shouldn't make any difference, I think.  The change here is just
> attempting to make chv_gpio_disable_free() reverse the action of
> chv_gpio_request_enable() - by restoring PADCTRL register fields to
> their previous state (whatever that was) before
> chv_gpio_request_enable() was called.

If it ever was called in the first place.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html