Hello Adam! On Fri, 26 Nov 2021 09:50:18 +0000 Adam Thomson <Adam.Thomson.Opensource@xxxxxxxxxxx> 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 ?
