LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] rtc: da9063: add as wakeup source
@ 2021-11-23 14:06 Nikita Shubin
  2021-11-23 14:37 ` Adam Thomson
  0 siblings, 1 reply; 6+ messages in thread
From: Nikita Shubin @ 2021-11-23 14:06 UTC (permalink / raw)
  Cc: David Abdurachmanov, Nikita Shubin, Support Opensource,
	Alessandro Zummo, Alexandre Belloni, linux-rtc, linux-kernel

As da9063 driver refuses to load without irq, we simply add it as a wakeup
source before registering rtc device.

Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
v1->v2:
Alexandre Belloni:

Dropped everything except device_init_wakeup, as driver refuses to load
without irq specified, we can always set it as a wakeup source, before
calling devm_rtc_register_device.
---
 drivers/rtc/rtc-da9063.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
index d4b72a9fa2ba..b9a73356bace 100644
--- a/drivers/rtc/rtc-da9063.c
+++ b/drivers/rtc/rtc-da9063.c
@@ -494,6 +494,8 @@ static int da9063_rtc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
 			irq_alarm, ret);
 
+	device_init_wakeup(&pdev->dev, true);
+
 	return devm_rtc_register_device(rtc->rtc_dev);
 }
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] rtc: da9063: add as wakeup source
  2021-11-23 14:06 [PATCH v2] rtc: da9063: add as wakeup source Nikita Shubin
@ 2021-11-23 14:37 ` Adam Thomson
  2021-11-26  9:09   ` Nikita Shubin
  0 siblings, 1 reply; 6+ messages in thread
From: Adam Thomson @ 2021-11-23 14:37 UTC (permalink / raw)
  To: Nikita Shubin
  Cc: David Abdurachmanov, Support Opensource, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, linux-kernel

On 23 November 2021 14:06, Nikita Shubin wrote:

> As da9063 driver refuses to load without irq, we simply add it as a wakeup
> source before registering rtc device.

Can you please make the commit message more detailed, explaining why you're
making this change; what it adds/fixes/removes/etc.? Right now just reading this
I'm unclear as to why you're adding a call to device_init_wakeup() here. The
generic I2C client code will mark the parent MFD device as a wake source, if
the relevant boolean 'wakeup' is defined in DT, so what does this add?

> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
> v1->v2:
> Alexandre Belloni:
> 
> Dropped everything except device_init_wakeup, as driver refuses to load
> without irq specified, we can always set it as a wakeup source, before
> calling devm_rtc_register_device.
> ---
>  drivers/rtc/rtc-da9063.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> index d4b72a9fa2ba..b9a73356bace 100644
> --- a/drivers/rtc/rtc-da9063.c
> +++ b/drivers/rtc/rtc-da9063.c
> @@ -494,6 +494,8 @@ static int da9063_rtc_probe(struct platform_device
> *pdev)
>  		dev_err(&pdev->dev, "Failed to request ALARM IRQ %d: %d\n",
>  			irq_alarm, ret);
> 
> +	device_init_wakeup(&pdev->dev, true);
> +
>  	return devm_rtc_register_device(rtc->rtc_dev);
>  }
> 
> --
> 2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtc: da9063: add as wakeup source
  2021-11-23 14:37 ` Adam Thomson
@ 2021-11-26  9:09   ` Nikita Shubin
  2021-11-26  9:50     ` Adam Thomson
  0 siblings, 1 reply; 6+ messages in thread
From: Nikita Shubin @ 2021-11-26  9:09 UTC (permalink / raw)
  To: Adam Thomson
  Cc: David Abdurachmanov, Support Opensource, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, linux-kernel

Hello Adam!

On Tue, 23 Nov 2021 14:37:42 +0000
Adam Thomson <Adam.Thomson.Opensource@diasemi.com> wrote:

> On 23 November 2021 14:06, Nikita Shubin wrote:
> 
> > As da9063 driver refuses to load without irq, we simply add it as a
> > wakeup source before registering rtc device.  
> 
> Can you please make the commit message more detailed, explaining why
> you're making this change; what it adds/fixes/removes/etc.? Right now
> just reading this I'm unclear as to why you're adding a call to
> device_init_wakeup() here. The generic I2C client code will mark the
> parent MFD device as a wake source, if the relevant boolean 'wakeup'
> is defined in DT, so what does this add?

Sorry for long response had to double check setting wakeup-source in
case i have missed something.

I2C_CLIENT_WAKE is set in of_i2c_get_board_info - the place da9063 rtc
would never get to.

Setting "wakeup-source" for pmic indeed marks it as wakeup source, but
that's not exactly we want.

What we want is "wakealarm" in RTC sysfs directory, to be able to set
alarm so we can wake up from SHUTDOWN/DELIVERY/RTC mode of da9063.

We do have /sys/class/rtc/rtc0/wakealarm if marking da9063-rtc as
device_init_wakeup.

Unfortunately marking pmic or rtc as wakeup-source in device tree gives
us nothing.

ls /proc/device-tree/soc/i2c\@10030000/pmic\@58/
compatible            interrupt-parent  name  regulators  wakeup-source
interrupt-controller  interrupts        reg   rtc         wdt

