linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip
@ 2017-11-20 15:19 Mika Westerberg
  2017-11-21  3:18 ` Chris Chiu
  2017-11-29 12:39 ` Linus Walleij
  0 siblings, 2 replies; 4+ messages in thread
From: Mika Westerberg @ 2017-11-20 15:19 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Heikki Krogerus, Daniel Drake, Chris Chiu, Andy Shevchenko,
	Mika Westerberg, linux-gpio, linux-kernel

When a GPIO is requested using gpiod_get_* APIs the intel pinctrl driver
switches the pin to GPIO mode and makes sure interrupts are routed to
the GPIO hardware instead of IOAPIC. However, if the GPIO is used
directly through irqchip, as is the case with many I2C-HID devices where
I2C core automatically configures interrupt for the device, the pin is
not initialized as GPIO. Instead we rely that the BIOS configures the
pin accordingly which seems not to be the case at least in Asus X540NA
SKU3 with Focaltech touchpad.

When the pin is not properly configured it might result weird behaviour
like interrupts suddenly stop firing completely and the touchpad stops
responding to user input.

Fix this by properly initializing the pin to GPIO mode also when it is
used directly through irqchip.

Reported-by: Daniel Drake <drake@endlessm.com>
Reported-by: Chris Chiu <chiu@endlessm.com>
Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Chris, Daniel,

Could you check that this still solves the issue and maybe provide your
Tested-by? Thanks!

 drivers/pinctrl/intel/pinctrl-intel.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3761fd29100f..a1fdda91f874 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -425,6 +425,18 @@ static void __intel_gpio_set_direction(void __iomem *padcfg0, bool input)
 	writel(value, padcfg0);
 }
 
+static void __intel_gpio_init_gpio(void __iomem *padcfg0)
+{
+	u32 value;
+
+	/* Put the pad into GPIO mode */
+	value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
+	/* Disable SCI/SMI/NMI generation */
+	value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
+	value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
+	writel(value, padcfg0);
+}
+
 static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 				     struct pinctrl_gpio_range *range,
 				     unsigned pin)
@@ -432,7 +444,6 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *padcfg0;
 	unsigned long flags;
-	u32 value;
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
@@ -442,13 +453,7 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	}
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
-	/* Put the pad into GPIO mode */
-	value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
-	/* Disable SCI/SMI/NMI generation */
-	value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
-	value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
-	writel(value, padcfg0);
-
+	__intel_gpio_init_gpio(padcfg0);
 	/* Disable TX buffer and enable RX (this will be input) */
 	__intel_gpio_set_direction(padcfg0, true);
 
@@ -935,6 +940,8 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned type)
 
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
+	__intel_gpio_init_gpio(reg);
+
 	value = readl(reg);
 
 	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
