[PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq

[PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq

[Raul E Rangel]

Today, i2c drivers are making the assumption that their IRQs can also
be used as wake IRQs. This isn't always the case and it can lead to
spurious wakes. This has recently started to affect AMD Chromebooks.
With the introduction of
d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
controller gained the capability to set the wake bit on each GPIO. The
ACPI specification defines two ways to inform the system if a device is
wake capable:
1) The _PRW object defines the GPE that can be used to wake the system.
2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.

Currently only the first method is supported. The i2c drivers don't have
any indication that the IRQ is wake capable, so they guess. This causes
spurious interrupts, for example:
* We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
  `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
  system.
* The IRQ line is active level low for this device and is pulled up by
  the power resource defined in `_PR0`/`_PR3`.
* The i2c driver will (incorrectly) arm the GPIO for wake by calling
  `enable_irq_wake` as part of its suspend hook.
* ACPI will power down the device since it doesn't have a wake GPE
  associated with it.
* When the device is powered down, the IRQ line will drop, and it will
  trigger a wake event.

See the following debug log:
[   42.335804] PM: Suspending system (s2idle)
[   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
[   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
[   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
[   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
[   42.535293] PM: Wakeup unrelated to ACPI SCI
[   42.535294] PM: resume from suspend-to-idle

In order to fix this, we need to take into account the wake capable bit
defined on the GpioInt. This is accomplished by:
* Migrating some of the i2c drivers over to using the PM subsystem to
  manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
  need to be migrated, I can do that depending on the feedback to this
  patch series.
* Expose the wake_capable bit from the ACPI GpioInt resource to the
  i2c core.
* Use the wake_capable bit in the i2c core to call
  `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
* Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
  handled by the i2c core.
* Make the ACPI device PM system aware of the wake_irq. This is
  necessary so the device doesn't incorrectly get powered down when a
  wake_irq is enabled.

I've tested this code with various combinations of having _PRW,
ExclusiveAndWake and power resources all defined or not defined, but it
would be great if others could test this out on their hardware.

Thanks,
Raul


Raul E Rangel (8):
  Input: elan_i2c - Use PM subsystem to manage wake irq
  HID: i2c-hid: Use PM subsystem to manage wake irq
  gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by
  i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq
  HID: i2c-hid: acpi: Stop setting wakeup_capable
  Input: elan_i2c - Don't set wake_irq when using ACPI
  HID: i2c-hid: Don't set wake_irq when using ACPI
  ACPI: PM: Take wake IRQ into consideration when entering
    suspend-to-idle

 drivers/acpi/device_pm.c            | 19 +++++++++++++++--
 drivers/gpio/gpio-pca953x.c         |  3 ++-
 drivers/gpio/gpiolib-acpi.c         | 11 +++++++++-
 drivers/gpio/gpiolib-acpi.h         |  2 ++
 drivers/hid/i2c-hid/i2c-hid-acpi.c  |  5 -----
 drivers/hid/i2c-hid/i2c-hid-core.c  | 33 +++++++++++------------------
 drivers/i2c/i2c-core-acpi.c         |  8 +++++--
 drivers/i2c/i2c-core-base.c         | 17 +++++++++------
 drivers/i2c/i2c-core.h              |  4 ++--
 drivers/input/mouse/elan_i2c_core.c | 14 +++++-------
 include/linux/acpi.h                | 14 +++++++++---
 11 files changed, 78 insertions(+), 52 deletions(-)

-- 
2.37.2.672.g94769d06f0-goog

[PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Raul E Rangel]

The Elan I2C touchpad driver is currently manually managing the wake
IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
and instead relies on the PM subsystem. This is done by calling
dev_pm_set_wake_irq.

i2c_device_probe already calls dev_pm_set_wake_irq when using device
tree, so it's only required when using ACPI. The net result is that this
change should be a no-op. i2c_device_remove also already calls
dev_pm_clear_wake_irq, so we don't need to do that in this driver.

I tested this on an ACPI system where the touchpad doesn't have _PRW
defined. I verified I can still wake the system and that the wake source
was the touchpad IRQ GPIO.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index e1758d5ffe4218..7d997d2b56436b 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -33,6 +33,7 @@
 #include <linux/jiffies.h>
 #include <linux/completion.h>
 #include <linux/of.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <asm/unaligned.h>
@@ -86,8 +87,6 @@ struct elan_tp_data {
 	u16			fw_page_size;
 	u32			fw_signature_address;
 
-	bool			irq_wake;
-
 	u8			min_baseline;
 	u8			max_baseline;
 	bool			baseline_ready;
@@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
 	 * Systems using device tree should set up wakeup via DTS,
 	 * the rest will configure device as wakeup source by default.
 	 */
-	if (!dev->of_node)
+	if (!dev->of_node) {
 		device_init_wakeup(dev, true);
+		dev_pm_set_wake_irq(dev, client->irq);
+	}
 
 	return 0;
 }
@@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
 
 	if (device_may_wakeup(dev)) {
 		ret = elan_sleep(data);
-		/* Enable wake from IRQ */
-		data->irq_wake = (enable_irq_wake(client->irq) == 0);
 	} else {
 		ret = elan_set_power(data, false);
 		if (ret)
@@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
 			dev_err(dev, "error %d enabling regulator\n", error);
 			goto err;
 		}
-	} else if (data->irq_wake) {
-		disable_irq_wake(client->irq);
-		data->irq_wake = false;
 	}
 
 	error = elan_set_power(data, true);
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 2/8] HID: i2c-hid: Use PM subsystem to manage wake irq

[Raul E Rangel]

The I2C hid driver is currently manually managing the wake
IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
and instead relies on the PM subsystem. This is done by calling
dev_pm_set_wake_irq.

i2c_device_probe already calls dev_pm_set_wake_irq when using device
tree, so it's only required when using ACPI. The net result is that this
change should be a no-op. i2c_device_remove also already calls
dev_pm_clear_wake_irq, so we don't need to do that in this driver.

I tested this on an ACPI system that has a HID touchscreen and verified
the IRQ was armed for wake on suspend.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-core.c | 33 +++++++++++-------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index baa169fadd6632..0b7a1a8b3e9a33 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/pm.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/device.h>
 #include <linux/wait.h>
 #include <linux/err.h>
@@ -116,7 +117,6 @@ struct i2c_hid {
 
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 
-	bool			irq_wake_enabled;
 	struct mutex		reset_lock;
 
 	struct i2chid_ops	*ops;
@@ -1036,6 +1036,15 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
 	if (ret < 0)
 		goto err_powered;
 
+	/*
+	 * Systems using device tree should set up wakeup via DTS,
+	 * the rest will configure device as wakeup source by default.
+	 */
+	if (!client->dev.of_node) {
+		device_init_wakeup(&client->dev, true);
+		dev_pm_set_wake_irq(&client->dev, client->irq);
+	}
+
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
 		ret = PTR_ERR(hid);
@@ -1119,7 +1128,6 @@ static int i2c_hid_core_suspend(struct device *dev)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid = ihid->hid;
 	int ret;
-	int wake_status;
 
 	ret = hid_driver_suspend(hid, PMSG_SUSPEND);
 	if (ret < 0)
@@ -1130,16 +1138,8 @@ static int i2c_hid_core_suspend(struct device *dev)
 
 	disable_irq(client->irq);
 
-	if (device_may_wakeup(&client->dev)) {
-		wake_status = enable_irq_wake(client->irq);
-		if (!wake_status)
-			ihid->irq_wake_enabled = true;
-		else
-			hid_warn(hid, "Failed to enable irq wake: %d\n",
-				wake_status);
-	} else {
+	if (!device_may_wakeup(&client->dev))
 		i2c_hid_core_power_down(ihid);
-	}
 
 	return 0;
 }
@@ -1150,18 +1150,9 @@ static int i2c_hid_core_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid = ihid->hid;
-	int wake_status;
 
-	if (!device_may_wakeup(&client->dev)) {
+	if (!device_may_wakeup(&client->dev))
 		i2c_hid_core_power_up(ihid);
-	} else if (ihid->irq_wake_enabled) {
-		wake_status = disable_irq_wake(client->irq);
-		if (!wake_status)
-			ihid->irq_wake_enabled = false;
-		else
-			hid_warn(hid, "Failed to disable irq wake: %d\n",
-				wake_status);
-	}
 
 	enable_irq(client->irq);
 
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by

[Raul E Rangel]

The ACPI spec defines the SharedAndWake and ExclusiveAndWake share type
keywords. This is an indication that the GPIO IRQ can also be used as a
wake source. This change exposes the wake_capable bit so drivers can
correctly enable wake functionality instead of making an assumption.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/gpio/gpio-pca953x.c |  3 ++-
 drivers/gpio/gpiolib-acpi.c | 11 ++++++++++-
 drivers/gpio/gpiolib-acpi.h |  2 ++
 include/linux/acpi.h        | 14 +++++++++++---
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
index ecd7d169470b06..df02c3eb34a294 100644
--- a/drivers/gpio/gpio-pca953x.c
+++ b/drivers/gpio/gpio-pca953x.c
@@ -130,7 +130,8 @@ static int pca953x_acpi_get_irq(struct device *dev)
 	if (ret)
 		dev_warn(dev, "can't add GPIO ACPI mapping\n");
 
-	ret = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), "irq-gpios", 0);
+	ret = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(dev), "irq-gpios", 0,
+				       NULL);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9be1376f9a627f..5cda2fcf7f43df 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -741,6 +741,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		lookup->info.pin_config = agpio->pin_config;
 		lookup->info.debounce = agpio->debounce_timeout;
 		lookup->info.gpioint = gpioint;
+		lookup->info.wake_capable = agpio->wake_capable;
 
 		/*
 		 * Polarity and triggering are only specified for GpioInt
@@ -991,6 +992,7 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
  * @adev: pointer to a ACPI device to get IRQ from
  * @name: optional name of GpioInt resource
  * @index: index of GpioInt resource (starting from %0)
+ * @wake_capable: Set to 1 if the IRQ is wake capable
  *
  * If the device has one or more GpioInt resources, this function can be
  * used to translate from the GPIO offset in the resource to the Linux IRQ
@@ -1002,9 +1004,13 @@ struct gpio_desc *acpi_node_get_gpiod(struct fwnode_handle *fwnode,
  * The function takes optional @name parameter. If the resource has a property
  * name, then only those will be taken into account.
  *
+ * The GPIO is considered wake capable if GpioInt specifies SharedAndWake or
+ * ExclusiveAndWake.
+ *
  * Return: Linux IRQ number (> %0) on success, negative errno on failure.
  */
-int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index)
+int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name,
+			     int index, int *wake_capable)
 {
 	int idx, i;
 	unsigned int irq_flags;
@@ -1061,6 +1067,9 @@ int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int ind
 				dev_dbg(&adev->dev, "IRQ %d already in use\n", irq);
 			}
 
+			if (wake_capable)
+				*wake_capable = info.wake_capable;
+
 			return irq;
 		}
 
diff --git a/drivers/gpio/gpiolib-acpi.h b/drivers/gpio/gpiolib-acpi.h
index e476558d947136..1ac6816839dbce 100644
--- a/drivers/gpio/gpiolib-acpi.h
+++ b/drivers/gpio/gpiolib-acpi.h
@@ -18,6 +18,7 @@ struct acpi_device;
  * @pin_config: pin bias as provided by ACPI
  * @polarity: interrupt polarity as provided by ACPI
  * @triggering: triggering type as provided by ACPI
