From: Ran Bi <ran.bi@mediatek.com>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Alessandro Zummo <a.zummo@towertech.it>,
"Mark Rutland" <mark.rutland@arm.com>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
"David S . Miller" <davem@davemloft.net>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
Linus Walleij <linus.walleij@linaro.org>,
Nicolas Ferre <nicolas.ferre@microchip.com>,
<linux-rtc@vger.kernel.org>, <devicetree@vger.kernel.org>,
<linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-mediatek@lists.infradead.org>,
<srv_heupstream@mediatek.com>, YT Shen <yt.shen@mediatek.com>,
Eddie Huang <eddie.huang@mediatek.com>,
Yingjoe Chen <yingjoe.chen@mediatek.com>,
"Flora Fu" <flora.fu@mediatek.com>,
Sean Wang <sean.wang@mediatek.com>
Subject: Re: [PATCH v2 2/4] rtc: Add support for the MediaTek MT2712 RTC
Date: Thu, 22 Aug 2019 20:34:14 +0800 [thread overview]
Message-ID: <1566477254.12318.41.camel@mhfsdcap03> (raw)
In-Reply-To: <20190820201744.GZ3545@piout.net>
Hi,
> > +
> > +#define MTK_RTC_DEV KBUILD_MODNAME
>
> You probably shouldn't do that and have a static string for the driver
> name. I probably doesn't matter much though because DT is used to probe
> the driver.
>
Will change it at next patch.
> > +/* we map HW YEAR 0 to 2000 because 2000 is the leap year */
> > +#define MT2712_MIN_YEAR 2000
> > +#define MT2712_BASE_YEAR 1900
> > +#define MT2712_MIN_YEAR_OFFSET (MT2712_MIN_YEAR - MT2712_BASE_YEAR)
> > +#define MT2712_MAX_YEAR_OFFSET (MT2712_MIN_YEAR_OFFSET + 127)
> > +
>
> All those defines are unecessary, see below.
>
Will change it at next patch.
> > +struct mt2712_rtc {
> > + struct device *dev;
>
> Looking at the code closely, it seems this is only used for debug and
> error messages. Maybe you could use rtc_dev->dev instead.
>
Will change it at next patch.
> > + mutex_lock(&rtc->rtc_dev->ops_lock);
> > +
> > + irqsta = mt2712_readl(rtc, MT2712_IRQ_STA);
>
> Do you have to lock that read? Is the register cleared on read?
>
Yes, this register is read clear register.
> > + do {
> > + __mt2712_rtc_read_time(rtc, tm, &sec);
> > + } while (sec < tm->tm_sec); /* SEC has carried */
>
> Shouldn't that be while (tm->tm_sec < sec)?
>
In __mt2712_rtc_read_time function, we read tm->tm_sec before read sec.
Sometimes we can meet situation like "tm->tm_sec == 59" and "sec == 0".
It means that TC_SEC has carried and we need to reload the tm struct. I
suppose it was correct that using "while (sec < tm->tm_sec)"
> > +
> > + /* HW register use 7 bits to store year data, minus
> > + * MT2712_MIN_YEAR_OFFSET brfore write year data to register, and plus
> > + * MT2712_MIN_YEAR_OFFSET back after read year from register
> > + */
> > + tm->tm_year += MT2712_MIN_YEAR_OFFSET;
>
> Simply add 100 in __mt2712_rtc_read_time
>
Will change it at next patch.
> > +
> > + /* HW register start mon from one, but tm_mon start from zero. */
> > + tm->tm_mon--;
> > +
>
> You can also do that in __mt2712_rtc_read_time.
>
Will change it at next patch.
> > + if (rtc_valid_tm(tm)) {
>
> This check is unnecessary, the validity is always checked by the core.
>
Will remove this at next patch.
> > + if (tm->tm_year > MT2712_MAX_YEAR_OFFSET) {
> > + dev_dbg(rtc->dev, "Set year %d out of range. (%d - %d)\n",
> > + 1900 + tm->tm_year, 1900 + MT2712_MIN_YEAR_OFFSET,
> > + 1900 + MT2712_MAX_YEAR_OFFSET);
> > + return -EINVAL;
> > + }
>
> This check is unnecessary, see below.
>
Will change it at next patch.
> > +
> > + tm->tm_year -= MT2712_MIN_YEAR_OFFSET;
> > + tm->tm_mon++;
>
> You should probably avoid modifying tm, move the substraction and
> addition in the mt2712_writel calls.
>
Will change it at next patch.
> > + if (tm->tm_year > MT2712_MAX_YEAR_OFFSET) {
> > + dev_dbg(rtc->dev, "Set year %d out of range. (%d - %d)\n",
> > + 1900 + tm->tm_year, 1900 + MT2712_MIN_YEAR_OFFSET,
> > + 1900 + MT2712_MAX_YEAR_OFFSET);
> > + return -EINVAL;
> > + }
> > +
>
> Unnecessary check.
>
Will change it at next patch.
> > + p1 = mt2712_readl(rtc, MT2712_POWERKEY1);
> > + p2 = mt2712_readl(rtc, MT2712_POWERKEY2);
> > + if (p1 != MT2712_POWERKEY1_KEY || p2 != MT2712_POWERKEY2_KEY)
> > + dev_dbg(rtc->dev, "powerkey not set (lost power)\n");
> > +
>
> This info is valuable, you should check that when reading the time and
> return -EINVAL if power was lost.
>
Will change it at next patch.
>
> > + /* RTC need POWERKEY1/2 match, then goto normal work mode */
> > + mt2712_writel(rtc, MT2712_POWERKEY1, MT2712_POWERKEY1_KEY);
> > + mt2712_writel(rtc, MT2712_POWERKEY2, MT2712_POWERKEY2_KEY);
>
> This should be written when setting the time after power was lost.
>
I suppose we can move this into mt2712_rtc_read_time function's "if
(p1 != MT2712_POWERKEY1_KEY || p2 != MT2712_POWERKEY2_KEY)" condition
which will be added at next patch. We need additional flag to mark this
condition or another if condition in mt2712_rtc_set_time fucntion if we
put these code in mt2712_rtc_set_time function.
> > +static const struct rtc_class_ops mt2712_rtc_ops = {
> > + .read_time = mt2712_rtc_read_time,
> > + .set_time = mt2712_rtc_set_time,
> > + .read_alarm = mt2712_rtc_read_alarm,
> > + .set_alarm = mt2712_rtc_set_alarm,
>
> For proper operations, you should also provide the .alarm_irq_enable
> callback.
>
Will change it at next patch.
> > + rtc->rtc_dev->ops = &mt2712_rtc_ops;
>
> If you set the range properly here using rtc_dev->range_min and
> rtc_dev->range_max, then the core will be able to do range checking and
> will also take care of the year offset/windowing calculations instead of
> having to hardcode that in the driver.
>
Will change it at next patch.
Best Regards,
Ran
next prev parent reply other threads:[~2019-08-22 12:34 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-01 11:01 [PATCH v2 0/4] Add Support for MediaTek MT2712 RTC Ran Bi
2019-08-01 11:01 ` [PATCH v2 1/4] bindings: rtc: add bindings for " Ran Bi
2019-08-22 9:17 ` Matthias Brugger
2019-08-23 6:35 ` Ran Bi
2019-08-01 11:01 ` [PATCH v2 2/4] rtc: Add support for the MediaTek " Ran Bi
2019-08-20 20:17 ` Alexandre Belloni
2019-08-22 12:34 ` Ran Bi [this message]
2019-08-22 12:46 ` Alexandre Belloni
2019-08-22 13:26 ` Ran Bi
2019-08-22 13:36 ` Alexandre Belloni
2019-08-23 6:37 ` Ran Bi
2019-08-22 9:12 ` Matthias Brugger
2019-08-22 9:20 ` Alexandre Belloni
2019-08-22 11:50 ` Ran Bi
2019-08-22 11:47 ` Ran Bi
2019-08-01 11:01 ` [PATCH v2 3/4] arm64: dts: add RTC nodes for MT2712 Ran Bi
2019-08-01 11:01 ` [PATCH v2 4/4] MAINTAINERS: add MT2712 RTC files Ran Bi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1566477254.12318.41.camel@mhfsdcap03 \
--to=ran.bi@mediatek.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=a.zummo@towertech.it \
--cc=alexandre.belloni@bootlin.com \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=eddie.huang@mediatek.com \
--cc=flora.fu@mediatek.com \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-rtc@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=mchehab+samsung@kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=robh+dt@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=srv_heupstream@mediatek.com \
--cc=yingjoe.chen@mediatek.com \
--cc=yt.shen@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).