Date:   Fri, 26 Nov 2021 12:09:35 +0300
From:   Nikita Shubin <nikita.shubin@maquefel.me>
To:     Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
Cc:     David Abdurachmanov <david.abdurachmanov@sifive.com>,
        Support Opensource <Support.Opensource@diasemi.com>,
        Alessandro Zummo <a.zummo@towertech.it>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        "linux-rtc@vger.kernel.org" <linux-rtc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] rtc: da9063: add as wakeup source
Message-ID: <20211126120935.188e672a@redslave.neermore.group>
In-Reply-To: <DB9PR10MB465224854946DABA0F75515980609@DB9PR10MB4652.EURPRD10.PROD.OUTLOOK.COM>
References: <20211123140604.21655-1-nikita.shubin@maquefel.me>
        <DB9PR10MB465224854946DABA0F75515980609@DB9PR10MB4652.EURPRD10.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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  
>