+ * @wake_capable: wake capability as provided by ACPI
  * @debounce: debounce timeout as provided by ACPI
  * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
  */
@@ -28,6 +29,7 @@ struct acpi_gpio_info {
 	int pin_config;
 	int polarity;
 	int triggering;
+	bool wake_capable;
 	unsigned int debounce;
 	unsigned int quirks;
 };
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 6f64b2f3dc5479..7ee946758c5bcc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1202,7 +1202,8 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 				struct acpi_resource_gpio **agpio);
 bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
 			       struct acpi_resource_gpio **agpio);
-int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index);
+int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name,
+			     int index, int *wake_capable);
 #else
 static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 					      struct acpi_resource_gpio **agpio)
@@ -1215,7 +1216,8 @@ static inline bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
 	return false;
 }
 static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
-					   const char *name, int index)
+					   const char *name, int index,
+					   int *wake_capable)
 {
 	return -ENXIO;
 }
@@ -1223,7 +1225,13 @@ static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
 
 static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 {
-	return acpi_dev_gpio_irq_get_by(adev, NULL, index);
+	return acpi_dev_gpio_irq_get_by(adev, NULL, index, NULL);
+}
+
+static inline int acpi_dev_gpio_irq_get_wake(struct acpi_device *adev,
+					     int index, int *wake_capable)
+{
+	return acpi_dev_gpio_irq_get_by(adev, NULL, index, wake_capable);
 }
 
 /* Device properties */
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Raul E Rangel]

Device tree already has a mechanism to pass the wake_irq. It does this
by looking for the wakeup-source property and setting the
I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
ACPI GpioInt wake flag to determine if the interrupt can be used to wake
the system. Previously the i2c drivers had to make assumptions and
blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
If there is a device with an Active Low interrupt and the device gets
powered off while suspending, the interrupt line will go low since it's
no longer powered and wake the system. For this reason we should respect
the board designers wishes and honor the wake bit defined on the
GpioInt.

This change does not cover the ACPI Interrupt or IRQ resources.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/i2c/i2c-core-acpi.c |  8 ++++++--
 drivers/i2c/i2c-core-base.c | 17 +++++++++++------
 drivers/i2c/i2c-core.h      |  4 ++--
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index c762a879c4cc6b..cfe82a6ba3ef28 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
 /**
  * i2c_acpi_get_irq - get device IRQ number from ACPI
  * @client: Pointer to the I2C client device
+ * @wake_capable: Set to 1 if the IRQ is wake capable
  *
  * Find the IRQ number used by a specific client device.
  *
  * Return: The IRQ number or an error code.
  */
-int i2c_acpi_get_irq(struct i2c_client *client)
+int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
 	struct list_head resource_list;
@@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
 
 	INIT_LIST_HEAD(&resource_list);
 
+	if (wake_capable)
+		*wake_capable = 0;
+
 	ret = acpi_dev_get_resources(adev, &resource_list,
 				     i2c_acpi_add_resource, &irq);
 	if (ret < 0)
@@ -204,7 +208,7 @@ int i2c_acpi_get_irq(struct i2c_client *client)
 	acpi_dev_free_resource_list(&resource_list);
 
 	if (irq == -ENOENT)
-		irq = acpi_dev_gpio_irq_get(adev, 0);
+		irq = acpi_dev_gpio_irq_get_wake(adev, 0, wake_capable);
 
 	return irq;
 }
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 91007558bcb260..88f4ef76235f4e 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -468,6 +468,7 @@ static int i2c_device_probe(struct device *dev)
 	struct i2c_client	*client = i2c_verify_client(dev);
 	struct i2c_driver	*driver;
 	int status;
+	int acpi_wake_capable = 0;
 
 	if (!client)
 		return 0;
@@ -487,7 +488,7 @@ static int i2c_device_probe(struct device *dev)
 			if (irq == -EINVAL || irq == -ENODATA)
 				irq = of_irq_get(dev->of_node, 0);
 		} else if (ACPI_COMPANION(dev)) {
-			irq = i2c_acpi_get_irq(client);
+			irq = i2c_acpi_get_irq(client, &acpi_wake_capable);
 		}
 		if (irq == -EPROBE_DEFER) {
 			status = irq;
@@ -513,13 +514,17 @@ static int i2c_device_probe(struct device *dev)
 		goto put_sync_adapter;
 	}
 
-	if (client->flags & I2C_CLIENT_WAKE) {
+	if (client->flags & I2C_CLIENT_WAKE || acpi_wake_capable) {
 		int wakeirq;
 
-		wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
-		if (wakeirq == -EPROBE_DEFER) {
-			status = wakeirq;
-			goto put_sync_adapter;
+		if (acpi_wake_capable) {
+			wakeirq = client->irq;
+		} else {
+			wakeirq = of_irq_get_byname(dev->of_node, "wakeup");
+			if (wakeirq == -EPROBE_DEFER) {
+				status = wakeirq;
+				goto put_sync_adapter;
+			}
 		}
 
 		device_init_wakeup(&client->dev, true);
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 87e2c914f1c57b..8e336638a0cd2e 100644
--- a/drivers/i2c/i2c-core.h
+++ b/drivers/i2c/i2c-core.h
@@ -61,11 +61,11 @@ static inline int __i2c_check_suspended(struct i2c_adapter *adap)
 #ifdef CONFIG_ACPI
 void i2c_acpi_register_devices(struct i2c_adapter *adap);
 
-int i2c_acpi_get_irq(struct i2c_client *client);
+int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable);
 #else /* CONFIG_ACPI */
 static inline void i2c_acpi_register_devices(struct i2c_adapter *adap) { }
 
-static inline int i2c_acpi_get_irq(struct i2c_client *client)
+static inline int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
 {
 	return 0;
 }
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 5/8] HID: i2c-hid: acpi: Stop setting wakeup_capable

[Raul E Rangel]

This is now handled by the i2c-core driver.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index b96ae15e0ad917..375c77c3db74d9 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
 
 	acpi_device_fix_up_power(adev);
 
-	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
-		device_set_wakeup_capable(dev, true);
-		device_set_wakeup_enable(dev, false);
-	}
-
 	return i2c_hid_core_probe(client, &ihid_acpi->ops,
 				  hid_descriptor_address, 0);
 }
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 6/8] Input: elan_i2c - Don't set wake_irq when using ACPI

[Raul E Rangel]

The i2c-core will now handle setting the wake_irq for ACPI systems.

I didn't delete the whole block since this also covers systems that
don't use ACPI or DT, but I'm honestly not sure if that's a valid
config.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/input/mouse/elan_i2c_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 7d997d2b56436b..d434c8ff8c4ca2 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1333,10 +1333,10 @@ static int elan_probe(struct i2c_client *client,
 	}
 
 	/*
-	 * Systems using device tree should set up wakeup via DTS,
+	 * Systems using device tree should set up wakeup via DTS or ACPI,
 	 * the rest will configure device as wakeup source by default.
 	 */
-	if (!dev->of_node) {
+	if (!dev->of_node && !has_acpi_companion(dev)) {
 		device_init_wakeup(dev, true);
 		dev_pm_set_wake_irq(dev, client->irq);
 	}
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 7/8] HID: i2c-hid: Don't set wake_irq when using ACPI

[Raul E Rangel]

The i2c-core will now handle setting the wake_irq for ACPI systems.

