[PATCH 0/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[PATCH 0/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Hans de Goede]

Hi All,

This is a rewrite of a patch which I wrote a while ago:
https://www.spinics.net/lists/kernel/msg3474327.html

The old version got nacked because it was changing behavior based on whether
s2idle or s3 suspend was used at probe-time, while this can be changed at
run-time.

This new version instead delays calling enable_irq_wake() until the drivers'
suspend call-back is called, at which point we know for sure which type of
suspend is being used (for this suspend/resume cycle), thus addressing the
problem with the old version.

Regards,

Hans


Hans de Goede (1):
  platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when
    using s2idle

 drivers/platform/x86/Kconfig               |  2 +-
 drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
 2 files changed, 57 insertions(+), 25 deletions(-)

-- 
2.31.1

[PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Hans de Goede]

Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement
irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to
the parents IRQ because this was breaking suspend (causing immediate
wakeups) on an Asus E202SA.

This workaround for the Asus E202SA is causing wakeup by USB keyboard to
not work on other devices with Airmont CPU cores such as the Medion Akoya
E1239T. In hindsight the problem with the Asus E202SA has nothing to do
with Silvermont vs Airmont CPU cores, so the differentiation between the
2 types of CPU cores introduced by the previous fix is wrong.

The real issue at hand is s2idle vs S3 suspend where the suspend is
mostly handled by firmware. The parent IRQ for the INT0002 device is shared
with the ACPI SCI and the real problem is that the INT0002 code should not
be messing with the wakeup settings of that IRQ when suspend/resume is
being handled by the firmware.

Note that on systems which support both s2idle and S3 suspend, which
suspend method to use can be changed at runtime.

This patch fixes both the Asus E202SA spurious wakeups issue as well as
the wakeup by USB keyboard not working on the Medion Akoya E1239T issue.

These are both fixed by replacing the old workaround with delaying the
enable_irq_wake(parent_irq) call till system-suspend time and protecting
it with a !pm_suspend_via_firmware() check so that we still do not call
it on devices using firmware-based (S3) suspend such as the Asus E202SA.

Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds
a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose
is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes
no sense.

Cc: Maxim Mikityanskiy <maxtram95@gmail.com>
Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/Kconfig               |  2 +-
 drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
 2 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 4b67e74a747b..c2f608d5f1b7 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -713,7 +713,7 @@ config INTEL_HID_EVENT
 
 config INTEL_INT0002_VGPIO
 	tristate "Intel ACPI INT0002 Virtual GPIO driver"
-	depends on GPIOLIB && ACPI
+	depends on GPIOLIB && ACPI && PM_SLEEP
 	select GPIOLIB_IRQCHIP
 	help
 	  Some peripherals on Bay Trail and Cherry Trail platforms signal a
diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
index 289c6655d425..569342aa8926 100644
--- a/drivers/platform/x86/intel_int0002_vgpio.c
+++ b/drivers/platform/x86/intel_int0002_vgpio.c
@@ -51,6 +51,12 @@
 #define GPE0A_STS_PORT			0x420
 #define GPE0A_EN_PORT			0x428
 
+struct int0002_data {
+	struct gpio_chip chip;
+	int parent_irq;
+	int wake_enable_count;
+};
+
 /*
  * As this is not a real GPIO at all, but just a hack to model an event in
  * ACPI the get / set functions are dummy functions.
@@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data)
 static int int0002_irq_set_wake(struct irq_data *data, unsigned int on)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
-	struct platform_device *pdev = to_platform_device(chip->parent);
-	int irq = platform_get_irq(pdev, 0);
+	struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip);
 
-	/* Propagate to parent irq */
+	/*
+	 * Applying of the wakeup flag to our parent IRQ is delayed till system
+	 * suspend, because we only want to do this when using s2idle.
+	 */
 	if (on)
-		enable_irq_wake(irq);
+		int0002->wake_enable_count++;
 	else
-		disable_irq_wake(irq);
+		int0002->wake_enable_count--;
 
 	return 0;
 }
@@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data)
 	return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT);
 }
 
-static struct irq_chip int0002_byt_irqchip = {
+static struct irq_chip int0002_irqchip = {
 	.name			= DRV_NAME,
 	.irq_ack		= int0002_irq_ack,
 	.irq_mask		= int0002_irq_mask,
@@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = {
 	.irq_set_wake		= int0002_irq_set_wake,
 };
 
-static struct irq_chip int0002_cht_irqchip = {
-	.name			= DRV_NAME,
-	.irq_ack		= int0002_irq_ack,
-	.irq_mask		= int0002_irq_mask,
-	.irq_unmask		= int0002_irq_unmask,
-	/*
-	 * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI
-	 * and we don't want to mess with the ACPI SCI irq settings.
-	 */
-	.flags			= IRQCHIP_SKIP_SET_WAKE,
-};
-
 static const struct x86_cpu_id int0002_cpu_ids[] = {
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT,	&int0002_byt_irqchip),
-	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT,	&int0002_cht_irqchip),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
+	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
 	{}
 };
 
