Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Aug 2023 09:30:15 +0200
From:   Alexandre Belloni <alexandre.belloni@bootlin.com>
To:     Jacky Huang <ychuang570808@gmail.com>
Cc:     a.zummo@towertech.it, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        soc@kernel.org, mjchen@nuvoton.com, schung@nuvoton.com,
        Jacky Huang <ychuang3@nuvoton.com>
Subject: Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc
 controller
Message-ID: <20230810073015d5545903@mail.local>
References: <20230809011542.429945-1-ychuang570808@gmail.com>
 <20230809011542.429945-4-ychuang570808@gmail.com>
 <20230809021025a7c0daec@mail.local>
 <426130f6-7b8a-91f9-559b-afc5afdc656e@gmail.com>
 <2023080922515326db190e@mail.local>
 <347cf148-bda8-852b-768c-fa2b57ce5bcb@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <347cf148-bda8-852b-768c-fa2b57ce5bcb@gmail.com>
Precedence: bulk

On 10/08/2023 15:21:47+0800, Jacky Huang wrote:
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> > > > > +{
> > > > > +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
> > > > > +	u32 regval;
> > > > > +
> > > > > +	if (device_may_wakeup(&pdev->dev))
> > > > > +		enable_irq_wake(rtc->irq_num);
> > > > > +
> > > > > +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
> > > > > +	regval &= ~RTC_INTEN_TICKIEN;
> > > > > +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
> > > > This is not what the user is asking, don't do this. Also, how was this
> > > > tested?
> > > Sure, I will remove these three lines of code.
> > > 
> > > We test it with "echo mem > /sys/power/state".
> > > 
> > Yes, my point is that if UIE is enabled, then the user wants to be woken
> > up every second. If this is not what is wanted, then UIE has to be
> > disabled before going to suspend.
> > 
> > My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
> > expect anyone to use an actual hardware tick interrupt, unless the alarm
> > is broken and can't be set every second. This is why I questioned the
> > RTC_UF path because I don't expect it to be taken at all.
> 
> Yes, we will remove TICKIEN from probe and modify ma35d1_alarm_irq_enable().
> TICKIEN will be enabled only if UIE is enabled.
> 
> static int ma35d1_alarm_irq_enable(struct device *dev, unsigned int enabled)
> {
> ?? ?struct ma35d1_rtc *rtc = dev_get_drvdata(dev);
> 
> ?? ?if (enabled) {
> ?? ???? if (rtc->rtc->uie_rtctimer.enabled)
> ?? ???? ??? rtc_reg_write(rtc, NVT_RTC_INTEN,
> ?? ???? ??? ??? ????? (rtc_reg_read(rtc,
> NVT_RTC_INTEN)|(RTC_INTEN_TICKIEN)));


Don't do that unless the regular alarm can't be set every second. Simply
always use ALMIEN, then check rtctest is passing properly.

> ?? ???? if (rtc->rtc->aie_timer.enabled)
> ?? ???? ??? rtc_reg_write(rtc, NVT_RTC_INTEN,
> ?? ???? ??? ??? ????? (rtc_reg_read(rtc,
> NVT_RTC_INTEN)|(RTC_INTEN_ALMIEN)));
> ?? ?} else {
> ?? ???? if (rtc->rtc->uie_rtctimer.enabled)
> ?? ???? ??? rtc_reg_write(rtc, NVT_RTC_INTEN,
> ?? ???? ??? ??? ????? (rtc_reg_read(rtc, NVT_RTC_INTEN) &
> (~RTC_INTEN_TICKIEN)));
> ?? ???? if (rtc->rtc->aie_timer.enabled)
> ?? ???? ??? rtc_reg_write(rtc, NVT_RTC_INTEN,
> ?? ???? ??? ??? ????? (rtc_reg_read(rtc, NVT_RTC_INTEN) &
> (~RTC_INTEN_ALMIEN)));
> ?? ?}
> ?? ?return 0;
> }
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com