I didn't delete the whole block since this also covers systems that
don't use ACPI or DT, but I'm honestly not sure if that's a valid
config.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 0b7a1a8b3e9a33..630e8dcda1100d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -1037,10 +1037,10 @@ int i2c_hid_core_probe(struct i2c_client *client, struct i2chid_ops *ops,
 		goto err_powered;
 
 	/*
-	 * Systems using device tree should set up wakeup via DTS,
+	 * Systems using device tree should set up wakeup via DTS or ACPI,
 	 * the rest will configure device as wakeup source by default.
 	 */
-	if (!client->dev.of_node) {
+	if (!client->dev.of_node && !has_acpi_companion(&client->dev)) {
 		device_init_wakeup(&client->dev, true);
 		dev_pm_set_wake_irq(&client->dev, client->irq);
 	}
-- 
2.37.2.672.g94769d06f0-goog

[PATCH 8/8] ACPI: PM: Take wake IRQ into consideration when entering suspend-to-idle

[Raul E Rangel]

This change adds support for ACPI devices that use ExclusiveAndWake or
SharedAndWake in their _CRS GpioInt definition (instead of using _PRW),
and also provide power resources. Previously the ACPI subsystem had no
idea if the device had a wake capable interrupt armed. This resulted
in the ACPI device PM system placing the device into D3Cold, and thus
cutting power to the device. With this change we will now query the
_S0W method to figure out the appropriate wake capable D-state.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/acpi/device_pm.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 9dce1245689ca2..6bc81f525d5160 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -681,8 +681,23 @@ static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device *adev,
 		d_min = ret;
 		wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
 			&& adev->wakeup.sleep_state >= target_state;
-	} else {
-		wakeup = adev->wakeup.flags.valid;
+	} else if (acpi_device_can_wakeup(adev)) {
+		/* ACPI GPE from specified by _PRW. */
+		wakeup = true;
+	} else if (device_may_wakeup(dev) && dev->power.wakeirq) {
+		/*
+		 * The ACPI subsystem doesn't manage the wake bit for IRQs
+		 * defined with ExclusiveAndWake and SharedAndWake. Instead we
+		 * expect them to be managed via the PM subsystem. Drivers
+		 * should call dev_pm_set_wake_irq to register an IRQ as a wake
+		 * source.
+		 *
+		 * If a device has a wake IRQ attached we need to check the
+		 * _S0W method to get the correct wake D-state. Otherwise we
+		 * end up putting the device into D3Cold which will more than
+		 * likely disable wake functionality.
+		 */
+		wakeup = true;
 	}
 
 	/*
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH 3/8] gpiolib: acpi: Add wake_capable parameter to acpi_dev_gpio_irq_get_by

[kernel test robot]

Hi Raul,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on hid/for-next]
[also build test ERROR on brgl/gpio/for-next rafael-pm/linux-next linus/master v6.0-rc3 next-20220830]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Raul-E-Rangel/acpi-i2c-Use-SharedAndWake-and-ExclusiveAndWake-to-enable-wake-irq/20220831-071916
base:   https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220831/202208311235.pxWOoHPE-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/131a07025a591f4ca6d7540f1055bc03f8cf64af
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Raul-E-Rangel/acpi-i2c-Use-SharedAndWake-and-ExclusiveAndWake-to-enable-wake-irq/20220831-071916
        git checkout 131a07025a591f4ca6d7540f1055bc03f8cf64af
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/net/ethernet/mellanox/mlxbf_gige/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c: In function 'mlxbf_gige_probe':
>> drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c:343:19: error: too few arguments to function 'acpi_dev_gpio_irq_get_by'
     343 |         phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c:8:
   include/linux/acpi.h:1212:19: note: declared here
    1212 | static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev,
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/acpi_dev_gpio_irq_get_by +343 drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c

f92e1869d74e1a David Thompson 2021-06-24  272  
f92e1869d74e1a David Thompson 2021-06-24  273  static int mlxbf_gige_probe(struct platform_device *pdev)
f92e1869d74e1a David Thompson 2021-06-24  274  {
f92e1869d74e1a David Thompson 2021-06-24  275  	struct phy_device *phydev;
f92e1869d74e1a David Thompson 2021-06-24  276  	struct net_device *netdev;
f92e1869d74e1a David Thompson 2021-06-24  277  	struct mlxbf_gige *priv;
f92e1869d74e1a David Thompson 2021-06-24  278  	void __iomem *llu_base;
f92e1869d74e1a David Thompson 2021-06-24  279  	void __iomem *plu_base;
f92e1869d74e1a David Thompson 2021-06-24  280  	void __iomem *base;
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  281  	int addr, phy_irq;
f92e1869d74e1a David Thompson 2021-06-24  282  	u64 control;
f92e1869d74e1a David Thompson 2021-06-24  283  	int err;
f92e1869d74e1a David Thompson 2021-06-24  284  
464a57281f29af Cai Huoqing    2021-08-31  285  	base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_MAC);
f92e1869d74e1a David Thompson 2021-06-24  286  	if (IS_ERR(base))
f92e1869d74e1a David Thompson 2021-06-24  287  		return PTR_ERR(base);
f92e1869d74e1a David Thompson 2021-06-24  288  
464a57281f29af Cai Huoqing    2021-08-31  289  	llu_base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_LLU);
f92e1869d74e1a David Thompson 2021-06-24  290  	if (IS_ERR(llu_base))
f92e1869d74e1a David Thompson 2021-06-24  291  		return PTR_ERR(llu_base);
f92e1869d74e1a David Thompson 2021-06-24  292  
464a57281f29af Cai Huoqing    2021-08-31  293  	plu_base = devm_platform_ioremap_resource(pdev, MLXBF_GIGE_RES_PLU);
f92e1869d74e1a David Thompson 2021-06-24  294  	if (IS_ERR(plu_base))
f92e1869d74e1a David Thompson 2021-06-24  295  		return PTR_ERR(plu_base);
f92e1869d74e1a David Thompson 2021-06-24  296  
f92e1869d74e1a David Thompson 2021-06-24  297  	/* Perform general init of GigE block */
f92e1869d74e1a David Thompson 2021-06-24  298  	control = readq(base + MLXBF_GIGE_CONTROL);
f92e1869d74e1a David Thompson 2021-06-24  299  	control |= MLXBF_GIGE_CONTROL_PORT_EN;
f92e1869d74e1a David Thompson 2021-06-24  300  	writeq(control, base + MLXBF_GIGE_CONTROL);
f92e1869d74e1a David Thompson 2021-06-24  301  
f92e1869d74e1a David Thompson 2021-06-24  302  	netdev = devm_alloc_etherdev(&pdev->dev, sizeof(*priv));
f92e1869d74e1a David Thompson 2021-06-24  303  	if (!netdev)
f92e1869d74e1a David Thompson 2021-06-24  304  		return -ENOMEM;
f92e1869d74e1a David Thompson 2021-06-24  305  
f92e1869d74e1a David Thompson 2021-06-24  306  	SET_NETDEV_DEV(netdev, &pdev->dev);
f92e1869d74e1a David Thompson 2021-06-24  307  	netdev->netdev_ops = &mlxbf_gige_netdev_ops;
f92e1869d74e1a David Thompson 2021-06-24  308  	netdev->ethtool_ops = &mlxbf_gige_ethtool_ops;
f92e1869d74e1a David Thompson 2021-06-24  309  	priv = netdev_priv(netdev);
f92e1869d74e1a David Thompson 2021-06-24  310  	priv->netdev = netdev;
f92e1869d74e1a David Thompson 2021-06-24  311  
f92e1869d74e1a David Thompson 2021-06-24  312  	platform_set_drvdata(pdev, priv);
f92e1869d74e1a David Thompson 2021-06-24  313  	priv->dev = &pdev->dev;
f92e1869d74e1a David Thompson 2021-06-24  314  	priv->pdev = pdev;
f92e1869d74e1a David Thompson 2021-06-24  315  
f92e1869d74e1a David Thompson 2021-06-24  316  	spin_lock_init(&priv->lock);
f92e1869d74e1a David Thompson 2021-06-24  317  
f92e1869d74e1a David Thompson 2021-06-24  318  	/* Attach MDIO device */
f92e1869d74e1a David Thompson 2021-06-24  319  	err = mlxbf_gige_mdio_probe(pdev, priv);
f92e1869d74e1a David Thompson 2021-06-24  320  	if (err)
f92e1869d74e1a David Thompson 2021-06-24  321  		return err;
f92e1869d74e1a David Thompson 2021-06-24  322  
f92e1869d74e1a David Thompson 2021-06-24  323  	priv->base = base;
f92e1869d74e1a David Thompson 2021-06-24  324  	priv->llu_base = llu_base;
f92e1869d74e1a David Thompson 2021-06-24  325  	priv->plu_base = plu_base;
f92e1869d74e1a David Thompson 2021-06-24  326  
f92e1869d74e1a David Thompson 2021-06-24  327  	priv->rx_q_entries = MLXBF_GIGE_DEFAULT_RXQ_SZ;
f92e1869d74e1a David Thompson 2021-06-24  328  	priv->tx_q_entries = MLXBF_GIGE_DEFAULT_TXQ_SZ;
f92e1869d74e1a David Thompson 2021-06-24  329  
f92e1869d74e1a David Thompson 2021-06-24  330  	/* Write initial MAC address to hardware */
f92e1869d74e1a David Thompson 2021-06-24  331  	mlxbf_gige_initial_mac(priv);
f92e1869d74e1a David Thompson 2021-06-24  332  
f92e1869d74e1a David Thompson 2021-06-24  333  	err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
f92e1869d74e1a David Thompson 2021-06-24  334  	if (err) {
f92e1869d74e1a David Thompson 2021-06-24  335  		dev_err(&pdev->dev, "DMA configuration failed: 0x%x\n", err);
f92e1869d74e1a David Thompson 2021-06-24  336  		goto out;
f92e1869d74e1a David Thompson 2021-06-24  337  	}
f92e1869d74e1a David Thompson 2021-06-24  338  
f92e1869d74e1a David Thompson 2021-06-24  339  	priv->error_irq = platform_get_irq(pdev, MLXBF_GIGE_ERROR_INTR_IDX);
f92e1869d74e1a David Thompson 2021-06-24  340  	priv->rx_irq = platform_get_irq(pdev, MLXBF_GIGE_RECEIVE_PKT_INTR_IDX);
f92e1869d74e1a David Thompson 2021-06-24  341  	priv->llu_plu_irq = platform_get_irq(pdev, MLXBF_GIGE_LLU_PLU_INTR_IDX);
f92e1869d74e1a David Thompson 2021-06-24  342  
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15 @343  	phy_irq = acpi_dev_gpio_irq_get_by(ACPI_COMPANION(&pdev->dev), "phy-gpios", 0);
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  344  	if (phy_irq < 0) {
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  345  		dev_err(&pdev->dev, "Error getting PHY irq. Use polling instead");
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  346  		phy_irq = PHY_POLL;
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  347  	}
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  348  
f92e1869d74e1a David Thompson 2021-06-24  349  	phydev = phy_find_first(priv->mdiobus);
f92e1869d74e1a David Thompson 2021-06-24  350  	if (!phydev) {
f92e1869d74e1a David Thompson 2021-06-24  351  		err = -ENODEV;
f92e1869d74e1a David Thompson 2021-06-24  352  		goto out;
f92e1869d74e1a David Thompson 2021-06-24  353  	}
f92e1869d74e1a David Thompson 2021-06-24  354  
f92e1869d74e1a David Thompson 2021-06-24  355  	addr = phydev->mdio.addr;
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  356  	priv->mdiobus->irq[addr] = phy_irq;
6c2a6ddca76327 Asmaa Mnebhi   2021-10-15  357  	phydev->irq = phy_irq;
f92e1869d74e1a David Thompson 2021-06-24  358  
f92e1869d74e1a David Thompson 2021-06-24  359  	err = phy_connect_direct(netdev, phydev,
f92e1869d74e1a David Thompson 2021-06-24  360  				 mlxbf_gige_adjust_link,
f92e1869d74e1a David Thompson 2021-06-24  361  				 PHY_INTERFACE_MODE_GMII);
f92e1869d74e1a David Thompson 2021-06-24  362  	if (err) {
f92e1869d74e1a David Thompson 2021-06-24  363  		dev_err(&pdev->dev, "Could not attach to PHY\n");
f92e1869d74e1a David Thompson 2021-06-24  364  		goto out;
f92e1869d74e1a David Thompson 2021-06-24  365  	}
f92e1869d74e1a David Thompson 2021-06-24  366  
f92e1869d74e1a David Thompson 2021-06-24  367  	/* MAC only supports 1000T full duplex mode */
f92e1869d74e1a David Thompson 2021-06-24  368  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
f92e1869d74e1a David Thompson 2021-06-24  369  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Full_BIT);
f92e1869d74e1a David Thompson 2021-06-24  370  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
f92e1869d74e1a David Thompson 2021-06-24  371  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Full_BIT);
f92e1869d74e1a David Thompson 2021-06-24  372  	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
f92e1869d74e1a David Thompson 2021-06-24  373  
f92e1869d74e1a David Thompson 2021-06-24  374  	/* Only symmetric pause with flow control enabled is supported so no
f92e1869d74e1a David Thompson 2021-06-24  375  	 * need to negotiate pause.
f92e1869d74e1a David Thompson 2021-06-24  376  	 */
f92e1869d74e1a David Thompson 2021-06-24  377  	linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev->advertising);
f92e1869d74e1a David Thompson 2021-06-24  378  	linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, phydev->advertising);
f92e1869d74e1a David Thompson 2021-06-24  379  
f92e1869d74e1a David Thompson 2021-06-24  380  	/* Display information about attached PHY device */
f92e1869d74e1a David Thompson 2021-06-24  381  	phy_attached_info(phydev);
f92e1869d74e1a David Thompson 2021-06-24  382  
f92e1869d74e1a David Thompson 2021-06-24  383  	err = register_netdev(netdev);
f92e1869d74e1a David Thompson 2021-06-24  384  	if (err) {
f92e1869d74e1a David Thompson 2021-06-24  385  		dev_err(&pdev->dev, "Failed to register netdev\n");
f92e1869d74e1a David Thompson 2021-06-24  386  		phy_disconnect(phydev);
f92e1869d74e1a David Thompson 2021-06-24  387  		goto out;
f92e1869d74e1a David Thompson 2021-06-24  388  	}
f92e1869d74e1a David Thompson 2021-06-24  389  
f92e1869d74e1a David Thompson 2021-06-24  390  	return 0;
f92e1869d74e1a David Thompson 2021-06-24  391  
f92e1869d74e1a David Thompson 2021-06-24  392  out:
f92e1869d74e1a David Thompson 2021-06-24  393  	mlxbf_gige_mdio_remove(priv);
f92e1869d74e1a David Thompson 2021-06-24  394  	return err;
f92e1869d74e1a David Thompson 2021-06-24  395  }
f92e1869d74e1a David Thompson 2021-06-24  396  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq

[Andy Shevchenko]

