Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad.
@ 2020-04-24  9:12 caizhaopeng
  2020-04-24  9:50 ` Andy Shevchenko
  2020-11-12 13:52 ` Andy Shevchenko
  0 siblings, 2 replies; 5+ messages in thread
From: caizhaopeng @ 2020-04-24  9:12 UTC (permalink / raw)
  To: Linus Walleij, Mika Westerberg, Andy Shevchenko
  Cc: linux-gpio, linux-kernel, Zhangyueqian, Zhangshuang, Hualet Wang,
	Zhanglei, Caicai

From: Caicai <caizhaopeng@uniontech.com>

I had tested two Notebook machines, the Intel i5(or amd ryzen)
with elan touchpad, and there's a probability that the touchpad
won't work after going to the S3/S4 to wake up, that it would
appear no more than 15 times. I found that there's no interrupt
to check for /proc/interrupt. It was found that the gpio
interrupt of i2c was also not on top. By adding the gpio
interrupt flags with IRQF_EARLY_RESUME, now the touchpad tested
200 + times works well.

Signed-off-by: Caicai <caizhaopeng@uniontech.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 2 +-
 drivers/pinctrl/pinctrl-amd.c         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 8fb6c9668c37..a350dade6aa0 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1189,7 +1189,7 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
 	 * controllers share the same interrupt line.
 	 */
 	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
-			       IRQF_SHARED | IRQF_NO_THREAD,
+			       IRQF_SHARED | IRQF_NO_THREAD | IRQF_EARLY_RESUME,
 			       dev_name(pctrl->dev), pctrl);
 	if (ret) {
 		dev_err(pctrl->dev, "failed to request interrupt\n");
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 977792654e01..70c37f4da2b1 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -937,7 +937,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
-			       IRQF_SHARED, KBUILD_MODNAME, gpio_dev);
+			       IRQF_SHARED | IRQF_EARLY_RESUME | IRQF_NO_THREAD, KBUILD_MODNAME, gpio_dev);
 	if (ret)
 		goto out2;
 
-- 
2.20.1




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

* Re: [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad.
  2020-04-24  9:12 [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad caizhaopeng
@ 2020-04-24  9:50 ` Andy Shevchenko
  2020-11-12 13:52 ` Andy Shevchenko
  1 sibling, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-04-24  9:50 UTC (permalink / raw)
  To: caizhaopeng
  Cc: Linus Walleij, Mika Westerberg, Andy Shevchenko,
	open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
	Zhangyueqian, Zhangshuang, Hualet Wang, Zhanglei

On Fri, Apr 24, 2020 at 12:16 PM <caizhaopeng@uniontech.com> wrote:
>
> From: Caicai <caizhaopeng@uniontech.com>
>
> I had tested two Notebook machines, the Intel i5(or amd ryzen)
> with elan touchpad, and there's a probability that the touchpad
> won't work after going to the S3/S4 to wake up, that it would
> appear no more than 15 times. I found that there's no interrupt
> to check for /proc/interrupt. It was found that the gpio
> interrupt of i2c was also not on top. By adding the gpio
> interrupt flags with IRQF_EARLY_RESUME, now the touchpad tested
> 200 + times works well.

Thanks for the contribution!

You need
a) to split this to patch per driver (I can't take or speak for AMD
code, I'm not a maintainer and don't have hardware)
b) explain what this flag actually means and does

