• Hans de Goede's avatar
    pinctrl: cherryview: Stop clearing the GPIO_EN bit from chv_gpio_disable_free · 1adde32a
    Hans de Goede authored
    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: default avatarHans de Goede <hdegoede@redhat.com>
    Acked-by: default avatarMika Westerberg <mika.westerberg@linux.intel.com>
    Signed-off-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
    1adde32a
pinctrl-cherryview.c 51.4 KB