On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
> Today, i2c drivers are making the assumption that their IRQs can also
> be used as wake IRQs. This isn't always the case and it can lead to
> spurious wakes. This has recently started to affect AMD Chromebooks.
> With the introduction of
> d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
> controller gained the capability to set the wake bit on each GPIO. The
> ACPI specification defines two ways to inform the system if a device is
> wake capable:
> 1) The _PRW object defines the GPE that can be used to wake the system.
> 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
> 
> Currently only the first method is supported. The i2c drivers don't have
> any indication that the IRQ is wake capable, so they guess. This causes
> spurious interrupts, for example:
> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
>   `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
>   system.
> * The IRQ line is active level low for this device and is pulled up by
>   the power resource defined in `_PR0`/`_PR3`.
> * The i2c driver will (incorrectly) arm the GPIO for wake by calling
>   `enable_irq_wake` as part of its suspend hook.
> * ACPI will power down the device since it doesn't have a wake GPE
>   associated with it.
> * When the device is powered down, the IRQ line will drop, and it will
>   trigger a wake event.
> 
> See the following debug log:
> [   42.335804] PM: Suspending system (s2idle)
> [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
> [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
> [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> [   42.535293] PM: Wakeup unrelated to ACPI SCI
> [   42.535294] PM: resume from suspend-to-idle
> 
> In order to fix this, we need to take into account the wake capable bit
> defined on the GpioInt. This is accomplished by:
> * Migrating some of the i2c drivers over to using the PM subsystem to
>   manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
>   need to be migrated, I can do that depending on the feedback to this
>   patch series.
> * Expose the wake_capable bit from the ACPI GpioInt resource to the
>   i2c core.
> * Use the wake_capable bit in the i2c core to call
>   `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
> * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
>   handled by the i2c core.
> * Make the ACPI device PM system aware of the wake_irq. This is
>   necessary so the device doesn't incorrectly get powered down when a
>   wake_irq is enabled.
> 
> I've tested this code with various combinations of having _PRW,
> ExclusiveAndWake and power resources all defined or not defined, but it
> would be great if others could test this out on their hardware.

I have got only cover letter and a single patch (#3). What's going on?

Note: I'm also reviewer of I²C DesignWare driver, you really have to
fix your tools / submission process and try again. No review for this
series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq

[Raul Rangel]

Interesting... The patch series is here:
https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@chromium.org/

I'll look into why you only got added to 2 of the emails.

On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
> > Today, i2c drivers are making the assumption that their IRQs can also
> > be used as wake IRQs. This isn't always the case and it can lead to
> > spurious wakes. This has recently started to affect AMD Chromebooks.
> > With the introduction of
> > d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
> > controller gained the capability to set the wake bit on each GPIO. The
> > ACPI specification defines two ways to inform the system if a device is
> > wake capable:
> > 1) The _PRW object defines the GPE that can be used to wake the system.
> > 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
> >
> > Currently only the first method is supported. The i2c drivers don't have
> > any indication that the IRQ is wake capable, so they guess. This causes
> > spurious interrupts, for example:
> > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> >   `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
> >   system.
> > * The IRQ line is active level low for this device and is pulled up by
> >   the power resource defined in `_PR0`/`_PR3`.
> > * The i2c driver will (incorrectly) arm the GPIO for wake by calling
> >   `enable_irq_wake` as part of its suspend hook.
> > * ACPI will power down the device since it doesn't have a wake GPE
> >   associated with it.
> > * When the device is powered down, the IRQ line will drop, and it will
> >   trigger a wake event.
> >
> > See the following debug log:
> > [   42.335804] PM: Suspending system (s2idle)
> > [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> > [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
> > [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
> > [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> > [   42.535293] PM: Wakeup unrelated to ACPI SCI
> > [   42.535294] PM: resume from suspend-to-idle
> >
> > In order to fix this, we need to take into account the wake capable bit
> > defined on the GpioInt. This is accomplished by:
> > * Migrating some of the i2c drivers over to using the PM subsystem to
> >   manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
> >   need to be migrated, I can do that depending on the feedback to this
> >   patch series.
> > * Expose the wake_capable bit from the ACPI GpioInt resource to the
> >   i2c core.
> > * Use the wake_capable bit in the i2c core to call
> >   `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
> > * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
> >   handled by the i2c core.
> > * Make the ACPI device PM system aware of the wake_irq. This is
> >   necessary so the device doesn't incorrectly get powered down when a
> >   wake_irq is enabled.
> >
> > I've tested this code with various combinations of having _PRW,
> > ExclusiveAndWake and power resources all defined or not defined, but it
> > would be great if others could test this out on their hardware.
>
> I have got only cover letter and a single patch (#3). What's going on?
>
> Note: I'm also reviewer of I涎 DesignWare driver, you really have to
> fix your tools / submission process and try again. No review for this
> series.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 0/8] acpi: i2c: Use SharedAndWake and ExclusiveAndWake to enable wake irq

[Hans de Goede]

Hi,

On 8/31/22 16:37, Raul Rangel wrote:
> Interesting... The patch series is here:
> https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@chromium.org/
> 
> I'll look into why you only got added to 2 of the emails.

FWIW I also received the full series without problems.

I'll try to reply to this soon-ish, but I have a bit of
a patch backlog to process and I'm trying to process
the backlog in FIFO order and this is one of the last
series in the backlog ...

Regards,

Hans


