Hello Adam!

On Fri, 26 Nov 2021 09:50:18 +0000
Adam Thomson [off-list ref] wrote:

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

quoted

quoted

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 ?