[PATCH] rtc: pcf2127: arm the alarm IRQ as a wake IRQ

[PATCH] rtc: pcf2127: arm the alarm IRQ as a wake IRQ

[Aristo Chen]

When an alarm IRQ is available, the driver calls device_init_wakeup(),
so the RTC shows up as wakeup capable and enabled, but it never marks
the IRQ itself as a wakeup interrupt. The PM core therefore disables
the IRQ for the whole system suspend, and the alarm cannot wake the
system: "rtcwake -m mem" on this RTC only resumes once another wakeup
source fires.

On the i.MX93 11x11 FRDM board the PCF2131 alarm is routed through a
PCAL6524 GPIO expander. Without a wakeup request on the expander pin,
the expander's own interrupt is not armed for wakeup either, so the
alarm never reaches the CPU. The BBNSM RTC on the same board uses
devm_pm_set_wake_irq() and wakes the system as expected.

Register the alarm IRQ with devm_pm_set_wake_irq() so that the PM core
arms it whenever the device may wake the system. A failure is not
fatal: the RTC keeps working, only wakeup from it is unavailable.

Fixes: 8a914bac44be ("rtc: pcf2127: add alarm support")
Assisted-by: LLM
Signed-off-by: Aristo Chen <aristo.chen@canonical.com>
---
Tested on an i.MX93 FRDM board, where the PCF2131 INTA reaches the SoC
through a PCAL6524 GPIO expander. The board ran an Ubuntu 6.8 kernel
with devm_pm_set_wake_irq() (fd8318a32573) and tcpci wakeup support
(a5d2edb2c9fa) backported. With this patch, the PCF2131 alarm wakes the
board from suspend-to-RAM and from s2idle, also after unbinding and
rebinding the driver. Without it, the alarm doesn't wake the board from
suspend-to-RAM. Compile-tested on rtc-next (arm64 defconfig plus
RTC_DRV_PCF2127, W=1).

 drivers/rtc/rtc-pcf2127.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
index ff2023908..51d8aebf3 100644
--- a/drivers/rtc/rtc-pcf2127.c
+++ b/drivers/rtc/rtc-pcf2127.c
@@ -27,6 +27,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/regmap.h>
 #include <linux/watchdog.h>
 
@@ -1251,6 +1252,12 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
 	if (alarm_irq > 0 || device_property_read_bool(dev, "wakeup-source")) {
 		device_init_wakeup(dev, true);
 		set_bit(RTC_FEATURE_ALARM, pcf2127->rtc->features);
+
+		if (alarm_irq > 0) {
+			ret = devm_pm_set_wake_irq(dev, alarm_irq);
+			if (ret)
+				dev_warn(dev, "failed to set wake IRQ: %d\n", ret);
+		}
 	}
 
 	if (pcf2127->cfg->has_int_a_b) {
-- 
2.43.0

Re: [PATCH] rtc: pcf2127: arm the alarm IRQ as a wake IRQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rtc: pcf2127: duplicate wake IRQ registration in pcf2127_probe() on I2C
--

commit bb7257d310771e0aa92e0b399681a4feb16cf42a
Author: Aristo Chen <aristo.chen@canonical.com>

rtc: pcf2127: arm the alarm IRQ as a wake IRQ

This commit registers the RTC alarm IRQ with devm_pm_set_wake_irq() in
pcf2127_probe() so the PM core arms the interrupt as a wakeup source
during system suspend.

> diff --git a/drivers/rtc/rtc-pcf2127.c b/drivers/rtc/rtc-pcf2127.c
> index ff2023908f5a3..51d8aebf34069 100644
> --- a/drivers/rtc/rtc-pcf2127.c
> +++ b/drivers/rtc/rtc-pcf2127.c
[ ... ]
> @@ -1251,6 +1252,12 @@ static int pcf2127_probe(struct device *dev, struct regmap *regmap,
>  	if (alarm_irq > 0 || device_property_read_bool(dev, "wakeup-source")) {
>  		device_init_wakeup(dev, true);
>  		set_bit(RTC_FEATURE_ALARM, pcf2127->rtc->features);
> +
> +		if (alarm_irq > 0) {
> +			ret = devm_pm_set_wake_irq(dev, alarm_irq);
> +			if (ret)
> +				dev_warn(dev, "failed to set wake IRQ: %d\n", ret);
> +		}
>  	}
>  
>  	if (pcf2127->cfg->has_int_a_b) {

[Severity: Medium]
Can calling devm_pm_set_wake_irq() unconditionally here trigger a kernel
warning when an I2C-attached PCF2127/2129/2131 has both interrupts and
wakeup-source specified in its device tree node (or has I2C_CLIENT_WAKE set
via ACPI)?

For I2C devices, of_i2c_get_board_info() in drivers/i2c/i2c-core-of.c sets
I2C_CLIENT_WAKE when wakeup-source is present:

    if (of_property_read_bool(node, "wakeup-source"))
            info->flags |= I2C_CLIENT_WAKE;

Before invoking the driver probe callback (pcf2127_i2c_probe() ->
pcf2127_probe()), i2c_device_probe() in drivers/i2c/i2c-core-base.c already
initializes the wake IRQ when I2C_CLIENT_WAKE is set:

    if (client->flags & I2C_CLIENT_WAKE) {
            int wakeirq;

            wakeirq = fwnode_irq_get_byname(fwnode, "wakeup");
            if (wakeirq == -EPROBE_DEFER) {
                    status = dev_err_probe(dev, wakeirq, "can't get wakeirq\n");
                    goto put_sync_adapter;
            }

            device_init_wakeup(&client->dev, true);

            if (wakeirq > 0 && wakeirq != client->irq)
                    status = dev_pm_set_dedicated_wake_irq(dev, wakeirq);
            else if (client->irq > 0)
                    status = dev_pm_set_wake_irq(dev, client->irq);

When pcf2127_probe() then calls devm_pm_set_wake_irq(dev, alarm_irq),
dev_pm_attach_wake_irq() in drivers/base/power/wakeirq.c sees
dev->power.wakeirq is already initialized and fires dev_WARN_ONCE():

    spin_lock_irqsave(&dev->power.lock, flags);
    if (dev_WARN_ONCE(dev, dev->power.wakeirq,
                      "wake irq already initialized\n")) {
            spin_unlock_irqrestore(&dev->power.lock, flags);
            return -EEXIST;
    }

returning -EEXIST and printing the warning in pcf2127_probe().

Would it make sense to skip calling devm_pm_set_wake_irq() when the bus core
has already configured the wakeup source (or when dev->power.wakeirq is
already set)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020125124.1097546-1-aristo.chen@canonical.com?part=1