> 
> On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>>
>> On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
>>> Today, i2c drivers are making the assumption that their IRQs can also
>>> be used as wake IRQs. This isn't always the case and it can lead to
>>> spurious wakes. This has recently started to affect AMD Chromebooks.
>>> With the introduction of
>>> d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
>>> controller gained the capability to set the wake bit on each GPIO. The
>>> ACPI specification defines two ways to inform the system if a device is
>>> wake capable:
>>> 1) The _PRW object defines the GPE that can be used to wake the system.
>>> 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
>>>
>>> Currently only the first method is supported. The i2c drivers don't have
>>> any indication that the IRQ is wake capable, so they guess. This causes
>>> spurious interrupts, for example:
>>> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
>>>   `_PRW` or `ExclusiveAndWake` so that means the device can't wake the
>>>   system.
>>> * The IRQ line is active level low for this device and is pulled up by
>>>   the power resource defined in `_PR0`/`_PR3`.
>>> * The i2c driver will (incorrectly) arm the GPIO for wake by calling
>>>   `enable_irq_wake` as part of its suspend hook.
>>> * ACPI will power down the device since it doesn't have a wake GPE
>>>   associated with it.
>>> * When the device is powered down, the IRQ line will drop, and it will
>>>   trigger a wake event.
>>>
>>> See the following debug log:
>>> [   42.335804] PM: Suspending system (s2idle)
>>> [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
>>> [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
>>> [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
>>> [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
>>> [   42.535293] PM: Wakeup unrelated to ACPI SCI
>>> [   42.535294] PM: resume from suspend-to-idle
>>>
>>> In order to fix this, we need to take into account the wake capable bit
>>> defined on the GpioInt. This is accomplished by:
>>> * Migrating some of the i2c drivers over to using the PM subsystem to
>>>   manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
>>>   need to be migrated, I can do that depending on the feedback to this
>>>   patch series.
>>> * Expose the wake_capable bit from the ACPI GpioInt resource to the
>>>   i2c core.
>>> * Use the wake_capable bit in the i2c core to call
>>>   `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
>>> * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
>>>   handled by the i2c core.
>>> * Make the ACPI device PM system aware of the wake_irq. This is
>>>   necessary so the device doesn't incorrectly get powered down when a
>>>   wake_irq is enabled.
>>>
>>> I've tested this code with various combinations of having _PRW,
>>> ExclusiveAndWake and power resources all defined or not defined, but it
>>> would be great if others could test this out on their hardware.
>>
>> I have got only cover letter and a single patch (#3). What's going on?
>>
>> Note: I'm also reviewer of I涎 DesignWare driver, you really have to
>> fix your tools / submission process and try again. No review for this
>> series.
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
>>
> 

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Rafael J. Wysocki]

On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
>
> The Elan I2C touchpad driver is currently manually managing the wake
> IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> and instead relies on the PM subsystem. This is done by calling
> dev_pm_set_wake_irq.
>
> i2c_device_probe already calls dev_pm_set_wake_irq when using device
> tree, so it's only required when using ACPI. The net result is that this
> change should be a no-op. i2c_device_remove also already calls
> dev_pm_clear_wake_irq, so we don't need to do that in this driver.
>
> I tested this on an ACPI system where the touchpad doesn't have _PRW
> defined. I verified I can still wake the system and that the wake source
> was the touchpad IRQ GPIO.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

I like this a lot, but the assumption in the wakeirq code is that the
IRQ in question will be dedicated for signaling wakeup.  Does it hold
here?

> ---
>
>  drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index e1758d5ffe4218..7d997d2b56436b 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -33,6 +33,7 @@
>  #include <linux/jiffies.h>
>  #include <linux/completion.h>
>  #include <linux/of.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <asm/unaligned.h>
> @@ -86,8 +87,6 @@ struct elan_tp_data {
>         u16                     fw_page_size;
>         u32                     fw_signature_address;
>
> -       bool                    irq_wake;
> -
>         u8                      min_baseline;
>         u8                      max_baseline;
>         bool                    baseline_ready;
> @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
>          * Systems using device tree should set up wakeup via DTS,
>          * the rest will configure device as wakeup source by default.
>          */
> -       if (!dev->of_node)
> +       if (!dev->of_node) {
>                 device_init_wakeup(dev, true);
> +               dev_pm_set_wake_irq(dev, client->irq);
> +       }
>
>         return 0;
>  }
> @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
>
>         if (device_may_wakeup(dev)) {
>                 ret = elan_sleep(data);
> -               /* Enable wake from IRQ */
> -               data->irq_wake = (enable_irq_wake(client->irq) == 0);
>         } else {
>                 ret = elan_set_power(data, false);
>                 if (ret)
> @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
>                         dev_err(dev, "error %d enabling regulator\n", error);
>                         goto err;
>                 }
> -       } else if (data->irq_wake) {
> -               disable_irq_wake(client->irq);
> -               data->irq_wake = false;
>         }
>
>         error = elan_set_power(data, true);
> --
> 2.37.2.672.g94769d06f0-goog
>

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Raul Rangel]

On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > The Elan I2C touchpad driver is currently manually managing the wake
> > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > and instead relies on the PM subsystem. This is done by calling
> > dev_pm_set_wake_irq.
> >
> > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > tree, so it's only required when using ACPI. The net result is that this
> > change should be a no-op. i2c_device_remove also already calls
> > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> >
> > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > defined. I verified I can still wake the system and that the wake source
> > was the touchpad IRQ GPIO.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>


> I like this a lot, but the assumption in the wakeirq code is that the
> IRQ in question will be dedicated for signaling wakeup.  Does it hold
> here?

The wakeirq code defines two methods: `dev_pm_set_wake_irq` and
`dev_pm_set_dedicated_wake_irq`.
The latter is used when you have a dedicated wakeup signal. In this
driver it's currently assumed
that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`.

This change in theory also fixes a bug where you define a dedicated
wake irq in DT, but
then the driver enables the `client->irq` as a wake source. In
practice this doesn't happen
since the elan touchpads only have a single IRQ line.

>
> > ---
> >
> >  drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
> >  1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> > index e1758d5ffe4218..7d997d2b56436b 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -33,6 +33,7 @@
> >  #include <linux/jiffies.h>
> >  #include <linux/completion.h>
> >  #include <linux/of.h>
> > +#include <linux/pm_wakeirq.h>
> >  #include <linux/property.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <asm/unaligned.h>
> > @@ -86,8 +87,6 @@ struct elan_tp_data {
> >         u16                     fw_page_size;
> >         u32                     fw_signature_address;
> >
> > -       bool                    irq_wake;
> > -
> >         u8                      min_baseline;
> >         u8                      max_baseline;
> >         bool                    baseline_ready;
> > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
> >          * Systems using device tree should set up wakeup via DTS,
> >          * the rest will configure device as wakeup source by default.
> >          */
> > -       if (!dev->of_node)
> > +       if (!dev->of_node) {
> >                 device_init_wakeup(dev, true);
> > +               dev_pm_set_wake_irq(dev, client->irq);
> > +       }
> >
> >         return 0;
> >  }
> > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
> >
> >         if (device_may_wakeup(dev)) {
> >                 ret = elan_sleep(data);
> > -               /* Enable wake from IRQ */
> > -               data->irq_wake = (enable_irq_wake(client->irq) == 0);
> >         } else {
> >                 ret = elan_set_power(data, false);
> >                 if (ret)
> > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
> >                         dev_err(dev, "error %d enabling regulator\n", error);
> >                         goto err;
> >                 }
> > -       } else if (data->irq_wake) {
> > -               disable_irq_wake(client->irq);
> > -               data->irq_wake = false;
> >         }
> >
> >         error = elan_set_power(data, true);
> > --
> > 2.37.2.672.g94769d06f0-goog
> >

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Rafael J. Wysocki]

On Wed, Aug 31, 2022 at 8:14 PM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > The Elan I2C touchpad driver is currently manually managing the wake
> > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > and instead relies on the PM subsystem. This is done by calling
> > > dev_pm_set_wake_irq.
> > >
> > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > tree, so it's only required when using ACPI. The net result is that this
> > > change should be a no-op. i2c_device_remove also already calls
> > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > >
> > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > defined. I verified I can still wake the system and that the wake source
> > > was the touchpad IRQ GPIO.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >
>
>
> > I like this a lot, but the assumption in the wakeirq code is that the
> > IRQ in question will be dedicated for signaling wakeup.  Does it hold
> > here?
>
> The wakeirq code defines two methods: `dev_pm_set_wake_irq` and
> `dev_pm_set_dedicated_wake_irq`.
> The latter is used when you have a dedicated wakeup signal. In this
> driver it's currently assumed
> that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`.
>
> This change in theory also fixes a bug where you define a dedicated
> wake irq in DT, but
> then the driver enables the `client->irq` as a wake source. In
> practice this doesn't happen
> since the elan touchpads only have a single IRQ line.

OK, thanks!

Please feel free to add

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

to the patch.

> >
> > > ---
> > >
> > >  drivers/input/mouse/elan_i2c_core.c | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> > > index e1758d5ffe4218..7d997d2b56436b 100644
> > > --- a/drivers/input/mouse/elan_i2c_core.c
> > > +++ b/drivers/input/mouse/elan_i2c_core.c
> > > @@ -33,6 +33,7 @@
> > >  #include <linux/jiffies.h>
> > >  #include <linux/completion.h>
> > >  #include <linux/of.h>
> > > +#include <linux/pm_wakeirq.h>
> > >  #include <linux/property.h>
> > >  #include <linux/regulator/consumer.h>
> > >  #include <asm/unaligned.h>
> > > @@ -86,8 +87,6 @@ struct elan_tp_data {
> > >         u16                     fw_page_size;
> > >         u32                     fw_signature_address;
> > >
> > > -       bool                    irq_wake;
> > > -
> > >         u8                      min_baseline;
> > >         u8                      max_baseline;
> > >         bool                    baseline_ready;
> > > @@ -1337,8 +1336,10 @@ static int elan_probe(struct i2c_client *client,
> > >          * Systems using device tree should set up wakeup via DTS,
> > >          * the rest will configure device as wakeup source by default.
> > >          */
> > > -       if (!dev->of_node)
> > > +       if (!dev->of_node) {
> > >                 device_init_wakeup(dev, true);
> > > +               dev_pm_set_wake_irq(dev, client->irq);
> > > +       }
> > >
> > >         return 0;
> > >  }
> > > @@ -1362,8 +1363,6 @@ static int __maybe_unused elan_suspend(struct device *dev)
> > >
> > >         if (device_may_wakeup(dev)) {
> > >                 ret = elan_sleep(data);
> > > -               /* Enable wake from IRQ */
> > > -               data->irq_wake = (enable_irq_wake(client->irq) == 0);
> > >         } else {
> > >                 ret = elan_set_power(data, false);
> > >                 if (ret)
> > > @@ -1394,9 +1393,6 @@ static int __maybe_unused elan_resume(struct device *dev)
> > >                         dev_err(dev, "error %d enabling regulator\n", error);
> > >                         goto err;
> > >                 }
> > > -       } else if (data->irq_wake) {
> > > -               disable_irq_wake(client->irq);
> > > -               data->irq_wake = false;
> > >         }
> > >
> > >         error = elan_set_power(data, true);
> > > --
> > > 2.37.2.672.g94769d06f0-goog
> > >

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Dmitry Torokhov]

On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > The Elan I2C touchpad driver is currently manually managing the wake
> > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > and instead relies on the PM subsystem. This is done by calling
> > dev_pm_set_wake_irq.
> >
> > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > tree, so it's only required when using ACPI. The net result is that this
> > change should be a no-op. i2c_device_remove also already calls
> > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> >
> > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > defined. I verified I can still wake the system and that the wake source
> > was the touchpad IRQ GPIO.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> 
> I like this a lot [...]

I also like this a lot, but this assumes that firmware has correct
settings for the interrupt... Unfortunately it is not always the case
and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
ect) do not mark interrupt as wakeup:

src/mainboard/google/glados/variants/chell/overridetree.cb

                        chip drivers/i2c/generic
                                register "hid" = ""ELAN0000""
                                register "desc" = ""ELAN Touchpad""
                                register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
                                register "wake" = "GPE0_DW0_05"
                                device i2c 15 on end

I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt
to be marked as wakeup.

(we do correctly mark GPE as wakeup).

So we need to do something about older devices....

-- 
Dmitry

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Dmitry Torokhov]

On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > The Elan I2C touchpad driver is currently manually managing the wake
> > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > and instead relies on the PM subsystem. This is done by calling
> > > dev_pm_set_wake_irq.
> > >
> > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > tree, so it's only required when using ACPI. The net result is that this
> > > change should be a no-op. i2c_device_remove also already calls
> > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > >
> > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > defined. I verified I can still wake the system and that the wake source
> > > was the touchpad IRQ GPIO.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > 
> > I like this a lot [...]
> 
> I also like this a lot, but this assumes that firmware has correct
> settings for the interrupt... Unfortunately it is not always the case
> and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> ect) do not mark interrupt as wakeup:
> 
> src/mainboard/google/glados/variants/chell/overridetree.cb
> 
>                         chip drivers/i2c/generic
>                                 register "hid" = ""ELAN0000""
>                                 register "desc" = ""ELAN Touchpad""
>                                 register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
>                                 register "wake" = "GPE0_DW0_05"
>                                 device i2c 15 on end
> 
> I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt
> to be marked as wakeup.
> 
> (we do correctly mark GPE as wakeup).
> 
> So we need to do something about older devices....

After re-reading the patch I believe this comment is more applicable to
the followup patch to elan_i2c, not this one, which is fine on its own.

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Raul Rangel]

On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > >
> > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > and instead relies on the PM subsystem. This is done by calling
> > > > dev_pm_set_wake_irq.
> > > >
> > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > tree, so it's only required when using ACPI. The net result is that this
> > > > change should be a no-op. i2c_device_remove also already calls
> > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > >
> > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > defined. I verified I can still wake the system and that the wake source
> > > > was the touchpad IRQ GPIO.
> > > >
> > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >
> > > I like this a lot [...]
> >

> > I also like this a lot, but this assumes that firmware has correct
> > settings for the interrupt... Unfortunately it is not always the case
> > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > ect) do not mark interrupt as wakeup:
> >
> > src/mainboard/google/glados/variants/chell/overridetree.cb
> >
> >                         chip drivers/i2c/generic
> >                                 register "hid" = ""ELAN0000""
> >                                 register "desc" = ""ELAN Touchpad""
> >                                 register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> >                                 register "wake" = "GPE0_DW0_05"
> >                                 device i2c 15 on end
> >

So the above entry specifies the `wake` register. This generates an
ACPI _PRW resource. The patch series will actually fix devices like
this. Today without this patch series we get two wake events for a
device. The ACPI wake GPE specified by the _PRW resource, and the
erroneous GPIO wake event. But you bring up a good point.

I wrote a quick and dirty script (https://0paste.com/391849) to parse
the coreboot device tree entries. Open source firmware is great isn't
it? ;)

$ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
src/mainboard/google/eve/devicetree.cb
1
chip drivers/i2c/hid
register "generic.hid" = ""ACPI0C50""
register "generic.desc" = ""Touchpad""
register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
register "hid_desc_reg_offset" = "0x1"
device i2c 49 on end
end
src/mainboard/google/eve/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""GOOG0008""
register "desc" = ""Touchpad EC Interface""
device i2c 1e on end
end
src/mainboard/google/drallion/variants/drallion/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
src/mainboard/google/drallion/variants/drallion/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 15 on end
end
src/mainboard/google/sarien/variants/arcada/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
src/mainboard/google/sarien/variants/arcada/devicetree.cb
1
chip drivers/i2c/hid
register "generic.hid" = ""PNP0C50""
register "generic.desc" = ""Cirque Touchpad""
register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
register "generic.probed" = "1"
register "hid_desc_reg_offset" = "0x20"
device i2c 2a on end
end
src/mainboard/google/sarien/variants/sarien/devicetree.cb
1
chip drivers/i2c/generic
register "hid" = ""ELAN0000""
register "desc" = ""ELAN Touchpad""
register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
register "probed" = "1"
device i2c 2c on end
end
Total Touchpad: 202
Total Wake: 195

Out of all the touchpads defined on ChromeOS it looks like only 4
devices are missing a wake declaration. I omitted touchpanels because
ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
defines some devices with i2c board_info and it sets the
`I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
expected. `i2c_device_probe` requires a `wakeup` irq to be present in
the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
the device tree was missing wake attributes.

Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
can figure out how to add the above boards to chromeos_laptop.c and
get the wake attribute plumbed, or I can add something directly to the
elan_i2c_core, etc so others can add overrides for their boards there.
I'll also send out CLs to fix the device tree configs (not that we
would run a FW qual just for this change).


> > I assume it should have been ACPI_IRQ_WAKE_LEVEL_LOW for the interrupt
> > to be marked as wakeup.
> >
> > (we do correctly mark GPE as wakeup).
> >
> > So we need to do something about older devices....
>
> After re-reading the patch I believe this comment is more applicable to
> the followup patch to elan_i2c, not this one, which is fine on its own.
>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Tony Lindgren]

* Rafael J. Wysocki <rafael@kernel.org> [220831 18:35]:
> On Wed, Aug 31, 2022 at 8:14 PM Raul Rangel <rrangel@chromium.org> wrote:
> >
> > On Wed, Aug 31, 2022 at 12:01 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > >
> > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > and instead relies on the PM subsystem. This is done by calling
> > > > dev_pm_set_wake_irq.
> > > >
> > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > tree, so it's only required when using ACPI. The net result is that this
> > > > change should be a no-op. i2c_device_remove also already calls
> > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > >
> > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > defined. I verified I can still wake the system and that the wake source
> > > > was the touchpad IRQ GPIO.
> > > >
> > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >
> >
> >
> > > I like this a lot, but the assumption in the wakeirq code is that the
> > > IRQ in question will be dedicated for signaling wakeup.  Does it hold
> > > here?
> >
> > The wakeirq code defines two methods: `dev_pm_set_wake_irq` and
> > `dev_pm_set_dedicated_wake_irq`.
> > The latter is used when you have a dedicated wakeup signal. In this
> > driver it's currently assumed
> > that the IRQ and the wake IRQ are the same, so I used `dev_pm_set_wake_irq`.
> >
> > This change in theory also fixes a bug where you define a dedicated
> > wake irq in DT, but
> > then the driver enables the `client->irq` as a wake source. In
> > practice this doesn't happen
> > since the elan touchpads only have a single IRQ line.
> 
> OK, thanks!
> 
> Please feel free to add
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> to the patch.

