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>
