Re: [PATCH 2/3] rtc: Add support for the MediaTek MT2712 RTC

[Ran Bi]

Hi Belloni,

On Sat, 2019-07-13 at 23:12 +0200, Alexandre Belloni wrote:

> > +#define RTC_BBPU		0x0000
> > +#define RTC_BBPU_CLRPKY		(1U << 4)
> 
> Please use BIT(). Also, I don't feel that the RTC prefix is adding any
> value. MT2712 would be a better choice here.
> 

Will change to MT2712 at next patch.

> > +
> > +/* we map HW YEAR 0 to 1968 not 1970 because 2000 is the leap year */
> > +#define RTC_MIN_YEAR		1968
> > +#define RTC_BASE_YEAR		1900
> > +#define RTC_MIN_YEAR_OFFSET	(RTC_MIN_YEAR - RTC_BASE_YEAR)
> 
> Do not do that. If this RTC range starts in 200, ths is what the driver
> has to support, you should not care about dates before 2000. Note that
> the RTC core can still properly shift the range if it is absolutely
> necessary.
> 

Do we need to care about default alarm date 1970-01-01? Or can I just
set it to 2000-01-01?

> > +
> > +static inline u32 rtc_readl(struct mt2712_rtc *rtc, u32 reg)
> 
> Please use a more descriptive prefix than just rtc_.
> 

Do you mean it's better to use prefix "mt2712_rtc_"?

> > +		mutex_lock(&rtc->lock);
> 
> You should take rtc->rtc_dev->ops_lock. This would remove the need for
> rtc->lock.
> 

Will change it at next patch.

> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +
> 
> As stated before, do not do that, simply add 100.
> 

Will change it at next patch.

> > +	/* rtc_tm_to_time64 convert Gregorian date to seconds since
> > +	 * 01-01-1970 00:00:00, and this date is Thursday
> > +	 */
> > +	time = rtc_tm_to_time64(tm);
> > +	days = div_s64(time, 86400);
> > +	tm->tm_wday = (days + 4) % 7;
> > +
> 
> This is not necessary, nobody cares about tm_wday, if you don't have it,
> do not set it.
> 

Will remove this part at next patch.

> > +	dev_info(rtc->dev, "set al time = %04d-%02d-%02d %02d:%02d:%02d (%d)\n",
> 
> Do not use dev_info, dev_dbg is probably what you want here. Also, use
> %ptR.
> 

Will change it at next patch.

> > +	mutex_lock(&rtc->lock);
> 
> You probably need to disable the alarm before starting to modify the
> registers.
> 

Will change it at next patch.

> > +static bool valid_rtc_time(struct mt2712_rtc *rtc)
> 
> This function is not necessary, see later.
> 

Will change it at next patch.

> > +	rtc_writel(rtc, RTC_IRQ_EN,  0);
> 
> Are you sure you want to disable interrupts every time you reboot? I
> guess the RTC has its own power domain and may be used across reboots.
> 

Will remove this at next patch.

> > +	dev_info(rtc->dev, "%s rtc p1 is %x, p2 is %x!\n", __func__, p1, p2);
> 
> This debug message has to be removed.
> 

Will remove this at next patch.

> > +
> > +	/*
> > +	 * register status was not correct,
> > +	 * need set time and alarm to default
> > +	 */
> > +	if (p1 != RTC_POWERKEY1_KEY || p2 != RTC_POWERKEY2_KEY
> > +	    || !valid_rtc_time(rtc)) {
> > +		reset_rtc_time(rtc);
> 
> Do not do that. This is valuable information. If the time is invalid,
> report it as such in read_time and read_alarm. Resetting the time here
> will lead to more issues later (i.e. userspace is not able to know
> whether the time is set correctly or not).
> 

When RTC's power run out, RTC will lost it's registers value and time
data at next boot up. We even cannot know what the date and time it
shows. We want to check this state here and set a default RTC date. Do
you think it's no need here and the date should be set by system?

> > +	ret = request_threaded_irq(rtc->irq, NULL,
> 
> devm_request_threaded_irq would remove the need for out_free_irq and
> mtk_rtc_remove().
> 

Will change it at next patch. And will remove mtk_rtc_remove() function.

--
Ran Bi, MediaTek