Looks good to me too:

Reviewed-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Dmitry Torokhov]

On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:
> On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > > >
> > > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > > and instead relies on the PM subsystem. This is done by calling
> > > > > dev_pm_set_wake_irq.
> > > > >
> > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > > tree, so it's only required when using ACPI. The net result is that this
> > > > > change should be a no-op. i2c_device_remove also already calls
> > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > > >
> > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > > defined. I verified I can still wake the system and that the wake source
> > > > > was the touchpad IRQ GPIO.
> > > > >
> > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > >
> > > > I like this a lot [...]
> > >
> 
> > > I also like this a lot, but this assumes that firmware has correct
> > > settings for the interrupt... Unfortunately it is not always the case
> > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > > ect) do not mark interrupt as wakeup:
> > >
> > > src/mainboard/google/glados/variants/chell/overridetree.cb
> > >
> > >                         chip drivers/i2c/generic
> > >                                 register "hid" = ""ELAN0000""
> > >                                 register "desc" = ""ELAN Touchpad""
> > >                                 register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > >                                 register "wake" = "GPE0_DW0_05"
> > >                                 device i2c 15 on end
> > >
> 
> So the above entry specifies the `wake` register. This generates an
> ACPI _PRW resource. The patch series will actually fix devices like
> this. Today without this patch series we get two wake events for a
> device. The ACPI wake GPE specified by the _PRW resource, and the
> erroneous GPIO wake event. But you bring up a good point.

Does this mean that the example that we currently have in coreboot
documentation (Documentation/acpi/devicetree.md) is not correct:

device pci 15.0 on
        chip drivers/i2c/generic
                register "hid" = ""ELAN0000""
                register "desc" = ""ELAN Touchpad""
                register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
                register "wake" = "GPE0_DW0_21"
                device i2c 15 on end
        end
end # I2C #0

Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
for the same device?

> 
> I wrote a quick and dirty script (https://0paste.com/391849) to parse
> the coreboot device tree entries. Open source firmware is great isn't
> it? ;)
> 
> $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
> src/mainboard/google/eve/devicetree.cb

...

> src/mainboard/google/sarien/variants/sarien/devicetree.cb
> 1
> chip drivers/i2c/generic
> register "hid" = ""ELAN0000""
> register "desc" = ""ELAN Touchpad""
> register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
> register "probed" = "1"
> device i2c 2c on end
> end
> Total Touchpad: 202
> Total Wake: 195
> 
> Out of all the touchpads defined on ChromeOS it looks like only 4
> devices are missing a wake declaration. I omitted touchpanels because
> ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
> defines some devices with i2c board_info and it sets the
> `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
> expected. `i2c_device_probe` requires a `wakeup` irq to be present in
> the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming

No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
will return an error and we'll take the "else if (client->irq > 0)"
branch and will set up client->irq as the wakeup irq.

> the device tree was missing wake attributes.

> 
> Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
> can figure out how to add the above boards to chromeos_laptop.c and
> get the wake attribute plumbed, or I can add something directly to the
> elan_i2c_core, etc so others can add overrides for their boards there.
> I'll also send out CLs to fix the device tree configs (not that we
> would run a FW qual just for this change).

My preference is to limit board-specific hacks in drivers if we can, so
adding missing properties to chromeos_laptop.c would be my preference.

Thanks.

-- 
Dmitry

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Raul Rangel]

On Fri, Sep 2, 2022 at 11:07 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:
> > On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > > > >
> > > > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > > > and instead relies on the PM subsystem. This is done by calling
> > > > > > dev_pm_set_wake_irq.
> > > > > >
> > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > > > tree, so it's only required when using ACPI. The net result is that this
> > > > > > change should be a no-op. i2c_device_remove also already calls
> > > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > > > >
> > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > > > defined. I verified I can still wake the system and that the wake source
> > > > > > was the touchpad IRQ GPIO.
> > > > > >
> > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > > >
> > > > > I like this a lot [...]
> > > >
> >
> > > > I also like this a lot, but this assumes that firmware has correct
> > > > settings for the interrupt... Unfortunately it is not always the case
> > > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > > > ect) do not mark interrupt as wakeup:
> > > >
> > > > src/mainboard/google/glados/variants/chell/overridetree.cb
> > > >
> > > >                         chip drivers/i2c/generic
> > > >                                 register "hid" = ""ELAN0000""
> > > >                                 register "desc" = ""ELAN Touchpad""
> > > >                                 register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > > >                                 register "wake" = "GPE0_DW0_05"
> > > >                                 device i2c 15 on end
> > > >
> >
> > So the above entry specifies the `wake` register. This generates an
> > ACPI _PRW resource. The patch series will actually fix devices like
> > this. Today without this patch series we get two wake events for a
> > device. The ACPI wake GPE specified by the _PRW resource, and the
> > erroneous GPIO wake event. But you bring up a good point.
>


> Does this mean that the example that we currently have in coreboot
> documentation (Documentation/acpi/devicetree.md) is not correct:
>
> device pci 15.0 on
>         chip drivers/i2c/generic
>                 register "hid" = ""ELAN0000""
>                 register "desc" = ""ELAN Touchpad""
>                 register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
>                 register "wake" = "GPE0_DW0_21"
>                 device i2c 15 on end
>         end
> end # I2C #0
>
> Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
> for the same device?

Hrmm, yeah that is wrong and will cause duplicate wake events for the
device. I'll push a CL to clean up the documentation.

>
> >
> > I wrote a quick and dirty script (https://0paste.com/391849) to parse
> > the coreboot device tree entries. Open source firmware is great isn't
> > it? ;)
> >
> > $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
> > src/mainboard/google/eve/devicetree.cb
>
> ...
>
> > src/mainboard/google/sarien/variants/sarien/devicetree.cb
> > 1
> > chip drivers/i2c/generic
> > register "hid" = ""ELAN0000""
> > register "desc" = ""ELAN Touchpad""
> > register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
> > register "probed" = "1"
> > device i2c 2c on end
> > end
> > Total Touchpad: 202
> > Total Wake: 195
> >
> > Out of all the touchpads defined on ChromeOS it looks like only 4
> > devices are missing a wake declaration. I omitted touchpanels because
> > ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
> > defines some devices with i2c board_info and it sets the
> > `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
> > expected. `i2c_device_probe` requires a `wakeup` irq to be present in
> > the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
>
> No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
> will return an error and we'll take the "else if (client->irq > 0)"
> branch and will set up client->irq as the wakeup irq.
>
> > the device tree was missing wake attributes.

Oh thanks for pointing that out. I might refactor patch #4 to just set
the `I2C_CLIENT_WAKE` flag when `acpi_wake_capable` is true.

>
> >
> > Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
> > can figure out how to add the above boards to chromeos_laptop.c and
> > get the wake attribute plumbed, or I can add something directly to the
> > elan_i2c_core, etc so others can add overrides for their boards there.
> > I'll also send out CLs to fix the device tree configs (not that we
> > would run a FW qual just for this change).
>
> My preference is to limit board-specific hacks in drivers if we can, so
> adding missing properties to chromeos_laptop.c would be my preference.

How should we handle non chromeos boards?

>
> Thanks.
>
> --
> Dmitry

Thanks!

Re: [PATCH 1/8] Input: elan_i2c - Use PM subsystem to manage wake irq

[Dmitry Torokhov]

On Tue, Sep 06, 2022 at 11:18:49AM -0600, Raul Rangel wrote:
> On Fri, Sep 2, 2022 at 11:07 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Wed, Aug 31, 2022 at 08:17:23PM -0600, Raul Rangel wrote:
> > > On Wed, Aug 31, 2022 at 1:16 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Wed, Aug 31, 2022 at 12:12:41PM -0700, Dmitry Torokhov wrote:
> > > > > On Wed, Aug 31, 2022 at 08:01:12PM +0200, Rafael J. Wysocki wrote:
> > > > > > On Wed, Aug 31, 2022 at 1:16 AM Raul E Rangel <rrangel@chromium.org> wrote:
> > > > > > >
> > > > > > > The Elan I2C touchpad driver is currently manually managing the wake
> > > > > > > IRQ. This change removes the explicit enable_irq_wake/disable_irq_wake
> > > > > > > and instead relies on the PM subsystem. This is done by calling
> > > > > > > dev_pm_set_wake_irq.
> > > > > > >
> > > > > > > i2c_device_probe already calls dev_pm_set_wake_irq when using device
> > > > > > > tree, so it's only required when using ACPI. The net result is that this
> > > > > > > change should be a no-op. i2c_device_remove also already calls
> > > > > > > dev_pm_clear_wake_irq, so we don't need to do that in this driver.
> > > > > > >
> > > > > > > I tested this on an ACPI system where the touchpad doesn't have _PRW
> > > > > > > defined. I verified I can still wake the system and that the wake source
> > > > > > > was the touchpad IRQ GPIO.
> > > > > > >
> > > > > > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > > > >
> > > > > > I like this a lot [...]
> > > > >
> > >
> > > > > I also like this a lot, but this assumes that firmware has correct
> > > > > settings for the interrupt... Unfortunately it is not always the case
> > > > > and I see that at least Chrome OS devices, such as glados line (cave, chell, sentry,
> > > > > ect) do not mark interrupt as wakeup:
> > > > >
> > > > > src/mainboard/google/glados/variants/chell/overridetree.cb
> > > > >
> > > > >                         chip drivers/i2c/generic
> > > > >                                 register "hid" = ""ELAN0000""
> > > > >                                 register "desc" = ""ELAN Touchpad""
> > > > >                                 register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_B3_IRQ)"
> > > > >                                 register "wake" = "GPE0_DW0_05"
> > > > >                                 device i2c 15 on end
> > > > >
> > >
> > > So the above entry specifies the `wake` register. This generates an
> > > ACPI _PRW resource. The patch series will actually fix devices like
> > > this. Today without this patch series we get two wake events for a
> > > device. The ACPI wake GPE specified by the _PRW resource, and the
> > > erroneous GPIO wake event. But you bring up a good point.
> >
> 
> 
> > Does this mean that the example that we currently have in coreboot
> > documentation (Documentation/acpi/devicetree.md) is not correct:
> >
> > device pci 15.0 on
> >         chip drivers/i2c/generic
> >                 register "hid" = ""ELAN0000""
> >                 register "desc" = ""ELAN Touchpad""
> >                 register "irq" = "ACPI_IRQ_WAKE_LEVEL_LOW(GPP_A21_IRQ)"
> >                 register "wake" = "GPE0_DW0_21"
> >                 device i2c 15 on end
> >         end
> > end # I2C #0
> >
> > Doesn't in say that we have both GpioIrq and GPE wakeup methods defined
> > for the same device?
> 
> Hrmm, yeah that is wrong and will cause duplicate wake events for the
> device. I'll push a CL to clean up the documentation.

Thanks. I think we also need to clean up our ADL boards (and likely
more).

