pinctrl: cherryview: Stop clearing the GPIO_EN bit from chv_gpio_disable_free

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.

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@redhat.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

1 files changed, 2 insertions, 5 deletions

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 46f647d2e287..05044e323ea5 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -921,14 +921,11 @@ static void chv_gpio_disable_free(struct pinctrl_dev *pctldev,
 {
 	struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	unsigned long flags;
-	void __iomem *reg;
-	u32 value;
 
 	raw_spin_lock_irqsave(&chv_lock, flags);
 
-	reg = chv_padreg(pctrl, offset, CHV_PADCTRL0);
-	value = readl(reg) & ~CHV_PADCTRL0_GPIOEN;
-	chv_writel(value, reg);
+	if (!chv_pad_locked(pctrl, offset))
+		chv_gpio_clear_triggering(pctrl, offset);
 
 	raw_spin_unlock_irqrestore(&chv_lock, flags);
 }