[PATCH v2 00/22] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[PATCH v2 00/22] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Mark Hasemeyer]

Currently the cros_ec driver assumes that its associated interrupt is
wake capable. This is an incorrect assumption as some Chromebooks use a
separate wake pin, while others overload the interrupt for wake and IO.
This patch train updates the driver to query the underlying ACPI/DT data
to determine whether or not the IRQ should be enabled for wake.

Both the device tree and ACPI systems have methods for reporting IRQ
wake capability. In device tree based systems, a node can advertise
itself as a 'wakeup-source'. In ACPI based systems, GpioInt and
Interrupt resource descriptors can use the 'SharedAndWake' or
'ExclusiveAndWake' share types.

Some logic is added to the platform, ACPI, and DT subsystems to more
easily pipe wakeirq information up to the driver.

Changes in v2:
-Rebase on linux-next
-Add cover letter
-See each patch for patch specific changes

Mark Hasemeyer (22):
  gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource
  i2c: acpi: Modify i2c_acpi_get_irq() to use resource
  Documentation: devicetree: Clarify wording for wakeup-source property
  ARM: dts: tegra: Enable cros-ec-spi as wake source
  ARM: dts: rockchip: rk3288: Enable cros-ec-spi as wake source
  ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source
  ARM: dts: samsung: exynos5800: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8173: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8183: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8192: Enable cros-ec-spi as wake source
  arm64: dts: mediatek: mt8195: Enable cros-ec-spi as wake source
  arm64: dts: tegra: Enable cros-ec-spi as wake source
  arm64: dts: qcom: sc7180: Enable cros-ec-spi as wake source
  arm64: dts: qcom: sc7280: Enable cros-ec-spi as wake source
  arm64: dts: qcom: sdm845: Enable cros-ec-spi as wake source
  arm64: dts: rockchip: rk3399: Enable cros-ec-spi as wake source
  of: irq: add wake capable bit to of_irq_resource()
  of: irq: Add default implementation for of_irq_to_resource()
  of: irq: Remove extern from function declarations
  device property: Modify fwnode irq_get() to use resource
  platform: Modify platform_get_irq_optional() to use resource
  platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

 .../bindings/power/wakeup-source.txt          | 18 +++--
 arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi   |  1 +
 arch/arm/boot/dts/nvidia/tegra124-venice2.dts |  1 +
 .../rockchip/rk3288-veyron-chromebook.dtsi    |  1 +
 .../boot/dts/samsung/exynos5420-peach-pit.dts |  1 +
 .../boot/dts/samsung/exynos5800-peach-pi.dts  |  1 +
 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi  |  1 +
 .../arm64/boot/dts/mediatek/mt8183-kukui.dtsi |  1 +
 .../boot/dts/mediatek/mt8192-asurada.dtsi     |  1 +
 .../boot/dts/mediatek/mt8195-cherry.dtsi      |  1 +
 .../arm64/boot/dts/nvidia/tegra132-norrin.dts |  1 +
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  1 +
 .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi |  1 +
 .../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi |  1 +
 arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi    |  1 +
 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi  |  1 +
 drivers/acpi/property.c                       | 11 ++-
 drivers/base/platform.c                       | 74 +++++++++++++------
 drivers/base/property.c                       | 24 +++++-
 drivers/gpio/gpiolib-acpi.c                   | 25 ++++---
 drivers/i2c/i2c-core-acpi.c                   | 38 +++++-----
 drivers/i2c/i2c-core-base.c                   |  6 +-
 drivers/i2c/i2c-core.h                        |  4 +-
 drivers/of/irq.c                              | 32 +++++++-
 drivers/of/property.c                         |  8 +-
 drivers/platform/chrome/cros_ec.c             |  9 ---
 drivers/platform/chrome/cros_ec_lpc.c         | 52 ++++++++++++-
 drivers/platform/chrome/cros_ec_spi.c         | 41 ++++++++--
 drivers/platform/chrome/cros_ec_uart.c        | 34 +++++++--
 include/linux/acpi.h                          | 23 +++---
 include/linux/fwnode.h                        |  8 +-
 include/linux/of_irq.h                        | 41 +++++-----
 include/linux/platform_data/cros_ec_proto.h   |  2 -
 include/linux/platform_device.h               |  3 +
 include/linux/property.h                      |  2 +
 35 files changed, 328 insertions(+), 142 deletions(-)

-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 01/22] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource

[Mark Hasemeyer]

Other information besides wake capability can be provided about GPIO
IRQs such as triggering, polarity, and sharability. Use resource flags
to provide this information to the caller if they want it.

This should keep the API more robust over time as flags are added,
modified, or removed. It also more closely matches acpi_irq_get() which
take a resource as an argument.

Rename the function to acpi_dev_get_gpio_irq_resource() to better
describe the function's new behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Remove explicit cast to struct resource
-irq -> IRQ

 drivers/gpio/gpiolib-acpi.c | 25 ++++++++++++++++---------
 drivers/i2c/i2c-core-acpi.c | 10 ++++++++--
 include/linux/acpi.h        | 23 ++++++++++-------------
 3 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 88066826d8e5b..ef98b50f42f86 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -111,6 +111,7 @@ struct acpi_gpio_info {
 	int polarity;
 	int triggering;
 	bool wake_capable;
+	bool shareable;
 	unsigned int debounce;
 	unsigned int quirks;
 };
@@ -760,6 +761,7 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		lookup->info.debounce = agpio->debounce_timeout;
 		lookup->info.gpioint = gpioint;
 		lookup->info.wake_capable = acpi_gpio_irq_is_wake(&lookup->info.adev->dev, agpio);
+		lookup->info.shareable = agpio->shareable == ACPI_SHARED;
 
 		/*
 		 * Polarity and triggering are only specified for GpioInt
@@ -1004,11 +1006,11 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
 }
 
 /**
- * acpi_dev_gpio_irq_wake_get_by() - Find GpioInt and translate it to Linux IRQ number
+ * acpi_dev_get_gpio_irq_resource() - Find GpioInt and populate resource struct
  * @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 true if the IRQ is wake capable
+ * @r: pointer to resource to populate with IRQ information.
  *
  * 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
@@ -1023,10 +1025,12 @@ struct gpio_desc *acpi_find_gpio(struct fwnode_handle *fwnode,
  * The GPIO is considered wake capable if the GpioInt resource specifies
  * SharedAndWake or ExclusiveAndWake.
  *
- * Return: Linux IRQ number (> %0) on success, negative errno on failure.
+ * IRQ number will be available in the resource structure.
+ *
+ * Return: 0 on success, negative errno on failure.
  */
-int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, int index,
-				  bool *wake_capable)
+int acpi_dev_get_gpio_irq_resource(struct acpi_device *adev, const char *name, int index,
+				   struct resource *r)
 {
 	int idx, i;
 	unsigned int irq_flags;
@@ -1084,16 +1088,19 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name, in
 			}
 
 			/* avoid suspend issues with GPIOs when systems are using S3 */
-			if (wake_capable && acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
-				*wake_capable = info.wake_capable;
+			if (info.wake_capable && !(acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0))
+				info.wake_capable = false;
 
-			return irq;
+			*r = DEFINE_RES_IRQ(irq);
+			r->flags = acpi_dev_irq_flags(info.triggering, info.polarity,
+						      info.shareable, info.wake_capable);
+			return 0;
 		}
 
 	}
 	return -ENOENT;
 }
-EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_wake_get_by);
+EXPORT_SYMBOL_GPL(acpi_dev_get_gpio_irq_resource);
 
 static acpi_status
 acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index d6037a3286690..8126a87baf3d4 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -203,6 +203,7 @@ int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
 	struct list_head resource_list;
+	struct resource irqres;
 	struct i2c_acpi_irq_context irq_ctx = {
 		.irq = -ENOENT,
 	};
@@ -217,8 +218,13 @@ int i2c_acpi_get_irq(struct i2c_client *client, bool *wake_capable)
 
 	acpi_dev_free_resource_list(&resource_list);
 
-	if (irq_ctx.irq == -ENOENT)
-		irq_ctx.irq = acpi_dev_gpio_irq_wake_get(adev, 0, &irq_ctx.wake_capable);
+	if (irq_ctx.irq == -ENOENT) {
+		ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
+		if (ret)
+			return ret;
+		irq_ctx.irq = irqres.start;
+		irq_ctx.wake_capable = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+	}
 
 	if (irq_ctx.irq < 0)
 		return irq_ctx.irq;
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 118a18b7ff844..83aa2fa8e81fc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -1221,8 +1221,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_wake_get_by(struct acpi_device *adev, const char *name, int index,
-				  bool *wake_capable);
+int acpi_dev_get_gpio_irq_resource(struct acpi_device *adev, const char *name, int index,
+				   struct resource *r);
 #else
 static inline bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 					      struct acpi_resource_gpio **agpio)
@@ -1234,28 +1234,25 @@ static inline bool acpi_gpio_get_io_resource(struct acpi_resource *ares,
 {
 	return false;
 }
-static inline int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *name,
-						int index, bool *wake_capable)
+static inline int acpi_dev_get_gpio_irq_resource(struct acpi_device *adev, const char *name,
+						 int index, struct resource *r)
 {
 	return -ENXIO;
 }
 #endif
 
-static inline int acpi_dev_gpio_irq_wake_get(struct acpi_device *adev, int index,
-					     bool *wake_capable)
+static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name, int index)
 {
-	return acpi_dev_gpio_irq_wake_get_by(adev, NULL, index, wake_capable);
-}
+	struct resource r;
+	int ret;
 
-static inline int acpi_dev_gpio_irq_get_by(struct acpi_device *adev, const char *name,
-					   int index)
-{
-	return acpi_dev_gpio_irq_wake_get_by(adev, name, index, NULL);
+	ret = acpi_dev_get_gpio_irq_resource(adev, name, index, &r);
+	return ret ?: r.start;
 }
 
 static inline int acpi_dev_gpio_irq_get(struct acpi_device *adev, int index)
 {
-	return acpi_dev_gpio_irq_wake_get_by(adev, NULL, index, NULL);
+	return acpi_dev_gpio_irq_get_by(adev, NULL, index);
 }
 
 /* Device properties */
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 02/22] i2c: acpi: Modify i2c_acpi_get_irq() to use resource

[Mark Hasemeyer]

The i2c_acpi_irq_context structure provides redundant information that
can be provided with struct resource.

