pinctrl: intel: remap the pin number to gpio offset for irq enabled pin
diff mbox series

Message ID 20190816093838.81461-1-chiu@endlessm.com
State New
Headers show
Series
  • pinctrl: intel: remap the pin number to gpio offset for irq enabled pin
Related show

Commit Message

Chris Chiu Aug. 16, 2019, 9:38 a.m. UTC
On Asus X571GT, GPIO 297 is configured as an interrupt and serves
for the touchpad. The touchpad will report input events much less
than expected after S3 suspend/resume, which results in extremely
slow cursor movement. However, the number of interrupts observed
from /proc/interrupts increases much more than expected even no
touching touchpad.

This is due to the value of PADCFG0 of PIN 225 for the interrupt
has been changed from 0x80800102 to 0x80100102. The GPIROUTIOXAPIC
is toggled on which results in the spurious interrupts. The PADCFG0
of PIN 225 is expected to be saved during suspend, but the 297 is
saved instead because the gpiochip_line_is_irq() expect the GPIO
offset but what's really passed to it is PIN number. In this case,
the /sys/kernel/debug/pinctrl/INT3450:00/gpio-ranges shows

288: INT3450:00 GPIOS [436 - 459] PINS [216 - 239]

So gpiochip_line_is_irq() returns true for GPIO offset 297, the
suspend routine spuriously saves the content for PIN 297 which
we expect to save for PIN 225.

This commit maps the PIN number to GPIO offset first in the
intel_pinctrl_should_save() to make sure the values for the
specific PINs can be correctly saved and then restored.

Signed-off-by: Chris Chiu <chiu@endlessm.com>

---
 drivers/pinctrl/intel/pinctrl-intel.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

Comments

mika.westerberg@linux.intel.com Aug. 19, 2019, 7:14 a.m. UTC | #1
On Fri, Aug 16, 2019 at 05:38:38PM +0800, Chris Chiu wrote:
> On Asus X571GT, GPIO 297 is configured as an interrupt and serves
> for the touchpad. The touchpad will report input events much less
> than expected after S3 suspend/resume, which results in extremely
> slow cursor movement. However, the number of interrupts observed
> from /proc/interrupts increases much more than expected even no
> touching touchpad.
> 
> This is due to the value of PADCFG0 of PIN 225 for the interrupt
> has been changed from 0x80800102 to 0x80100102. The GPIROUTIOXAPIC
> is toggled on which results in the spurious interrupts. The PADCFG0
> of PIN 225 is expected to be saved during suspend, but the 297 is
> saved instead because the gpiochip_line_is_irq() expect the GPIO
> offset but what's really passed to it is PIN number. In this case,
> the /sys/kernel/debug/pinctrl/INT3450:00/gpio-ranges shows
> 
> 288: INT3450:00 GPIOS [436 - 459] PINS [216 - 239]
> 
> So gpiochip_line_is_irq() returns true for GPIO offset 297, the
> suspend routine spuriously saves the content for PIN 297 which
> we expect to save for PIN 225.

Nice work nailing the issue!

> This commit maps the PIN number to GPIO offset first in the
> intel_pinctrl_should_save() to make sure the values for the
> specific PINs can be correctly saved and then restored.
> 
> Signed-off-by: Chris Chiu <chiu@endlessm.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I think this should also have:

Fixes: c538b9436751 ("pinctrl: intel: Only restore pins that are used by the driver")
Andy Shevchenko Aug. 19, 2019, 9:42 a.m. UTC | #2
On Mon, Aug 19, 2019 at 10:14:13AM +0300, Mika Westerberg wrote:
> On Fri, Aug 16, 2019 at 05:38:38PM +0800, Chris Chiu wrote:
> > On Asus X571GT, GPIO 297 is configured as an interrupt and serves
> > for the touchpad. The touchpad will report input events much less
> > than expected after S3 suspend/resume, which results in extremely
> > slow cursor movement. However, the number of interrupts observed
> > from /proc/interrupts increases much more than expected even no
> > touching touchpad.
> > 
> > This is due to the value of PADCFG0 of PIN 225 for the interrupt
> > has been changed from 0x80800102 to 0x80100102. The GPIROUTIOXAPIC
> > is toggled on which results in the spurious interrupts. The PADCFG0
> > of PIN 225 is expected to be saved during suspend, but the 297 is
> > saved instead because the gpiochip_line_is_irq() expect the GPIO
> > offset but what's really passed to it is PIN number. In this case,
> > the /sys/kernel/debug/pinctrl/INT3450:00/gpio-ranges shows
> > 
> > 288: INT3450:00 GPIOS [436 - 459] PINS [216 - 239]
> > 
> > So gpiochip_line_is_irq() returns true for GPIO offset 297, the
> > suspend routine spuriously saves the content for PIN 297 which
> > we expect to save for PIN 225.
> 
> Nice work nailing the issue!
> 
> > This commit maps the PIN number to GPIO offset first in the
> > intel_pinctrl_should_save() to make sure the values for the
> > specific PINs can be correctly saved and then restored.
> > 
> > Signed-off-by: Chris Chiu <chiu@endlessm.com>
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I think this should also have:
> 
> Fixes: c538b9436751 ("pinctrl: intel: Only restore pins that are used by the driver")

Pushed to my review and testing queue, thanks!

P.S. I have added Fixes tag.

Patch
diff mbox series

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index a18d6eefe672..8d6a843bbc7e 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -796,6 +796,29 @@  static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset,
 	return -EINVAL;
 }
 
+/**
+ * intel_pin_to_gpio() - Translate from pin number to GPIO offset
+ * @pctrl: Pinctrl structure
+ * @pin: pin number
+ *
+ * Translate the pin number of pinctrl to GPIO offset
+ */
+static int intel_pin_to_gpio(struct intel_pinctrl *pctrl, int pin)
+{
+	const struct intel_community *community;
+	const struct intel_padgroup *padgrp;
+
+	community = intel_get_community(pctrl, pin);
+	if (!community)
+		return -EINVAL;
+
+	padgrp = intel_community_get_padgroup(community, pin);
+	if (!padgrp)
+		return -EINVAL;
+
+	return pin - padgrp->base + padgrp->gpio_base;
+}
+
 static int intel_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
@@ -1443,7 +1466,8 @@  static bool intel_pinctrl_should_save(struct intel_pinctrl *pctrl, unsigned int
 	 * them alone.
 	 */
 	if (pd->mux_owner || pd->gpio_owner ||
-	    gpiochip_line_is_irq(&pctrl->chip, pin))
+	    gpiochip_line_is_irq(&pctrl->chip,
+				 intel_pin_to_gpio(pctrl, pin)))
 		return true;
 
 	return false;