[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 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 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 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 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 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 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 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