@@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	const struct x86_cpu_id *cpu_id;
-	struct gpio_chip *chip;
+	struct int0002_data *int0002;
 	struct gpio_irq_chip *girq;
+	struct gpio_chip *chip;
 	int irq, ret;
 
 	/* Menlow has a different INT0002 device? <sigh> */
@@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
-	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
-	if (!chip)
+	int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL);
+	if (!int0002)
 		return -ENOMEM;
 
+	int0002->parent_irq = irq;
+
+	chip = &int0002->chip;
 	chip->label = DRV_NAME;
 	chip->parent = dev;
 	chip->owner = THIS_MODULE;
@@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev)
 	}
 
 	girq = &chip->irq;
-	girq->chip = (struct irq_chip *)cpu_id->driver_data;
+	girq->chip = &int0002_irqchip;
 	/* This let us handle the parent IRQ in the driver */
 	girq->parent_handler = NULL;
 	girq->num_parents = 0;
@@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev)
 
 	acpi_register_wakeup_handler(irq, int0002_check_wake, NULL);
 	device_init_wakeup(dev, true);
+	dev_set_drvdata(dev, int0002);
 	return 0;
 }
 
@@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int int0002_suspend(struct device *dev)
+{
+	struct int0002_data *int0002 = dev_get_drvdata(dev);
+
+	/*
+	 * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't
+	 * muck with it when firmware based suspend is used, otherwise we may
+	 * cause spurious wakeups from firmware managed suspend.
+	 */
+	if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
+		enable_irq_wake(int0002->parent_irq);
+
+	return 0;
+}
+
+static int int0002_resume(struct device *dev)
+{
+	struct int0002_data *int0002 = dev_get_drvdata(dev);
+
+	if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
+		disable_irq_wake(int0002->parent_irq);
+
+	return 0;
+}
+
+static const struct dev_pm_ops int0002_pm_ops = {
+	.suspend = int0002_suspend,
+	.resume = int0002_resume,
+};
+
 static const struct acpi_device_id int0002_acpi_ids[] = {
 	{ "INT0002", 0 },
 	{ },
@@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = {
 	.driver = {
 		.name			= DRV_NAME,
 		.acpi_match_table	= int0002_acpi_ids,
+		.pm			= &int0002_pm_ops,
 	},
 	.probe	= int0002_probe,
 	.remove	= int0002_remove,
-- 
2.31.1

Re: [PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Andy Shevchenko]

On Wed, May 12, 2021 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement
> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to
> the parents IRQ because this was breaking suspend (causing immediate
> wakeups) on an Asus E202SA.
>
> This workaround for the Asus E202SA is causing wakeup by USB keyboard to
> not work on other devices with Airmont CPU cores such as the Medion Akoya
> E1239T. In hindsight the problem with the Asus E202SA has nothing to do
> with Silvermont vs Airmont CPU cores, so the differentiation between the
> 2 types of CPU cores introduced by the previous fix is wrong.
>
> The real issue at hand is s2idle vs S3 suspend where the suspend is
> mostly handled by firmware. The parent IRQ for the INT0002 device is shared
> with the ACPI SCI and the real problem is that the INT0002 code should not
> be messing with the wakeup settings of that IRQ when suspend/resume is
> being handled by the firmware.
>
> Note that on systems which support both s2idle and S3 suspend, which
> suspend method to use can be changed at runtime.
>
> This patch fixes both the Asus E202SA spurious wakeups issue as well as
> the wakeup by USB keyboard not working on the Medion Akoya E1239T issue.
>
> These are both fixed by replacing the old workaround with delaying the
> enable_irq_wake(parent_irq) call till system-suspend time and protecting
> it with a !pm_suspend_via_firmware() check so that we still do not call
> it on devices using firmware-based (S3) suspend such as the Asus E202SA.

> Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds
> a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose
> is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes
> no sense.

I like the new approach.
One remark (or two :) is to the PM SLEEP thingy. Can we add a separate
line for "depends on", so it will be easier to maintain in case we
need to amend it somehow? Another one is to amend a helpline to
reflect that the driver is dealing solely with wake events (I haven't
checked the current text, so it might be already enforced).

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Cc: Maxim Mikityanskiy <maxtram95@gmail.com>
> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/Kconfig               |  2 +-
>  drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
>  2 files changed, 57 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4b67e74a747b..c2f608d5f1b7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -713,7 +713,7 @@ config INTEL_HID_EVENT
>
>  config INTEL_INT0002_VGPIO
>         tristate "Intel ACPI INT0002 Virtual GPIO driver"
> -       depends on GPIOLIB && ACPI
> +       depends on GPIOLIB && ACPI && PM_SLEEP
>         select GPIOLIB_IRQCHIP
>         help
>           Some peripherals on Bay Trail and Cherry Trail platforms signal a
> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
> index 289c6655d425..569342aa8926 100644
> --- a/drivers/platform/x86/intel_int0002_vgpio.c
> +++ b/drivers/platform/x86/intel_int0002_vgpio.c
> @@ -51,6 +51,12 @@
>  #define GPE0A_STS_PORT                 0x420
>  #define GPE0A_EN_PORT                  0x428
>
> +struct int0002_data {
> +       struct gpio_chip chip;
> +       int parent_irq;
> +       int wake_enable_count;
> +};
> +
>  /*
>   * As this is not a real GPIO at all, but just a hack to model an event in
>   * ACPI the get / set functions are dummy functions.
> @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data)
>  static int int0002_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> -       struct platform_device *pdev = to_platform_device(chip->parent);
> -       int irq = platform_get_irq(pdev, 0);
> +       struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip);
>
> -       /* Propagate to parent irq */
> +       /*
> +        * Applying of the wakeup flag to our parent IRQ is delayed till system
> +        * suspend, because we only want to do this when using s2idle.
> +        */
>         if (on)
> -               enable_irq_wake(irq);
> +               int0002->wake_enable_count++;
>         else
> -               disable_irq_wake(irq);
> +               int0002->wake_enable_count--;
>
>         return 0;
>  }
> @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data)
>         return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT);
>  }
>
> -static struct irq_chip int0002_byt_irqchip = {
> +static struct irq_chip int0002_irqchip = {
>         .name                   = DRV_NAME,
>         .irq_ack                = int0002_irq_ack,
>         .irq_mask               = int0002_irq_mask,
> @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = {
>         .irq_set_wake           = int0002_irq_set_wake,
>  };
>
> -static struct irq_chip int0002_cht_irqchip = {
> -       .name                   = DRV_NAME,
> -       .irq_ack                = int0002_irq_ack,
> -       .irq_mask               = int0002_irq_mask,
> -       .irq_unmask             = int0002_irq_unmask,
> -       /*
> -        * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI
> -        * and we don't want to mess with the ACPI SCI irq settings.
> -        */
> -       .flags                  = IRQCHIP_SKIP_SET_WAKE,
> -};
> -
>  static const struct x86_cpu_id int0002_cpu_ids[] = {
> -       X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT,     &int0002_byt_irqchip),
> -       X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT,        &int0002_cht_irqchip),
> +       X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
> +       X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
>         {}
>  };
>
> @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         const struct x86_cpu_id *cpu_id;
> -       struct gpio_chip *chip;
> +       struct int0002_data *int0002;
>         struct gpio_irq_chip *girq;
> +       struct gpio_chip *chip;
>         int irq, ret;
>
>         /* Menlow has a different INT0002 device? <sigh> */
> @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev)
>         if (irq < 0)
>                 return irq;
>
> -       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> -       if (!chip)
> +       int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL);
> +       if (!int0002)
>                 return -ENOMEM;
>
> +       int0002->parent_irq = irq;
> +
> +       chip = &int0002->chip;
>         chip->label = DRV_NAME;
>         chip->parent = dev;
>         chip->owner = THIS_MODULE;
> @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev)
>         }
>
>         girq = &chip->irq;
> -       girq->chip = (struct irq_chip *)cpu_id->driver_data;
> +       girq->chip = &int0002_irqchip;
>         /* This let us handle the parent IRQ in the driver */
>         girq->parent_handler = NULL;
>         girq->num_parents = 0;
> @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev)
>
>         acpi_register_wakeup_handler(irq, int0002_check_wake, NULL);
>         device_init_wakeup(dev, true);
> +       dev_set_drvdata(dev, int0002);
>         return 0;
>  }
>
> @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int int0002_suspend(struct device *dev)
> +{
> +       struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +       /*
> +        * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't
> +        * muck with it when firmware based suspend is used, otherwise we may
> +        * cause spurious wakeups from firmware managed suspend.
> +        */
> +       if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +               enable_irq_wake(int0002->parent_irq);
> +
> +       return 0;
> +}
> +
> +static int int0002_resume(struct device *dev)
> +{
> +       struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +       if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +               disable_irq_wake(int0002->parent_irq);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops int0002_pm_ops = {
> +       .suspend = int0002_suspend,
> +       .resume = int0002_resume,
> +};
> +
>  static const struct acpi_device_id int0002_acpi_ids[] = {
>         { "INT0002", 0 },
>         { },
> @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = {
>         .driver = {
>                 .name                   = DRV_NAME,
>                 .acpi_match_table       = int0002_acpi_ids,
> +               .pm                     = &int0002_pm_ops,
>         },
>         .probe  = int0002_probe,
>         .remove = int0002_remove,
> --
> 2.31.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Rafael J. Wysocki]

On Wed, May 12, 2021 at 2:56 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement
> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to
> the parents IRQ because this was breaking suspend (causing immediate
> wakeups) on an Asus E202SA.
>
> This workaround for the Asus E202SA is causing wakeup by USB keyboard to
> not work on other devices with Airmont CPU cores such as the Medion Akoya
> E1239T. In hindsight the problem with the Asus E202SA has nothing to do
> with Silvermont vs Airmont CPU cores, so the differentiation between the
> 2 types of CPU cores introduced by the previous fix is wrong.
>
> The real issue at hand is s2idle vs S3 suspend where the suspend is
> mostly handled by firmware. The parent IRQ for the INT0002 device is shared
> with the ACPI SCI and the real problem is that the INT0002 code should not
> be messing with the wakeup settings of that IRQ when suspend/resume is
> being handled by the firmware.
>
> Note that on systems which support both s2idle and S3 suspend, which
> suspend method to use can be changed at runtime.
>
> This patch fixes both the Asus E202SA spurious wakeups issue as well as
> the wakeup by USB keyboard not working on the Medion Akoya E1239T issue.
>
> These are both fixed by replacing the old workaround with delaying the
> enable_irq_wake(parent_irq) call till system-suspend time and protecting
> it with a !pm_suspend_via_firmware() check so that we still do not call
> it on devices using firmware-based (S3) suspend such as the Asus E202SA.
>
> Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds
> a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose
> is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes
> no sense.
>
> Cc: Maxim Mikityanskiy <maxtram95@gmail.com>
> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Makes sense to me.

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/platform/x86/Kconfig               |  2 +-
>  drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
>  2 files changed, 57 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4b67e74a747b..c2f608d5f1b7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -713,7 +713,7 @@ config INTEL_HID_EVENT
>
>  config INTEL_INT0002_VGPIO
>         tristate "Intel ACPI INT0002 Virtual GPIO driver"
> -       depends on GPIOLIB && ACPI
> +       depends on GPIOLIB && ACPI && PM_SLEEP
>         select GPIOLIB_IRQCHIP
>         help
>           Some peripherals on Bay Trail and Cherry Trail platforms signal a
> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
> index 289c6655d425..569342aa8926 100644
> --- a/drivers/platform/x86/intel_int0002_vgpio.c
> +++ b/drivers/platform/x86/intel_int0002_vgpio.c
> @@ -51,6 +51,12 @@
>  #define GPE0A_STS_PORT                 0x420
>  #define GPE0A_EN_PORT                  0x428
>
> +struct int0002_data {
> +       struct gpio_chip chip;
> +       int parent_irq;
> +       int wake_enable_count;
> +};
> +
>  /*
>   * As this is not a real GPIO at all, but just a hack to model an event in
>   * ACPI the get / set functions are dummy functions.
> @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data)
>  static int int0002_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> -       struct platform_device *pdev = to_platform_device(chip->parent);
> -       int irq = platform_get_irq(pdev, 0);
> +       struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip);
>
> -       /* Propagate to parent irq */
> +       /*
> +        * Applying of the wakeup flag to our parent IRQ is delayed till system
> +        * suspend, because we only want to do this when using s2idle.
> +        */
>         if (on)
> -               enable_irq_wake(irq);
> +               int0002->wake_enable_count++;
>         else
> -               disable_irq_wake(irq);
> +               int0002->wake_enable_count--;
>
>         return 0;
>  }
> @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data)
>         return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT);
>  }
>
> -static struct irq_chip int0002_byt_irqchip = {
> +static struct irq_chip int0002_irqchip = {
>         .name                   = DRV_NAME,
>         .irq_ack                = int0002_irq_ack,
>         .irq_mask               = int0002_irq_mask,
> @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = {
>         .irq_set_wake           = int0002_irq_set_wake,
>  };
>
> -static struct irq_chip int0002_cht_irqchip = {
> -       .name                   = DRV_NAME,
> -       .irq_ack                = int0002_irq_ack,
> -       .irq_mask               = int0002_irq_mask,
> -       .irq_unmask             = int0002_irq_unmask,
> -       /*
> -        * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI
> -        * and we don't want to mess with the ACPI SCI irq settings.
> -        */
> -       .flags                  = IRQCHIP_SKIP_SET_WAKE,
> -};
> -
>  static const struct x86_cpu_id int0002_cpu_ids[] = {
> -       X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT,     &int0002_byt_irqchip),
> -       X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT,        &int0002_cht_irqchip),
> +       X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
> +       X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
>         {}
>  };
>
> @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         const struct x86_cpu_id *cpu_id;
> -       struct gpio_chip *chip;
> +       struct int0002_data *int0002;
>         struct gpio_irq_chip *girq;
> +       struct gpio_chip *chip;
>         int irq, ret;
>
>         /* Menlow has a different INT0002 device? <sigh> */
> @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev)
>         if (irq < 0)
>                 return irq;
>
> -       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> -       if (!chip)
> +       int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL);
> +       if (!int0002)
>                 return -ENOMEM;
>
> +       int0002->parent_irq = irq;
> +
> +       chip = &int0002->chip;
>         chip->label = DRV_NAME;
>         chip->parent = dev;
>         chip->owner = THIS_MODULE;
> @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev)
>         }
>
>         girq = &chip->irq;
> -       girq->chip = (struct irq_chip *)cpu_id->driver_data;
> +       girq->chip = &int0002_irqchip;
>         /* This let us handle the parent IRQ in the driver */
>         girq->parent_handler = NULL;
>         girq->num_parents = 0;
> @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev)
>
>         acpi_register_wakeup_handler(irq, int0002_check_wake, NULL);
>         device_init_wakeup(dev, true);
> +       dev_set_drvdata(dev, int0002);
>         return 0;
>  }
>
> @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int int0002_suspend(struct device *dev)
> +{
> +       struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +       /*
> +        * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't
> +        * muck with it when firmware based suspend is used, otherwise we may
> +        * cause spurious wakeups from firmware managed suspend.
> +        */
> +       if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +               enable_irq_wake(int0002->parent_irq);
> +
> +       return 0;
> +}
> +
> +static int int0002_resume(struct device *dev)
> +{
> +       struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +       if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +               disable_irq_wake(int0002->parent_irq);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops int0002_pm_ops = {
> +       .suspend = int0002_suspend,
> +       .resume = int0002_resume,
> +};
> +
>  static const struct acpi_device_id int0002_acpi_ids[] = {
>         { "INT0002", 0 },
>         { },
> @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = {
>         .driver = {
>                 .name                   = DRV_NAME,
>                 .acpi_match_table       = int0002_acpi_ids,
> +               .pm                     = &int0002_pm_ops,
>         },
>         .probe  = int0002_probe,
>         .remove = int0002_remove,
> --
> 2.31.1
>

Re: [PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Hans de Goede]

Hi,

On 5/12/21 3:20 PM, Andy Shevchenko wrote:
> On Wed, May 12, 2021 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement
>> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to
>> the parents IRQ because this was breaking suspend (causing immediate
>> wakeups) on an Asus E202SA.
>>
>> This workaround for the Asus E202SA is causing wakeup by USB keyboard to
>> not work on other devices with Airmont CPU cores such as the Medion Akoya
>> E1239T. In hindsight the problem with the Asus E202SA has nothing to do
>> with Silvermont vs Airmont CPU cores, so the differentiation between the
>> 2 types of CPU cores introduced by the previous fix is wrong.
>>
>> The real issue at hand is s2idle vs S3 suspend where the suspend is
>> mostly handled by firmware. The parent IRQ for the INT0002 device is shared
>> with the ACPI SCI and the real problem is that the INT0002 code should not
>> be messing with the wakeup settings of that IRQ when suspend/resume is
>> being handled by the firmware.
>>
>> Note that on systems which support both s2idle and S3 suspend, which
>> suspend method to use can be changed at runtime.
>>
>> This patch fixes both the Asus E202SA spurious wakeups issue as well as
>> the wakeup by USB keyboard not working on the Medion Akoya E1239T issue.
>>
>> These are both fixed by replacing the old workaround with delaying the
>> enable_irq_wake(parent_irq) call till system-suspend time and protecting
>> it with a !pm_suspend_via_firmware() check so that we still do not call
>> it on devices using firmware-based (S3) suspend such as the Asus E202SA.
> 
>> Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds
>> a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose
>> is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes
>> no sense.
> 
> I like the new approach.
> One remark (or two :) is to the PM SLEEP thingy. Can we add a separate
> line for "depends on", so it will be easier to maintain in case we
> need to amend it somehow?

The depends on is already using && and I prefer to keep it that way
over splitting it into 3 separate depends on lines.

> Another one is to amend a helpline to
> reflect that the driver is dealing solely with wake events (I haven't
> checked the current text, so it might be already enforced).

The helptext already begins with this:

"Some peripherals on Bay Trail and Cherry Trail platforms signal a
Power Management Event (PME) to the Power Management Controller (PMC)
to wakeup the system."

Which IMHO makes it pretty clear this is all about wake events.

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you.

Regards,

Hans


> 
>> Cc: Maxim Mikityanskiy <maxtram95@gmail.com>
>> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/Kconfig               |  2 +-
>>  drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
>>  2 files changed, 57 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> index 4b67e74a747b..c2f608d5f1b7 100644
>> --- a/drivers/platform/x86/Kconfig
>> +++ b/drivers/platform/x86/Kconfig
>> @@ -713,7 +713,7 @@ config INTEL_HID_EVENT
>>
>>  config INTEL_INT0002_VGPIO
>>         tristate "Intel ACPI INT0002 Virtual GPIO driver"
>> -       depends on GPIOLIB && ACPI
>> +       depends on GPIOLIB && ACPI && PM_SLEEP
>>         select GPIOLIB_IRQCHIP
>>         help
>>           Some peripherals on Bay Trail and Cherry Trail platforms signal a
>> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
>> index 289c6655d425..569342aa8926 100644
>> --- a/drivers/platform/x86/intel_int0002_vgpio.c
>> +++ b/drivers/platform/x86/intel_int0002_vgpio.c
>> @@ -51,6 +51,12 @@
>>  #define GPE0A_STS_PORT                 0x420
>>  #define GPE0A_EN_PORT                  0x428
>>
>> +struct int0002_data {
>> +       struct gpio_chip chip;
>> +       int parent_irq;
>> +       int wake_enable_count;
>> +};
>> +
>>  /*
>>   * As this is not a real GPIO at all, but just a hack to model an event in
>>   * ACPI the get / set functions are dummy functions.
>> @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data)
>>  static int int0002_irq_set_wake(struct irq_data *data, unsigned int on)
>>  {
>>         struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
>> -       struct platform_device *pdev = to_platform_device(chip->parent);
>> -       int irq = platform_get_irq(pdev, 0);
>> +       struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip);
>>
>> -       /* Propagate to parent irq */
>> +       /*
>> +        * Applying of the wakeup flag to our parent IRQ is delayed till system
>> +        * suspend, because we only want to do this when using s2idle.
>> +        */
>>         if (on)
>> -               enable_irq_wake(irq);
>> +               int0002->wake_enable_count++;
>>         else
>> -               disable_irq_wake(irq);
>> +               int0002->wake_enable_count--;
>>
>>         return 0;
>>  }
>> @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data)
>>         return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT);
>>  }
>>
>> -static struct irq_chip int0002_byt_irqchip = {
>> +static struct irq_chip int0002_irqchip = {
>>         .name                   = DRV_NAME,
>>         .irq_ack                = int0002_irq_ack,
>>         .irq_mask               = int0002_irq_mask,
>> @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = {
>>         .irq_set_wake           = int0002_irq_set_wake,
>>  };
>>
>> -static struct irq_chip int0002_cht_irqchip = {
>> -       .name                   = DRV_NAME,
>> -       .irq_ack                = int0002_irq_ack,
>> -       .irq_mask               = int0002_irq_mask,
>> -       .irq_unmask             = int0002_irq_unmask,
>> -       /*
>> -        * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI
>> -        * and we don't want to mess with the ACPI SCI irq settings.
>> -        */
>> -       .flags                  = IRQCHIP_SKIP_SET_WAKE,
>> -};
>> -
>>  static const struct x86_cpu_id int0002_cpu_ids[] = {
>> -       X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT,     &int0002_byt_irqchip),
>> -       X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT,        &int0002_cht_irqchip),
>> +       X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
>> +       X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
>>         {}
>>  };
>>
>> @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev)
>>  {
>>         struct device *dev = &pdev->dev;
>>         const struct x86_cpu_id *cpu_id;
>> -       struct gpio_chip *chip;
>> +       struct int0002_data *int0002;
>>         struct gpio_irq_chip *girq;
>> +       struct gpio_chip *chip;
>>         int irq, ret;
>>
>>         /* Menlow has a different INT0002 device? <sigh> */
>> @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev)
>>         if (irq < 0)
>>                 return irq;
>>
>> -       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>> -       if (!chip)
>> +       int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL);
>> +       if (!int0002)
>>                 return -ENOMEM;
>>
>> +       int0002->parent_irq = irq;
>> +
>> +       chip = &int0002->chip;
>>         chip->label = DRV_NAME;
>>         chip->parent = dev;
>>         chip->owner = THIS_MODULE;
>> @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev)
>>         }
>>
>>         girq = &chip->irq;
>> -       girq->chip = (struct irq_chip *)cpu_id->driver_data;
>> +       girq->chip = &int0002_irqchip;
>>         /* This let us handle the parent IRQ in the driver */
>>         girq->parent_handler = NULL;
>>         girq->num_parents = 0;
>> @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev)
>>
>>         acpi_register_wakeup_handler(irq, int0002_check_wake, NULL);
>>         device_init_wakeup(dev, true);
>> +       dev_set_drvdata(dev, int0002);
>>         return 0;
>>  }
>>
>> @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev)
>>         return 0;
>>  }
>>
>> +static int int0002_suspend(struct device *dev)
>> +{
>> +       struct int0002_data *int0002 = dev_get_drvdata(dev);
>> +
>> +       /*
>> +        * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't
>> +        * muck with it when firmware based suspend is used, otherwise we may
>> +        * cause spurious wakeups from firmware managed suspend.
>> +        */
>> +       if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
>> +               enable_irq_wake(int0002->parent_irq);
>> +
>> +       return 0;
>> +}
>> +
>> +static int int0002_resume(struct device *dev)
>> +{
>> +       struct int0002_data *int0002 = dev_get_drvdata(dev);
>> +
>> +       if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
>> +               disable_irq_wake(int0002->parent_irq);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct dev_pm_ops int0002_pm_ops = {
>> +       .suspend = int0002_suspend,
>> +       .resume = int0002_resume,
>> +};
>> +
>>  static const struct acpi_device_id int0002_acpi_ids[] = {
>>         { "INT0002", 0 },
>>         { },
>> @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = {
>>         .driver = {
>>                 .name                   = DRV_NAME,
>>                 .acpi_match_table       = int0002_acpi_ids,
>> +               .pm                     = &int0002_pm_ops,
>>         },
>>         .probe  = int0002_probe,
>>         .remove = int0002_remove,
>> --
>> 2.31.1
>>
> 
> 

Re: [PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Hans de Goede]

Hi,

On 5/12/21 2:55 PM, Hans de Goede wrote:
> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement
> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to
> the parents IRQ because this was breaking suspend (causing immediate
> wakeups) on an Asus E202SA.
> 
> This workaround for the Asus E202SA is causing wakeup by USB keyboard to
> not work on other devices with Airmont CPU cores such as the Medion Akoya
> E1239T. In hindsight the problem with the Asus E202SA has nothing to do
> with Silvermont vs Airmont CPU cores, so the differentiation between the
> 2 types of CPU cores introduced by the previous fix is wrong.
> 
> The real issue at hand is s2idle vs S3 suspend where the suspend is
> mostly handled by firmware. The parent IRQ for the INT0002 device is shared
> with the ACPI SCI and the real problem is that the INT0002 code should not
> be messing with the wakeup settings of that IRQ when suspend/resume is
> being handled by the firmware.
> 
> Note that on systems which support both s2idle and S3 suspend, which
> suspend method to use can be changed at runtime.
> 
> This patch fixes both the Asus E202SA spurious wakeups issue as well as
> the wakeup by USB keyboard not working on the Medion Akoya E1239T issue.
> 
> These are both fixed by replacing the old workaround with delaying the
> enable_irq_wake(parent_irq) call till system-suspend time and protecting
> it with a !pm_suspend_via_firmware() check so that we still do not call
> it on devices using firmware-based (S3) suspend such as the Asus E202SA.
> 
> Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds
> a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose
> is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes
> no sense.
> 
> Cc: Maxim Mikityanskiy <maxtram95@gmail.com>
> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I've added this to my review-hans branch now and I will also include this
in the next pdx86-fixes pull-req for 5.13.

Regards,

Hans



> ---
>  drivers/platform/x86/Kconfig               |  2 +-
>  drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++-------
>  2 files changed, 57 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 4b67e74a747b..c2f608d5f1b7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -713,7 +713,7 @@ config INTEL_HID_EVENT
>  
>  config INTEL_INT0002_VGPIO
>  	tristate "Intel ACPI INT0002 Virtual GPIO driver"
> -	depends on GPIOLIB && ACPI
> +	depends on GPIOLIB && ACPI && PM_SLEEP
>  	select GPIOLIB_IRQCHIP
>  	help
>  	  Some peripherals on Bay Trail and Cherry Trail platforms signal a
> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
> index 289c6655d425..569342aa8926 100644
> --- a/drivers/platform/x86/intel_int0002_vgpio.c
> +++ b/drivers/platform/x86/intel_int0002_vgpio.c
> @@ -51,6 +51,12 @@
>  #define GPE0A_STS_PORT			0x420
>  #define GPE0A_EN_PORT			0x428
>  
> +struct int0002_data {
> +	struct gpio_chip chip;
> +	int parent_irq;
> +	int wake_enable_count;
> +};
> +
>  /*
>   * As this is not a real GPIO at all, but just a hack to model an event in
>   * ACPI the get / set functions are dummy functions.
> @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data)
>  static int int0002_irq_set_wake(struct irq_data *data, unsigned int on)
>  {
>  	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
> -	struct platform_device *pdev = to_platform_device(chip->parent);
> -	int irq = platform_get_irq(pdev, 0);
> +	struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip);
>  
> -	/* Propagate to parent irq */
> +	/*
> +	 * Applying of the wakeup flag to our parent IRQ is delayed till system
> +	 * suspend, because we only want to do this when using s2idle.
> +	 */
>  	if (on)
> -		enable_irq_wake(irq);
> +		int0002->wake_enable_count++;
>  	else
> -		disable_irq_wake(irq);
> +		int0002->wake_enable_count--;
>  
>  	return 0;
>  }
> @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data)
>  	return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT);
>  }
>  
> -static struct irq_chip int0002_byt_irqchip = {
> +static struct irq_chip int0002_irqchip = {
>  	.name			= DRV_NAME,
>  	.irq_ack		= int0002_irq_ack,
>  	.irq_mask		= int0002_irq_mask,
> @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = {
>  	.irq_set_wake		= int0002_irq_set_wake,
>  };
>  
> -static struct irq_chip int0002_cht_irqchip = {
> -	.name			= DRV_NAME,
> -	.irq_ack		= int0002_irq_ack,
> -	.irq_mask		= int0002_irq_mask,
> -	.irq_unmask		= int0002_irq_unmask,
> -	/*
> -	 * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI
> -	 * and we don't want to mess with the ACPI SCI irq settings.
> -	 */
> -	.flags			= IRQCHIP_SKIP_SET_WAKE,
> -};
> -
>  static const struct x86_cpu_id int0002_cpu_ids[] = {
> -	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT,	&int0002_byt_irqchip),
> -	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT,	&int0002_cht_irqchip),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL),
> +	X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL),
>  	{}
>  };
>  
> @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	const struct x86_cpu_id *cpu_id;
> -	struct gpio_chip *chip;
> +	struct int0002_data *int0002;
>  	struct gpio_irq_chip *girq;
> +	struct gpio_chip *chip;
>  	int irq, ret;
>  
>  	/* Menlow has a different INT0002 device? <sigh> */
> @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return irq;
>  
> -	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> -	if (!chip)
> +	int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL);
> +	if (!int0002)
>  		return -ENOMEM;
>  
> +	int0002->parent_irq = irq;
> +
> +	chip = &int0002->chip;
>  	chip->label = DRV_NAME;
>  	chip->parent = dev;
>  	chip->owner = THIS_MODULE;
> @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev)
>  	}
>  
>  	girq = &chip->irq;
> -	girq->chip = (struct irq_chip *)cpu_id->driver_data;
> +	girq->chip = &int0002_irqchip;
>  	/* This let us handle the parent IRQ in the driver */
>  	girq->parent_handler = NULL;
>  	girq->num_parents = 0;
> @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev)
>  
>  	acpi_register_wakeup_handler(irq, int0002_check_wake, NULL);
>  	device_init_wakeup(dev, true);
> +	dev_set_drvdata(dev, int0002);
>  	return 0;
>  }
>  
> @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int int0002_suspend(struct device *dev)
> +{
> +	struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +	/*
> +	 * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't
> +	 * muck with it when firmware based suspend is used, otherwise we may
> +	 * cause spurious wakeups from firmware managed suspend.
> +	 */
> +	if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +		enable_irq_wake(int0002->parent_irq);
> +
> +	return 0;
> +}
> +
> +static int int0002_resume(struct device *dev)
> +{
> +	struct int0002_data *int0002 = dev_get_drvdata(dev);
> +
> +	if (!pm_suspend_via_firmware() && int0002->wake_enable_count)
> +		disable_irq_wake(int0002->parent_irq);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops int0002_pm_ops = {
> +	.suspend = int0002_suspend,
> +	.resume = int0002_resume,
> +};
> +
>  static const struct acpi_device_id int0002_acpi_ids[] = {
>  	{ "INT0002", 0 },
>  	{ },
> @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = {
>  	.driver = {
>  		.name			= DRV_NAME,
>  		.acpi_match_table	= int0002_acpi_ids,
> +		.pm			= &int0002_pm_ops,
>  	},
>  	.probe	= int0002_probe,
>  	.remove	= int0002_remove,
>