Refactor i2c_acpi_get_irq() to use struct resource instead of struct
i2c_acpi_irq_context.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-New patch

 drivers/i2c/i2c-core-acpi.c | 44 ++++++++++++++-----------------------
 drivers/i2c/i2c-core-base.c |  6 ++---
 drivers/i2c/i2c-core.h      |  4 ++--
 3 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8126a87baf3d4..01cf140da21af 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -175,64 +175,54 @@ static int i2c_acpi_do_lookup(struct acpi_device *adev,
 
 static int i2c_acpi_add_irq_resource(struct acpi_resource *ares, void *data)
 {
-	struct i2c_acpi_irq_context *irq_ctx = data;
-	struct resource r;
+	struct resource *r = data;
 
-	if (irq_ctx->irq > 0)
+	if (r->start > 0)
 		return 1;
 
-	if (!acpi_dev_resource_interrupt(ares, 0, &r))
+	if (!acpi_dev_resource_interrupt(ares, 0, r))
 		return 1;
 
-	irq_ctx->irq = i2c_dev_irq_from_resources(&r, 1);
-	irq_ctx->wake_capable = r.flags & IORESOURCE_IRQ_WAKECAPABLE;
+	i2c_dev_irq_from_resources(r, 1);
 
 	return 1; /* No need to add resource to the list */
 }
 
 /**
- * i2c_acpi_get_irq - get device IRQ number from ACPI
+ * i2c_acpi_get_irq - get device IRQ number from ACPI and populate resource
  * @client: Pointer to the I2C client device
- * @wake_capable: Set to true if the IRQ is wake capable
+ * @r: resource with populated IRQ information
  *
  * 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, bool *wake_capable)
+int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r)
 {
 	struct acpi_device *adev = ACPI_COMPANION(&client->dev);
 	struct list_head resource_list;
-	struct resource irqres;
-	struct i2c_acpi_irq_context irq_ctx = {
-		.irq = -ENOENT,
-	};
 	int ret;
 
+	if (IS_ERR_OR_NULL(r))
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&resource_list);
 
 	ret = acpi_dev_get_resources(adev, &resource_list,
-				     i2c_acpi_add_irq_resource, &irq_ctx);
+				     i2c_acpi_add_irq_resource, r);
 	if (ret < 0)
 		return ret;
 
 	acpi_dev_free_resource_list(&resource_list);
 
-	if (irq_ctx.irq == -ENOENT) {
-		ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
-		if (ret)
-			return ret;
-		irq_ctx.irq = irqres.start;
-		irq_ctx.wake_capable = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
-	}
-
-	if (irq_ctx.irq < 0)
-		return irq_ctx.irq;
+	if (r->start > 0)
+		return r->start;
 
-	if (wake_capable)
-		*wake_capable = irq_ctx.wake_capable;
+	ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, r);
+	if (!ret)
+		return r->start;
 
-	return irq_ctx.irq;
+	return ret;
 }
 
 static int i2c_acpi_get_info(struct acpi_device *adev,
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 3bd48d4b6318f..8b8c7581a60c2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -513,10 +513,10 @@ 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)) {
-			bool wake_capable;
+			struct resource r = {0};
 
-			irq = i2c_acpi_get_irq(client, &wake_capable);
-			if (irq > 0 && wake_capable)
+			irq = i2c_acpi_get_irq(client, &r);
+			if (irq > 0 && r.flags & IORESOURCE_IRQ_WAKECAPABLE)
 				client->flags |= I2C_CLIENT_WAKE;
 		}
 		if (irq == -EPROBE_DEFER) {
diff --git a/drivers/i2c/i2c-core.h b/drivers/i2c/i2c-core.h
index 05b8b8dfa9bdd..b5dc559c49d11 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, bool *wake_capable);
+int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r);
 #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, bool *wake_capable)
+static inline int i2c_acpi_get_irq(struct i2c_client *client, struct resource *r)
 {
 	return 0;
 }
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 03/22] Documentation: devicetree: Clarify wording for wakeup-source property

[Mark Hasemeyer]

The wording in the current documentation is a little strong. The
intention was not to fix any particular interrupt as wakeup capable but
leave those details to the device. It wasn't intended to enforce any
rules as what can be or can't be a wakeup interrupt.

Soften the wording to not mandate that the 'wakeup-source' property be
used, and clarify what it means when an interrupt is marked (or not
marked) for wakeup.

Link: https://lore.kernel.org/all/ZYAjxxHcCOgDVMTQ@bogus/
Link: https://lore.kernel.org/all/CAL_Jsq+MYwOG40X26cYmO9EkZ9xqWrXDi03MaRfxnV-+VGkXWQ@mail.gmail.com/
Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-New patch

 .../bindings/power/wakeup-source.txt           | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/wakeup-source.txt b/Documentation/devicetree/bindings/power/wakeup-source.txt
index 697333a56d5e2..75bc20b95688f 100644
--- a/Documentation/devicetree/bindings/power/wakeup-source.txt
+++ b/Documentation/devicetree/bindings/power/wakeup-source.txt
@@ -3,16 +3,20 @@ Specifying wakeup capability for devices
 
 Any device nodes
 ----------------
-Nodes that describe devices which has wakeup capability must contain an
+Nodes that describe devices which have wakeup capability may contain a
 "wakeup-source" boolean property.
 
-Also, if device is marked as a wakeup source, then all the primary
-interrupt(s) can be used as wakeup interrupt(s).
+If the device is marked as a wakeup-source, interrupt wake capability depends
+on the device specific "interrupt-names" property. If no interrupts are labeled
+as wake capable, then it is up to the device to determine which interrupts can
+wake the system.
 
-However if the devices have dedicated interrupt as the wakeup source
-then they need to specify/identify the same using device specific
-interrupt name. In such cases only that interrupt can be used as wakeup
-interrupt.
+However if a device has a dedicated interrupt as the wakeup source, then it
+needs to specify/identify it using a device specific interrupt name. In such
+cases only that interrupt can be used as a wakeup interrupt.
+
+While various legacy interrupt names exist, new devices should use "wakeup" as
+the canonical interrupt name.
 
 List of legacy properties and respective binding document
 ---------------------------------------------------------
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 04/22] ARM: dts: tegra: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi   | 1 +
 arch/arm/boot/dts/nvidia/tegra124-venice2.dts | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi b/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi
index a2ee371802004..8125c1b3e8d79 100644
--- a/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi
+++ b/arch/arm/boot/dts/nvidia/tegra124-nyan.dtsi
@@ -338,6 +338,7 @@ cros_ec: cros-ec@0 {
 			interrupt-parent = <&gpio>;
 			interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>;
 			reg = <0>;
+			wakeup-source;
 
 			google,cros-ec-spi-msg-delay = <2000>;
 
diff --git a/arch/arm/boot/dts/nvidia/tegra124-venice2.dts b/arch/arm/boot/dts/nvidia/tegra124-venice2.dts
index 3924ee385dee0..df98dc2a67b85 100644
--- a/arch/arm/boot/dts/nvidia/tegra124-venice2.dts
+++ b/arch/arm/boot/dts/nvidia/tegra124-venice2.dts
@@ -857,6 +857,7 @@ cros_ec: cros-ec@0 {
 			interrupt-parent = <&gpio>;
 			interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>;
 			reg = <0>;
+			wakeup-source;
 
 			google,cros-ec-spi-msg-delay = <2000>;
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 05/22] ARM: dts: rockchip: rk3288: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi b/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi
index 092316be67f74..1554fe36e60fe 100644
--- a/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3288-veyron-chromebook.dtsi
@@ -112,6 +112,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&ec_int>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		i2c_tunnel: i2c-tunnel {
 			compatible = "google,cros-ec-i2c-tunnel";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts b/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts
index 4e757b6e28e1c..3759742d38cac 100644
--- a/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts
+++ b/arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts
@@ -967,6 +967,7 @@ cros_ec: cros-ec@0 {
 		reg = <0>;
 		spi-max-frequency = <3125000>;
 		google,has-vbc-nvram;
+		wakeup-source;
 
 		controller-data {
 			samsung,spi-feedback-delay = <1>;
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 07/22] ARM: dts: samsung: exynos5800: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts b/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts
index f91bc4ae008e4..9bbbdce9103a6 100644
--- a/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/samsung/exynos5800-peach-pi.dts
@@ -949,6 +949,7 @@ cros_ec: cros-ec@0 {
 		reg = <0>;
 		spi-max-frequency = <3125000>;
 		google,has-vbc-nvram;
+		wakeup-source;
 
 		controller-data {
 			samsung,spi-feedback-delay = <1>;
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 08/22] arm64: dts: mediatek: mt8173: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
index 111495622cacd..190a3ffd81471 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
@@ -1163,6 +1163,7 @@ cros_ec: ec@0 {
 		interrupt-parent = <&pio>;
 		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
 		google,cros-ec-spi-msg-delay = <500>;
+		wakeup-source;
 
 		i2c_tunnel: i2c-tunnel0 {
 			compatible = "google,cros-ec-i2c-tunnel";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 09/22] arm64: dts: mediatek: mt8183: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
index 7881a27be0297..06cbf29d16215 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183-kukui.dtsi
@@ -922,6 +922,7 @@ cros_ec: cros-ec@0 {
 		interrupts = <151 IRQ_TYPE_LEVEL_LOW>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&ec_ap_int_odl>;
+		wakeup-source;
 
 		i2c_tunnel: i2c-tunnel {
 			compatible = "google,cros-ec-i2c-tunnel";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 10/22] arm64: dts: mediatek: mt8192: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
index f2281250ac35d..ab44d382f757e 100644
--- a/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi
@@ -1332,6 +1332,7 @@ cros_ec: ec@0 {
 		spi-max-frequency = <3000000>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&cros_ec_int>;
+		wakeup-source;
 
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 11/22] arm64: dts: mediatek: mt8195: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
index 5a7cab489ff3a..4292b72566bcf 100644
--- a/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8195-cherry.dtsi
@@ -1067,6 +1067,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&cros_ec_int>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		keyboard-backlight {
 			compatible = "google,cros-kbd-led-backlight";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 12/22] arm64: dts: tegra: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/nvidia/tegra132-norrin.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts b/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts
index bbc2e9bef08da..14d58859bb55c 100644
--- a/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra132-norrin.dts
@@ -762,6 +762,7 @@ ec: cros-ec@0 {
 			interrupt-parent = <&gpio>;
 			interrupts = <TEGRA_GPIO(C, 7) IRQ_TYPE_LEVEL_LOW>;
 			reg = <0>;
+			wakeup-source;
 
 			google,cros-ec-spi-msg-delay = <2000>;
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 13/22] arm64: dts: qcom: sc7180: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 46aaeba286047..f3a6da8b28901 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -649,6 +649,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&ap_ec_int_l>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		cros_ec_pwm: pwm {
 			compatible = "google,cros-ec-pwm";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 14/22] arm64: dts: qcom: sc7280: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
 arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
index 9ea6636125ad9..2ba4ea60cb147 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
@@ -548,6 +548,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&ap_ec_int_l>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		cros_ec_pwm: pwm {
 			compatible = "google,cros-ec-pwm";
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
index ebae545c587c4..fbfac7534d3c6 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi
@@ -19,6 +19,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&ap_ec_int_l>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		cros_ec_pwm: pwm {
 			compatible = "google,cros-ec-pwm";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 15/22] arm64: dts: qcom: sdm845: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
index 0ab5e8f53ac9f..e8276db9eabb2 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi
@@ -852,6 +852,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&ec_ap_int_l>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		cros_ec_pwm: pwm {
 			compatible = "google,cros-ec-pwm";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 16/22] arm64: dts: rockchip: rk3399: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

The cros_ec driver currently assumes that cros-ec-spi compatible device
nodes are a wakeup-source even though the wakeup-source property is not
defined.

Add the wakeup-source property to all cros-ec-spi compatible device
nodes to match expected behavior.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Split by arch/soc

 arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
index 789fd0dcc88ba..b5734e056aef1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru.dtsi
@@ -603,6 +603,7 @@ cros_ec: ec@0 {
 		pinctrl-names = "default";
 		pinctrl-0 = <&ec_ap_int_l>;
 		spi-max-frequency = <3000000>;
+		wakeup-source;
 
 		i2c_tunnel: i2c-tunnel {
 			compatible = "google,cros-ec-i2c-tunnel";
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 17/22] of: irq: add wake capable bit to of_irq_resource()

[Mark Hasemeyer]

Add wake capability information to the IRQ resource. Wake capability is
assumed based on conventions provided in the devicetree wakeup-source
binding documentation. An interrupt is considered wake capable if the
following are true:
1. A wakeup-source property exits in the same device node as the
   interrupt.
2. The IRQ is marked as dedicated by setting its interrupt-name to
   "wakeup".

The wakeup-source documentation states that dedicated interrupts can use
device specific interrupt names and device drivers are still welcome to
use their own naming schemes. This API is provided as a helper if one is
willing to conform to the above conventions.

The ACPI subsystems already provides similar APIs that allow one to
query the wake capability of an IRQ. This brings closer feature parity
to the devicetree.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Update logic to return true only if wakeup-source property and
 "wakeup" interrupt-name are defined
-irq->IRQ, api->API

 drivers/of/irq.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 174900072c18c..7583adf386220 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -383,11 +383,39 @@ int of_irq_parse_one(struct device_node *device, int index, struct of_phandle_ar
 }
 EXPORT_SYMBOL_GPL(of_irq_parse_one);
 
+/**
+ * __of_irq_wake_capable - Determine whether a given IRQ index is wake capable
+ *
+ * The IRQ is considered wake capable if the following are true:
+ * 1. wakeup-source property exists
+ * 2. provided IRQ index is labelled as a dedicated wakeirq
+ *
+ * This logic assumes the provided IRQ index is valid.
+ *
+ * @dev: pointer to device tree node
+ * @index: zero-based index of the IRQ
+ * Return: True if provided IRQ index for #dev is wake capable. False otherwise.
+ */
+static bool __of_irq_wake_capable(const struct device_node *dev, int index)
+{
+	int wakeindex;
+
+	if (!of_property_read_bool(dev, "wakeup-source"))
+		return false;
+
+	wakeindex = of_property_match_string(dev, "interrupt-names", "wakeup");
+	return wakeindex >= 0 && wakeindex == index;
+}
+
 /**
  * of_irq_to_resource - Decode a node's IRQ and return it as a resource
  * @dev: pointer to device tree node
- * @index: zero-based index of the irq
+ * @index: zero-based index of the IRQ
  * @r: pointer to resource structure to return result into.
+ *
+ * Return: Linux IRQ number on success, or 0 on the IRQ mapping failure, or
+ * -EPROBE_DEFER if the IRQ domain is not yet created, or error code in case
+ * of any other failure.
  */
 int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 {
@@ -411,6 +439,8 @@ int of_irq_to_resource(struct device_node *dev, int index, struct resource *r)
 
 		r->start = r->end = irq;
 		r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
+		if (__of_irq_wake_capable(dev, index))
+			r->flags |= IORESOURCE_IRQ_WAKECAPABLE;
 		r->name = name ? name : of_node_full_name(dev);
 	}
 
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 18/22] of: irq: Add default implementation for of_irq_to_resource()

[Mark Hasemeyer]

Similar to of_irq_to_resource_table(), add a default implementation of
of_irq_to_resource() for systems that don't have CONFIG_OF_IRQ defined.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-None

 include/linux/of_irq.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index d6d3eae2f1452..0d73b2ca92d31 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -34,8 +34,6 @@ static inline int of_irq_parse_oldworld(const struct device_node *device, int in
 
 extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
 extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
-extern int of_irq_to_resource(struct device_node *dev, int index,
-			      struct resource *r);
 
 #ifdef CONFIG_OF_IRQ
 extern void of_irq_init(const struct of_device_id *matches);
@@ -44,6 +42,7 @@ extern int of_irq_parse_one(struct device_node *device, int index,
 extern int of_irq_count(struct device_node *dev);
 extern int of_irq_get(struct device_node *dev, int index);
 extern int of_irq_get_byname(struct device_node *dev, const char *name);
+extern int of_irq_to_resource(struct device_node *dev, int index, struct resource *r);
 extern int of_irq_to_resource_table(struct device_node *dev,
 		struct resource *res, int nr_irqs);
 extern struct device_node *of_irq_find_parent(struct device_node *child);
@@ -76,6 +75,11 @@ static inline int of_irq_get_byname(struct device_node *dev, const char *name)
 {
 	return 0;
 }
+static inline int of_irq_to_resource(struct device_node *dev, int index,
+			      struct resource *r)
+{
+	return 0;
+}
 static inline int of_irq_to_resource_table(struct device_node *dev,
 					   struct resource *res, int nr_irqs)
 {
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 19/22] of: irq: Remove extern from function declarations

[Mark Hasemeyer]

The extern keyword is implicit for function declarations.
Remove it where possible and adjust the line wrapping accordingly.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-New patch

 include/linux/of_irq.h | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
index 0d73b2ca92d31..a130dcbc4bb45 100644
--- a/include/linux/of_irq.h
+++ b/include/linux/of_irq.h
@@ -32,27 +32,26 @@ static inline int of_irq_parse_oldworld(const struct device_node *device, int in
 }
 #endif /* CONFIG_PPC32 && CONFIG_PPC_PMAC */
 
-extern int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
-extern unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
+int of_irq_parse_raw(const __be32 *addr, struct of_phandle_args *out_irq);
+unsigned int irq_create_of_mapping(struct of_phandle_args *irq_data);
 
 #ifdef CONFIG_OF_IRQ
-extern void of_irq_init(const struct of_device_id *matches);
-extern int of_irq_parse_one(struct device_node *device, int index,
-			  struct of_phandle_args *out_irq);
-extern int of_irq_count(struct device_node *dev);
-extern int of_irq_get(struct device_node *dev, int index);
-extern int of_irq_get_byname(struct device_node *dev, const char *name);
-extern int of_irq_to_resource(struct device_node *dev, int index, struct resource *r);
-extern int of_irq_to_resource_table(struct device_node *dev,
-		struct resource *res, int nr_irqs);
-extern struct device_node *of_irq_find_parent(struct device_node *child);
-extern struct irq_domain *of_msi_get_domain(struct device *dev,
+void of_irq_init(const struct of_device_id *matches);
+int of_irq_parse_one(struct device_node *device, int index,
+		    struct of_phandle_args *out_irq);
+int of_irq_count(struct device_node *dev);
+int of_irq_get(struct device_node *dev, int index);
+int of_irq_get_byname(struct device_node *dev, const char *name);
+int of_irq_to_resource(struct device_node *dev, int index, struct resource *r);
+int of_irq_to_resource_table(struct device_node *dev, struct resource *res,
+			    int nr_irqs);
+struct device_node *of_irq_find_parent(struct device_node *child);
+struct irq_domain *of_msi_get_domain(struct device *dev,
 					    struct device_node *np,
 					    enum irq_domain_bus_token token);
-extern struct irq_domain *of_msi_map_get_device_domain(struct device *dev,
-							u32 id,
-							u32 bus_token);
-extern void of_msi_configure(struct device *dev, struct device_node *np);
+struct irq_domain *of_msi_map_get_device_domain(struct device *dev, u32 id,
+					       u32 bus_token);
+void of_msi_configure(struct device *dev, struct device_node *np);
 u32 of_msi_map_id(struct device *dev, struct device_node *msi_np, u32 id_in);
 #else
 static inline void of_irq_init(const struct of_device_id *matches)
@@ -117,7 +116,7 @@ static inline u32 of_msi_map_id(struct device *dev,
  * implements it differently.  However, the prototype is the same for all,
  * so declare it here regardless of the CONFIG_OF_IRQ setting.
  */
-extern unsigned int irq_of_parse_and_map(struct device_node *node, int index);
+unsigned int irq_of_parse_and_map(struct device_node *node, int index);
 
 #else /* !CONFIG_OF && !CONFIG_SPARC */
 static inline unsigned int irq_of_parse_and_map(struct device_node *dev,
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Mark Hasemeyer]

The underlying ACPI and OF subsystems provide their own APIs which
provide IRQ information as a struct resource. This allows callers to get
more information about the IRQ by looking at the resource flags.  For
example, whether or not an IRQ is wake capable.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-New patch

 drivers/acpi/property.c  | 11 +++++------
 drivers/base/property.c  | 24 +++++++++++++++++++++---
 drivers/of/property.c    |  8 ++++----
 include/linux/fwnode.h   |  8 +++++---
 include/linux/property.h |  2 ++
 5 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index a6ead5204046b..259869987ffff 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1627,17 +1627,16 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
-static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
-			       unsigned int index)
+static int acpi_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
+					unsigned int index, struct resource *r)
 {
-	struct resource res;
 	int ret;
 
-	ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, &res);
+	ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, r);
 	if (ret)
 		return ret;
 
-	return res.start;
+	return r->start;
 }
 
 #define DECLARE_ACPI_FWNODE_OPS(ops) \