ls /proc/device-tree/soc/i2c\@10030000/pmic\@58/rtc/
compatible  name  wakeup-source

ls /sys/class/rtc/rtc0/wakealarm
ls: cannot access '/sys/class/rtc/rtc0/wakealarm': No such file or
directory

So i currently see that either da9063 RTC should be marked as wakeup
source, or the da9063 MFD should somehow set that for RTC.

And we want this even if CONFIG_PM is off.

Mentioning "/sys/class/rtc/rtc0/wakealarm" in commit message would be
enough ?

> 
> > 
> > Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > ---
> > v1->v2:
> > Alexandre Belloni:
> > 
> > Dropped everything except device_init_wakeup, as driver refuses to
> > load without irq specified, we can always set it as a wakeup
> > source, before calling devm_rtc_register_device.
> > ---
> >  drivers/rtc/rtc-da9063.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/rtc/rtc-da9063.c b/drivers/rtc/rtc-da9063.c
> > index d4b72a9fa2ba..b9a73356bace 100644
> > --- a/drivers/rtc/rtc-da9063.c
> > +++ b/drivers/rtc/rtc-da9063.c
> > @@ -494,6 +494,8 @@ static int da9063_rtc_probe(struct
> > platform_device *pdev)
> >  		dev_err(&pdev->dev, "Failed to request ALARM IRQ
> > %d: %d\n", irq_alarm, ret);
> > 
> > +	device_init_wakeup(&pdev->dev, true);
> > +
> >  	return devm_rtc_register_device(rtc->rtc_dev);
> >  }
> > 
> > --
> > 2.31.1  
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] rtc: da9063: add as wakeup source
  2021-11-26  9:09   ` Nikita Shubin
@ 2021-11-26  9:50     ` Adam Thomson
  2021-11-26 10:23       ` Nikita Shubin
  0 siblings, 1 reply; 6+ messages in thread
From: Adam Thomson @ 2021-11-26  9:50 UTC (permalink / raw)
  To: Nikita Shubin, Adam Thomson
  Cc: David Abdurachmanov, Support Opensource, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, linux-kernel

On 26 November 2021 09:10, Nikita Shubin wrote:

> > Can you please make the commit message more detailed, explaining why
> > you're making this change; what it adds/fixes/removes/etc.? Right now
> > just reading this I'm unclear as to why you're adding a call to
> > device_init_wakeup() here. The generic I2C client code will mark the
> > parent MFD device as a wake source, if the relevant boolean 'wakeup'
> > is defined in DT, so what does this add?
> 
> Sorry for long response had to double check setting wakeup-source in
> case i have missed something.
> 
> I2C_CLIENT_WAKE is set in of_i2c_get_board_info - the place da9063 rtc
> would never get to.
> 
> Setting "wakeup-source" for pmic indeed marks it as wakeup source, but
> that's not exactly we want.
> 
> What we want is "wakealarm" in RTC sysfs directory, to be able to set
> alarm so we can wake up from SHUTDOWN/DELIVERY/RTC mode of da9063.
> 
> We do have /sys/class/rtc/rtc0/wakealarm if marking da9063-rtc as
> device_init_wakeup.
> 
> Unfortunately marking pmic or rtc as wakeup-source in device tree gives
> us nothing.
> 
> ls /proc/device-tree/soc/i2c\@10030000/pmic\@58/
> compatible            interrupt-parent  name  regulators  wakeup-source
> interrupt-controller  interrupts        reg   rtc         wdt
> 
> ls /proc/device-tree/soc/i2c\@10030000/pmic\@58/rtc/
> compatible  name  wakeup-source
> 
> ls /sys/class/rtc/rtc0/wakealarm
> ls: cannot access '/sys/class/rtc/rtc0/wakealarm': No such file or
> directory
> 
> So i currently see that either da9063 RTC should be marked as wakeup
> source, or the da9063 MFD should somehow set that for RTC.
> 
> And we want this even if CONFIG_PM is off.
> 
> Mentioning "/sys/class/rtc/rtc0/wakealarm" in commit message would be
> enough ?

Thanks for the detailed response; it helped a lot. Having reviewed the core code
along with your description I know understand what's happening here. Basically
marking as 'wakeup-source' is simply a means to expose the sysfs attribute to
user-space.

Yes I think in the commit message you should be clear that there's a need to
access the sys attribute 'wakealarm' in the RTC core and clarify exactly why
there is that need. Your commit log should be good enough so that if anyone else
needs to look at this later they completely understand the intention behind the
change.

By the way, I assume the functionality you're looking for could also have been
achieved through using the /dev/rtcX instance for DA9063?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] rtc: da9063: add as wakeup source
  2021-11-26  9:50     ` Adam Thomson
@ 2021-11-26 10:23       ` Nikita Shubin
  2021-11-26 11:14         ` Adam Thomson
  0 siblings, 1 reply; 6+ messages in thread
From: Nikita Shubin @ 2021-11-26 10:23 UTC (permalink / raw)
  To: Adam Thomson
  Cc: David Abdurachmanov, Support Opensource, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, linux-kernel