On top of that we need to test this widely to avoid regressions.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad.
  2020-04-24  9:12 [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad caizhaopeng
  2020-04-24  9:50 ` Andy Shevchenko
@ 2020-11-12 13:52 ` Andy Shevchenko
  2020-11-23 12:46   ` Hans de Goede
  1 sibling, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2020-11-12 13:52 UTC (permalink / raw)
  To: caizhaopeng
  Cc: Linus Walleij, Mika Westerberg, Andy Shevchenko, linux-gpio,
	linux-kernel, Zhangyueqian, Zhangshuang, Hualet Wang, Zhanglei,
	Hans de Goede


+Cc: Hans.

I can't speak for AMD, but I think it may be useful for Intel pin control.
However, I didn't check what may be the side effects of this change and neither
contributor answered to my comments...

So, just heads up.

On Fri, Apr 24, 2020 at 05:12:01PM +0800, caizhaopeng@uniontech.com wrote:
> From: Caicai <caizhaopeng@uniontech.com>
> 
> I had tested two Notebook machines, the Intel i5(or amd ryzen)
> with elan touchpad, and there's a probability that the touchpad
> won't work after going to the S3/S4 to wake up, that it would
> appear no more than 15 times. I found that there's no interrupt
> to check for /proc/interrupt. It was found that the gpio
> interrupt of i2c was also not on top. By adding the gpio
> interrupt flags with IRQF_EARLY_RESUME, now the touchpad tested
> 200 + times works well.
> 
> Signed-off-by: Caicai <caizhaopeng@uniontech.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 2 +-
>  drivers/pinctrl/pinctrl-amd.c         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 8fb6c9668c37..a350dade6aa0 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1189,7 +1189,7 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>  	 * controllers share the same interrupt line.
>  	 */
>  	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
> -			       IRQF_SHARED | IRQF_NO_THREAD,
> +			       IRQF_SHARED | IRQF_NO_THREAD | IRQF_EARLY_RESUME,
>  			       dev_name(pctrl->dev), pctrl);
>  	if (ret) {
>  		dev_err(pctrl->dev, "failed to request interrupt\n");
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 977792654e01..70c37f4da2b1 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -937,7 +937,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
> -			       IRQF_SHARED, KBUILD_MODNAME, gpio_dev);
> +			       IRQF_SHARED | IRQF_EARLY_RESUME | IRQF_NO_THREAD, KBUILD_MODNAME, gpio_dev);
>  	if (ret)
>  		goto out2;
>  
> -- 
> 2.20.1
> 
> 
> 

-- 
With Best Regards,
Andy Shevchenko



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

* Re: [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad.
  2020-11-12 13:52 ` Andy Shevchenko
@ 2020-11-23 12:46   ` Hans de Goede
  2020-11-23 13:35     ` Andy Shevchenko
  0 siblings, 1 reply; 5+ messages in thread
From: Hans de Goede @ 2020-11-23 12:46 UTC (permalink / raw)
  To: Andy Shevchenko, caizhaopeng
  Cc: Linus Walleij, Mika Westerberg, Andy Shevchenko, linux-gpio,
	linux-kernel, Zhangyueqian, Zhangshuang, Hualet Wang, Zhanglei

Hi,

On 11/12/20 2:52 PM, Andy Shevchenko wrote:
> 
> +Cc: Hans.
> 
> I can't speak for AMD, but I think it may be useful for Intel pin control.
> However, I didn't check what may be the side effects of this change and neither
> contributor answered to my comments...

AFAICT setting IRQF_EARLY_RESUME causes it to not be resumed during
normal IRQ resuming, but earlier during the syscore_resume() call.

There are 2 problems with this:

1. When using S3 suspend syscore_resume() runs before any device
suspend handlers, so any state restoring which the GPIO controller's
resume handler is doing has not been done yet. While esp. after
S3 suspend the restoring may be important (s2idle suspend should
not touch the GPIO registers contents).

2. When using S2idle suspend syscore_resume() *never* runs, and
IRQs marked with IRQF_EARLY_RESUME are skipped during the normal
IRQ resume phase, so these IRQs will never be resumed.

Also IRQF_EARLY_RESUME is used almost no where:

[hans@x1 linux]$ ack -l IRQF_EARLY_RESUME drivers
drivers/xen/events/events_base.c
drivers/rtc/rtc-sc27xx.c
drivers/mfd/twl4030-irq.c

Which is probably why we have not yet hit the s2idle issue with it.

So this all in all seems like a bad idea, and we need to better root
cause the issue with these Elan touchpads and fix the actual issue.

Regards,

Hans




> 
> So, just heads up.
> 
> On Fri, Apr 24, 2020 at 05:12:01PM +0800, caizhaopeng@uniontech.com wrote:
>> From: Caicai <caizhaopeng@uniontech.com>
>>
>> I had tested two Notebook machines, the Intel i5(or amd ryzen)
>> with elan touchpad, and there's a probability that the touchpad
>> won't work after going to the S3/S4 to wake up, that it would
>> appear no more than 15 times. I found that there's no interrupt
>> to check for /proc/interrupt. It was found that the gpio
>> interrupt of i2c was also not on top. By adding the gpio
>> interrupt flags with IRQF_EARLY_RESUME, now the touchpad tested
>> 200 + times works well.
>>
>> Signed-off-by: Caicai <caizhaopeng@uniontech.com>
>> ---
>>  drivers/pinctrl/intel/pinctrl-intel.c | 2 +-
>>  drivers/pinctrl/pinctrl-amd.c         | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
>> index 8fb6c9668c37..a350dade6aa0 100644
>> --- a/drivers/pinctrl/intel/pinctrl-intel.c
>> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
>> @@ -1189,7 +1189,7 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq)
>>  	 * controllers share the same interrupt line.
>>  	 */
>>  	ret = devm_request_irq(pctrl->dev, irq, intel_gpio_irq,
>> -			       IRQF_SHARED | IRQF_NO_THREAD,
>> +			       IRQF_SHARED | IRQF_NO_THREAD | IRQF_EARLY_RESUME,
>>  			       dev_name(pctrl->dev), pctrl);
>>  	if (ret) {
>>  		dev_err(pctrl->dev, "failed to request interrupt\n");
>> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
>> index 977792654e01..70c37f4da2b1 100644
>> --- a/drivers/pinctrl/pinctrl-amd.c
>> +++ b/drivers/pinctrl/pinctrl-amd.c
>> @@ -937,7 +937,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
>> -			       IRQF_SHARED, KBUILD_MODNAME, gpio_dev);
>> +			       IRQF_SHARED | IRQF_EARLY_RESUME | IRQF_NO_THREAD, KBUILD_MODNAME, gpio_dev);
>>  	if (ret)
>>  		goto out2;
>>  
>> -- 
>> 2.20.1
>>
>>
>>
> 


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