-- 
2.15.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip
  2017-11-20 15:19 [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip Mika Westerberg
@ 2017-11-21  3:18 ` Chris Chiu
  2017-11-29 12:39 ` Linus Walleij
  1 sibling, 0 replies; 4+ messages in thread
From: Chris Chiu @ 2017-11-21  3:18 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Linus Walleij, Heikki Krogerus, Daniel Drake, Andy Shevchenko,
	linux-gpio, Linux Kernel

On Mon, Nov 20, 2017 at 11:19 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> When a GPIO is requested using gpiod_get_* APIs the intel pinctrl driver
> switches the pin to GPIO mode and makes sure interrupts are routed to
> the GPIO hardware instead of IOAPIC. However, if the GPIO is used
> directly through irqchip, as is the case with many I2C-HID devices where
> I2C core automatically configures interrupt for the device, the pin is
> not initialized as GPIO. Instead we rely that the BIOS configures the
> pin accordingly which seems not to be the case at least in Asus X540NA
> SKU3 with Focaltech touchpad.
>
> When the pin is not properly configured it might result weird behaviour
> like interrupts suddenly stop firing completely and the touchpad stops
> responding to user input.
>
> Fix this by properly initializing the pin to GPIO mode also when it is
> used directly through irqchip.
>
> Reported-by: Daniel Drake <drake@endlessm.com>
> Reported-by: Chris Chiu <chiu@endlessm.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Chris, Daniel,
>
> Could you check that this still solves the issue and maybe provide your
> Tested-by? Thanks!
>

I've verified on X540NA here and it's working fine w/o any abnormal touchpad
stop.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip
  2017-11-20 15:19 [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip Mika Westerberg
  2017-11-21  3:18 ` Chris Chiu
@ 2017-11-29 12:39 ` Linus Walleij
  2017-11-29 13:11   ` Mika Westerberg
  1 sibling, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2017-11-29 12:39 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Heikki Krogerus, Daniel Drake, Chris Chiu, Andy Shevchenko,
	linux-gpio, linux-kernel

On Mon, Nov 20, 2017 at 4:19 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> When a GPIO is requested using gpiod_get_* APIs the intel pinctrl driver
> switches the pin to GPIO mode and makes sure interrupts are routed to
> the GPIO hardware instead of IOAPIC. However, if the GPIO is used
> directly through irqchip, as is the case with many I2C-HID devices where
> I2C core automatically configures interrupt for the device, the pin is
> not initialized as GPIO. Instead we rely that the BIOS configures the
> pin accordingly which seems not to be the case at least in Asus X540NA
> SKU3 with Focaltech touchpad.
>
> When the pin is not properly configured it might result weird behaviour
> like interrupts suddenly stop firing completely and the touchpad stops
> responding to user input.
>
> Fix this by properly initializing the pin to GPIO mode also when it is
> used directly through irqchip.
>
> Reported-by: Daniel Drake <drake@endlessm.com>
> Reported-by: Chris Chiu <chiu@endlessm.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Should this have a Fixes:?

Should this have a Cc: stable@vger.kernel.org?

> Chris, Daniel,
>
> Could you check that this still solves the issue and maybe provide your
> Tested-by? Thanks!

Pls pick up Tested-by's when resending.

> +static void __intel_gpio_init_gpio(void __iomem *padcfg0)

I just don't like __underscore_means_inner_function() I
strongly prefer function_has_a_proper_name() so please
come up with something that describes what it is really doing
and name it like that.

With that fixes, I'll apply it pronto.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip
  2017-11-29 12:39 ` Linus Walleij
@ 2017-11-29 13:11   ` Mika Westerberg
  0 siblings, 0 replies; 4+ messages in thread
From: Mika Westerberg @ 2017-11-29 13:11 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Heikki Krogerus, Daniel Drake, Chris Chiu, Andy Shevchenko,
	linux-gpio, linux-kernel

On Wed, Nov 29, 2017 at 01:39:00PM +0100, Linus Walleij wrote:
> On Mon, Nov 20, 2017 at 4:19 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > When a GPIO is requested using gpiod_get_* APIs the intel pinctrl driver
> > switches the pin to GPIO mode and makes sure interrupts are routed to
> > the GPIO hardware instead of IOAPIC. However, if the GPIO is used
> > directly through irqchip, as is the case with many I2C-HID devices where
> > I2C core automatically configures interrupt for the device, the pin is
> > not initialized as GPIO. Instead we rely that the BIOS configures the
> > pin accordingly which seems not to be the case at least in Asus X540NA
> > SKU3 with Focaltech touchpad.
> >
> > When the pin is not properly configured it might result weird behaviour
> > like interrupts suddenly stop firing completely and the touchpad stops
> > responding to user input.
> >
> > Fix this by properly initializing the pin to GPIO mode also when it is
> > used directly through irqchip.
> >
> > Reported-by: Daniel Drake <drake@endlessm.com>
> > Reported-by: Chris Chiu <chiu@endlessm.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Should this have a Fixes:?
> 
> Should this have a Cc: stable@vger.kernel.org?

Right, I'll add those in v2.

> > Chris, Daniel,
> >
> > Could you check that this still solves the issue and maybe provide your
> > Tested-by? Thanks!
> 
> Pls pick up Tested-by's when resending.

Sure.

> > +static void __intel_gpio_init_gpio(void __iomem *padcfg0)
> 
> I just don't like __underscore_means_inner_function() I
> strongly prefer function_has_a_proper_name() so please
> come up with something that describes what it is really doing
> and name it like that.
> 
> With that fixes, I'll apply it pronto.

No problem. I'll make v2 and send it out soon.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-11-29 13:11 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-20 15:19 [PATCH] pinctrl: intel: Initialize GPIO properly when used through irqchip Mika Westerberg
2017-11-21  3:18 ` Chris Chiu
2017-11-29 12:39 ` Linus Walleij
2017-11-29 13:11   ` Mika Westerberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).