@@ -1664,7 +1663,7 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
 			acpi_graph_get_remote_endpoint,			\
 		.graph_get_port_parent = acpi_fwnode_get_parent,	\
 		.graph_parse_endpoint = acpi_fwnode_graph_parse_endpoint, \
-		.irq_get = acpi_fwnode_irq_get,				\
+		.irq_get_resource = acpi_fwnode_irq_get_resource,	\
 	};								\
 	EXPORT_SYMBOL_GPL(ops)
 
diff --git a/drivers/base/property.c b/drivers/base/property.c
index a1b01ab420528..4f5d5ab5ab8cf 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1047,23 +1047,41 @@ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index)
 EXPORT_SYMBOL(fwnode_iomap);
 
 /**
- * fwnode_irq_get - Get IRQ directly from a fwnode
+ * fwnode_irq_get_resource - Get IRQ directly from a fwnode and populate
+ *			     the resource struct
  * @fwnode:	Pointer to the firmware node
  * @index:	Zero-based index of the IRQ
+ * @r:		Pointer to resource to populate with IRQ information.
  *
  * Return: Linux IRQ number on success. Negative errno on failure.
  */
-int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
+int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
+			    unsigned int index, struct resource *r)
 {
 	int ret;
 
-	ret = fwnode_call_int_op(fwnode, irq_get, index);
+	ret = fwnode_call_int_op(fwnode, irq_get_resource, index, r);
 	/* We treat mapping errors as invalid case */
 	if (ret == 0)
 		return -EINVAL;
 
 	return ret;
 }
+EXPORT_SYMBOL(fwnode_irq_get_resource);
+
+/**
+ * fwnode_irq_get - Get IRQ directly from a fwnode
+ * @fwnode:	Pointer to the firmware node
+ * @index:	Zero-based index of the IRQ
+ *
+ * Return: Linux IRQ number on success. Negative errno on failure.
+ */
+int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
+{
+	struct resource r;
+
+	return fwnode_irq_get_resource(fwnode, index, &r);
+}
 EXPORT_SYMBOL(fwnode_irq_get);
 
 /**
diff --git a/drivers/of/property.c b/drivers/of/property.c
index afdaefbd03f61..864ea5fa5702b 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1425,10 +1425,10 @@ static void __iomem *of_fwnode_iomap(struct fwnode_handle *fwnode, int index)
 #endif
 }
 
-static int of_fwnode_irq_get(const struct fwnode_handle *fwnode,
-			     unsigned int index)
+static int of_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
+				      unsigned int index, struct resource *r)
 {
-	return of_irq_get(to_of_node(fwnode), index);
+	return of_irq_to_resource(to_of_node(fwnode), index, r);
 }
 
 static int of_fwnode_add_links(struct fwnode_handle *fwnode)
@@ -1469,7 +1469,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.graph_get_port_parent = of_fwnode_graph_get_port_parent,
 	.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
 	.iomap = of_fwnode_iomap,
-	.irq_get = of_fwnode_irq_get,
+	.irq_get_resource = of_fwnode_irq_get_resource,
 	.add_links = of_fwnode_add_links,
 };
 EXPORT_SYMBOL_GPL(of_fwnode_ops);
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 2a72f55d26eb8..716ed863acde0 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -9,10 +9,11 @@
 #ifndef _LINUX_FWNODE_H_
 #define _LINUX_FWNODE_H_
 
-#include <linux/types.h>
-#include <linux/list.h>
 #include <linux/bits.h>
 #include <linux/err.h>
+#include <linux/ioport.h>
+#include <linux/list.h>
+#include <linux/types.h>
 
 struct fwnode_operations;
 struct device;
@@ -164,7 +165,8 @@ struct fwnode_operations {
 	int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
 				    struct fwnode_endpoint *endpoint);
 	void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
-	int (*irq_get)(const struct fwnode_handle *fwnode, unsigned int index);
+	int (*irq_get_resource)(const struct fwnode_handle *fwnode,
+				unsigned int index, struct resource *r);
 	int (*add_links)(struct fwnode_handle *fwnode);
 };
 
diff --git a/include/linux/property.h b/include/linux/property.h
index e6516d0b7d52a..685ba72a8ce9e 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -190,6 +190,8 @@ struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
 int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
+int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
+			    unsigned int index, struct resource *r);
 int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
 
 unsigned int device_get_child_node_count(const struct device *dev);
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 21/22] platform: Modify platform_get_irq_optional() to use resource

[Mark Hasemeyer]

Unify handling of ACPI, GPIO, devictree, and platform resource
interrupts in platform_get_irq_optional(). Each of these subsystems
provide their own APIs which provide IRQ information as a struct
resource. This simplifies the logic of the function and allows callers
to get more information about the IRQ by looking at the resource flags.
For example, whether or not an IRQ is wake capable.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-irq->IRQ
-Remove cast to struct resource
-Conform to get_optional() function naming
-Revert move of irq_get_irq_data()
-Add NULL check on struct resource*
-Use fwnode to retrieve IRQ for DT/ACPI

 drivers/base/platform.c         | 74 +++++++++++++++++++++++----------
 include/linux/platform_device.h |  3 ++
 2 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 10c5779634182..3a556faddd40d 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -151,9 +151,11 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
 #endif /* CONFIG_HAS_IOMEM */
 
 /**
- * platform_get_irq_optional - get an optional IRQ for a device
+ * platform_get_irq_resource_optional - get an optional IRQ for a device and
+ *					populate the resource struct
  * @dev: platform device
  * @num: IRQ number index
+ * @r: pointer to resource to populate with IRQ information.
  *
  * Gets an IRQ for a platform device. Device drivers should check the return
  * value for errors so as to not pass a negative integer value to the
@@ -162,47 +164,42 @@ EXPORT_SYMBOL_GPL(devm_platform_ioremap_resource_byname);
  *
  * For example::
  *
- *		int irq = platform_get_irq_optional(pdev, 0);
+ *		int irq = platform_get_irq_resource_optional(pdev, 0, &res);
  *		if (irq < 0)
  *			return irq;
  *
  * Return: non-zero IRQ number on success, negative error number on failure.
  */
-int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+int platform_get_irq_resource_optional(struct platform_device *dev,
+				       unsigned int num, struct resource *r)
 {
 	int ret;
+	if (IS_ERR_OR_NULL(r))
+		return -EINVAL;
 #ifdef CONFIG_SPARC
 	/* sparc does not have irqs represented as IORESOURCE_IRQ resources */
 	if (!dev || num >= dev->archdata.num_irqs)
 		goto out_not_found;
 	ret = dev->archdata.irqs[num];
+	if (ret > 0)
+		*r = DEFINE_RES_IRQ(ret);
 	goto out;
 #else
 	struct fwnode_handle *fwnode = dev_fwnode(&dev->dev);
-	struct resource *r;
+	struct resource *platform_res;
 
-	if (is_of_node(fwnode)) {
-		ret = of_irq_get(to_of_node(fwnode), num);
-		if (ret > 0 || ret == -EPROBE_DEFER)
-			goto out;
-	}
-
-	r = platform_get_resource(dev, IORESOURCE_IRQ, num);
-	if (is_acpi_device_node(fwnode)) {
-		if (r && r->flags & IORESOURCE_DISABLED) {
-			ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), num, r);
-			if (ret)
-				goto out;
-		}
-	}
+	ret = fwnode_irq_get_resource(fwnode, num, r);
+	if (ret > 0 || ret == -EPROBE_DEFER)
+		goto out;
 
+	platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);
 	/*
 	 * The resources may pass trigger flags to the irqs that need
 	 * to be set up. It so happens that the trigger flags for
 	 * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
 	 * settings.
 	 */
