[{"id":1773345,"web_url":"http://patchwork.ozlabs.org/comment/1773345/","msgid":"<1506067636.26847.2.camel@mtksdaap41>","list_archive_url":null,"date":"2017-09-22T08:07:16","subject":"Re: [PATCH 3/4] rtc: mediatek: enhance the description for MediaTek\n\tPMIC based RTC","submitter":{"id":65011,"url":"http://patchwork.ozlabs.org/api/people/65011/","name":"Eddie Huang","email":"eddie.huang@mediatek.com"},"content":"Hi Sean,\n\nOn Fri, 2017-09-22 at 11:33 +0800, sean.wang@mediatek.com wrote:\n> From: Sean Wang \n> \n> Give a better description for original MediaTek RTC driver as PMIC based\n> RTC in order to distinguish SoC based RTC. Also turning all words with\n> Mediatek to MediaTek here.\n> \n> Cc: Eddie Huang \n> Signed-off-by: Sean Wang \n> ---\n> drivers/rtc/Kconfig | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig\n> index 4226295..4500f77 100644\n> --- a/drivers/rtc/Kconfig\n> +++ b/drivers/rtc/Kconfig\n> @@ -1716,14 +1716,14 @@ config RTC_DRV_MEDIATEK\n> \t will be called rtc-mediatek.\n> \n> config RTC_DRV_MT6397\n> -\ttristate \"Mediatek Real Time Clock driver\"\n> +\ttristate \"MediaTek PMIC based RTC\"\n> \tdepends on MFD_MT6397 || (COMPILE_TEST && IRQ_DOMAIN)\n> \thelp\n> -\t This selects the Mediatek(R) RTC driver. RTC is part of Mediatek\n> +\t This selects the MediaTek(R) RTC driver. RTC is part of MediaTek\n> \t MT6397 PMIC. You should enable MT6397 PMIC MFD before select\n> -\t Mediatek(R) RTC driver.\n> +\t MediaTek(R) RTC driver.\n> \n> -\t If you want to use Mediatek(R) RTC interface, select Y or M here.\n> +\t If you want to use MediaTek(R) RTC interface, select Y or M here.\n> \n> config RTC_DRV_XGENE\n> \ttristate \"APM X-Gene RTC\"\n\nThanks your correction.\n\nAcked-by: Eddie Huang \n\nRegards,\n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xz5gJ2j9sz9sP1\n\tfor ;\n\tFri, 22 Sep 2017 18:07:24 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751808AbdIVIHX (ORCPT\n\t);\n\tFri, 22 Sep 2017 04:07:23 -0400","from mailgw02.mediatek.com ([210.61.82.184]:48264 \"EHLO\n\tmailgw02.mediatek.com\" rhost-flags-OK-FAIL-OK-FAIL) by\n\tvger.kernel.org with ESMTP id S1751704AbdIVIHW (ORCPT\n\t); Fri, 22 Sep 2017 04:07:22 -0400","from mtkexhb02.mediatek.inc [(172.21.101.103)] by\n\tmailgw02.mediatek.com (envelope-from )\n\t(mhqrelay.mediatek.com ESMTP with TLS)\n\twith ESMTP id 1358763777; Fri, 22 Sep 2017 16:07:16 +0800","from mtkcas08.mediatek.inc (172.21.101.126) by\n\tmtkmbs08n1.mediatek.inc (172.21.101.55) with Microsoft SMTP Server\n\t(TLS) id 15.0.1210.3; Fri, 22 Sep 2017 16:06:42 +0800","from [172.21.77.4] (172.21.77.4) by mtkcas08.mediatek.inc\n\t(172.21.101.73) with Microsoft SMTP Server id 15.0.1210.3 via\n\tFrontend Transport; Fri, 22 Sep 2017 16:06:36 +0800"],"X-UUID":"18cc0420f1724bce8072e767df83536f-20170922","Message-ID":"<1506067636.26847.2.camel@mtksdaap41>","Subject":"Re: [PATCH 3/4] rtc: mediatek: enhance the description for MediaTek\n\tPMIC based RTC","From":"Eddie Huang ","To":"","CC":", ,\n\t, ,\n\t, ,\n\t, ","Date":"Fri, 22 Sep 2017 16:07:16 +0800","In-Reply-To":"<0d9d5559d9d884c98ef589a8f56ab0dcb140e5e0.1506049341.git.sean.wang@mediatek.com>","References":"\n\t<0d9d5559d9d884c98ef589a8f56ab0dcb140e5e0.1506049341.git.sean.wang@mediatek.com>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.2.3-0ubuntu6 ","Content-Transfer-Encoding":"7bit","MIME-Version":"1.0","X-MTK":"N","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1785841,"web_url":"http://patchwork.ozlabs.org/comment/1785841/","msgid":"<20171012212052.xkhbgdjrghmnvcfe@piout.net>","list_archive_url":null,"date":"2017-10-12T21:20:52","subject":"Re: [PATCH 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC","submitter":{"id":26276,"url":"http://patchwork.ozlabs.org/api/people/26276/","name":"Alexandre Belloni","email":"alexandre.belloni@free-electrons.com"},"content":"Hi,\n\nOn 22/09/2017 at 11:33:15 +0800, sean.wang@mediatek.com wrote:\n> diff --git a/drivers/rtc/rtc-mediatek.c b/drivers/rtc/rtc-mediatek.c\n\nI'm pretty sure this should be named rtc-mt7622.c instead of\nrtc-mediatek.c, exactly for the same reason you have patch 3/4.\n\n> +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)\n> +{\n> +\t__raw_writel(val, rtc->base + reg);\n\nDo you really need the __raw accessors? What about running your SoC in\nBE mode? I guess the _relaxed version are fast enough.\n\n> +}\n> +\n> +static u32 mtk_r32(struct mtk_rtc *rtc, u32 reg)\n> +{\n> +\treturn __raw_readl(rtc->base + reg);\n> +}\n> +\n\n\n> +static void mtk_rtc_hw_init(struct mtk_rtc *hw)\n> +{\n> +\t/* The setup of the init sequence is for allowing RTC got to work */\n> +\tmtk_w32(hw, MTK_RTC_PWRCHK1, RTC_PWRCHK1_MAGIC);\n> +\tmtk_w32(hw, MTK_RTC_PWRCHK2, RTC_PWRCHK2_MAGIC);\n> +\tmtk_w32(hw, MTK_RTC_KEY, RTC_KEY_MAGIC);\n> +\tmtk_w32(hw, MTK_RTC_PROT1, RTC_PROT1_MAGIC);\n> +\tmtk_w32(hw, MTK_RTC_PROT2, RTC_PROT2_MAGIC);\n> +\tmtk_w32(hw, MTK_RTC_PROT3, RTC_PROT3_MAGIC);\n> +\tmtk_w32(hw, MTK_RTC_PROT4, RTC_PROT4_MAGIC);\n> +\tmtk_rmw(hw, MTK_RTC_DEBNCE, RTC_DEBNCE_MASK, 0);\n> +\tmtk_clr(hw, MTK_RTC_CTL, RTC_RC_STOP);\n> +}\n> +\n> +static void mtk_rtc_get_alarm_or_time(struct mtk_rtc *hw, struct rtc_time *tm,\n> +\t\t\t\t int time_alarm)\n> +{\n> +\tu32 year, mon, mday, wday, hour, min, sec;\n> +\n> +\t/*\n> +\t * Read again until all fields are not changed for all fields in the\n> +\t * consistent state.\n> +\t */\n> +\tdo {\n> +\t\tyear = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_YEA));\n> +\t\tmon = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MON));\n> +\t\twday = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOW));\n> +\t\tmday = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOM));\n> +\t\thour = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_HOU));\n> +\t\tmin = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MIN));\n> +\t\tsec = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_SEC));\n> +\t} while (year != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_YEA)) ||\n> +\t\t mon != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MON)) ||\n> +\t\t mday != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOM))\t||\n> +\t\t wday != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOW))\t||\n> +\t\t hour != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_HOU))\t||\n> +\t\t min != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MIN))\t||\n> +\t\t sec != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_SEC))\n> +\t\t);\n\nI'm pretty sure only checking sec is enough because it is highly\nunlikely that 7 reads take a minute.\n\n> +static irqreturn_t mtk_rtc_alarmirq(int irq, void *id)\n> +{\n> +\tstruct mtk_rtc *hw = (struct mtk_rtc *)id;\n> +\tu32 irq_sta;\n> +\n> +\t/* Stop alarm also implicitly disable the alarm interrupt */\n> +\tmtk_w32(hw, MTK_RTC_AL_CTL, 0);\n\nYou stop the alarm here, before testing whether the alarm really\nhappened.\n\n> +\tirq_sta = mtk_r32(hw, MTK_RTC_INT);\n> +\tif (irq_sta & RTC_INT_AL_STA) {\n> +\t\trtc_update_irq(hw->rtc, 1, RTC_IRQF | RTC_AF);\n> +\n> +\t\t/* Ack alarm interrupt status */\n> +\t\tmtk_w32(hw, MTK_RTC_INT, RTC_INT_AL_STA);\n> +\t\treturn IRQ_HANDLED;\n> +\t}\n> +\n> +\treturn IRQ_NONE;\n> +}\n> +\n> +static int mtk_rtc_gettime(struct device *dev, struct rtc_time *tm)\n> +{\n> +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> +\n> +\tmtk_rtc_get_alarm_or_time(hw, tm, MTK_TC);\n> +\n> +\treturn rtc_valid_tm(tm);\n> +}\n> +\n> +static int mtk_rtc_settime(struct device *dev, struct rtc_time *tm)\n> +{\n> +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> +\n> +\t/* Stop time counter before setting a new one*/\n> +\tmtk_set(hw, MTK_RTC_CTL, RTC_RC_STOP);\n> +\n> +\t/* Epoch == 1900 */\n> +\tif (tm->tm_year < 100 || tm->tm_year > 199)\n> +\t\treturn -EINVAL;\n\nYear is a 32 bits register, what makes the RTC fail in 2100? Is it\nbecause of the leap year handling?\n\n> +static int mtk_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)\n> +{\n> +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> +\tstruct rtc_time *alrm_tm = &wkalrm->time;\n> +\n> +\t/* Epoch == 1900 */\n> +\tif (alrm_tm->tm_year < 100 || alrm_tm->tm_year > 199)\n> +\t\treturn -EINVAL;\n> +\n\nDitto.\n\n> +\n> +\tdev_info(&pdev->dev, \"MediaTek SoC based RTC enabled\\n\");\n> +\n\nI think the rtc core is verbose enough that this message is not needed.","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yCkKh3Qtnz9sNw\n\tfor ;\n\tFri, 13 Oct 2017 08:20:56 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752767AbdJLVUy (ORCPT\n\t);\n\tThu, 12 Oct 2017 17:20:54 -0400","from mail.free-electrons.com ([62.4.15.54]:51996 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751961AbdJLVUy (ORCPT\n\t); Thu, 12 Oct 2017 17:20:54 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 9DDA820848; Thu, 12 Oct 2017 23:20:51 +0200 (CEST)","from localhost (unknown [88.191.26.124])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 73F5D20773;\n\tThu, 12 Oct 2017 23:20:51 +0200 (CEST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Thu, 12 Oct 2017 23:20:52 +0200","From":"Alexandre Belloni ","To":"sean.wang@mediatek.com","Cc":"a.zummo@towertech.it, robh+dt@kernel.org, mark.rutland@arm.com,\n\tlinux-rtc@vger.kernel.org, devicetree@vger.kernel.org,\n\tlinux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org","Subject":"Re: [PATCH 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC","Message-ID":"<20171012212052.xkhbgdjrghmnvcfe@piout.net>","References":"\n\t<5b2a7e5c9c5bc179f89e48fb614b2ae789be4254.1506049341.git.sean.wang@mediatek.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<5b2a7e5c9c5bc179f89e48fb614b2ae789be4254.1506049341.git.sean.wang@mediatek.com>","User-Agent":"NeoMutt/20170609 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1787226,"web_url":"http://patchwork.ozlabs.org/comment/1787226/","msgid":"<1508141876.21840.75.camel@mtkswgap22>","list_archive_url":null,"date":"2017-10-16T08:17:56","subject":"Re: [PATCH 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC","submitter":{"id":69660,"url":"http://patchwork.ozlabs.org/api/people/69660/","name":"Sean Wang","email":"sean.wang@mediatek.com"},"content":"Hi Alexandre,\n\nThanks for your valuable suggestions on the driver.\n\nI added comments inline and will have following-ups in the next version\n\n\tSean\n\nOn Thu, 2017-10-12 at 23:20 +0200, Alexandre Belloni wrote:\n> Hi,\n> \n> On 22/09/2017 at 11:33:15 +0800, sean.wang@mediatek.com wrote:\n> > diff --git a/drivers/rtc/rtc-mediatek.c b/drivers/rtc/rtc-mediatek.c\n> \n> I'm pretty sure this should be named rtc-mt7622.c instead of\n> rtc-mediatek.c, exactly for the same reason you have patch 3/4.\n> \n\nIt's okay for naming with rtc-mt7622.c at this moment. But if more SoCs\nsupport gets into the driver, I will consider again to give a more\ngeneric name for the driver.\n\n> > +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)\n> > +{\n> > +\t__raw_writel(val, rtc->base + reg);\n> \n> Do you really need the __raw accessors? What about running your SoC in\n> BE mode? I guess the _relaxed version are fast enough.\n> \n\nSoC runs on LE mode. I also think it's fine and enough to use _relaxed\nversion instead of __raw version.\n\n> > +}\n> > +\n> > +static u32 mtk_r32(struct mtk_rtc *rtc, u32 reg)\n> > +{\n> > +\treturn __raw_readl(rtc->base + reg);\n> > +}\n> > +\n> \n> \n> > +static void mtk_rtc_hw_init(struct mtk_rtc *hw)\n> > +{\n> > +\t/* The setup of the init sequence is for allowing RTC got to work */\n> > +\tmtk_w32(hw, MTK_RTC_PWRCHK1, RTC_PWRCHK1_MAGIC);\n> > +\tmtk_w32(hw, MTK_RTC_PWRCHK2, RTC_PWRCHK2_MAGIC);\n> > +\tmtk_w32(hw, MTK_RTC_KEY, RTC_KEY_MAGIC);\n> > +\tmtk_w32(hw, MTK_RTC_PROT1, RTC_PROT1_MAGIC);\n> > +\tmtk_w32(hw, MTK_RTC_PROT2, RTC_PROT2_MAGIC);\n> > +\tmtk_w32(hw, MTK_RTC_PROT3, RTC_PROT3_MAGIC);\n> > +\tmtk_w32(hw, MTK_RTC_PROT4, RTC_PROT4_MAGIC);\n> > +\tmtk_rmw(hw, MTK_RTC_DEBNCE, RTC_DEBNCE_MASK, 0);\n> > +\tmtk_clr(hw, MTK_RTC_CTL, RTC_RC_STOP);\n> > +}\n> > +\n> > +static void mtk_rtc_get_alarm_or_time(struct mtk_rtc *hw, struct rtc_time *tm,\n> > +\t\t\t\t int time_alarm)\n> > +{\n> > +\tu32 year, mon, mday, wday, hour, min, sec;\n> > +\n> > +\t/*\n> > +\t * Read again until all fields are not changed for all fields in the\n> > +\t * consistent state.\n> > +\t */\n> > +\tdo {\n> > +\t\tyear = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_YEA));\n> > +\t\tmon = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MON));\n> > +\t\twday = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOW));\n> > +\t\tmday = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOM));\n> > +\t\thour = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_HOU));\n> > +\t\tmin = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MIN));\n> > +\t\tsec = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_SEC));\n> > +\t} while (year != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_YEA)) ||\n> > +\t\t mon != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MON)) ||\n> > +\t\t mday != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOM))\t||\n> > +\t\t wday != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOW))\t||\n> > +\t\t hour != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_HOU))\t||\n> > +\t\t min != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MIN))\t||\n> > +\t\t sec != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_SEC))\n> > +\t\t);\n> \n> I'm pretty sure only checking sec is enough because it is highly\n> unlikely that 7 reads take a minute.\n> \n\nYou're right. I made something stupid here. Only checking on sec is\nenough and will give simpler and better code.\n\n> > +static irqreturn_t mtk_rtc_alarmirq(int irq, void *id)\n> > +{\n> > +\tstruct mtk_rtc *hw = (struct mtk_rtc *)id;\n> > +\tu32 irq_sta;\n> > +\n> > +\t/* Stop alarm also implicitly disable the alarm interrupt */\n> > +\tmtk_w32(hw, MTK_RTC_AL_CTL, 0);\n> \n> You stop the alarm here, before testing whether the alarm really\n> happened.\n> \n\nOkay. I will exchange the order for alarm stopping and the examination\nwhether the alarm is really expired. \n\n> > +\tirq_sta = mtk_r32(hw, MTK_RTC_INT);\n> > +\tif (irq_sta & RTC_INT_AL_STA) {\n> > +\t\trtc_update_irq(hw->rtc, 1, RTC_IRQF | RTC_AF);\n> > +\n> > +\t\t/* Ack alarm interrupt status */\n> > +\t\tmtk_w32(hw, MTK_RTredundantC_INT, RTC_INT_AL_STA);\n> > +\t\treturn IRQ_HANDLED;\n> > +\t}\n> > +\n> > +\treturn IRQ_NONE;\n> > +}\n> > +\n> > +static int mtk_rtc_gettime(struct device *dev, struct rtc_time *tm)\n> > +{\n> > +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> > +\n> > +\tmtk_rtc_get_alarm_or_time(hw, tm, MTK_TC);\n> > +\n> > +\treturn rtc_valid_tm(tm);\n> > +}\n> > +\n> > +static int mtk_rtc_settime(struct device *dev, struct rtc_time *tm)\n> > +{\n> > +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> > +\n> > +\t/* Stop time counter before setting a new one*/\n> > +\tmtk_set(hw, MTK_RTC_CTL, RTC_RC_STOP);\n> > +\n> > +\t/* Epoch == 1900 */\n> > +\tif (tm->tm_year < 100 || tm->tm_year > 199)\n> > +\t\treturn -EINVAL;\n> \n> Year is a 32 bits register, what makes the RTC fail in 2100? Is it\n> because of the leap year handling?\n> \n\nI made something stupid again here: rtc hardware doesn't have such the\nlimitation. I just felt alarm set up prior to 2100 is enough in my\ninitial thought, but it seemed I shouldn't do this. I will remove the\nsanity condition.\n\n\n> > +static int mtk_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)\n> > +{\n> > +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> > +\tstruct rtc_time *alrm_tm = &wkalrm->time;\n> > +\n> > +\t/* Epoch == 1900 */\n> > +\tif (alrm_tm->tm_year < 100 || alrm_tm->tm_year > 199)\n> > +\t\treturn -EINVAL;\n> > +\n> \n> Ditto.\n> \nDitto. those condition will be removed.\n\n> > +\n> > +\tdev_info(&pdev->dev, \"MediaTek SoC based RTC enabled\\n\");\n> > +\n> \n> I think the rtc core is verbose enough that this message is not needed.\n> \n\nOkay. the redundant and specific log prompt would be removed as well.\n\n\n> \n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yFrmX6mgNz9t1t\n\tfor ;\n\tMon, 16 Oct 2017 19:18:04 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751438AbdJPISD (ORCPT\n\t);\n\tMon, 16 Oct 2017 04:18:03 -0400","from mailgw02.mediatek.com ([210.61.82.184]:25130 \"EHLO\n\tmailgw02.mediatek.com\" rhost-flags-OK-FAIL-OK-FAIL) by\n\tvger.kernel.org with ESMTP id S1751284AbdJPISC (ORCPT\n\t); Mon, 16 Oct 2017 04:18:02 -0400","from mtkcas09.mediatek.inc [(172.21.101.178)] by\n\tmailgw02.mediatek.com (envelope-from )\n\t(mhqrelay.mediatek.com ESMTP with TLS)\n\twith ESMTP id 1113686827; Mon, 16 Oct 2017 16:17:57 +0800","from mtkcas07.mediatek.inc (172.21.101.84) by\n\tmtkmbs01n1.mediatek.inc (172.21.101.68) with Microsoft SMTP Server\n\t(TLS) id 15.0.1210.3; Mon, 16 Oct 2017 16:17:56 +0800","from [172.21.77.33] (172.21.77.33) by mtkcas07.mediatek.inc\n\t(172.21.101.73) with Microsoft SMTP Server id 15.0.1210.3 via\n\tFrontend Transport; Mon, 16 Oct 2017 16:17:56 +0800"],"X-UUID":"c065060ccc414f1889c752b514f3b1a4-20171016","Message-ID":"<1508141876.21840.75.camel@mtkswgap22>","Subject":"Re: [PATCH 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC","From":"Sean Wang ","To":"Alexandre Belloni ","CC":", ,\n\t, ,\n\t, ,\n\t","Date":"Mon, 16 Oct 2017 16:17:56 +0800","In-Reply-To":"<20171012212052.xkhbgdjrghmnvcfe@piout.net>","References":"\n\t<5b2a7e5c9c5bc179f89e48fb614b2ae789be4254.1506049341.git.sean.wang@mediatek.com>\n\t<20171012212052.xkhbgdjrghmnvcfe@piout.net>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.2.3-0ubuntu6 ","Content-Transfer-Encoding":"7bit","MIME-Version":"1.0","X-MTK":"N","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1787976,"web_url":"http://patchwork.ozlabs.org/comment/1787976/","msgid":"<1508210661.21840.86.camel@mtkswgap22>","list_archive_url":null,"date":"2017-10-17T03:24:21","subject":"Re: [PATCH 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC","submitter":{"id":69660,"url":"http://patchwork.ozlabs.org/api/people/69660/","name":"Sean Wang","email":"sean.wang@mediatek.com"},"content":"On Mon, 2017-10-16 at 16:17 +0800, Sean Wang wrote:\n> Hi Alexandre,\n> \n> Thanks for your valuable suggestions on the driver.\n> \n> I added comments inline and will have following-ups in the next version\n> \n> \tSean\n> \n> On Thu, 2017-10-12 at 23:20 +0200, Alexandre Belloni wrote:\n> > Hi,\n> > \n> > On 22/09/2017 at 11:33:15 +0800, sean.wang@mediatek.com wrote:\n> > > diff --git a/drivers/rtc/rtc-mediatek.c b/drivers/rtc/rtc-mediatek.c\n> > \n> > I'm pretty sure this should be named rtc-mt7622.c instead of\n> > rtc-mediatek.c, exactly for the same reason you have patch 3/4.\n> > \n> \n> It's okay for naming with rtc-mt7622.c at this moment. But if more SoCs\n> support gets into the driver, I will consider again to give a more\n> generic name for the driver.\n> \n> > > +static void mtk_w32(struct mtk_rtc *rtc, u32 reg, u32 val)\n> > > +{\n> > > +\t__raw_writel(val, rtc->base + reg);\n> > \n> > Do you really need the __raw accessors? What about running your SoC in\n> > BE mode? I guess the _relaxed version are fast enough.\n> > \n> \n> SoC runs on LE mode. I also think it's fine and enough to use _relaxed\n> version instead of __raw version.\n> \n> > > +}\n> > > +\n> > > +static u32 mtk_r32(struct mtk_rtc *rtc, u32 reg)\n> > > +{\n> > > +\treturn __raw_readl(rtc->base + reg);\n> > > +}\n> > > +\n> > \n> > \n> > > +static void mtk_rtc_hw_init(struct mtk_rtc *hw)\n> > > +{\n> > > +\t/* The setup of the init sequence is for allowing RTC got to work */\n> > > +\tmtk_w32(hw, MTK_RTC_PWRCHK1, RTC_PWRCHK1_MAGIC);\n> > > +\tmtk_w32(hw, MTK_RTC_PWRCHK2, RTC_PWRCHK2_MAGIC);\n> > > +\tmtk_w32(hw, MTK_RTC_KEY, RTC_KEY_MAGIC);\n> > > +\tmtk_w32(hw, MTK_RTC_PROT1, RTC_PROT1_MAGIC);\n> > > +\tmtk_w32(hw, MTK_RTC_PROT2, RTC_PROT2_MAGIC);\n> > > +\tmtk_w32(hw, MTK_RTC_PROT3, RTC_PROT3_MAGIC);\n> > > +\tmtk_w32(hw, MTK_RTC_PROT4, RTC_PROT4_MAGIC);\n> > > +\tmtk_rmw(hw, MTK_RTC_DEBNCE, RTC_DEBNCE_MASK, 0);\n> > > +\tmtk_clr(hw, MTK_RTC_CTL, RTC_RC_STOP);\n> > > +}\n> > > +\n> > > +static void mtk_rtc_get_alarm_or_time(struct mtk_rtc *hw, struct rtc_time *tm,\n> > > +\t\t\t\t int time_alarm)\n> > > +{\n> > > +\tu32 year, mon, mday, wday, hour, min, sec;\n> > > +\n> > > +\t/*\n> > > +\t * Read again until all fields are not changed for all fields in the\n> > > +\t * consistent state.\n> > > +\t */\n> > > +\tdo {\n> > > +\t\tyear = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_YEA));\n> > > +\t\tmon = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MON));\n> > > +\t\twday = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOW));\n> > > +\t\tmday = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOM));\n> > > +\t\thour = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_HOU));\n> > > +\t\tmin = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MIN));\n> > > +\t\tsec = mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_SEC));\n> > > +\t} while (year != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_YEA)) ||\n> > > +\t\t mon != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MON)) ||\n> > > +\t\t mday != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOM))\t||\n> > > +\t\t wday != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_DOW))\t||\n> > > +\t\t hour != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_HOU))\t||\n> > > +\t\t min != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_MIN))\t||\n> > > +\t\t sec != mtk_r32(hw, MTK_RTC_TREG(time_alarm, MTK_SEC))\n> > > +\t\t);\n> > \n> > I'm pretty sure only checking sec is enough because it is highly\n> > unlikely that 7 reads take a minute.\n> > \n> \n> You're right. I made something stupid here. Only checking on sec is\n> enough and will give simpler and better code.\n> \n> > > +static irqreturn_t mtk_rtc_alarmirq(int irq, void *id)\n> > > +{\n> > > +\tstruct mtk_rtc *hw = (struct mtk_rtc *)id;\n> > > +\tu32 irq_sta;\n> > > +\n> > > +\t/* Stop alarm also implicitly disable the alarm interrupt */\n> > > +\tmtk_w32(hw, MTK_RTC_AL_CTL, 0);\n> > \n> > You stop the alarm here, before testing whether the alarm really\n> > happened.\n> > \n> \n> Okay. I will exchange the order for alarm stopping and the examination\n> whether the alarm is really expired. \n> \n> > > +\tirq_sta = mtk_r32(hw, MTK_RTC_INT);\n> > > +\tif (irq_sta & RTC_INT_AL_STA) {\n> > > +\t\trtc_update_irq(hw->rtc, 1, RTC_IRQF | RTC_AF);\n> > > +\n> > > +\t\t/* Ack alarm interrupt status */\n> > > +\t\tmtk_w32(hw, MTK_RTredundantC_INT, RTC_INT_AL_STA);\n> > > +\t\treturn IRQ_HANDLED;\n> > > +\t}\n> > > +\n> > > +\treturn IRQ_NONE;\n> > > +}\n> > > +\n> > > +static int mtk_rtc_gettime(struct device *dev, struct rtc_time *tm)\n> > > +{\n> > > +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> > > +\n> > > +\tmtk_rtc_get_alarm_or_time(hw, tm, MTK_TC);\n> > > +\n> > > +\treturn rtc_valid_tm(tm);\n> > > +}\n> > > +\n> > > +static int mtk_rtc_settime(struct device *dev, struct rtc_time *tm)\n> > > +{\n> > > +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> > > +\n> > > +\t/* Stop time counter before setting a new one*/\n> > > +\tmtk_set(hw, MTK_RTC_CTL, RTC_RC_STOP);\n> > > +\n> > > +\t/* Epoch == 1900 */\n> > > +\tif (tm->tm_year < 100 || tm->tm_year > 199)\n> > > +\t\treturn -EINVAL;\n> > \n> > Year is a 32 bits register, what makes the RTC fail in 2100? Is it\n> > because of the leap year handling?\n> > \n> \n> I made something stupid again here: rtc hardware doesn't have such the\n> limitation. I just felt alarm set up prior to 2100 is enough in my\n> initial thought, but it seemed I shouldn't do this. I will remove the\n> sanity condition.\n> \nSorry for that I gave incorrect information for the RTC in the previous\nreply: After check again the usage of the register, the maximum number\nof the year the RTC can hold is 99 and then wraparound to 0 when\noverflow occurs although the year register is a 32 bits register.\n\nTherefore, the sanity for tm->tm_year is still required for the both\nsetup handler on alarm and rtc to ensure the user input data is valid,\nwhere the current driver assume it's valid when tm->tm_year is between\n2000 and 2099. I'll add more comments for the hardware limitation.\n\n\tSean\n> \n> > > +static int mtk_rtc_setalarm(struct device *dev, struct rtc_wkalrm *wkalrm)\n> > > +{\n> > > +\tstruct mtk_rtc *hw = dev_get_drvdata(dev);\n> > > +\tstruct rtc_time *alrm_tm = &wkalrm->time;\n> > > +\n> > > +\t/* Epoch == 1900 */\n> > > +\tif (alrm_tm->tm_year < 100 || alrm_tm->tm_year > 199)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > \n> > Ditto.\n> > \n> Ditto. those condition will be removed.\n> \n> > > +\n> > > +\tdev_info(&pdev->dev, \"MediaTek SoC based RTC enabled\\n\");\n> > > +\n> > \n> > I think the rtc core is verbose enough that this message is not needed.\n> > \n> \n> Okay. the redundant and specific log prompt would be removed as well.\n> \n> \n> > \n> \n\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3yGLCV130Xz9sRq\n\tfor ;\n\tTue, 17 Oct 2017 14:24:38 +1100 (AEDT)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1755164AbdJQDYg (ORCPT\n\t);\n\tMon, 16 Oct 2017 23:24:36 -0400","from mailgw02.mediatek.com ([210.61.82.184]:5891 \"EHLO\n\tmailgw02.mediatek.com\" rhost-flags-OK-FAIL-OK-FAIL) by\n\tvger.kernel.org with ESMTP id S1754506AbdJQDYf (ORCPT\n\t); Mon, 16 Oct 2017 23:24:35 -0400","from mtkcas06.mediatek.inc [(172.21.101.30)] by\n\tmailgw02.mediatek.com (envelope-from )\n\t(mhqrelay.mediatek.com ESMTP with TLS)\n\twith ESMTP id 550524731; Tue, 17 Oct 2017 11:24:29 +0800","from MTKCAS06.mediatek.inc (172.21.101.30) by\n\tmtkmbs01n2.mediatek.inc (172.21.101.79) with Microsoft SMTP Server\n\t(TLS) id 15.0.1210.3; Tue, 17 Oct 2017 11:24:27 +0800","from [172.21.77.33] (172.21.77.33) by MTKCAS06.mediatek.inc\n\t(172.21.101.73) with Microsoft SMTP Server id 15.0.1210.3 via\n\tFrontend Transport; Tue, 17 Oct 2017 11:24:21 +0800"],"X-UUID":"626e8c7cd1824c4d9cca2310cee5005e-20171017","Message-ID":"<1508210661.21840.86.camel@mtkswgap22>","Subject":"Re: [PATCH 2/4] rtc: mediatek: add driver for RTC on MT7622 SoC","From":"Sean Wang ","To":"Alexandre Belloni ","CC":", ,\n\t, ,\n\t, ,\n\t","Date":"Tue, 17 Oct 2017 11:24:21 +0800","In-Reply-To":"<1508141876.21840.75.camel@mtkswgap22>","References":"\n\t<5b2a7e5c9c5bc179f89e48fb614b2ae789be4254.1506049341.git.sean.wang@mediatek.com>\n\t<20171012212052.xkhbgdjrghmnvcfe@piout.net>\n\t<1508141876.21840.75.camel@mtkswgap22>","Content-Type":"text/plain; charset=\"UTF-8\"","X-Mailer":"Evolution 3.2.3-0ubuntu6 ","Content-Transfer-Encoding":"7bit","MIME-Version":"1.0","X-MTK":"N","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]