| Message ID | 20191225132812.90889-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] pinctrl: baytrail: Remove WARN when setting direct-irq pin to output | expand |
On Wed, Dec 25, 2019 at 02:28:12PM +0100, Hans de Goede wrote: > Suspending Goodix touchscreens requires changing the interrupt pin to > output before sending them a power-down command. Followed by wiggling > the interrupt pin to wake the device up, after which it is put back > in input mode. > > On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI > resource so we can do this without problems as long as we release the > irq before changing the pin to output mode. > > On Bay Trail devices with a Goodix touchscreen direct-irq mode is used > in combination with listing the pin as a normal GpioIo resource. This > works fine, but this triggers the WARN in byt_gpio_set_direction-s output > path because direct-irq support is enabled on the pin. > > This commit removes the WARN call, fixing a bunch of WARN splats in > dmesg on each suspend/resume cycle. But this is still something we don't expect to do normally, right? How about changing this to dev_warn() or dev_info() so it is still visible in dmesg and possibly helps future debugging.
Hi, On 27-12-2019 15:12, Mika Westerberg wrote: > On Wed, Dec 25, 2019 at 02:28:12PM +0100, Hans de Goede wrote: >> Suspending Goodix touchscreens requires changing the interrupt pin to >> output before sending them a power-down command. Followed by wiggling >> the interrupt pin to wake the device up, after which it is put back >> in input mode. >> >> On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI >> resource so we can do this without problems as long as we release the >> irq before changing the pin to output mode. >> >> On Bay Trail devices with a Goodix touchscreen direct-irq mode is used >> in combination with listing the pin as a normal GpioIo resource. This >> works fine, but this triggers the WARN in byt_gpio_set_direction-s output >> path because direct-irq support is enabled on the pin. >> >> This commit removes the WARN call, fixing a bunch of WARN splats in >> dmesg on each suspend/resume cycle. > > But this is still something we don't expect to do normally, right? Well on devices which a Goodix touchscreen, of which there are quite a few we do. And it seems that Windows allows this. > How > about changing this to dev_warn() or dev_info() so it is still visible > in dmesg and possibly helps future debugging. The problem is that we hit this path everytime we output a value on the pin. I guess we can change the WARN to dev_info_once() if you prefer that. Regards, Hans
On Fri, Dec 27, 2019 at 11:47:38PM +0100, Hans de Goede wrote: > The problem is that we hit this path everytime we output a value on the > pin. I guess we can change the WARN to dev_info_once() if you prefer that. Yes, I think dev_info_once() would be fine.
Hi, On 30-12-2019 11:08, Mika Westerberg wrote: > On Fri, Dec 27, 2019 at 11:47:38PM +0100, Hans de Goede wrote: >> The problem is that we hit this path everytime we output a value on the >> pin. I guess we can change the WARN to dev_info_once() if you prefer that. > > Yes, I think dev_info_once() would be fine. Ok, version 3 replacing the WARN() with a dev_info_once coming up. Regards, Hans
diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c index c6f53ed626c9..db55761c90cc 100644 --- a/drivers/pinctrl/intel/pinctrl-baytrail.c +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c @@ -801,7 +801,6 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev, { struct intel_pinctrl *vg = pinctrl_dev_get_drvdata(pctl_dev); void __iomem *val_reg = byt_gpio_reg(vg, offset, BYT_VAL_REG); - void __iomem *conf_reg = byt_gpio_reg(vg, offset, BYT_CONF0_REG); unsigned long flags; u32 value; @@ -811,15 +810,7 @@ static int byt_gpio_set_direction(struct pinctrl_dev *pctl_dev, value &= ~BYT_DIR_MASK; if (input) value |= BYT_OUTPUT_EN; - else - /* - * Before making any direction modifications, do a check if gpio - * is set for direct IRQ. On baytrail, setting GPIO to output - * does not make sense, so let's at least warn the caller before - * they shoot themselves in the foot. - */ - WARN(readl(conf_reg) & BYT_DIRECT_IRQ_EN, - "Potential Error: Setting GPIO with direct_irq_en to output"); + writel(value, val_reg); raw_spin_unlock_irqrestore(&byt_lock, flags);
Suspending Goodix touchscreens requires changing the interrupt pin to output before sending them a power-down command. Followed by wiggling the interrupt pin to wake the device up, after which it is put back in input mode. On Cherry Trail device the interrupt pin is listed as a GpioInt ACPI resource so we can do this without problems as long as we release the irq before changing the pin to output mode. On Bay Trail devices with a Goodix touchscreen direct-irq mode is used in combination with listing the pin as a normal GpioIo resource. This works fine, but this triggers the WARN in byt_gpio_set_direction-s output path because direct-irq support is enabled on the pin. This commit removes the WARN call, fixing a bunch of WARN splats in dmesg on each suspend/resume cycle. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v2: - Drop now unused conf_ref local variable --- drivers/pinctrl/intel/pinctrl-baytrail.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)