Re: [PATCH 2/2] pinctrl: cherryview: Stop clearing the GPIO_EN bit from chv_gpio_disable_free

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

On Tue, Dec 04, 2018 at 08:42:47PM +0100, Hans de Goede wrote:
> Clearing the GPIO_EN bit from chv_gpio_disable_free is a bad idea and
> pinctrl-cherryview.c is the only Intel pinctrl driver doing something
> like this.
> 
> Clearing the GPIO_EN bit means that if the pin was an output it is now
> effectively floating. The datasheet is not clear what happens to pull ups /
> downs in this case, but from testing it looks like these are disabled too,
> also floating input pins.
> 
> One example where this is causing issues is the soc_button_array input
> driver, this parses ACPI tables to create 2 platform devices for the
> gpio_keys input driver. The list of GPIOs is passed through struct
> gpio_keys_platform_data which uses gpio numbers rather then gpio_desc
> pointers.
> 
> The buttons handled by this drivers short the pin to ground when pressed
> and the volume buttons rely on the SoC's internal pull-up to pull the
> pin high when the button is not pressed.
> 
> To get the gpio number, the soc_button_array code calls gpiod_get_index
> followed by a desc_to_gpio call and then gpiod_put on the gpio_desc.
> This last call causes chv_gpio_disable_free to clear the GPIO_EN bit.
> 
> When the gpio_keys driver then loads next it gets the gpio_desc again
> causing the GPIO_EN bit to be set again and immediately reads the GPIO
> value which for the volume buttons reads 0 at this time, causing a spurious
> press of the volume buttons to get reported.

Nice detective work :)

> Putting a small delay between the gpio_desc request and the read fixes
> this, I assume that this is caused by the pull-up being temporarily
> disabled while the GPIO_EN bit is cleared as the powerbutton which also
> has its GPIO_EN bit cleared does not have this problem.
> 
> The soc_button_array code is not the only code temporarily requesting GPIOs
> the DWC3 PCI code also does this, to set the enable and reset GPIOs for the
> external phy, so that the code instantiating the ULPI phy can read the
> vendor and product ID registers from the phy. These GPIOs are released
> after this so that the PHY driver can claim and use them when it loads.
> 
> Another example of temporary GPIO usage would be a user-space set_gpio
> utility using the userspace ioctls to set a GPIO as output value 0 or 1,
> having the GPIO revert to floating as soon as this utility exits would
> certainly be unexpected behavior.
> 
> One argument in favor of clearing the GPIO_EN bit is if the GPIO is going
> to be muxed to another function after being released, but in that case
> chv_pinmux_set_mux() already clears it.
> 
> TL;DR: Clearing the GPIO_EN bit from is a bad idea, this commit therefor
> removes the clearing from chv_gpio_disable_free(), replacing it with code
> to clear the interrupt-trigger condition so that the GPIO stops generating
> interrupts when released, as pinctrl-baytrail.c does.
> 
> Note this commit adds a !chv_pad_locked() condition to the trigger clearing
> call, which the original GPIO_EN clearing code was missing.
> 
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>

Acked-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>