-	if (r && r->flags & IORESOURCE_BITS) {
+	if (platform_res && platform_res->flags & IORESOURCE_BITS) {
 		struct irq_data *irqd;
 
 		irqd = irq_get_irq_data(r->start);
@@ -211,7 +208,8 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		irqd_set_trigger_type(irqd, r->flags & IORESOURCE_BITS);
 	}
 
-	if (r) {
+	if (platform_res) {
+		*r = *platform_res;
 		ret = r->start;
 		goto out;
 	}
@@ -224,10 +222,12 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 	 * allows a common code path across either kind of resource.
 	 */
 	if (num == 0 && is_acpi_device_node(fwnode)) {
-		ret = acpi_dev_gpio_irq_get(to_acpi_device_node(fwnode), num);
+		ret = acpi_dev_get_gpio_irq_resource(to_acpi_device_node(fwnode), NULL, num, r);
 		/* Our callers expect -ENXIO for missing IRQs. */
-		if (ret >= 0 || ret == -EPROBE_DEFER)
+		if (!ret || ret == -EPROBE_DEFER) {
+			ret = ret ?: r->start;
 			goto out;
+		}
 	}
 
 #endif
@@ -238,7 +238,8 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
 		return -EINVAL;
 	return ret;
 }
-EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+EXPORT_SYMBOL_GPL(platform_get_irq_resource_optional);
+
 
 /**
  * platform_get_irq - get an IRQ for a device
@@ -270,6 +271,33 @@ int platform_get_irq(struct platform_device *dev, unsigned int num)
 }
 EXPORT_SYMBOL_GPL(platform_get_irq);
 
+
+/**
+ * platform_get_irq_optional - get an optional IRQ for a device
+ * @dev: platform device
+ * @num: IRQ number index
+ *
+ * Gets an IRQ for a platform device. Device drivers should check the return
+ * value for errors so as to not pass a negative integer value to the
+ * request_irq() APIs. This is the same as platform_get_irq(), except that it
+ * does not print an error message if an IRQ can not be obtained.
+ *
+ * For example::
+ *
+ *		int irq = platform_get_irq_optional(pdev, 0);
+ *		if (irq < 0)
+ *			return irq;
+ *
+ * Return: non-zero IRQ number on success, negative error number on failure.
+ */
+int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
+{
+	struct resource r;
+
+	return platform_get_irq_resource_optional(dev, num, &r);
+}
+EXPORT_SYMBOL_GPL(platform_get_irq_optional);
+
 /**
  * platform_irq_count - Count the number of IRQs a platform device uses
  * @dev: platform device
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 7a41c72c19591..2117f817d9c9c 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -102,6 +102,9 @@ devm_platform_ioremap_resource_byname(struct platform_device *pdev,
 
 extern int platform_get_irq(struct platform_device *, unsigned int);
 extern int platform_get_irq_optional(struct platform_device *, unsigned int);
+extern int platform_get_irq_resource_optional(struct platform_device *dev,
+					      unsigned int num,
+					      struct resource *r);
 extern int platform_irq_count(struct platform_device *);
 extern int devm_platform_get_irqs_affinity(struct platform_device *dev,
 					   struct irq_affinity *affd,
-- 
2.43.0.472.g3155946c3a-goog

[PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

The cros ec driver is manually managing the wake IRQ by calling
enable_irq_wake()/disable_irq_wake() during suspend/resume.

Modify the driver to use the power management subsystem to manage the
wakeirq.

Rather than assuming that the IRQ is wake capable, use the underlying
firmware/device tree to determine whether or not to enable it as a wake
source. Some Chromebooks rely solely on the ec_sync pin to wake the AP
but do not specify the interrupt as wake capable in the ACPI _CRS. For
LPC/ACPI based systems a DMI quirk is introduced listing boards whose
firmware should not be trusted to provide correct wake capable values.
For device tree base systems, it is not an issue as the relevant device
tree entries have been updated and DTS is built from source for each
ChromeOS update.

The IRQ wake logic was added on an interface basis (lpc, spi, uart) as
opposed to adding it to cros_ec.c because the i2c subsystem already
enables the wakirq (if applicable) on our behalf.

Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
---

Changes in v2:
-Rebase on linux-next
-Add cover letter
-See each patch for patch specific changes
-Look for 'wakeup-source' property in cros_ec_spi.c

 drivers/platform/chrome/cros_ec.c           |  9 ----
 drivers/platform/chrome/cros_ec_lpc.c       | 52 +++++++++++++++++++--
 drivers/platform/chrome/cros_ec_spi.c       | 41 ++++++++++++----
 drivers/platform/chrome/cros_ec_uart.c      | 34 ++++++++++++--
 include/linux/platform_data/cros_ec_proto.h |  2 -
 5 files changed, 110 insertions(+), 28 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index badc68bbae8cc..f24d2f2084399 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -353,12 +353,6 @@ EXPORT_SYMBOL(cros_ec_suspend_prepare);
 
 static void cros_ec_disable_irq(struct cros_ec_device *ec_dev)
 {
-	struct device *dev = ec_dev->dev;
-	if (device_may_wakeup(dev))
-		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
-	else
-		ec_dev->wake_enabled = false;
-
 	disable_irq(ec_dev->irq);
 	ec_dev->suspended = true;
 }
@@ -440,9 +434,6 @@ static void cros_ec_enable_irq(struct cros_ec_device *ec_dev)
 	ec_dev->suspended = false;
 	enable_irq(ec_dev->irq);
 
-	if (ec_dev->wake_enabled)
-		disable_irq_wake(ec_dev->irq);
-
 	/*
 	 * Let the mfd devices know about events that occur during
 	 * suspend. This way the clients know what to do with them.
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index f0f3d3d561572..fec5aefd6f177 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -21,6 +21,7 @@
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/printk.h>
 #include <linux/reboot.h>
 #include <linux/suspend.h>
@@ -48,6 +49,28 @@ struct lpc_driver_ops {
 
 static struct lpc_driver_ops cros_ec_lpc_ops = { };
 
+static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = {
+	{
+		.ident = "Brya",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya")
+		}
+	},
+	{
+		.ident = "Brask",
+		.matches = {
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask")
+		}
+	},
+	{ }
+}
+MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
+
+static bool cros_ec_should_force_irq_wake_capable(void)
+{
+	return dmi_first_match(untrusted_fw_irq_wake_capable) != NULL;
+}
+
 /*
  * A generic instance of the read function of struct lpc_driver_ops, used for
  * the LPC EC.
@@ -350,9 +373,11 @@ static void cros_ec_lpc_acpi_notify(acpi_handle device, u32 value, void *data)
 static int cros_ec_lpc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	bool irq_wake = false;
 	struct acpi_device *adev;
 	acpi_status status;
 	struct cros_ec_device *ec_dev;
+	struct resource irqres;
 	u8 buf[2] = {};
 	int irq, ret;
 
@@ -428,20 +453,36 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 	 * Some boards do not have an IRQ allotted for cros_ec_lpc,
 	 * which makes ENXIO an expected (and safe) scenario.
 	 */
-	irq = platform_get_irq_optional(pdev, 0);
-	if (irq > 0)
+	irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
+	if (irq > 0) {
 		ec_dev->irq = irq;
-	else if (irq != -ENXIO) {
+		if (cros_ec_should_force_irq_wake_capable())
+			irq_wake = true;
+		else
+			irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+		dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", irq, irq_wake);
+	} else if (irq != -ENXIO) {
 		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
 		return irq;
 	}
 
 	ret = cros_ec_register(ec_dev);
 	if (ret) {
-		dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
+		dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
 		return ret;
 	}
 
+	if (irq_wake) {
+		ret = device_init_wakeup(dev, true);
+		if (ret) {
+			dev_err_probe(dev, ret, "Failed to init device for wakeup");
+			return ret;
+		}
+		ret = dev_pm_set_wake_irq(dev, irq);
+		if (ret)
+			return ret;
+	}
+
 	/*
 	 * Connect a notify handler to process MKBP messages if we have a
 	 * companion ACPI device.
@@ -463,6 +504,7 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
 static void cros_ec_lpc_remove(struct platform_device *pdev)
 {
 	struct cros_ec_device *ec_dev = platform_get_drvdata(pdev);
+	struct device *dev = ec_dev->dev;
 	struct acpi_device *adev;
 
 	adev = ACPI_COMPANION(&pdev->dev);
@@ -470,6 +512,8 @@ static void cros_ec_lpc_remove(struct platform_device *pdev)
 		acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
 					   cros_ec_lpc_acpi_notify);
 
+	dev_pm_clear_wake_irq(dev);
+	device_init_wakeup(dev, false);
 	cros_ec_unregister(ec_dev);
 }
 
diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
index 3e88cc92e8192..0aad8b2f007f6 100644
--- a/drivers/platform/chrome/cros_ec_spi.c
+++ b/drivers/platform/chrome/cros_ec_spi.c
@@ -7,9 +7,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
 #include <linux/platform_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <uapi/linux/sched/types.h>
@@ -70,6 +72,7 @@
  * @end_of_msg_delay: used to set the delay_usecs on the spi_transfer that
  *      is sent when we want to turn off CS at the end of a transaction.
  * @high_pri_worker: Used to schedule high priority work.
+ * @irq_wake: Whether or not irq assertion should wake the system.
  */
 struct cros_ec_spi {
 	struct spi_device *spi;
@@ -77,6 +80,7 @@ struct cros_ec_spi {
 	unsigned int start_of_msg_delay;
 	unsigned int end_of_msg_delay;
 	struct kthread_worker *high_pri_worker;
+	bool irq_wake;
 };
 
 typedef int (*cros_ec_xfer_fn_t) (struct cros_ec_device *ec_dev,
@@ -689,12 +693,16 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 	return cros_ec_xfer_high_pri(ec_dev, ec_msg, do_cros_ec_cmd_xfer_spi);
 }
 
-static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
+static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct spi_device *spi)
 {
-	struct device_node *np = dev->of_node;
+	struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
+	struct device_node *np = spi->dev.of_node;
 	u32 val;
 	int ret;
 
+	if (!np)
+		return;
+
 	ret = of_property_read_u32(np, "google,cros-ec-spi-pre-delay", &val);
 	if (!ret)
 		ec_spi->start_of_msg_delay = val;
@@ -702,6 +710,11 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
 	ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
 	if (!ret)
 		ec_spi->end_of_msg_delay = val;
+
+	if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {
+		ec_spi->irq_wake = true;
+		dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %i\n", ec_dev->irq, ec_spi->irq_wake);
+	}
 }
 
 static void cros_ec_spi_high_pri_release(void *worker)
@@ -753,9 +766,6 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 	if (!ec_dev)
 		return -ENOMEM;
 
-	/* Check for any DT properties */
-	cros_ec_spi_dt_probe(ec_spi, dev);
-
 	spi_set_drvdata(spi, ec_dev);
 	ec_dev->dev = dev;
 	ec_dev->priv = ec_spi;
@@ -768,6 +778,9 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 			   sizeof(struct ec_response_get_protocol_info);
 	ec_dev->dout_size = sizeof(struct ec_host_request);
 
+	/* Check for any DT properties */
+	cros_ec_spi_dt_probe(ec_spi, spi);
+
 	ec_spi->last_transfer_ns = ktime_get_ns();
 
 	err = cros_ec_spi_devm_high_pri_alloc(dev, ec_spi);
@@ -776,19 +789,31 @@ static int cros_ec_spi_probe(struct spi_device *spi)
 
 	err = cros_ec_register(ec_dev);
 	if (err) {
-		dev_err(dev, "cannot register EC\n");
+		dev_err_probe(dev, err, "cannot register EC\n");
 		return err;
 	}
 
-	device_init_wakeup(&spi->dev, true);
+	if (ec_spi->irq_wake) {
+		err = device_init_wakeup(dev, true);
+		if (err) {
+			dev_err_probe(dev, err, "Failed to init device for wakeup\n");
+			return err;
+		}
+		err = dev_pm_set_wake_irq(dev, ec_dev->irq);
+		if (err)
+			dev_err_probe(dev, err, "Failed to set irq(%d) for wake\n", ec_dev->irq);
+	}
 
-	return 0;
+	return err;
 }
 
 static void cros_ec_spi_remove(struct spi_device *spi)
 {
 	struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
+	struct device *dev = ec_dev->dev;
 
+	dev_pm_clear_wake_irq(dev);
+	device_init_wakeup(dev, false);
 	cros_ec_unregister(ec_dev);
 }
 
diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
index 68d80559fddc2..ced53bb40b2ef 100644
--- a/drivers/platform/chrome/cros_ec_uart.c
+++ b/drivers/platform/chrome/cros_ec_uart.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_data/cros_ec_proto.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/serdev.h>
 #include <linux/slab.h>
 #include <uapi/linux/sched/types.h>
@@ -70,6 +71,7 @@ struct response_info {
  * @baudrate:		UART baudrate of attached EC device.
  * @flowcontrol:	UART flowcontrol of attached device.
  * @irq:		Linux IRQ number of associated serial device.
+ * @irq_wake:		Whether or not irq assertion should wake the system.
  * @response:		Response info passing between cros_ec_uart_pkt_xfer()
  *			and cros_ec_uart_rx_bytes()
  */
@@ -78,6 +80,7 @@ struct cros_ec_uart {
 	u32 baudrate;
 	u8 flowcontrol;
 	u32 irq;
+	bool irq_wake;
 	struct response_info response;
 };
 
@@ -224,8 +227,10 @@ static int cros_ec_uart_resource(struct acpi_resource *ares, void *data)
 static int cros_ec_uart_acpi_probe(struct cros_ec_uart *ec_uart)
 {
 	int ret;
+	struct resource irqres;
 	LIST_HEAD(resources);
-	struct acpi_device *adev = ACPI_COMPANION(&ec_uart->serdev->dev);
+	struct device *dev = &ec_uart->serdev->dev;
+	struct acpi_device *adev = ACPI_COMPANION(dev);
 
 	ret = acpi_dev_get_resources(adev, &resources, cros_ec_uart_resource, ec_uart);
 	if (ret < 0)
@@ -234,12 +239,13 @@ static int cros_ec_uart_acpi_probe(struct cros_ec_uart *ec_uart)
 	acpi_dev_free_resource_list(&resources);
 
 	/* Retrieve GpioInt and translate it to Linux IRQ number */
-	ret = acpi_dev_gpio_irq_get(adev, 0);
+	ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, &irqres);
 	if (ret < 0)
 		return ret;
 
-	ec_uart->irq = ret;
-	dev_dbg(&ec_uart->serdev->dev, "IRQ number %d\n", ec_uart->irq);
+	ec_uart->irq = irqres.start;
+	ec_uart->irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
+	dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", ec_uart->irq, ec_uart->irq_wake);
 
 	return 0;
 }
@@ -301,13 +307,31 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
 
 	serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
 
-	return cros_ec_register(ec_dev);
+	/* Register a new cros_ec device */
+	ret = cros_ec_register(ec_dev);
+	if (ret) {
+		dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
+		return ret;
+	}
+
+	if (ec_uart->irq_wake) {
+		ret = device_init_wakeup(dev, true);
+		if (ret) {
+			dev_err_probe(dev, ret, "Failed to init device for wakeup");
+			return ret;
+		}
+		ret = dev_pm_set_wake_irq(dev, ec_uart->irq);
+	}
+	return ret;
 }
 
 static void cros_ec_uart_remove(struct serdev_device *serdev)
 {
 	struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev);
+	struct device *dev = ec_dev->dev;
 
+	dev_pm_clear_wake_irq(dev);
+	device_init_wakeup(dev, false);
 	cros_ec_unregister(ec_dev);
 };
 
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 8865e350c12a5..91e32db4ef715 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -115,7 +115,6 @@ struct cros_ec_command {
  *        performance advantage to using dword.
  * @din_size: Size of din buffer to allocate (zero to use static din).
  * @dout_size: Size of dout buffer to allocate (zero to use static dout).
- * @wake_enabled: True if this device can wake the system from sleep.
  * @suspended: True if this device had been suspended.
  * @cmd_xfer: Send command to EC and get response.
  *            Returns the number of bytes received if the communication
@@ -173,7 +172,6 @@ struct cros_ec_device {
 	u8 *dout;
 	int din_size;
 	int dout_size;
-	bool wake_enabled;
 	bool suspended;
 	int (*cmd_xfer)(struct cros_ec_device *ec,
 			struct cros_ec_command *msg);
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH v2 13/22] arm64: dts: qcom: sc7180: Enable cros-ec-spi as wake source

[Doug Anderson]

Hi,

On Wed, Dec 20, 2023 at 3:55 PM Mark Hasemeyer <markhas@chromium.org> wrote:
>
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
>
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
>
> Changes in v2:
> -Split by arch/soc
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 +
>  1 file changed, 1 insertion(+)

It's hard to get context with just the dts patches, but digging up the
cover letter and other patches from lore I see you point at
`Documentation/devicetree/bindings/power/wakeup-source.txt` which says
that devices that can wakeup should have this property. ...and our EC
can wake us up, so this looks right from that point of view.

Also the yaml file for cros-ec says it's fine to have this property. I
think it was used when things were connected via i2c since the i2c
subsystem needed it. ...so from a bindings perspective it also seems
fine to me.

...and looking at the code in Linux, I guess things work today because
cros_ec_spi_probe() unconditionally calls device_init_wakeup(). ...but
even with the code today I believe it should be fine to add this
property.

So with all that, this patch looks fine to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 14/22] arm64: dts: qcom: sc7280: Enable cros-ec-spi as wake source

[Doug Anderson]

Hi,

On Wed, Dec 20, 2023 at 3:55 PM Mark Hasemeyer <markhas@chromium.org> wrote:
>
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
>
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
>
> Changes in v2:
> -Split by arch/soc
>
>  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 1 +
>  arch/arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 1 +
>  2 files changed, 2 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 15/22] arm64: dts: qcom: sdm845: Enable cros-ec-spi as wake source

[Doug Anderson]

Hi,

On Wed, Dec 20, 2023 at 3:55 PM Mark Hasemeyer <markhas@chromium.org> wrote:
>
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
>
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
>
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
>
> Changes in v2:
> -Split by arch/soc
>
>  arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 03/22] Documentation: devicetree: Clarify wording for wakeup-source property

[Krzysztof Kozlowski]

On 21/12/2023 00:54, Mark Hasemeyer wrote:
> The wording in the current documentation is a little strong. The
> intention was not to fix any particular interrupt as wakeup capable but
> leave those details to the device. It wasn't intended to enforce any
> rules as what can be or can't be a wakeup interrupt.
> 
> Soften the wording to not mandate that the 'wakeup-source' property be
> used, and clarify what it means when an interrupt is marked (or not
> marked) for wakeup.
> 
> Link: https://lore.kernel.org/all/ZYAjxxHcCOgDVMTQ@bogus/
> Link: https://lore.kernel.org/all/CAL_Jsq+MYwOG40X26cYmO9EkZ9xqWrXDi03MaRfxnV-+VGkXWQ@mail.gmail.com/
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

You nicely skipped all my filters... No need to resend to fix this, but
fix it if sending a new version.

Best regards,
Krzysztof

Re: [PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Krzysztof Kozlowski]

On 21/12/2023 00:54, Mark Hasemeyer wrote:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
> 
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
> 

You do not need this property, if driver assumes that. Just enable it
unconditionally. I don't think anything from previous discussion was
resolved.

Best regards,
Krzysztof

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Tzung-Bi Shih]

On Wed, Dec 20, 2023 at 04:54:36PM -0700, Mark Hasemeyer wrote:
> The IRQ wake logic was added on an interface basis (lpc, spi, uart) as
> opposed to adding it to cros_ec.c because the i2c subsystem already
> enables the wakirq (if applicable) on our behalf.

The setting flow are all the same.  I think helper functions in cros_ec.c help
to deduplicate the code.

> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
[...]
> +static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = {
> +	{
> +		.ident = "Brya",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya")
> +		}
> +	},
> +	{
> +		.ident = "Brask",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask")
> +		}
> +	},
> +	{ }
> +}
> +MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);

Does it really need `MODULE_DEVICE_TABLE`?

> +static bool cros_ec_should_force_irq_wake_capable(void)

Suggestion: either drop "cros_ec_" prefix or use "cros_ec_lpc_" prefix.

> @@ -428,20 +453,36 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
>  	 * Some boards do not have an IRQ allotted for cros_ec_lpc,
>  	 * which makes ENXIO an expected (and safe) scenario.
>  	 */
> -	irq = platform_get_irq_optional(pdev, 0);
> -	if (irq > 0)
> +	irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> +	if (irq > 0) {
>  		ec_dev->irq = irq;
> -	else if (irq != -ENXIO) {
> +		if (cros_ec_should_force_irq_wake_capable())

Please see suggestion above.

>  	ret = cros_ec_register(ec_dev);
>  	if (ret) {
> -		dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> +		dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);

The change is irrelevant to the series.

> +	if (irq_wake) {
> +		ret = device_init_wakeup(dev, true);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Failed to init device for wakeup");
> +			return ret;
> +		}
> +		ret = dev_pm_set_wake_irq(dev, irq);
> +		if (ret)
> +			return ret;
> +	}
[...]
> @@ -470,6 +512,8 @@ static void cros_ec_lpc_remove(struct platform_device *pdev)
>  		acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
>  					   cros_ec_lpc_acpi_notify);
>  
> +	dev_pm_clear_wake_irq(dev);
> +	device_init_wakeup(dev, false);

Is it safe to call them anyway regardless of `irq_wake` in cros_ec_lpc_probe()?

> diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
[...]
> -static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> +static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct spi_device *spi)
>  {
> -	struct device_node *np = dev->of_node;
> +	struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
> +	struct device_node *np = spi->dev.of_node;

struct spi_device *spi = ec_spi->spi; [1]

[1]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome/cros_ec_spi.c#L751

> +	if (!np)
> +		return;
> +

The change is an improvement (or rather say it could change behavior).  But
strictly speaking, the change is irrelevant to the series.

> @@ -702,6 +710,11 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
>  	ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
>  	if (!ret)
>  		ec_spi->end_of_msg_delay = val;
> +
> +	if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {

Or just use `spi->irq`[2].

[2]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome/cros_ec_spi.c#L762

They are the same, but does of_property_present() make more sense?

> @@ -768,6 +778,9 @@ static int cros_ec_spi_probe(struct spi_device *spi)
>  			   sizeof(struct ec_response_get_protocol_info);
>  	ec_dev->dout_size = sizeof(struct ec_host_request);
>  
> +	/* Check for any DT properties */
> +	cros_ec_spi_dt_probe(ec_spi, spi);

`spi` is possibly not needed.  See comment above.

> @@ -776,19 +789,31 @@ static int cros_ec_spi_probe(struct spi_device *spi)
>  
>  	err = cros_ec_register(ec_dev);
>  	if (err) {
> -		dev_err(dev, "cannot register EC\n");
> +		dev_err_probe(dev, err, "cannot register EC\n");

The change is irrelevant to the series.

> -	device_init_wakeup(&spi->dev, true);
> +	if (ec_spi->irq_wake) {
> +		err = device_init_wakeup(dev, true);
> +		if (err) {
> +			dev_err_probe(dev, err, "Failed to init device for wakeup\n");
> +			return err;
> +		}
> +		err = dev_pm_set_wake_irq(dev, ec_dev->irq);
> +		if (err)
> +			dev_err_probe(dev, err, "Failed to set irq(%d) for wake\n", ec_dev->irq);

The part is different from what the patch changed in cros_ec_lpc.c.  Better to
be consistent.
- Just return vs. dev_err_probe().
- %i vs. %d.

>  static void cros_ec_spi_remove(struct spi_device *spi)
>  {
>  	struct cros_ec_device *ec_dev = spi_get_drvdata(spi);
> +	struct device *dev = ec_dev->dev;
>  
> +	dev_pm_clear_wake_irq(dev);
> +	device_init_wakeup(dev, false);

Ditto, is it safe to just call them regardless of `ec_spi->irq_wake`?

> diff --git a/drivers/platform/chrome/cros_ec_uart.c b/drivers/platform/chrome/cros_ec_uart.c
[...]
> @@ -301,13 +307,31 @@ static int cros_ec_uart_probe(struct serdev_device *serdev)
>  
>  	serdev_device_set_client_ops(serdev, &cros_ec_uart_client_ops);
>  
> -	return cros_ec_register(ec_dev);
> +	/* Register a new cros_ec device */
> +	ret = cros_ec_register(ec_dev);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);

From reading the changes above, I thought it would use dev_err_probe().

> +	if (ec_uart->irq_wake) {
> +		ret = device_init_wakeup(dev, true);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Failed to init device for wakeup");
> +			return ret;
> +		}
> +		ret = dev_pm_set_wake_irq(dev, ec_uart->irq);

Ditto, better to be consistent.

>  static void cros_ec_uart_remove(struct serdev_device *serdev)
>  {
>  	struct cros_ec_device *ec_dev = serdev_device_get_drvdata(serdev);
> +	struct device *dev = ec_dev->dev;
>  
> +	dev_pm_clear_wake_irq(dev);
> +	device_init_wakeup(dev, false);

Ditto, is it safe to just call them regardless of `ec_uart->irq_wake`?

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Sakari Ailus]

Hi Mark,

Thank you for the patch.

On Wed, Dec 20, 2023 at 04:54:34PM -0700, Mark Hasemeyer wrote:
> The underlying ACPI and OF subsystems provide their own APIs which
> provide IRQ information as a struct resource. This allows callers to get
> more information about the IRQ by looking at the resource flags.  For
> example, whether or not an IRQ is wake capable.
> 
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
> 
> Changes in v2:
> -New patch
> 
>  drivers/acpi/property.c  | 11 +++++------
>  drivers/base/property.c  | 24 +++++++++++++++++++++---
>  drivers/of/property.c    |  8 ++++----
>  include/linux/fwnode.h   |  8 +++++---
>  include/linux/property.h |  2 ++
>  5 files changed, 37 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index a6ead5204046b..259869987ffff 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -1627,17 +1627,16 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
>  	return 0;
>  }
>  
> -static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
> -			       unsigned int index)
> +static int acpi_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +					unsigned int index, struct resource *r)
>  {
> -	struct resource res;
>  	int ret;
>  
> -	ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, &res);
> +	ret = acpi_irq_get(ACPI_HANDLE_FWNODE(fwnode), index, r);
>  	if (ret)
>  		return ret;
>  
> -	return res.start;
> +	return r->start;
>  }
>  
>  #define DECLARE_ACPI_FWNODE_OPS(ops) \
> @@ -1664,7 +1663,7 @@ static int acpi_fwnode_irq_get(const struct fwnode_handle *fwnode,
>  			acpi_graph_get_remote_endpoint,			\
>  		.graph_get_port_parent = acpi_fwnode_get_parent,	\
>  		.graph_parse_endpoint = acpi_fwnode_graph_parse_endpoint, \
> -		.irq_get = acpi_fwnode_irq_get,				\
> +		.irq_get_resource = acpi_fwnode_irq_get_resource,	\
>  	};								\
>  	EXPORT_SYMBOL_GPL(ops)
>  
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index a1b01ab420528..4f5d5ab5ab8cf 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -1047,23 +1047,41 @@ void __iomem *fwnode_iomap(struct fwnode_handle *fwnode, int index)
>  EXPORT_SYMBOL(fwnode_iomap);
>  
>  /**
> - * fwnode_irq_get - Get IRQ directly from a fwnode
> + * fwnode_irq_get_resource - Get IRQ directly from a fwnode and populate
> + *			     the resource struct
>   * @fwnode:	Pointer to the firmware node
>   * @index:	Zero-based index of the IRQ
> + * @r:		Pointer to resource to populate with IRQ information.
>   *
>   * Return: Linux IRQ number on success. Negative errno on failure.
>   */
> -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +			    unsigned int index, struct resource *r)
>  {
>  	int ret;
>  
> -	ret = fwnode_call_int_op(fwnode, irq_get, index);
> +	ret = fwnode_call_int_op(fwnode, irq_get_resource, index, r);
>  	/* We treat mapping errors as invalid case */
>  	if (ret == 0)
>  		return -EINVAL;
>  
>  	return ret;
>  }
> +EXPORT_SYMBOL(fwnode_irq_get_resource);

Both acpi_irq_get() and of_irq_to_resourece() use EXPORT_SYMBOL_GPL()
instead. I don't see why fwnode_irq_get_resource() shouldn't do the same.

With this changed,

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

In fact this should have always been the case for fwnode_irq_get(). I
wouldn't mind changing that, too, in a separate patch.

> +
> +/**
> + * fwnode_irq_get - Get IRQ directly from a fwnode
> + * @fwnode:	Pointer to the firmware node
> + * @index:	Zero-based index of the IRQ
> + *
> + * Return: Linux IRQ number on success. Negative errno on failure.
> + */
> +int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +{
> +	struct resource r;
> +
> +	return fwnode_irq_get_resource(fwnode, index, &r);
> +}
>  EXPORT_SYMBOL(fwnode_irq_get);
>  
>  /**
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index afdaefbd03f61..864ea5fa5702b 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1425,10 +1425,10 @@ static void __iomem *of_fwnode_iomap(struct fwnode_handle *fwnode, int index)
>  #endif
>  }
>  
> -static int of_fwnode_irq_get(const struct fwnode_handle *fwnode,
> -			     unsigned int index)
> +static int of_fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +				      unsigned int index, struct resource *r)
>  {
> -	return of_irq_get(to_of_node(fwnode), index);
> +	return of_irq_to_resource(to_of_node(fwnode), index, r);
>  }
>  
>  static int of_fwnode_add_links(struct fwnode_handle *fwnode)
> @@ -1469,7 +1469,7 @@ const struct fwnode_operations of_fwnode_ops = {
>  	.graph_get_port_parent = of_fwnode_graph_get_port_parent,
>  	.graph_parse_endpoint = of_fwnode_graph_parse_endpoint,
>  	.iomap = of_fwnode_iomap,
> -	.irq_get = of_fwnode_irq_get,
> +	.irq_get_resource = of_fwnode_irq_get_resource,
>  	.add_links = of_fwnode_add_links,
>  };
>  EXPORT_SYMBOL_GPL(of_fwnode_ops);
> diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> index 2a72f55d26eb8..716ed863acde0 100644
> --- a/include/linux/fwnode.h
> +++ b/include/linux/fwnode.h
> @@ -9,10 +9,11 @@
>  #ifndef _LINUX_FWNODE_H_
>  #define _LINUX_FWNODE_H_
>  
> -#include <linux/types.h>
> -#include <linux/list.h>
>  #include <linux/bits.h>
>  #include <linux/err.h>
> +#include <linux/ioport.h>
> +#include <linux/list.h>
> +#include <linux/types.h>
>  
>  struct fwnode_operations;
>  struct device;
> @@ -164,7 +165,8 @@ struct fwnode_operations {
>  	int (*graph_parse_endpoint)(const struct fwnode_handle *fwnode,
>  				    struct fwnode_endpoint *endpoint);
>  	void __iomem *(*iomap)(struct fwnode_handle *fwnode, int index);
> -	int (*irq_get)(const struct fwnode_handle *fwnode, unsigned int index);
> +	int (*irq_get_resource)(const struct fwnode_handle *fwnode,
> +				unsigned int index, struct resource *r);
>  	int (*add_links)(struct fwnode_handle *fwnode);
>  };
>  
> diff --git a/include/linux/property.h b/include/linux/property.h
> index e6516d0b7d52a..685ba72a8ce9e 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -190,6 +190,8 @@ struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
>  void fwnode_handle_put(struct fwnode_handle *fwnode);
>  
>  int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
> +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +			    unsigned int index, struct resource *r);
>  int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
>  
>  unsigned int device_get_child_node_count(const struct device *dev);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 00/22] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:14PM -0700, Mark Hasemeyer wrote:
> Currently the cros_ec driver assumes that its associated interrupt is
> wake capable. This is an incorrect assumption as some Chromebooks use a
> separate wake pin, while others overload the interrupt for wake and IO.
> This patch train updates the driver to query the underlying ACPI/DT data
> to determine whether or not the IRQ should be enabled for wake.
> 
> Both the device tree and ACPI systems have methods for reporting IRQ
> wake capability. In device tree based systems, a node can advertise
> itself as a 'wakeup-source'. In ACPI based systems, GpioInt and
> Interrupt resource descriptors can use the 'SharedAndWake' or
> 'ExclusiveAndWake' share types.
> 
> Some logic is added to the platform, ACPI, and DT subsystems to more
> easily pipe wakeirq information up to the driver.

Just wondering if you used --histogram diff algo when preparing patches.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 02/22] i2c: acpi: Modify i2c_acpi_get_irq() to use resource

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:16PM -0700, Mark Hasemeyer wrote:
> The i2c_acpi_irq_context structure provides redundant information that
> can be provided with struct resource.
> 
> Refactor i2c_acpi_get_irq() to use struct resource instead of struct
> i2c_acpi_irq_context.

Suggested-by?

...

>  static int i2c_acpi_add_irq_resource(struct acpi_resource *ares, void *data)
>  {
> -	struct i2c_acpi_irq_context *irq_ctx = data;
> -	struct resource r;
> +	struct resource *r = data;

> -	if (irq_ctx->irq > 0)
> +	if (r->start > 0)
>  		return 1;

Checking flags is more robust.

	if (r->flags)
		return 1;

> -	if (!acpi_dev_resource_interrupt(ares, 0, &r))
> +	if (!acpi_dev_resource_interrupt(ares, 0, r))
>  		return 1;
>  
> -	irq_ctx->irq = i2c_dev_irq_from_resources(&r, 1);
> -	irq_ctx->wake_capable = r.flags & IORESOURCE_IRQ_WAKECAPABLE;
> +	i2c_dev_irq_from_resources(r, 1);
>  
>  	return 1; /* No need to add resource to the list */
>  }

...

> +	if (IS_ERR_OR_NULL(r))
> +		return -EINVAL;

Hmm... Do we expect this to be an error pointer in some cases?

...

> +	ret = acpi_dev_get_gpio_irq_resource(adev, NULL, 0, r);
> +	if (!ret)
> +		return r->start;
>  
> -	return irq_ctx.irq;
> +	return ret;

What's wrong with the standard pattern?

	if (ret)
		return ret;
	...
	return ...;

...

> +			struct resource r = {0};

'0' is redundant.

...

> +			irq = i2c_acpi_get_irq(client, &r);
> +			if (irq > 0 && r.flags & IORESOURCE_IRQ_WAKECAPABLE)

Why checking just flags is not enough?

>  				client->flags |= I2C_CLIENT_WAKE;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:34PM -0700, Mark Hasemeyer wrote:
> The underlying ACPI and OF subsystems provide their own APIs which
> provide IRQ information as a struct resource. This allows callers to get
> more information about the IRQ by looking at the resource flags.  For

Double space when other lines have a single space.

> example, whether or not an IRQ is wake capable.

Suggested-by?

...

> -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> +			    unsigned int index, struct resource *r)

It's perfectly fine to replace ) by , on the previous line, no need
to make it shorter.

...

> +int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> +{
> +	struct resource r;

	struct resource r = {};

?

> +	return fwnode_irq_get_resource(fwnode, index, &r);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/22] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:15PM -0700, Mark Hasemeyer wrote:
> Other information besides wake capability can be provided about GPIO
> IRQs such as triggering, polarity, and sharability. Use resource flags
> to provide this information to the caller if they want it.
> 
> This should keep the API more robust over time as flags are added,
> modified, or removed. It also more closely matches acpi_irq_get() which
> take a resource as an argument.
> 
> Rename the function to acpi_dev_get_gpio_irq_resource() to better
> describe the function's new behavior.

...

> +			*r = DEFINE_RES_IRQ(irq);
> +			r->flags = acpi_dev_irq_flags(info.triggering, info.polarity,
> +						      info.shareable, info.wake_capable);

Looking at this I am wondering if we can actually to have

			unsigned long irq_flags;
			...
			irq_flags = acpi_dev_irq_flags(info.triggering, info.polarity,
						       info.shareable, info.wake_capable);
			*r = DEFINE_RES_NAMED(irq, 1, NULL, irq_flags);

