| Message ID | 20211118084008.30327-1-nikita.shubin@maquefel.me |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND] rtc: da9063: add as wakeup source | expand |
Hello, On 18/11/2021 11:40:08+0300, Nikita Shubin wrote: > in case if threaded irq registered successfully - add da9063 > as a wakeup source if "wakeup-source" node present in device tree, > set as wakeup capable otherwise. > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > --- > drivers/rtc/rtc-da9063.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c > index d4b72a9fa2ba..1aceb5ba6992 100644 > --- a/drivers/rtc/rtc-da9063.c > +++ b/drivers/rtc/rtc-da9063.c > @@ -490,7 +490,15 @@ static int da9063_rtc_probe(struct platform_device *pdev) > da9063_alarm_event, > IRQF_TRIGGER_LOW | IRQF_ONESHOT, > "ALARM", rtc); > - if (ret) > + if (!ret) { > + if (device_property_present(&pdev->dev, "wakeup-source")) { > + device_init_wakeup(&pdev->dev, true); If wakeup-source is present, then this should be done regardless of the registration of the interrupt handler. Note that wakeup-source and interrupt are supposed to be mutually exclusive. > + dev_info(&pdev->dev, "registered as wakeup source.\n"); This is too verbose please avoid adding new strings > + } else { > + device_set_wakeup_capable(&pdev->dev, true); I think this is misusing the wakeup-source property for configuration that should be left to userspace. > + dev_info(&pdev->dev, "marked as wakeup capable.\n"); Ditto > + } > + } else unbalanced brackets > dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n", > irq_alarm, ret); > > -- > 2.31.1 >
Hello Alexandre, Sorry for the rush - I should have to think more before sending this patch ... On Thu, 18 Nov 2021 10:33:34 +0100 Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > Hello, > > On 18/11/2021 11:40:08+0300, Nikita Shubin wrote: > > in case if threaded irq registered successfully - add da9063 > > as a wakeup source if "wakeup-source" node present in device tree, > > set as wakeup capable otherwise. > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > --- > > drivers/rtc/rtc-da9063.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c > > index d4b72a9fa2ba..1aceb5ba6992 100644 > > --- a/drivers/rtc/rtc-da9063.c > > +++ b/drivers/rtc/rtc-da9063.c > > @@ -490,7 +490,15 @@ static int da9063_rtc_probe(struct > > platform_device *pdev) da9063_alarm_event, > > IRQF_TRIGGER_LOW | > > IRQF_ONESHOT, "ALARM", rtc); > > - if (ret) > > + if (!ret) { > > + if (device_property_present(&pdev->dev, > > "wakeup-source")) { > > + device_init_wakeup(&pdev->dev, true); > > If wakeup-source is present, then this should be done regardless of > the registration of the interrupt handler. Note that wakeup-source and > interrupt are supposed to be mutually exclusive. > We still able to wakeup either ALARM IRQ is present or not. Actually the only thing is needed in this particular case is the ability to set "wakealarm" via sysfs - so we can wakeup from POWER-DOWN/DELIVERY/RTC modes, namely shutdown, regardless of CONFIG_PM. Setting dev->power.can_wakeup to true is enough for that. On the other hand device_init_wakeup also sets can_wakeup. May be it's enough to use device_init_wakeup in case if ALARM IRQ is present or "wakeup-source" is set ? I see some construction in drivers/rtc like : ``` rtc/rtc-pcf2127.c:673: if (alarm_irq > 0 || device_property_read_bool(dev, "wakeup-source")) { rtc/rtc-ab-eoz9.c:552: if (client->irq > 0 || device_property_read_bool(dev, "wakeup-source")) { ``` > > + dev_info(&pdev->dev, "registered as wakeup > > source.\n"); > > This is too verbose please avoid adding new strings > > > + } else { > > + device_set_wakeup_capable(&pdev->dev, > > true); > > I think this is misusing the wakeup-source property for configuration > that should be left to userspace. > > > + dev_info(&pdev->dev, "marked as wakeup > > capable.\n"); > > Ditto > > > + } > > + } else > > unbalanced brackets > > > > dev_err(&pdev->dev, "Failed to request ALARM IRQ > > %d: %d\n", irq_alarm, ret); > > > > -- > > 2.31.1 > > >
On 19/11/2021 12:07:10+0300, Nikita Shubin wrote: > Hello Alexandre, > > Sorry for the rush - I should have to think more before sending this > patch ... > > On Thu, 18 Nov 2021 10:33:34 +0100 > Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > > Hello, > > > > On 18/11/2021 11:40:08+0300, Nikita Shubin wrote: > > > in case if threaded irq registered successfully - add da9063 > > > as a wakeup source if "wakeup-source" node present in device tree, > > > set as wakeup capable otherwise. > > > > > > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> > > > --- > > > drivers/rtc/rtc-da9063.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c > > > index d4b72a9fa2ba..1aceb5ba6992 100644 > > > --- a/drivers/rtc/rtc-da9063.c > > > +++ b/drivers/rtc/rtc-da9063.c > > > @@ -490,7 +490,15 @@ static int da9063_rtc_probe(struct > > > platform_device *pdev) da9063_alarm_event, > > > IRQF_TRIGGER_LOW | > > > IRQF_ONESHOT, "ALARM", rtc); > > > - if (ret) > > > + if (!ret) { > > > + if (device_property_present(&pdev->dev, > > > "wakeup-source")) { > > > + device_init_wakeup(&pdev->dev, true); > > > > If wakeup-source is present, then this should be done regardless of > > the registration of the interrupt handler. Note that wakeup-source and > > interrupt are supposed to be mutually exclusive. > > > > We still able to wakeup either ALARM IRQ is present or not. > > Actually the only thing is needed in this particular case is the ability > to set "wakealarm" via sysfs - so we can wakeup from > POWER-DOWN/DELIVERY/RTC modes, namely shutdown, regardless of CONFIG_PM. > > Setting dev->power.can_wakeup to true is enough for that. > > On the other hand device_init_wakeup also sets can_wakeup. > > May be it's enough to use device_init_wakeup in case if ALARM IRQ is > present or "wakeup-source" is set ? > > I see some construction in drivers/rtc like : > > ``` > rtc/rtc-pcf2127.c:673: if (alarm_irq > 0 || > device_property_read_bool(dev, "wakeup-source")) { > rtc/rtc-ab-eoz9.c:552: if (client->irq > 0 || > device_property_read_bool(dev, "wakeup-source")) { > ``` > Yes, this is what I meant, call device_init_wakeup when an irq has been successfully requested or wakeup-source is present.
diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c index d4b72a9fa2ba..1aceb5ba6992 100644 --- a/drivers/rtc/rtc-da9063.c +++ b/drivers/rtc/rtc-da9063.c @@ -490,7 +490,15 @@ static int da9063_rtc_probe(struct platform_device *pdev) da9063_alarm_event, IRQF_TRIGGER_LOW | IRQF_ONESHOT, "ALARM", rtc); - if (ret) + if (!ret) { + if (device_property_present(&pdev->dev, "wakeup-source")) { + device_init_wakeup(&pdev->dev, true); + dev_info(&pdev->dev, "registered as wakeup source.\n"); + } else { + device_set_wakeup_capable(&pdev->dev, true); + dev_info(&pdev->dev, "marked as wakeup capable.\n"); + } + } else dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n", irq_alarm, ret);
in case if threaded irq registered successfully - add da9063 as a wakeup source if "wakeup-source" node present in device tree, set as wakeup capable otherwise. Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me> --- drivers/rtc/rtc-da9063.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)