* Re: [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad.
  2020-11-23 12:46   ` Hans de Goede
@ 2020-11-23 13:35     ` Andy Shevchenko
  0 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2020-11-23 13:35 UTC (permalink / raw)
  To: Hans de Goede
  Cc: caizhaopeng, Linus Walleij, Mika Westerberg, Andy Shevchenko,
	linux-gpio, linux-kernel, Zhangyueqian, Zhangshuang, Hualet Wang,
	Zhanglei

On Mon, Nov 23, 2020 at 01:46:50PM +0100, Hans de Goede wrote:
> On 11/12/20 2:52 PM, Andy Shevchenko wrote:
> > 
> > +Cc: Hans.
> > 
> > I can't speak for AMD, but I think it may be useful for Intel pin control.
> > However, I didn't check what may be the side effects of this change and neither
> > contributor answered to my comments...
> 
> AFAICT setting IRQF_EARLY_RESUME causes it to not be resumed during
> normal IRQ resuming, but earlier during the syscore_resume() call.
> 
> There are 2 problems with this:
> 
> 1. When using S3 suspend syscore_resume() runs before any device
> suspend handlers, so any state restoring which the GPIO controller's
> resume handler is doing has not been done yet. While esp. after
> S3 suspend the restoring may be important (s2idle suspend should
> not touch the GPIO registers contents).
> 
> 2. When using S2idle suspend syscore_resume() *never* runs, and
> IRQs marked with IRQF_EARLY_RESUME are skipped during the normal
> IRQ resume phase, so these IRQs will never be resumed.
> 
> Also IRQF_EARLY_RESUME is used almost no where:

Yes, I also noticed this.

> [hans@x1 linux]$ ack -l IRQF_EARLY_RESUME drivers
> drivers/xen/events/events_base.c
> drivers/rtc/rtc-sc27xx.c
> drivers/mfd/twl4030-irq.c
> 
> Which is probably why we have not yet hit the s2idle issue with it.
> 
> So this all in all seems like a bad idea, and we need to better root
> cause the issue with these Elan touchpads and fix the actual issue.

Thanks! That is useful to know.

> > So, just heads up.
> > 
> > On Fri, Apr 24, 2020 at 05:12:01PM +0800, caizhaopeng@uniontech.com wrote:
> >> From: Caicai <caizhaopeng@uniontech.com>
> >>
> >> I had tested two Notebook machines, the Intel i5(or amd ryzen)
> >> with elan touchpad, and there's a probability that the touchpad
> >> won't work after going to the S3/S4 to wake up, that it would
> >> appear no more than 15 times. I found that there's no interrupt
> >> to check for /proc/interrupt. It was found that the gpio
> >> interrupt of i2c was also not on top. By adding the gpio
> >> interrupt flags with IRQF_EARLY_RESUME, now the touchpad tested
> >> 200 + times works well.

-- 
With Best Regards,
Andy Shevchenko



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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-24  9:12 [PATCH 1/1] pinctrl: add IRQF_EARLY_RESUME flags with gpio irq for elan touchpad caizhaopeng
2020-04-24  9:50 ` Andy Shevchenko
2020-11-12 13:52 ` Andy Shevchenko
2020-11-23 12:46   ` Hans de Goede
2020-11-23 13:35     ` Andy Shevchenko

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git