Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

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
Date: Thu, 10 Aug 2023 09:30:15 +0200	[thread overview]
Message-ID: <20230810073015d5545903@mail.local> (raw)
In-Reply-To: <347cf148-bda8-852b-768c-fa2b57ce5bcb@gmail.com>

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