as we don't need to duplicate IORESOURCE_IRQ.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 17/22] of: irq: add wake capable bit to of_irq_resource()

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:31PM -0700, Mark Hasemeyer wrote:
> Add wake capability information to the IRQ resource. Wake capability is
> assumed based on conventions provided in the devicetree wakeup-source
> binding documentation. An interrupt is considered wake capable if the
> following are true:
> 1. A wakeup-source property exits in the same device node as the
>    interrupt.
> 2. The IRQ is marked as dedicated by setting its interrupt-name to
>    "wakeup".
> 
> The wakeup-source documentation states that dedicated interrupts can use
> device specific interrupt names and device drivers are still welcome to
> use their own naming schemes. This API is provided as a helper if one is
> willing to conform to the above conventions.
> 
> The ACPI subsystems already provides similar APIs that allow one to
> query the wake capability of an IRQ. This brings closer feature parity
> to the devicetree.

...

>  		r->start = r->end = irq;
>  		r->flags = IORESOURCE_IRQ | irqd_get_trigger_type(irq_get_irq_data(irq));
> +		if (__of_irq_wake_capable(dev, index))
> +			r->flags |= IORESOURCE_IRQ_WAKECAPABLE;
>  		r->name = name ? name : of_node_full_name(dev);

		irq_flags = irqd_get_trigger_type(irq_get_irq_data(irq));
		if (__of_irq_wake_capable(dev, index))
			irq_flags |= IORESOURCE_IRQ_WAKECAPABLE;

		*r = DEFINE_RES_NAMED(irq, 1, name ?: of_node_full_name(dev), irq_flags);

?

...

Or even refactor ioport.h (in a separate patch) as we seems already have
two users (and might be more in the existing code):

#define DEFINE_RES_IRQ_NAMED_FLAGS(_irq, _name, _flags)			\
	DEFINE_RES_NAMED((_irq), 1, (_name), (_flags) | IORESOURCE_IRQ)
#define DEFINE_RES_IRQ_NAMED(_irq, _name)				\
	DEFINE_RES_IRQ_NAMED_FLAGS((_irq), (_name), 0)
#define DEFINE_RES_IRQ(_irq)						\
	DEFINE_RES_IRQ_NAMED((_irq), NULL)

(Note, I will Ack such a patch once it appears.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 21/22] platform: Modify platform_get_irq_optional() to use resource

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:35PM -0700, Mark Hasemeyer wrote:
> Unify handling of ACPI, GPIO, devictree, and platform resource
> interrupts in platform_get_irq_optional(). Each of these subsystems
> provide their own APIs which provide IRQ information as a struct
> resource. This simplifies the logic of the function and allows callers
> to get more information about the IRQ by looking at the resource flags.
> For example, whether or not an IRQ is wake capable.

...

>   * For example::
>   *
> - *		int irq = platform_get_irq_optional(pdev, 0);
> + *		int irq = platform_get_irq_resource_optional(pdev, 0, &res);
>   *		if (irq < 0)
>   *			return irq;
>   *
>   * Return: non-zero IRQ number on success, negative error number on failure.

Why do we need the irq to be returned via error code?

...

>  	int ret;

Missing blank line, have you run checkpatch.pl?

> +	if (IS_ERR_OR_NULL(r))
> +		return -EINVAL;

If we ever have an error pointer in r, I prefer to see

	if (!r)
		return -EINVAL;
	if (IS_ERR(r))
		return PTR_ERR(r);

But Q is the same as earlier: when would we have the error pointer in @r?

...

> +	platform_res = platform_get_resource(dev, IORESOURCE_IRQ, num);

I would move this closer to the condition...

>  	/*
>  	 * The resources may pass trigger flags to the irqs that need
>  	 * to be set up. It so happens that the trigger flags for
>  	 * IORESOURCE_BITS correspond 1-to-1 to the IRQF_TRIGGER*
>  	 * settings.
>  	 */

...i.e. here.

> -	if (r && r->flags & IORESOURCE_BITS) {
> +	if (platform_res && platform_res->flags & IORESOURCE_BITS) {

>  	}

...

>  	if (num == 0 && is_acpi_device_node(fwnode)) {
> -		ret = acpi_dev_gpio_irq_get(to_acpi_device_node(fwnode), num);
> +		ret = acpi_dev_get_gpio_irq_resource(to_acpi_device_node(fwnode), NULL, num, r);
>  		/* Our callers expect -ENXIO for missing IRQs. */

> -		if (ret >= 0 || ret == -EPROBE_DEFER)
> +		if (!ret || ret == -EPROBE_DEFER) {

Can we save this and be consistent with above fwnode API return code check?

> +			ret = ret ?: r->start;
>  			goto out;
> +		}
>  	}

...

> -EXPORT_SYMBOL_GPL(platform_get_irq_optional);
> +EXPORT_SYMBOL_GPL(platform_get_irq_resource_optional);
> +

Stray blank line change.

...

>  EXPORT_SYMBOL_GPL(platform_get_irq);
>  
> +

Ditto.

...

> +int platform_get_irq_optional(struct platform_device *dev, unsigned int num)
> +{
> +	struct resource r;

	struct resource r = {};

?

> +	return platform_get_irq_resource_optional(dev, num, &r);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Andy Shevchenko]

On Wed, Dec 20, 2023 at 04:54:36PM -0700, Mark Hasemeyer wrote:
> The cros ec driver is manually managing the wake IRQ by calling
> enable_irq_wake()/disable_irq_wake() during suspend/resume.
> 
> Modify the driver to use the power management subsystem to manage the
> wakeirq.
> 
> Rather than assuming that the IRQ is wake capable, use the underlying
> firmware/device tree to determine whether or not to enable it as a wake
> source. Some Chromebooks rely solely on the ec_sync pin to wake the AP
> but do not specify the interrupt as wake capable in the ACPI _CRS. For
> LPC/ACPI based systems a DMI quirk is introduced listing boards whose
> firmware should not be trusted to provide correct wake capable values.
> For device tree base systems, it is not an issue as the relevant device
> tree entries have been updated and DTS is built from source for each
> ChromeOS update.
> 
> The IRQ wake logic was added on an interface basis (lpc, spi, uart) as
> opposed to adding it to cros_ec.c because the i2c subsystem already
> enables the wakirq (if applicable) on our behalf.

...

> +static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = {
> +	{
> +		.ident = "Brya",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya")
> +		}

Leave trailing comma.

> +	},
> +	{
> +		.ident = "Brask",
> +		.matches = {
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask")
> +		}

Ditto.

It will reduce a churn in the future if adding more fields here.

> +	},
> +	{ }
> +}

...

> +static bool cros_ec_should_force_irq_wake_capable(void)
> +{
> +	return dmi_first_match(untrusted_fw_irq_wake_capable) != NULL;

' != NULL' is redundant.

> +}

...

>  	struct device *dev = &pdev->dev;

> +	bool irq_wake = false;

Why not put this...

>  	struct acpi_device *adev;
>  	acpi_status status;
>  	struct cros_ec_device *ec_dev;
> +	struct resource irqres;

...here?

>  	u8 buf[2] = {};
>  	int irq, ret;

...

> +	irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> +	if (irq > 0) {
>  		ec_dev->irq = irq;
> -	else if (irq != -ENXIO) {
> +		if (cros_ec_should_force_irq_wake_capable())
> +			irq_wake = true;
> +		else
> +			irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> +		dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", irq, irq_wake);
> +	} else if (irq != -ENXIO) {
>  		dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
>  		return irq;
>  	}

Yeah, this is confusing now. Which one should I trust more: irq or irqres.start?
What is the point to have irqres with this duplication?

...

> -		dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> +		dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
>  		return ret;

		return dev_err_probe(...);

...

> +			dev_err_probe(dev, ret, "Failed to init device for wakeup");
> +			return ret;

Ditto.

...

> +	if (!np)
> +		return;

Why do you need this now?

I would expect either agnostic code or the very first mandatory of_*() call
will fail with the error anyway.

...

>  	ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
>  	if (!ret)
>  		ec_spi->end_of_msg_delay = val;

> +	if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {
> +		ec_spi->irq_wake = true;
> +		dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %i\n", ec_dev->irq, ec_spi->irq_wake);
> +	}

	if (ret)
		return;

	ec_spi->irq_wake = of_property_read_bool(np, "wakeup-source"));
	dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %s\n", ec_dev->irq, str_yes_no(ec_spi->irq_wake));

?

...

> +	if (ec_spi->irq_wake) {
> +		err = device_init_wakeup(dev, true);
> +		if (err) {
> +			dev_err_probe(dev, err, "Failed to init device for wakeup\n");
> +			return err;

			return dev_err_probe(...);

> +		}
> +		err = dev_pm_set_wake_irq(dev, ec_dev->irq);
> +		if (err)
> +			dev_err_probe(dev, err, "Failed to set irq(%d) for wake\n", ec_dev->irq);

		Ditto.

> +	}

> -	return 0;
> +	return err;

Unneeded change (see above how to use dev_err_probe() in an efficient way).

ret / err... Even in one file already some inconsistency...

...

> @@ -78,6 +80,7 @@ struct cros_ec_uart {
>  	u32 baudrate;
>  	u8 flowcontrol;
>  	u32 irq;
> +	bool irq_wake;
>  	struct response_info response;
>  };

Run `pahole` and amend respectively to avoid wasting memory.

...

> +	dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", ec_uart->irq, ec_uart->irq_wake);

str_yes_no() from string_choices.h?

...

> +	/* Register a new cros_ec device */
> +	ret = cros_ec_register(ec_dev);
> +	if (ret) {
> +		dev_err(dev, "Couldn't register ec_dev (%d)\n", ret);
> +		return ret;

Why not dev_err_probe() here...

> +	}
> +
> +	if (ec_uart->irq_wake) {
> +		ret = device_init_wakeup(dev, true);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "Failed to init device for wakeup");
> +			return ret;

...and here?

> +		}
> +		ret = dev_pm_set_wake_irq(dev, ec_uart->irq);
> +	}
> +	return ret;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/22] Documentation: devicetree: Clarify wording for wakeup-source property

[Mark Hasemeyer]

> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
>
> You nicely skipped all my filters... No need to resend to fix this, but
> fix it if sending a new version.

I picked up the tags by using that exact command against "wakeup-source.txt".
"Documentation: devicetree:" was used in the originating commit and is
why I used it. There isn't really a consistent history wrt to tags on
this file. Looking at the containing directory, "dt-bindings: power"
looks pretty common. I'll use that unless you'd prefer something else.

Re: [PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

> You do not need this property, if driver assumes that. Just enable it
> unconditionally.

The goal of this patch series is to change exactly that: to prevent
the driver from unconditionally enabling the irq for wake.
The driver works across numerous buses (spi, uart, i2c, lpc) and
supports DT and ACPI.
SPI+DT systems all happen to need irq wake enabled.

> I don't think anything from previous discussion was
> resolved.

Which previous discussion do you mean? In v1 it was suggested to split
up the DTS changes by arch/soc and add a cover letter (which I did).
Wrt to the binding discussion, Sudeep said the new documentation
wording looked good to him [1].

[1]: https://lore.kernel.org/all/ZYAjxxHcCOgDVMTQ@bogus/

Re: [PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Krzysztof Kozlowski]

On 21/12/2023 19:29, Mark Hasemeyer wrote:
>> You do not need this property, if driver assumes that. Just enable it
>> unconditionally.
> 
> The goal of this patch series is to change exactly that: to prevent
> the driver from unconditionally enabling the irq for wake.

But why? What is the problem being solved? Is unconditional wakeup in
the driver incorrect? If so, mention it shortly in the commit msg, what
is rationale because existing one does not justify this change.

> The driver works across numerous buses (spi, uart, i2c, lpc) and
> supports DT and ACPI.
> SPI+DT systems all happen to need irq wake enabled.
> 
>> I don't think anything from previous discussion was
>> resolved.
> 
> Which previous discussion do you mean? In v1 it was suggested to split

https://lore.kernel.org/all/20231213221124.GB2115075-robh@kernel.org/


Best regards,
Krzysztof

Re: [PATCH v2 03/22] Documentation: devicetree: Clarify wording for wakeup-source property

[Krzysztof Kozlowski]

On 21/12/2023 19:16, Mark Hasemeyer wrote:
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
>>
>> You nicely skipped all my filters... No need to resend to fix this, but
>> fix it if sending a new version.
> 
> I picked up the tags by using that exact command against "wakeup-source.txt".
> "Documentation: devicetree:" was used in the originating commit and is
> why I used it. There isn't really a consistent history wrt to tags on
> this file. Looking at the containing directory, "dt-bindings: power"

All bindings use dt-bindings: prefix. Either first or second. It's the
only correct, even though you will find way too many wrong ones... yet
still my command gives you the answer.

Best regards,
Krzysztof

Re: [PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Mark Hasemeyer]

> >> You do not need this property, if driver assumes that. Just enable it
> >> unconditionally.
> >
> > The goal of this patch series is to change exactly that: to prevent
> > the driver from unconditionally enabling the irq for wake.
>
> But why? What is the problem being solved? Is unconditional wakeup in
> the driver incorrect? If so, mention it shortly in the commit msg, what
> is rationale because existing one does not justify this change.

The cover letter talks about it:
"Currently the cros_ec driver assumes that its associated interrupt is
wake capable. This is an incorrect assumption as some Chromebooks use
a separate wake pin, while others overload the interrupt for wake and
IO."
With the current assumption, spurious wakes can occur on systems that
use a separate wake pin.
I can add wording to the dts patches to help clarify.

> > The driver works across numerous buses (spi, uart, i2c, lpc) and
> > supports DT and ACPI.
> > SPI+DT systems all happen to need irq wake enabled.
> >
> >> I don't think anything from previous discussion was
> >> resolved.
> >
> > Which previous discussion do you mean? In v1 it was suggested to split
>
> https://lore.kernel.org/all/20231213221124.GB2115075-robh@kernel.org/

Hmm, I thought that was addressed [2]. I was referencing the existing
binding documentation. From there, there was discussion about updating
the docs to clarify what was actually intended (patch 3 in this
series). I also addressed the ABI break concern in the thread and
mentioned it in patch 22.
"For device tree base systems, it is not an issue as the relevant
device tree entries have been updated and DTS is built from source for
each ChromeOS update."

Is there a specific concern you feel is not resolved? Or can I make
something more clear?

[2] https://lore.kernel.org/all/CANg-bXCG61HFW7JFuAd3k+OrCG_F9F3e8brjM-pmBauS53aobQ@mail.gmail.com/

Re: [PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Krzysztof Kozlowski]

On 21/12/2023 20:24, Mark Hasemeyer wrote:
>>>> You do not need this property, if driver assumes that. Just enable it
>>>> unconditionally.
>>>
>>> The goal of this patch series is to change exactly that: to prevent
>>> the driver from unconditionally enabling the irq for wake.
>>
>> But why? What is the problem being solved? Is unconditional wakeup in
>> the driver incorrect? If so, mention it shortly in the commit msg, what
>> is rationale because existing one does not justify this change.
> 
> The cover letter talks about it:
> "Currently the cros_ec driver assumes that its associated interrupt is
> wake capable. This is an incorrect assumption as some Chromebooks use
> a separate wake pin, while others overload the interrupt for wake and
> IO."
> With the current assumption, spurious wakes can occur on systems that
> use a separate wake pin.

This sentence would be enough.

> I can add wording to the dts patches to help clarify.
> 
>>> The driver works across numerous buses (spi, uart, i2c, lpc) and
>>> supports DT and ACPI.
>>> SPI+DT systems all happen to need irq wake enabled.
>>>
>>>> I don't think anything from previous discussion was
>>>> resolved.
>>>
>>> Which previous discussion do you mean? In v1 it was suggested to split
>>
>> https://lore.kernel.org/all/20231213221124.GB2115075-robh@kernel.org/
> 
> Hmm, I thought that was addressed [2]. I was referencing the existing
> binding documentation. From there, there was discussion about updating
> the docs to clarify what was actually intended (patch 3 in this
> series). I also addressed the ABI break concern in the thread and
> mentioned it in patch 22.
> "For device tree base systems, it is not an issue as the relevant
> device tree entries have been updated and DTS is built from source for
> each ChromeOS update."
> 
> Is there a specific concern you feel is not resolved? Or can I make
> something more clear?
> 
Seems fine, thanks.

Best regards,
Krzysztof

Re: [PATCH v2 17/22] of: irq: add wake capable bit to of_irq_resource()

[Rob Herring]

On Wed, 20 Dec 2023 16:54:31 -0700, Mark Hasemeyer wrote:
> Add wake capability information to the IRQ resource. Wake capability is
> assumed based on conventions provided in the devicetree wakeup-source
> binding documentation. An interrupt is considered wake capable if the
> following are true:
> 1. A wakeup-source property exits in the same device node as the
>    interrupt.
> 2. The IRQ is marked as dedicated by setting its interrupt-name to
>    "wakeup".
> 
> The wakeup-source documentation states that dedicated interrupts can use
> device specific interrupt names and device drivers are still welcome to
> use their own naming schemes. This API is provided as a helper if one is
> willing to conform to the above conventions.
> 
> The ACPI subsystems already provides similar APIs that allow one to
> query the wake capability of an IRQ. This brings closer feature parity
> to the devicetree.
> 
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
> 
> Changes in v2:
> -Update logic to return true only if wakeup-source property and
>  "wakeup" interrupt-name are defined
> -irq->IRQ, api->API
> 
>  drivers/of/irq.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 18/22] of: irq: Add default implementation for of_irq_to_resource()