> 
> >
> > >
> > > I wrote a quick and dirty script (https://0paste.com/391849) to parse
> > > the coreboot device tree entries. Open source firmware is great isn't
> > > it? ;)
> > >
> > > $ find src/mainboard/google/ -iname '*.cb' | xargs awk -f touch.awk --
> > > src/mainboard/google/eve/devicetree.cb
> >
> > ...
> >
> > > src/mainboard/google/sarien/variants/sarien/devicetree.cb
> > > 1
> > > chip drivers/i2c/generic
> > > register "hid" = ""ELAN0000""
> > > register "desc" = ""ELAN Touchpad""
> > > register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_B3_IRQ)"
> > > register "probed" = "1"
> > > device i2c 2c on end
> > > end
> > > Total Touchpad: 202
> > > Total Wake: 195
> > >
> > > Out of all the touchpads defined on ChromeOS it looks like only 4
> > > devices are missing a wake declaration. I omitted touchpanels because
> > > ChromeOS doesn't use those as a wake source. chromeos_laptop.c already
> > > defines some devices with i2c board_info and it sets the
> > > `I2C_CLIENT_WAKE` flag. I'm not sure if this is actually working as
> > > expected. `i2c_device_probe` requires a `wakeup` irq to be present in
> > > the device tree if the `I2C_CLIENT_WAKE` flag is set, but I'm assuming
> >
> > No it does not. If there is no wakeup IRQ defined of_irq_get_byname()
> > will return an error and we'll take the "else if (client->irq > 0)"
> > branch and will set up client->irq as the wakeup irq.
> >
> > > the device tree was missing wake attributes.
> 
> Oh thanks for pointing that out. I might refactor patch #4 to just set
> the `I2C_CLIENT_WAKE` flag when `acpi_wake_capable` is true.
> 
> >
> > >
> > > Anyway, patches 6, and 7 are the ones that drop the legacy behavior. I
> > > can figure out how to add the above boards to chromeos_laptop.c and
> > > get the wake attribute plumbed, or I can add something directly to the
> > > elan_i2c_core, etc so others can add overrides for their boards there.
> > > I'll also send out CLs to fix the device tree configs (not that we
> > > would run a FW qual just for this change).
> >
> > My preference is to limit board-specific hacks in drivers if we can, so
> > adding missing properties to chromeos_laptop.c would be my preference.
> 
> How should we handle non chromeos boards?

My preference would be to shove something like chromeos_laptop into
drivers/platform/x86... Something like
drivers/platform/x86/x86-android-tablets.c

Thanks.

-- 
Dmitry

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Dmitry Torokhov]

On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> Device tree already has a mechanism to pass the wake_irq. It does this
> by looking for the wakeup-source property and setting the
> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> the system. Previously the i2c drivers had to make assumptions and
> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> If there is a device with an Active Low interrupt and the device gets
> powered off while suspending, the interrupt line will go low since it's
> no longer powered and wake the system. For this reason we should respect
> the board designers wishes and honor the wake bit defined on the
> GpioInt.
> 
> This change does not cover the ACPI Interrupt or IRQ resources.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
>  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
>  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
>  drivers/i2c/i2c-core.h      |  4 ++--
>  3 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
>  /**
>   * i2c_acpi_get_irq - get device IRQ number from ACPI
>   * @client: Pointer to the I2C client device
> + * @wake_capable: Set to 1 if the IRQ is wake capable
>   *
>   * Find the IRQ number used by a specific client device.
>   *
>   * Return: The IRQ number or an error code.
>   */
> -int i2c_acpi_get_irq(struct i2c_client *client)
> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>  	struct list_head resource_list;
> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
>  
>  	INIT_LIST_HEAD(&resource_list);
>  
> +	if (wake_capable)
> +		*wake_capable = 0;
> +
>  	ret = acpi_dev_get_resources(adev, &resource_list,
>  				     i2c_acpi_add_resource, &irq);

You also need to handle "Interrupt(..., ...AndWake)" case here. I would
look into maybe defining

#define IORESOURCE_IRQ_WAKECAPABLE	(1<<6)

in include/linux/ioport.h and plumbing it through from ACPI layer.

Thanks.

-- 
Dmitry

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Raul Rangel]

On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > Device tree already has a mechanism to pass the wake_irq. It does this
> > by looking for the wakeup-source property and setting the
> > I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > the system. Previously the i2c drivers had to make assumptions and
> > blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > If there is a device with an Active Low interrupt and the device gets
> > powered off while suspending, the interrupt line will go low since it's
> > no longer powered and wake the system. For this reason we should respect
> > the board designers wishes and honor the wake bit defined on the
> > GpioInt.
> >
> > This change does not cover the ACPI Interrupt or IRQ resources.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> >  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
> >  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> >  drivers/i2c/i2c-core.h      |  4 ++--
> >  3 files changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > --- a/drivers/i2c/i2c-core-acpi.c
> > +++ b/drivers/i2c/i2c-core-acpi.c
> > @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> >  /**
> >   * i2c_acpi_get_irq - get device IRQ number from ACPI
> >   * @client: Pointer to the I2C client device
> > + * @wake_capable: Set to 1 if the IRQ is wake capable
> >   *
> >   * Find the IRQ number used by a specific client device.
> >   *
> >   * Return: The IRQ number or an error code.
> >   */
> > -int i2c_acpi_get_irq(struct i2c_client *client)
> > +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> >  {
> >       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> >       struct list_head resource_list;
> > @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> >
> >       INIT_LIST_HEAD(&resource_list);
> >
> > +     if (wake_capable)
> > +             *wake_capable = 0;
> > +
> >       ret = acpi_dev_get_resources(adev, &resource_list,
> >                                    i2c_acpi_add_resource, &irq);
>


> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> look into maybe defining
>
> #define IORESOURCE_IRQ_WAKECAPABLE      (1<<6)
>
> in include/linux/ioport.h and plumbing it through from ACPI layer.
>
> Thanks.

AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
wake a system from suspend/suspend-to-idle. It requires either a GPE
or GPIO controller to wake the system. This is the reason I haven't
pushed patches to handle the Interrupt/IRQ resource. Can anyone
confirm?

Thanks

>
> --
> Dmitry

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Dmitry Torokhov]

On Tue, Sep 06, 2022 at 08:00:00PM -0600, Raul Rangel wrote:
> On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > > Device tree already has a mechanism to pass the wake_irq. It does this
> > > by looking for the wakeup-source property and setting the
> > > I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > > ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > > the system. Previously the i2c drivers had to make assumptions and
> > > blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > > If there is a device with an Active Low interrupt and the device gets
> > > powered off while suspending, the interrupt line will go low since it's
> > > no longer powered and wake the system. For this reason we should respect
> > > the board designers wishes and honor the wake bit defined on the
> > > GpioInt.
> > >
> > > This change does not cover the ACPI Interrupt or IRQ resources.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > ---
> > >
> > >  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
> > >  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > >  drivers/i2c/i2c-core.h      |  4 ++--
> > >  3 files changed, 19 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > > index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > > --- a/drivers/i2c/i2c-core-acpi.c
> > > +++ b/drivers/i2c/i2c-core-acpi.c
> > > @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > >  /**
> > >   * i2c_acpi_get_irq - get device IRQ number from ACPI
> > >   * @client: Pointer to the I2C client device
> > > + * @wake_capable: Set to 1 if the IRQ is wake capable
> > >   *
> > >   * Find the IRQ number used by a specific client device.
> > >   *
> > >   * Return: The IRQ number or an error code.
> > >   */
> > > -int i2c_acpi_get_irq(struct i2c_client *client)
> > > +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > >  {
> > >       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > >       struct list_head resource_list;
> > > @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> > >
> > >       INIT_LIST_HEAD(&resource_list);
> > >
> > > +     if (wake_capable)
> > > +             *wake_capable = 0;
> > > +
> > >       ret = acpi_dev_get_resources(adev, &resource_list,
> > >                                    i2c_acpi_add_resource, &irq);
> >
> 
> 
> > You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> > look into maybe defining
> >
> > #define IORESOURCE_IRQ_WAKECAPABLE      (1<<6)
> >
> > in include/linux/ioport.h and plumbing it through from ACPI layer.
> >
> > Thanks.
> 
> AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> wake a system from suspend/suspend-to-idle. It requires either a GPE
> or GPIO controller to wake the system. This is the reason I haven't
> pushed patches to handle the Interrupt/IRQ resource. Can anyone
> confirm?

I've heard there are ARM ACPI systems...

Thanks.

-- 
Dmitry

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Hans de Goede]

Hi,

On 9/7/22 04:00, Raul Rangel wrote:
> On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
>>> Device tree already has a mechanism to pass the wake_irq. It does this
>>> by looking for the wakeup-source property and setting the
>>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
>>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
>>> the system. Previously the i2c drivers had to make assumptions and
>>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
>>> If there is a device with an Active Low interrupt and the device gets
>>> powered off while suspending, the interrupt line will go low since it's
>>> no longer powered and wake the system. For this reason we should respect
>>> the board designers wishes and honor the wake bit defined on the
>>> GpioInt.
>>>
>>> This change does not cover the ACPI Interrupt or IRQ resources.
>>>
>>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>>> ---
>>>
>>>  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
>>>  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
>>>  drivers/i2c/i2c-core.h      |  4 ++--
>>>  3 files changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
>>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
>>> --- a/drivers/i2c/i2c-core-acpi.c
>>> +++ b/drivers/i2c/i2c-core-acpi.c
>>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
>>>  /**
>>>   * i2c_acpi_get_irq - get device IRQ number from ACPI
>>>   * @client: Pointer to the I2C client device
>>> + * @wake_capable: Set to 1 if the IRQ is wake capable
>>>   *
>>>   * Find the IRQ number used by a specific client device.
>>>   *
>>>   * Return: The IRQ number or an error code.
>>>   */
>>> -int i2c_acpi_get_irq(struct i2c_client *client)
>>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
>>>  {
>>>       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
>>>       struct list_head resource_list;
>>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
>>>
>>>       INIT_LIST_HEAD(&resource_list);
>>>
>>> +     if (wake_capable)
>>> +             *wake_capable = 0;
>>> +
>>>       ret = acpi_dev_get_resources(adev, &resource_list,
>>>                                    i2c_acpi_add_resource, &irq);
>>
> 
> 
>> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
>> look into maybe defining
>>
>> #define IORESOURCE_IRQ_WAKECAPABLE      (1<<6)
>>
>> in include/linux/ioport.h and plumbing it through from ACPI layer.
>>
>> Thanks.
> 
> AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> wake a system from suspend/suspend-to-idle.

That may be true for S3 suspend (it sounds about right) there
certainly is no way to "arm for wakeup" on the APIC, but with
s2idle all IRQs which are not explicitly disabled by the OS
still function normally so there any IRQ can be a wakeup
source (AFAIK).

And even with S3 suspend I think some IRQs can act as wakeup,
but that is configured by the BIOS then and not something which
linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
controllers on x86 is an APIC IRQ ...

Regards,

Hans

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Raul Rangel]

