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

[Dan O'Donovan <dan@emutex.com>]

On 06/10/2016 07:00 AM, Mika Westerberg wrote:
> 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.
Ah!  I had assumed that chv_gpio_disable_free() would not be called unless chv_gpio_request_enable() had been called previously, but it seems that's not a safe assumption.  I'll just drop that patch so.  Thanks for the feedback!