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

[Jacky Huang <ychuang570808@gmail.com>]

On 2023/8/10 下午 03:30, Alexandre Belloni wrote:
> 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.

OK, I got it. I will drop the TICKINT and use ALMIEN only.

MA35D1 RTC has an alarm mask register which supports alarm mask for 
seconds, minutes, and hours.
We will use the alarm mask to have RTC generate an alarm interrupt per 
second, and make sure
the driver can pass rtctest.

>>          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;
>> }
>>


Best Regards,
Jacky Huang