On Wed, Sep 7, 2022 at 2:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 9/7/22 04:00, Raul Rangel wrote:
> > On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >>
> >> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> >>> Device tree already has a mechanism to pass the wake_irq. It does this
> >>> by looking for the wakeup-source property and setting the
> >>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> >>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> >>> the system. Previously the i2c drivers had to make assumptions and
> >>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> >>> If there is a device with an Active Low interrupt and the device gets
> >>> powered off while suspending, the interrupt line will go low since it's
> >>> no longer powered and wake the system. For this reason we should respect
> >>> the board designers wishes and honor the wake bit defined on the
> >>> GpioInt.
> >>>
> >>> This change does not cover the ACPI Interrupt or IRQ resources.
> >>>
> >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> >>> ---
> >>>
> >>>  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
> >>>  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> >>>  drivers/i2c/i2c-core.h      |  4 ++--
> >>>  3 files changed, 19 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> >>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> >>> --- a/drivers/i2c/i2c-core-acpi.c
> >>> +++ b/drivers/i2c/i2c-core-acpi.c
> >>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> >>>  /**
> >>>   * i2c_acpi_get_irq - get device IRQ number from ACPI
> >>>   * @client: Pointer to the I2C client device
> >>> + * @wake_capable: Set to 1 if the IRQ is wake capable
> >>>   *
> >>>   * Find the IRQ number used by a specific client device.
> >>>   *
> >>>   * Return: The IRQ number or an error code.
> >>>   */
> >>> -int i2c_acpi_get_irq(struct i2c_client *client)
> >>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> >>>  {
> >>>       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> >>>       struct list_head resource_list;
> >>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> >>>
> >>>       INIT_LIST_HEAD(&resource_list);
> >>>
> >>> +     if (wake_capable)
> >>> +             *wake_capable = 0;
> >>> +
> >>>       ret = acpi_dev_get_resources(adev, &resource_list,
> >>>                                    i2c_acpi_add_resource, &irq);
> >>
> >
> >
> >> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> >> look into maybe defining
> >>
> >> #define IORESOURCE_IRQ_WAKECAPABLE      (1<<6)
> >>
> >> in include/linux/ioport.h and plumbing it through from ACPI layer.
> >>
> >> Thanks.
> >
> > AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> > wake a system from suspend/suspend-to-idle.
>
> That may be true for S3 suspend (it sounds about right) there
> certainly is no way to "arm for wakeup" on the APIC, but with
> s2idle all IRQs which are not explicitly disabled by the OS
> still function normally so there any IRQ can be a wakeup
> source (AFAIK).
>
> And even with S3 suspend I think some IRQs can act as wakeup,
> but that is configured by the BIOS then and not something which
> linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
> controllers on x86 is an APIC IRQ ...
>
> Regards,
>
> Hans
>

SGTM. I wanted to make sure there was interest before I invested the
time in adding the functionality. Hopefully I can push up a new patch
set tomorrow.

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Rafael J. Wysocki]

On Thu, Sep 8, 2022 at 4:40 PM Raul Rangel <rrangel@chromium.org> wrote:
>
> On Wed, Sep 7, 2022 at 2:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Hi,
> >
> > On 9/7/22 04:00, Raul Rangel wrote:
> > > On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > >>
> > >> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > >>> Device tree already has a mechanism to pass the wake_irq. It does this
> > >>> by looking for the wakeup-source property and setting the
> > >>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > >>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > >>> the system. Previously the i2c drivers had to make assumptions and
> > >>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > >>> If there is a device with an Active Low interrupt and the device gets
> > >>> powered off while suspending, the interrupt line will go low since it's
> > >>> no longer powered and wake the system. For this reason we should respect
> > >>> the board designers wishes and honor the wake bit defined on the
> > >>> GpioInt.
> > >>>
> > >>> This change does not cover the ACPI Interrupt or IRQ resources.
> > >>>
> > >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > >>> ---
> > >>>
> > >>>  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
> > >>>  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > >>>  drivers/i2c/i2c-core.h      |  4 ++--
> > >>>  3 files changed, 19 insertions(+), 10 deletions(-)
> > >>>
> > >>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > >>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > >>> --- a/drivers/i2c/i2c-core-acpi.c
> > >>> +++ b/drivers/i2c/i2c-core-acpi.c
> > >>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > >>>  /**
> > >>>   * i2c_acpi_get_irq - get device IRQ number from ACPI
> > >>>   * @client: Pointer to the I2C client device
> > >>> + * @wake_capable: Set to 1 if the IRQ is wake capable
> > >>>   *
> > >>>   * Find the IRQ number used by a specific client device.
> > >>>   *
> > >>>   * Return: The IRQ number or an error code.
> > >>>   */
> > >>> -int i2c_acpi_get_irq(struct i2c_client *client)
> > >>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > >>>  {
> > >>>       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > >>>       struct list_head resource_list;
> > >>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> > >>>
> > >>>       INIT_LIST_HEAD(&resource_list);
> > >>>
> > >>> +     if (wake_capable)
> > >>> +             *wake_capable = 0;
> > >>> +
> > >>>       ret = acpi_dev_get_resources(adev, &resource_list,
> > >>>                                    i2c_acpi_add_resource, &irq);
> > >>
> > >
> > >
> > >> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> > >> look into maybe defining
> > >>
> > >> #define IORESOURCE_IRQ_WAKECAPABLE      (1<<6)
> > >>
> > >> in include/linux/ioport.h and plumbing it through from ACPI layer.
> > >>
> > >> Thanks.
> > >
> > > AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> > > wake a system from suspend/suspend-to-idle.
> >
> > That may be true for S3 suspend (it sounds about right) there
> > certainly is no way to "arm for wakeup" on the APIC, but with
> > s2idle all IRQs which are not explicitly disabled by the OS
> > still function normally so there any IRQ can be a wakeup
> > source (AFAIK).

That's true.

Moreover, even for S3 there are transitions into it and there may be
wakeup interrupts taking place during those transitions.  Those may be
any IRQs too.

> > And even with S3 suspend I think some IRQs can act as wakeup,
> > but that is configured by the BIOS then and not something which
> > linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
> > controllers on x86 is an APIC IRQ ...

It's more about how the system is wired up AFAICS.  Basically, in
order to wake up the system from S3, the given IRQ needs to be
physically attached to an input that will trigger the platform wakeup
logic while in S3.

> >
>
> SGTM. I wanted to make sure there was interest before I invested the
> time in adding the functionality. Hopefully I can push up a new patch
> set tomorrow.

Sounds good. :-)

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Raul Rangel]

It looks like `i2c_acpi_get_irq` and `platform_get_irq_optional` are
doing pretty much the same thing. Can we replace `i2c_acpi_get_irq`
and switch over to `platform_get_irq_optional`? Is it possible to get
a `platform_device` from an `i2c_client`?

On Thu, Sep 8, 2022 at 9:23 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Sep 8, 2022 at 4:40 PM Raul Rangel <rrangel@chromium.org> wrote:
> >
> > On Wed, Sep 7, 2022 at 2:12 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 9/7/22 04:00, Raul Rangel wrote:
> > > > On Tue, Sep 6, 2022 at 7:00 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > >>
> > > >> On Tue, Aug 30, 2022 at 05:15:37PM -0600, Raul E Rangel wrote:
> > > >>> Device tree already has a mechanism to pass the wake_irq. It does this
> > > >>> by looking for the wakeup-source property and setting the
> > > >>> I2C_CLIENT_WAKE flag. This CL adds the ACPI equivalent. It uses at the
> > > >>> ACPI GpioInt wake flag to determine if the interrupt can be used to wake
> > > >>> the system. Previously the i2c drivers had to make assumptions and
> > > >>> blindly enable the wake IRQ. This can cause spurious wake events. e.g.,
> > > >>> If there is a device with an Active Low interrupt and the device gets
> > > >>> powered off while suspending, the interrupt line will go low since it's
> > > >>> no longer powered and wake the system. For this reason we should respect
> > > >>> the board designers wishes and honor the wake bit defined on the
> > > >>> GpioInt.
> > > >>>
> > > >>> This change does not cover the ACPI Interrupt or IRQ resources.
> > > >>>
> > > >>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > >>> ---
> > > >>>
> > > >>>  drivers/i2c/i2c-core-acpi.c |  8 ++++++--
> > > >>>  drivers/i2c/i2c-core-base.c | 17 +++++++++++------
> > > >>>  drivers/i2c/i2c-core.h      |  4 ++--
> > > >>>  3 files changed, 19 insertions(+), 10 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> > > >>> index c762a879c4cc6b..cfe82a6ba3ef28 100644
> > > >>> --- a/drivers/i2c/i2c-core-acpi.c
> > > >>> +++ b/drivers/i2c/i2c-core-acpi.c
> > > >>> @@ -182,12 +182,13 @@ static int i2c_acpi_add_resource(struct acpi_resource *ares, void *data)
> > > >>>  /**
> > > >>>   * i2c_acpi_get_irq - get device IRQ number from ACPI
> > > >>>   * @client: Pointer to the I2C client device
> > > >>> + * @wake_capable: Set to 1 if the IRQ is wake capable
> > > >>>   *
> > > >>>   * Find the IRQ number used by a specific client device.
> > > >>>   *
> > > >>>   * Return: The IRQ number or an error code.
> > > >>>   */
> > > >>> -int i2c_acpi_get_irq(struct i2c_client *client)
> > > >>> +int i2c_acpi_get_irq(struct i2c_client *client, int *wake_capable)
> > > >>>  {
> > > >>>       struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> > > >>>       struct list_head resource_list;
> > > >>> @@ -196,6 +197,9 @@ int i2c_acpi_get_irq(struct i2c_client *client)
> > > >>>
> > > >>>       INIT_LIST_HEAD(&resource_list);
> > > >>>
> > > >>> +     if (wake_capable)
> > > >>> +             *wake_capable = 0;
> > > >>> +
> > > >>>       ret = acpi_dev_get_resources(adev, &resource_list,
> > > >>>                                    i2c_acpi_add_resource, &irq);
> > > >>
> > > >
> > > >
> > > >> You also need to handle "Interrupt(..., ...AndWake)" case here. I would
> > > >> look into maybe defining
> > > >>
> > > >> #define IORESOURCE_IRQ_WAKECAPABLE      (1<<6)
> > > >>
> > > >> in include/linux/ioport.h and plumbing it through from ACPI layer.
> > > >>
> > > >> Thanks.
> > > >
> > > > AFAIK the Intel (Not 100% certain) and AMD IO-APIC's can't actually
> > > > wake a system from suspend/suspend-to-idle.
> > >
> > > That may be true for S3 suspend (it sounds about right) there
> > > certainly is no way to "arm for wakeup" on the APIC, but with
> > > s2idle all IRQs which are not explicitly disabled by the OS
> > > still function normally so there any IRQ can be a wakeup
> > > source (AFAIK).
>
> That's true.
>
> Moreover, even for S3 there are transitions into it and there may be
> wakeup interrupts taking place during those transitions.  Those may be
> any IRQs too.
>
> > > And even with S3 suspend I think some IRQs can act as wakeup,
> > > but that is configured by the BIOS then and not something which
> > > linux can enable/disable. E.g IIRC the parent IRQ of the GPIO
> > > controllers on x86 is an APIC IRQ ...
>
> It's more about how the system is wired up AFAICS.  Basically, in
> order to wake up the system from S3, the given IRQ needs to be
> physically attached to an input that will trigger the platform wakeup
> logic while in S3.
>
> > >
> >
> > SGTM. I wanted to make sure there was interest before I invested the
> > time in adding the functionality. Hopefully I can push up a new patch
> > set tomorrow.
>
> Sounds good. :-)

Re: [PATCH 4/8] i2c: acpi: Use ACPI GPIO wake capability bit to set wake_irq

[Dmitry Torokhov]

On Fri, Sep 09, 2022 at 12:47:11PM -0600, Raul Rangel wrote:
> It looks like `i2c_acpi_get_irq` and `platform_get_irq_optional` are
> doing pretty much the same thing. Can we replace `i2c_acpi_get_irq`
> and switch over to `platform_get_irq_optional`? Is it possible to get
> a `platform_device` from an `i2c_client`?

No, they are completely different objects.


		struct device
	/		|		\
platform_device		i2c_client	spi_device ...

Also, please no top-posting on kernel mailing lists.

Thanks.

-- 
Dmitry