[Rob Herring]

On Wed, 20 Dec 2023 16:54:32 -0700, Mark Hasemeyer wrote:
> Similar to of_irq_to_resource_table(), add a default implementation of
> of_irq_to_resource() for systems that don't have CONFIG_OF_IRQ defined.
> 
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
> 
> Changes in v2:
> -None
> 
>  include/linux/of_irq.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 19/22] of: irq: Remove extern from function declarations

[Rob Herring]

On Wed, 20 Dec 2023 16:54:33 -0700, Mark Hasemeyer wrote:
> The extern keyword is implicit for function declarations.
> Remove it where possible and adjust the line wrapping accordingly.
> 
> Signed-off-by: Mark Hasemeyer <markhas@chromium.org>
> ---
> 
> Changes in v2:
> -New patch
> 
>  include/linux/of_irq.h | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Mark Hasemeyer]

> > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> > +                         unsigned int index, struct resource *r)
>
> It's perfectly fine to replace ) by , on the previous line, no need
> to make it shorter.

That puts the line at 115 chars? checkpatch.pl allows a maximum line
length of 100. I can bump the 'index' argument up a line and keep it
to a length of 95?

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Mark Hasemeyer]

> Both acpi_irq_get() and of_irq_to_resourece() use EXPORT_SYMBOL_GPL()
> instead. I don't see why fwnode_irq_get_resource() shouldn't do the same.
>
> With this changed,
>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> In fact this should have always been the case for fwnode_irq_get(). I
> wouldn't mind changing that, too, in a separate patch.

Sure. It looks like fwnode_iomap(), fwnode_irq_get(),
fwnode_irq_get_byname(), and fwnode_graph_parse_endpoint() could all
be updated. I'll add another patch with these changes unless there's a
reason some of those functions shouldn't be GPL'd?

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Mark Hasemeyer]

>> > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
>> > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
>> > > +                         unsigned int index, struct resource *r)
>> >
>> > It's perfectly fine to replace ) by , on the previous line, no need
>> > to make it shorter.
>>
>> That puts the line at 115 chars? checkpatch.pl allows a maximum line
>> length of 100. I can bump the 'index' argument up a line and keep it
>> to a length of 95?
>
>
> clang-format should do the right thing.

It formats the line as-is in the patch: with 'unsigned int index' on
the next line.

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 04:46:11PM -0700, Mark Hasemeyer wrote:
> > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> > > +                         unsigned int index, struct resource *r)
> >
> > It's perfectly fine to replace ) by , on the previous line, no need
> > to make it shorter.
> 
> That puts the line at 115 chars? checkpatch.pl allows a maximum line
> length of 100. I can bump the 'index' argument up a line and keep it
> to a length of 95?

Yes, the point is to leave index on the previous line and add a new one with
the r.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 04:47:59PM -0700, Raul Rangel wrote:
> On Thu, Dec 21, 2023 at 4:46 PM Mark Hasemeyer <markhas@chromium.org> wrote:
> 
> > > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int
> > index)
> > > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> > > > +                         unsigned int index, struct resource *r)
> > >
> > > It's perfectly fine to replace ) by , on the previous line, no need
> > > to make it shorter.
> >
> > That puts the line at 115 chars? checkpatch.pl allows a maximum line
> > length of 100. I can bump the 'index' argument up a line and keep it
> > to a length of 95?
> 
> clang-format should do the right thing.

Sorry, but no. It has some interesting results sometimes.
Like any other tool it has to be used with caution and
common sense. If it works in the particular case, fine.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 04:59:42PM -0700, Mark Hasemeyer wrote:
> >> > > -int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index)
> >> > > +int fwnode_irq_get_resource(const struct fwnode_handle *fwnode,
> >> > > +                         unsigned int index, struct resource *r)
> >> >
> >> > It's perfectly fine to replace ) by , on the previous line, no need
> >> > to make it shorter.
> >>
> >> That puts the line at 115 chars? checkpatch.pl allows a maximum line
> >> length of 100. I can bump the 'index' argument up a line and keep it
> >> to a length of 95?
> >
> > clang-format should do the right thing.
> 
> It formats the line as-is in the patch: with 'unsigned int index' on
> the next line.

Exactly, and I don't think we need that "smartness" in this case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 20/22] device property: Modify fwnode irq_get() to use resource

[Andy Shevchenko]

On Thu, Dec 21, 2023 at 04:52:15PM -0700, Mark Hasemeyer wrote:
> > Both acpi_irq_get() and of_irq_to_resourece() use EXPORT_SYMBOL_GPL()
> > instead. I don't see why fwnode_irq_get_resource() shouldn't do the same.
> >
> > With this changed,
> >
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >
> > In fact this should have always been the case for fwnode_irq_get(). I
> > wouldn't mind changing that, too, in a separate patch.
> 
> Sure. It looks like fwnode_iomap(), fwnode_irq_get(),
> fwnode_irq_get_byname(), and fwnode_graph_parse_endpoint() could all
> be updated. I'll add another patch with these changes unless there's a
> reason some of those functions shouldn't be GPL'd?

Interesting... Once should look at the history of those changes.
I don't think anything prevents us from moving to GPLed versions.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 21/22] platform: Modify platform_get_irq_optional() to use resource

[Mark Hasemeyer]

> >   * For example::
> >   *
> > - *           int irq = platform_get_irq_optional(pdev, 0);
> > + *           int irq = platform_get_irq_resource_optional(pdev, 0, &res);
> >   *           if (irq < 0)
> >   *                   return irq;
> >   *
> >   * Return: non-zero IRQ number on success, negative error number on failure.
>
> Why do we need the irq to be returned via error code?

We don't really. It just matches the convention of
'platform_get_irq()' and 'of_irq_to_resource()'.

> >       int ret;
>
> Missing blank line, have you run checkpatch.pl?

Yes, I normally run checkpatch.pl. I may have missed the warning or it
didn't catch it. I'll add it.

>
> > +     if (IS_ERR_OR_NULL(r))
> > +             return -EINVAL;
>
> If we ever have an error pointer in r, I prefer to see
>
>         if (!r)
>                 return -EINVAL;
>         if (IS_ERR(r))
>                 return PTR_ERR(r);
>
> But Q is the same as earlier: when would we have the error pointer in @r?

I don't see when we would. I'll drop it.

> Can we save this and be consistent with above fwnode API return code check?
>
> > +                     ret = ret ?: r->start;
> >                       goto out;
> > +             }
> >       }

Yep!

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

On Thu, Dec 21, 2023 at 1:58 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> On Wed, Dec 20, 2023 at 04:54:36PM -0700, Mark Hasemeyer wrote:
> > The IRQ wake logic was added on an interface basis (lpc, spi, uart) as
> > opposed to adding it to cros_ec.c because the i2c subsystem already
> > enables the wakirq (if applicable) on our behalf.
>
> The setting flow are all the same.  I think helper functions in cros_ec.c help
> to deduplicate the code.

I'll see what I can do.

> > +MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
>
> Does it really need `MODULE_DEVICE_TABLE`?

Nope. Will drop.

> >       ret = cros_ec_register(ec_dev);
> >       if (ret) {
> > -             dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> > +             dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
>
> The change is irrelevant to the series.

I'll drop the use of dev_err_probe() to stay consistent with current
conventions. Perhaps it can be added in a follow-up patch.

> > @@ -470,6 +512,8 @@ static void cros_ec_lpc_remove(struct platform_device *pdev)
> >               acpi_remove_notify_handler(adev->handle, ACPI_ALL_NOTIFY,
> >                                          cros_ec_lpc_acpi_notify);
> >
> > +     dev_pm_clear_wake_irq(dev);
> > +     device_init_wakeup(dev, false);
>
> Is it safe to call them anyway regardless of `irq_wake` in cros_ec_lpc_probe()?

According to bench tests, it's not a problem. That said, I am
refactoring the code to move the logic into cros_ec.c and will
conditionally call the cleanup functions.

> > +     if (!np)
> > +             return;
> > +
>
> The change is an improvement (or rather say it could change behavior).  But
> strictly speaking, the change is irrelevant to the series.

Will drop.

>
> > @@ -702,6 +710,11 @@ static void cros_ec_spi_dt_probe(struct cros_ec_spi *ec_spi, struct device *dev)
> >       ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> >       if (!ret)
> >               ec_spi->end_of_msg_delay = val;
> > +
> > +     if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {
>
> Or just use `spi->irq`[2].
>
> [2]: https://elixir.bootlin.com/linux/v6.6/source/drivers/platform/chrome/cros_ec_spi.c#L762
>
> They are the same, but does of_property_present() make more sense?

Yes it does. I'll use it.

> > @@ -768,6 +778,9 @@ static int cros_ec_spi_probe(struct spi_device *spi)
> >                          sizeof(struct ec_response_get_protocol_info);
> >       ec_dev->dout_size = sizeof(struct ec_host_request);
> >
> > +     /* Check for any DT properties */
> > +     cros_ec_spi_dt_probe(ec_spi, spi);
>
> `spi` is possibly not needed.  See comment above.

Agreed.

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

> > +     irq = platform_get_irq_resource_optional(pdev, 0, &irqres);
> > +     if (irq > 0) {
> >               ec_dev->irq = irq;
> > -     else if (irq != -ENXIO) {
> > +             if (cros_ec_should_force_irq_wake_capable())
> > +                     irq_wake = true;
> > +             else
> > +                     irq_wake = irqres.flags & IORESOURCE_IRQ_WAKECAPABLE;
> > +             dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", irq, irq_wake);
> > +     } else if (irq != -ENXIO) {
> >               dev_err(dev, "couldn't retrieve IRQ number (%d)\n", irq);
> >               return irq;
> >       }
>
> Yeah, this is confusing now. Which one should I trust more: irq or irqres.start?
> What is the point to have irqres with this duplication?

irqres is needed to pull the wake capability of the irq. I agree tha
irq and irqres.start should have the same information. I chose irq
because it's more obvious what it represents over irqres.start. It's
also more concise.

>
> ...
>
> > -             dev_err(dev, "couldn't register ec_dev (%d)\n", ret);
> > +             dev_err_probe(dev, ret, "couldn't register ec_dev (%d)\n", ret);
> >               return ret;
>
>                 return dev_err_probe(...);
>
> ...
>
> > +                     dev_err_probe(dev, ret, "Failed to init device for wakeup");
> > +                     return ret;
>

Tzung-Bi pointed out there are other areas of the driver that just use
dev_err() in the probe path. My vote is to drop the use of
dev_err_probe() to stay consistent with what exists. A separate patch
can be sent to modify the statements to use the
dev_err_probe() variant.

>
> ...
>
> > +     if (!np)
> > +             return;
>
> Why do you need this now?

It felt intuitive to return early from cros_ec_spi_dt_probe() if no
device node exists. This does not fix any known bugs though. Dropping
as irrelevant.

> >       ret = of_property_read_u32(np, "google,cros-ec-spi-msg-delay", &val);
> >       if (!ret)
> >               ec_spi->end_of_msg_delay = val;
>
> > +     if (ec_dev->irq > 0 && of_property_read_bool(np, "wakeup-source")) {
> > +             ec_spi->irq_wake = true;
> > +             dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %i\n", ec_dev->irq, ec_spi->irq_wake);
> > +     }
>
>         if (ret)
>                 return;
>
>         ec_spi->irq_wake = of_property_read_bool(np, "wakeup-source"));

The other interface drivers only analyze irq_wake if an irq exists. I
think that makes sense and would like to stay consistent.
Thinking:
ec_spi->irq_wake = spi->irq > 0 && of_property_read_bool(np, "wakeup-source"));

>         dev_dbg(&spi->dev, "IRQ: %i, wake_capable: %s\n", ec_dev->irq, str_yes_no(ec_spi->irq_wake));

> ...
>
> > @@ -78,6 +80,7 @@ struct cros_ec_uart {
> >       u32 baudrate;
> >       u8 flowcontrol;
> >       u32 irq;
> > +     bool irq_wake;
> >       struct response_info response;
> >  };
>
> Run `pahole` and amend respectively to avoid wasting memory.

No savings to be had, but we can defrag the holes from sizes 3 and 3,
to 2 and 4 by moving irq_wake above irq.

pahole --reorganize -C cros_ec_uart cros_ec_uart.o

struct cros_ec_uart {
        struct serdev_device *     serdev;               /*     0     8 */
        u32                        baudrate;             /*     8     4 */
        u8                         flowcontrol;          /*    12     1 */
        bool                       irq_wake;             /*    13     1 */

        /* XXX 2 bytes hole, try to pack */

        u32                        irq;                  /*    16     4 */

        /* XXX 4 bytes hole, try to pack */

        struct response_info       response;             /*    24    64 */

        /* size: 88, cachelines: 2, members: 6 */
        /* sum members: 82, holes: 2, sum holes: 6 */
        /* last cacheline: 24 bytes */
};

> > +     dev_dbg(dev, "IRQ: %i, wake_capable: %i\n", ec_uart->irq, ec_uart->irq_wake);
>
> str_yes_no() from string_choices.h?

SGTM

Re: [PATCH v2 17/22] of: irq: add wake capable bit to of_irq_resource()

[Mark Hasemeyer]

> Or even refactor ioport.h (in a separate patch) as we seems already have
> two users (and might be more in the existing code):
>
> #define DEFINE_RES_IRQ_NAMED_FLAGS(_irq, _name, _flags)                 \
>         DEFINE_RES_NAMED((_irq), 1, (_name), (_flags) | IORESOURCE_IRQ)
> #define DEFINE_RES_IRQ_NAMED(_irq, _name)                               \
>         DEFINE_RES_IRQ_NAMED_FLAGS((_irq), (_name), 0)
> #define DEFINE_RES_IRQ(_irq)                                            \
>         DEFINE_RES_IRQ_NAMED((_irq), NULL)
>
> (Note, I will Ack such a patch once it appears.)

I'll add a new patch to the series. I'll probably include the MEM, IO,
and RES variants as well.

Re: [PATCH v2 00/22] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Mark Hasemeyer]