Hello Adam!

On Fri, 26 Nov 2021 09:50:18 +0000
Adam Thomson <Adam.Thomson.Opensource@diasemi.com> wrote:

> On 26 November 2021 09:10, Nikita Shubin wrote:
> 
> > > Can you please make the commit message more detailed, explaining
> > > why you're making this change; what it adds/fixes/removes/etc.?
> > > Right now just reading this I'm unclear as to why you're adding a
> > > call to device_init_wakeup() here. The generic I2C client code
> > > will mark the parent MFD device as a wake source, if the relevant
> > > boolean 'wakeup' is defined in DT, so what does this add?  
> > 
> > Sorry for long response had to double check setting wakeup-source in
> > case i have missed something.
> > 
> > I2C_CLIENT_WAKE is set in of_i2c_get_board_info - the place da9063
> > rtc would never get to.
> > 
> > Setting "wakeup-source" for pmic indeed marks it as wakeup source,
> > but that's not exactly we want.
> > 
> > What we want is "wakealarm" in RTC sysfs directory, to be able to
> > set alarm so we can wake up from SHUTDOWN/DELIVERY/RTC mode of
> > da9063.
> > 
> > We do have /sys/class/rtc/rtc0/wakealarm if marking da9063-rtc as
> > device_init_wakeup.
> > 
> > Unfortunately marking pmic or rtc as wakeup-source in device tree
> > gives us nothing.
> > 
> > ls /proc/device-tree/soc/i2c\@10030000/pmic\@58/
> > compatible            interrupt-parent  name  regulators
> > wakeup-source interrupt-controller  interrupts        reg   rtc
> >     wdt
> > 
> > ls /proc/device-tree/soc/i2c\@10030000/pmic\@58/rtc/
> > compatible  name  wakeup-source
> > 
> > ls /sys/class/rtc/rtc0/wakealarm
> > ls: cannot access '/sys/class/rtc/rtc0/wakealarm': No such file or
> > directory
> > 
> > So i currently see that either da9063 RTC should be marked as wakeup
> > source, or the da9063 MFD should somehow set that for RTC.
> > 
> > And we want this even if CONFIG_PM is off.
> > 
> > Mentioning "/sys/class/rtc/rtc0/wakealarm" in commit message would
> > be enough ?  
> 
> Thanks for the detailed response; it helped a lot. Having reviewed
> the core code along with your description I know understand what's
> happening here. Basically marking as 'wakeup-source' is simply a
> means to expose the sysfs attribute to user-space.
> 
> Yes I think in the commit message you should be clear that there's a
> need to access the sys attribute 'wakealarm' in the RTC core and
> clarify exactly why there is that need. Your commit log should be
> good enough so that if anyone else needs to look at this later they
> completely understand the intention behind the change.
> 
> By the way, I assume the functionality you're looking for could also
> have been achieved through using the /dev/rtcX instance for DA9063?

Thank you for pointing this out, indeed i missed that obvious thing.

We can also simply set alarm via rtcwake, even if CONFIG_PM is off:

rtcwake -m no -s 60

Now i am not sure we should make changes to da9063-rtc driver - what do
you think ?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] rtc: da9063: add as wakeup source
  2021-11-26 10:23       ` Nikita Shubin
@ 2021-11-26 11:14         ` Adam Thomson
  0 siblings, 0 replies; 6+ messages in thread
From: Adam Thomson @ 2021-11-26 11:14 UTC (permalink / raw)
  To: Nikita Shubin, Adam Thomson
  Cc: David Abdurachmanov, Support Opensource, Alessandro Zummo,
	Alexandre Belloni, linux-rtc, linux-kernel

On 26 November 2021 10:23, Nikita Shubin wrote:

> > Thanks for the detailed response; it helped a lot. Having reviewed
> > the core code along with your description I know understand what's
> > happening here. Basically marking as 'wakeup-source' is simply a
> > means to expose the sysfs attribute to user-space.
> >
> > Yes I think in the commit message you should be clear that there's a
> > need to access the sys attribute 'wakealarm' in the RTC core and
> > clarify exactly why there is that need. Your commit log should be
> > good enough so that if anyone else needs to look at this later they
> > completely understand the intention behind the change.
> >
> > By the way, I assume the functionality you're looking for could also
> > have been achieved through using the /dev/rtcX instance for DA9063?
> 
> Thank you for pointing this out, indeed i missed that obvious thing.
> 
> We can also simply set alarm via rtcwake, even if CONFIG_PM is off:
> 
> rtcwake -m no -s 60
> 
> Now i am not sure we should make changes to da9063-rtc driver - what do
> you think ?

I think the change is still valid from what I can tell. Just be clear on intent
in the commit log of the patch. :)

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-26 11:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-23 14:06 [PATCH v2] rtc: da9063: add as wakeup source Nikita Shubin
2021-11-23 14:37 ` Adam Thomson
2021-11-26  9:09   ` Nikita Shubin
2021-11-26  9:50     ` Adam Thomson
2021-11-26 10:23       ` Nikita Shubin
2021-11-26 11:14         ` Adam Thomson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).