| Message ID | 20181204194247.671-2-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] pinctrl: cherryview: Add chv_gpio_clear_triggering() helper function | expand |
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@redhat.com> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c index 4ff7d60a2bf1..2042f5f6833d 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); }
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> --- drivers/pinctrl/intel/pinctrl-cherryview.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)