> Just wondering if you used --histogram diff algo when preparing patches.

Not knowingly. I use patman which uses 'git format-patch' under the
covers with some added options:
https://github.com/siemens/u-boot/blob/master/tools/patman/gitutil.py#L308

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[kernel test robot]

Hi Mark,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on chrome-platform/for-next chrome-platform/for-firmware-next wsa/i2c/for-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.7-rc6 next-20231222]
[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/Mark-Hasemeyer/gpiolib-acpi-Modify-acpi_dev_irq_wake_get_by-to-use-resource/20231222-172104
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231220165423.v2.22.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28%40changeid
patch subject: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq
config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20231223/202312230846.9DkpFRNv-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231223/202312230846.9DkpFRNv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312230846.9DkpFRNv-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/device/driver.h:21,
                    from include/linux/device.h:32,
                    from include/linux/acpi.h:14,
                    from drivers/platform/chrome/cros_ec_lpc.c:14:
>> include/linux/module.h:244:1: error: expected ',' or ';' before 'extern'
     244 | extern typeof(name) __mod_##type##__##name##_device_table               \
         | ^~~~~~
   drivers/platform/chrome/cros_ec_lpc.c:67:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
      67 | MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
         | ^~~~~~~~~~~~~~~~~~~


vim +244 include/linux/module.h

^1da177e4c3f41 Linus Torvalds    2005-04-16  240  
cff26a51da5d20 Rusty Russell     2014-02-03  241  #ifdef MODULE
cff26a51da5d20 Rusty Russell     2014-02-03  242  /* Creates an alias so file2alias.c can find device table. */
^1da177e4c3f41 Linus Torvalds    2005-04-16  243  #define MODULE_DEVICE_TABLE(type, name)					\
0bf8bf50eddc75 Matthias Kaehlcke 2017-07-24 @244  extern typeof(name) __mod_##type##__##name##_device_table		\
cff26a51da5d20 Rusty Russell     2014-02-03  245    __attribute__ ((unused, alias(__stringify(name))))
cff26a51da5d20 Rusty Russell     2014-02-03  246  #else  /* !MODULE */
cff26a51da5d20 Rusty Russell     2014-02-03  247  #define MODULE_DEVICE_TABLE(type, name)
cff26a51da5d20 Rusty Russell     2014-02-03  248  #endif
^1da177e4c3f41 Linus Torvalds    2005-04-16  249  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 01/22] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource

[kernel test robot]

Hi Mark,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on chrome-platform/for-next chrome-platform/for-firmware-next wsa/i2c/for-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.7-rc6 next-20231222]
[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/Mark-Hasemeyer/gpiolib-acpi-Modify-acpi_dev_irq_wake_get_by-to-use-resource/20231222-172104
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231220165423.v2.1.Ifd0903f1c351e84376d71dbdadbd43931197f5ea%40changeid
patch subject: [PATCH v2 01/22] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource
config: x86_64-randconfig-161-20231222 (https://download.01.org/0day-ci/archive/20231223/202312230907.szXqJyXq-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231223/202312230907.szXqJyXq-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312230907.szXqJyXq-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpio/gpiolib-acpi.c:117: warning: Function parameter or member 'shareable' not described in 'acpi_gpio_info'


vim +117 drivers/gpio/gpiolib-acpi.c

aa92b6f689acf1 Mika Westerberg 2014-03-10   93  
b7452d670fdef8 Dmitry Torokhov 2022-11-15   94  /**
b7452d670fdef8 Dmitry Torokhov 2022-11-15   95   * struct acpi_gpio_info - ACPI GPIO specific information
b7452d670fdef8 Dmitry Torokhov 2022-11-15   96   * @adev: reference to ACPI device which consumes GPIO resource
b7452d670fdef8 Dmitry Torokhov 2022-11-15   97   * @flags: GPIO initialization flags
b7452d670fdef8 Dmitry Torokhov 2022-11-15   98   * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
b7452d670fdef8 Dmitry Torokhov 2022-11-15   99   * @pin_config: pin bias as provided by ACPI
b7452d670fdef8 Dmitry Torokhov 2022-11-15  100   * @polarity: interrupt polarity as provided by ACPI
b7452d670fdef8 Dmitry Torokhov 2022-11-15  101   * @triggering: triggering type as provided by ACPI
b7452d670fdef8 Dmitry Torokhov 2022-11-15  102   * @wake_capable: wake capability as provided by ACPI
b7452d670fdef8 Dmitry Torokhov 2022-11-15  103   * @debounce: debounce timeout as provided by ACPI
b7452d670fdef8 Dmitry Torokhov 2022-11-15  104   * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
b7452d670fdef8 Dmitry Torokhov 2022-11-15  105   */
b7452d670fdef8 Dmitry Torokhov 2022-11-15  106  struct acpi_gpio_info {
b7452d670fdef8 Dmitry Torokhov 2022-11-15  107  	struct acpi_device *adev;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  108  	enum gpiod_flags flags;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  109  	bool gpioint;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  110  	int pin_config;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  111  	int polarity;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  112  	int triggering;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  113  	bool wake_capable;
189f4620fa2d51 Mark Hasemeyer  2023-12-20  114  	bool shareable;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  115  	unsigned int debounce;
b7452d670fdef8 Dmitry Torokhov 2022-11-15  116  	unsigned int quirks;
b7452d670fdef8 Dmitry Torokhov 2022-11-15 @117  };
b7452d670fdef8 Dmitry Torokhov 2022-11-15  118  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 01/22] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource

[Mark Hasemeyer]

On Fri, Dec 22, 2023 at 7:09 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Mark,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on robh/for-next]
> [also build test WARNING on chrome-platform/for-next chrome-platform/for-firmware-next wsa/i2c/for-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.7-rc6 next-20231222]
> [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/Mark-Hasemeyer/gpiolib-acpi-Modify-acpi_dev_irq_wake_get_by-to-use-resource/20231222-172104
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> patch link:    https://lore.kernel.org/r/20231220165423.v2.1.Ifd0903f1c351e84376d71dbdadbd43931197f5ea%40changeid
> patch subject: [PATCH v2 01/22] gpiolib: acpi: Modify acpi_dev_irq_wake_get_by() to use resource
> config: x86_64-randconfig-161-20231222 (https://download.01.org/0day-ci/archive/20231223/202312230907.szXqJyXq-lkp@intel.com/config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231223/202312230907.szXqJyXq-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312230907.szXqJyXq-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> >> drivers/gpio/gpiolib-acpi.c:117: warning: Function parameter or member 'shareable' not described in 'acpi_gpio_info'
>
>
> vim +117 drivers/gpio/gpiolib-acpi.c
>
> aa92b6f689acf1 Mika Westerberg 2014-03-10   93
> b7452d670fdef8 Dmitry Torokhov 2022-11-15   94  /**
> b7452d670fdef8 Dmitry Torokhov 2022-11-15   95   * struct acpi_gpio_info - ACPI GPIO specific information
> b7452d670fdef8 Dmitry Torokhov 2022-11-15   96   * @adev: reference to ACPI device which consumes GPIO resource
> b7452d670fdef8 Dmitry Torokhov 2022-11-15   97   * @flags: GPIO initialization flags
> b7452d670fdef8 Dmitry Torokhov 2022-11-15   98   * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
> b7452d670fdef8 Dmitry Torokhov 2022-11-15   99   * @pin_config: pin bias as provided by ACPI
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  100   * @polarity: interrupt polarity as provided by ACPI
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  101   * @triggering: triggering type as provided by ACPI
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  102   * @wake_capable: wake capability as provided by ACPI
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  103   * @debounce: debounce timeout as provided by ACPI
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  104   * @quirks: Linux specific quirks as provided by struct acpi_gpio_mapping
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  105   */
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  106  struct acpi_gpio_info {
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  107          struct acpi_device *adev;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  108          enum gpiod_flags flags;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  109          bool gpioint;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  110          int pin_config;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  111          int polarity;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  112          int triggering;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  113          bool wake_capable;
> 189f4620fa2d51 Mark Hasemeyer  2023-12-20  114          bool shareable;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  115          unsigned int debounce;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  116          unsigned int quirks;
> b7452d670fdef8 Dmitry Torokhov 2022-11-15 @117  };
> b7452d670fdef8 Dmitry Torokhov 2022-11-15  118
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Ack. Missing documentation for acpi_gpio_info.shareable member. Will
add in next version.

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[Mark Hasemeyer]

On Fri, Dec 22, 2023 at 5:37 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Mark,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on robh/for-next]
> [also build test ERROR on chrome-platform/for-next chrome-platform/for-firmware-next wsa/i2c/for-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.7-rc6 next-20231222]
> [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/Mark-Hasemeyer/gpiolib-acpi-Modify-acpi_dev_irq_wake_get_by-to-use-resource/20231222-172104
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
> patch link:    https://lore.kernel.org/r/20231220165423.v2.22.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28%40changeid
> patch subject: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq
> config: riscv-allmodconfig (https://download.01.org/0day-ci/archive/20231223/202312230846.9DkpFRNv-lkp@intel.com/config)
> compiler: riscv64-linux-gcc (GCC) 13.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231223/202312230846.9DkpFRNv-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202312230846.9DkpFRNv-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
>    In file included from include/linux/device/driver.h:21,
>                     from include/linux/device.h:32,
>                     from include/linux/acpi.h:14,
>                     from drivers/platform/chrome/cros_ec_lpc.c:14:
> >> include/linux/module.h:244:1: error: expected ',' or ';' before 'extern'
>      244 | extern typeof(name) __mod_##type##__##name##_device_table               \
>          | ^~~~~~
>    drivers/platform/chrome/cros_ec_lpc.c:67:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
>       67 | MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
>          | ^~~~~~~~~~~~~~~~~~~
>
>
> vim +244 include/linux/module.h
>
> ^1da177e4c3f41 Linus Torvalds    2005-04-16  240
> cff26a51da5d20 Rusty Russell     2014-02-03  241  #ifdef MODULE
> cff26a51da5d20 Rusty Russell     2014-02-03  242  /* Creates an alias so file2alias.c can find device table. */
> ^1da177e4c3f41 Linus Torvalds    2005-04-16  243  #define MODULE_DEVICE_TABLE(type, name)                                       \
> 0bf8bf50eddc75 Matthias Kaehlcke 2017-07-24 @244  extern typeof(name) __mod_##type##__##name##_device_table             \
> cff26a51da5d20 Rusty Russell     2014-02-03  245    __attribute__ ((unused, alias(__stringify(name))))
> cff26a51da5d20 Rusty Russell     2014-02-03  246  #else  /* !MODULE */
> cff26a51da5d20 Rusty Russell     2014-02-03  247  #define MODULE_DEVICE_TABLE(type, name)
> cff26a51da5d20 Rusty Russell     2014-02-03  248  #endif
> ^1da177e4c3f41 Linus Torvalds    2005-04-16  249
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Failed for riscv arch. Regardless, MODULE_DEVICE_TABLE macro call is
being removed in the next version.

Re: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq

[kernel test robot]

Hi Mark,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on chrome-platform/for-next chrome-platform/for-firmware-next wsa/i2c/for-next driver-core/driver-core-testing driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.7-rc6 next-20231222]
[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/Mark-Hasemeyer/gpiolib-acpi-Modify-acpi_dev_irq_wake_get_by-to-use-resource/20231222-172104
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20231220165423.v2.22.Ieee574a0e94fbaae01fd6883ffe2ceeb98d7df28%40changeid
patch subject: [PATCH v2 22/22] platform/chrome: cros_ec: Use PM subsystem to manage wakeirq
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20231224/202312240045.wiFeDc1T-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231224/202312240045.wiFeDc1T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312240045.wiFeDc1T-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/platform/chrome/cros_ec_lpc.c:66:2: error: expected ';' after top level declarator
   }
    ^
    ;
   1 error generated.


vim +66 drivers/platform/chrome/cros_ec_lpc.c

    51	
    52	static const struct dmi_system_id untrusted_fw_irq_wake_capable[] = {
    53		{
    54			.ident = "Brya",
    55			.matches = {
    56				DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brya")
    57			}
    58		},
    59		{
    60			.ident = "Brask",
    61			.matches = {
    62				DMI_MATCH(DMI_PRODUCT_FAMILY, "Google_Brask")
    63			}
    64		},
    65		{ }
  > 66	}
    67	MODULE_DEVICE_TABLE(dmi, untrusted_fw_irq_wake_capable);
    68	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 00/22] Improve IRQ wake capability reporting and update the cros_ec driver to use it

[Andy Shevchenko]

On Fri, Dec 22, 2023 at 03:30:43PM -0700, Mark Hasemeyer wrote:
> > Just wondering if you used --histogram diff algo when preparing patches.
> 
> Not knowingly. I use patman which uses 'git format-patch' under the
> covers with some added options:
> https://github.com/siemens/u-boot/blob/master/tools/patman/gitutil.py#L308

Add a configuration into your ~/.gitconfig (or local for the project),
it really makes the difference.

-- 
With Best Regards,
Andy Shevchenko

Re: (subset) [PATCH v2 06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source

[Krzysztof Kozlowski]

On Wed, 20 Dec 2023 16:54:20 -0700, Mark Hasemeyer wrote:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
> 
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
> 
> [...]

Applied, thanks!

[06/22] ARM: dts: samsung: exynos5420: Enable cros-ec-spi as wake source
        https://git.kernel.org/krzk/linux/c/8f51b5290ff4f8a9f1c634cf42ca37cd9e90018c

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Re: (subset) [PATCH v2 07/22] ARM: dts: samsung: exynos5800: Enable cros-ec-spi as wake source

[Krzysztof Kozlowski]

On Wed, 20 Dec 2023 16:54:21 -0700, Mark Hasemeyer wrote:
> The cros_ec driver currently assumes that cros-ec-spi compatible device
> nodes are a wakeup-source even though the wakeup-source property is not
> defined.
> 
> Add the wakeup-source property to all cros-ec-spi compatible device
> nodes to match expected behavior.
> 
> [...]

Applied, thanks!

[07/22] ARM: dts: samsung: exynos5800: Enable cros-ec-spi as wake source
        https://git.kernel.org/krzk/linux/c/df294f4ec618c9f0d7e9a2fde1c541b731972389

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>