| Message ID | 20200904172141.180363-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | pinctrl: cherryview: Preserve CHV_PADCTRL1_INVRXTX_TXDATA flag on GPIOs | expand |
On Fri, Sep 04, 2020 at 07:21:41PM +0200, Hans de Goede wrote: > One some devices the GPIO should output the inverted value from what > Fixes: a7d4b171660c ("Input: goodix - add support for getting IRQ + reset GPIOs on Cherry Trail devices") Hans, a side note. Can you clean up that driver using ACPI GPIO quirks and the idea that IRQ line will be retrieved in client->irq? I recently submit a patch against BT Intel driver [1] where similar approach is utilized. [1]: https://lore.kernel.org/linux-bluetooth/20200903184850.53055-1-andriy.shevchenko@linux.intel.com/T/
Hi, On 9/4/20 8:09 PM, Andy Shevchenko wrote: > On Fri, Sep 04, 2020 at 07:21:41PM +0200, Hans de Goede wrote: >> One some devices the GPIO should output the inverted value from what > >> Fixes: a7d4b171660c ("Input: goodix - add support for getting IRQ + reset GPIOs on Cherry Trail devices") > > Hans, a side note. Can you clean up that driver using ACPI GPIO quirks and the > idea that IRQ line will be retrieved in client->irq? Nope, the chip uses the IRQ line in a bi-directional manner, when the chip is reset (by us) the IRQ line is a bootstrap pin and we need to drive it either high or low to make sure the chip re-appears on the same i2c-address as it had before. So we need the get gpiod and then call gpiod_to_irq abstraction so that we also have the gpiod to call allow calling gpiod_direction_input() and gpiod_direction_output() on the irq pin. And to make things more interesting on some generations x86 hw (apollo lake) the IRQ is actually listed as an Interrupt resource in the ACPI device resources (_CRS) and there are GDIX specific ACPI methods on the ACPI device which directly poke the GPIO chip to allow setting it high/low (for during reset) or config it as input. So the goodix.c has its own internal abstraction around gpiod_direction_input(() and gpiod_direction_output() for controlling the IRQ pin which does different things on different x86 hw generations (while also maintaining device-tree compat). So all in all I'm happy that we have this all working atm (and it also is reasonably clean already IMHO) and I would rather not muck with this. Regards, Hans
On Fri, Sep 04, 2020 at 07:21:41PM +0200, Hans de Goede wrote: > One some devices the GPIO should output the inverted value from what > device-drivers / ACPI code expects. The reason for this is unknown, > perhaps these systems use an external buffer chip on the GPIO which > inverts the signal. The BIOS makes this work by setting the > CHV_PADCTRL1_INVRXTX_TXDATA flag. > > Before this commit we would unconditionally clear all INVRXTX flags, > including the CHV_PADCTRL1_INVRXTX_TXDATA flag when a GPIO is requested > by a driver (from chv_gpio_request_enable()). > > This breaks systems using this setup. Specifically it is causing > problems for systems with a goodix touchscreen, where the BIOS sets the > INVRXTX_TXDATA flag on the GPIO used for the touchscreen's reset pin. > > The goodix touchscreen driver by defaults configures this pin as input > (relying on the pull-up to keep it high), but the clearing of the > INVRXTX_TXDATA flag done by chv_gpio_request_enable() causes it to be > driven low for a brief time before the GPIO gets set to input mode. > > This causes the touchscreen controller to get reset. On most CHT devs > with this touchscreen this leads to: > > [ 31.596534] Goodix-TS i2c-GDIX1001:00: i2c test failed attempt 1: -121 > > The driver retries this though and then everything is fine. But during > reset the touchscreen uses its interrupt pin as bootstrap to determine > which i2c address to use and on the Acer One S1003 the spurious reset > caused by the clearing of the INVRXTX_TXDATA flag causes the controller > to come back up again on the wrong i2c address, breaking things. > > This commit fixes both the -121 errors, as well as the total breakage > on the Acer One S1003, by making chv_gpio_clear_triggering() not clear > the INVRXTX_TXDATA flag if the pin is already configured as a GPIO. > > Note that chv_pinmux_set_mux() does still unconditionally clear the > flag, so this only affects GPIO usage. > > Fixes: a7d4b171660c ("Input: goodix - add support for getting IRQ + reset GPIOs on Cherry Trail devices") > 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 33862bb1e0e2..41ee77bf893e 100644 --- a/drivers/pinctrl/intel/pinctrl-cherryview.c +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c @@ -64,6 +64,7 @@ #define CHV_PADCTRL1_CFGLOCK BIT(31) #define CHV_PADCTRL1_INVRXTX_SHIFT 4 #define CHV_PADCTRL1_INVRXTX_MASK GENMASK(7, 4) +#define CHV_PADCTRL1_INVRXTX_TXDATA BIT(7) #define CHV_PADCTRL1_INVRXTX_RXDATA BIT(6) #define CHV_PADCTRL1_INVRXTX_TXENABLE BIT(5) #define CHV_PADCTRL1_ODEN BIT(3) @@ -798,11 +799,22 @@ static int chv_pinmux_set_mux(struct pinctrl_dev *pctldev, static void chv_gpio_clear_triggering(struct chv_pinctrl *pctrl, unsigned int offset) { + u32 invrxtx_mask = CHV_PADCTRL1_INVRXTX_MASK; u32 value; + /* + * One some devices the GPIO should output the inverted value from what + * device-drivers / ACPI code expects (inverted external buffer?). The + * BIOS makes this work by setting the CHV_PADCTRL1_INVRXTX_TXDATA flag, + * preserve this flag if the pin is already setup as GPIO. + */ + value = chv_readl(pctrl, offset, CHV_PADCTRL0); + if (value & CHV_PADCTRL0_GPIOEN) + invrxtx_mask &= ~CHV_PADCTRL1_INVRXTX_TXDATA; + value = chv_readl(pctrl, offset, CHV_PADCTRL1); value &= ~CHV_PADCTRL1_INTWAKECFG_MASK; - value &= ~CHV_PADCTRL1_INVRXTX_MASK; + value &= ~invrxtx_mask; chv_writel(pctrl, offset, CHV_PADCTRL1, value); }
One some devices the GPIO should output the inverted value from what device-drivers / ACPI code expects. The reason for this is unknown, perhaps these systems use an external buffer chip on the GPIO which inverts the signal. The BIOS makes this work by setting the CHV_PADCTRL1_INVRXTX_TXDATA flag. Before this commit we would unconditionally clear all INVRXTX flags, including the CHV_PADCTRL1_INVRXTX_TXDATA flag when a GPIO is requested by a driver (from chv_gpio_request_enable()). This breaks systems using this setup. Specifically it is causing problems for systems with a goodix touchscreen, where the BIOS sets the INVRXTX_TXDATA flag on the GPIO used for the touchscreen's reset pin. The goodix touchscreen driver by defaults configures this pin as input (relying on the pull-up to keep it high), but the clearing of the INVRXTX_TXDATA flag done by chv_gpio_request_enable() causes it to be driven low for a brief time before the GPIO gets set to input mode. This causes the touchscreen controller to get reset. On most CHT devs with this touchscreen this leads to: [ 31.596534] Goodix-TS i2c-GDIX1001:00: i2c test failed attempt 1: -121 The driver retries this though and then everything is fine. But during reset the touchscreen uses its interrupt pin as bootstrap to determine which i2c address to use and on the Acer One S1003 the spurious reset caused by the clearing of the INVRXTX_TXDATA flag causes the controller to come back up again on the wrong i2c address, breaking things. This commit fixes both the -121 errors, as well as the total breakage on the Acer One S1003, by making chv_gpio_clear_triggering() not clear the INVRXTX_TXDATA flag if the pin is already configured as a GPIO. Note that chv_pinmux_set_mux() does still unconditionally clear the flag, so this only affects GPIO usage. Fixes: a7d4b171660c ("Input: goodix - add support for getting IRQ + reset GPIOs on Cherry Trail devices") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/pinctrl/intel/pinctrl-cherryview.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)