[PATCH 0/2] Add Mediatek RTC driver

[PATCH 0/2] Add Mediatek RTC driver

[Eddie Huang]

RTC is one submodule of Mediatek MT6397 PMIC chip[1]. This series 
support RTC driver that work with Mediatek SoC like MT8135, MT8173.
It implements second counter and also provide alarm function.

This series base on 3.19-rc1, and Sascha's PMIC mfd patch[1]. Test ok 
on MT8135 platform.

[1] https://lkml.org/lkml/2015/1/23/325

Eddie Huang (1):
  dt: bindings: Add Mediatek RTC driver binding document

Tianping Fang (1):
  rtc: mediatek: Add MT63xx RTC driver

 .../bindings/rtc/mediatek,mt63xx-rtc.txt           |  18 ++
 drivers/rtc/Kconfig                                |  10 +
 drivers/rtc/Makefile                               |   1 +
 drivers/rtc/rtc-mt6397.c                           | 352 +++++++++++++++++++++
 4 files changed, 381 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/mediatek,mt63xx-rtc.txt
 create mode 100644 drivers/rtc/rtc-mt6397.c

-- 
1.8.1.1.dirty

[PATCH 1/2] dt: bindings: Add Mediatek RTC driver binding document

[Eddie Huang]

Add MT63xx RTC driver binding document.

Signed-off-by: Tianping Fang <tianping.fang@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 .../devicetree/bindings/rtc/mediatek,mt63xx-rtc.txt    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/mediatek,mt63xx-rtc.txt

diff --git a/Documentation/devicetree/bindings/rtc/mediatek,mt63xx-rtc.txt b/Documentation/devicetree/bindings/rtc/mediatek,mt63xx-rtc.txt
new file mode 100644
index 0000000..6347d4a
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/mediatek,mt63xx-rtc.txt
@@ -0,0 +1,18 @@
+Mediatek MT63xx real-time clock
+
+MT63xx is Mediatek PMIC chip series. RTC is one submodule of PMIC chip.
+The RTC maintains seconds counter, also provide alarm function.
+
+Required properties:
+- compatible: Should be "mediatek,mt6397-rtc".
+- reg: base physical address and size of the registers.
+- interrupt-parent: phandle for the interrupt controller
+- interrupts: RTC alarm interrupt.
+
+Examples:
+rtc: rtc at e000 {
+	compatible = "mediatek,mt6397-rtc";
+	reg = <0xe000 0x3e>;
+	interrupt-parent = <&pmic>;
+	interrupts = <20 IRQ_TYPE_LEVEL_HIGH>;
+};
-- 
1.8.1.1.dirty

[PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

From: Tianping Fang <tianping.fang@mediatek.com>

Add Mediatek MT63xx RTC driver

Signed-off-by: Tianping Fang <tianping.fang@mediatek.com>
Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 drivers/rtc/Kconfig      |  10 ++
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-mt6397.c | 352 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 363 insertions(+)
 create mode 100644 drivers/rtc/rtc-mt6397.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index f15cddf..8ac52d8 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART
 	   This driver can also be built as a module. If so, the module
 	   will be called rtc-moxart
 
+config RTC_DRV_MT63XX
+	tristate "Mediatek Real Time Clock driver"
+	depends on MFD_MT6397
+	help
+	  This selects the Mediatek(R) RTC driver, you should add support
+	  for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver.
+
+	  If you want to use Mediatek(R) RTC interface, select Y or M here.
+	  If unsure, Please select N.
+
 config RTC_DRV_XGENE
 	tristate "APM X-Gene RTC"
 	depends on HAS_IOMEM
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index c8ef3e1..53e0085 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -150,3 +150,4 @@ obj-$(CONFIG_RTC_DRV_X1205)	+= rtc-x1205.o
 obj-$(CONFIG_RTC_DRV_XGENE)	+= rtc-xgene.o
 obj-$(CONFIG_RTC_DRV_SIRFSOC)	+= rtc-sirfsoc.o
 obj-$(CONFIG_RTC_DRV_MOXART)	+= rtc-moxart.o
+obj-$(CONFIG_RTC_DRV_MT63XX)	+= rtc-mt6397.o
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
new file mode 100644
index 0000000..b54d605
--- /dev/null
+++ b/drivers/rtc/rtc-mt6397.c
@@ -0,0 +1,352 @@
+/*
+* Copyright (c) 2014-2015 MediaTek Inc.
+* Author: Tianping.Fang <tianping.fang@mediatek.com>
+*
+* This program is free software; you can redistribute it and/or modify
+* it under the terms of the GNU General Public License version 2 as
+* published by the Free Software Foundation.
+*
+* This program is distributed in the hope that it will be useful,
+* but WITHOUT ANY WARRANTY; without even the implied warranty of
+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+* GNU General Public License for more details.
+*/
+
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+#include <linux/irqdomain.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/of_address.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/mfd/mt6397/core.h>
+
+#define RTC_BBPU		0x0000
+#define RTC_WRTGR		0x003c
+#define RTC_IRQ_EN		0x0004
+#define RTC_IRQ_STA		0x0002
+
+#define RTC_BBPU_CBUSY		(1 << 6)
+#define RTC_BBPU_KEY		(0x43 << 8)
+#define RTC_BBPU_AUTO		(1 << 3)
+#define RTC_IRQ_STA_AL		(1 << 0)
+#define RTC_IRQ_STA_LP		(1 << 3)
+
+#define RTC_TC_SEC		0x000a
+#define RTC_TC_MIN		0x000c
+#define RTC_TC_HOU		0x000e
+#define RTC_TC_DOM		0x0010
+#define RTC_TC_MTH		0x0014
+#define RTC_TC_YEA		0x0016
+#define RTC_AL_SEC		0x0018
+#define RTC_AL_MIN		0x001a
+
+#define RTC_IRQ_EN_AL		(1 << 0)
+#define RTC_IRQ_EN_ONESHOT	(1 << 2)
+#define RTC_IRQ_EN_LP		(1 << 3)
+#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
+
+#define RTC_TC_MIN_MASK		0x003f
+#define RTC_TC_SEC_MASK		0x003f
+#define RTC_TC_HOU_MASK		0x001f
+#define RTC_TC_DOM_MASK		0x001f
+#define RTC_TC_MTH_MASK		0x000f
+#define RTC_TC_YEA_MASK		0x007f
+
+#define RTC_AL_SEC_MASK		0x003f
+#define RTC_AL_MIN_MASK		0x003f
+#define RTC_AL_MASK_DOW		(1 << 4)
+
+#define RTC_AL_HOU		0x001c
+#define RTC_NEW_SPARE_FG_MASK	0xff00
+#define RTC_NEW_SPARE_FG_SHIFT	8
+#define RTC_AL_HOU_MASK		0x001f
+
+#define RTC_AL_DOM		0x001e
+#define RTC_NEW_SPARE1		0xff00
+#define RTC_AL_DOM_MASK		0x001f
+#define RTC_AL_MASK		0x0008
+
+#define RTC_AL_MTH		0x0022
+#define RTC_NEW_SPARE3		0xff00
+#define RTC_AL_MTH_MASK		0x000f
+
+#define RTC_AL_YEA		0x0024
+#define RTC_AL_YEA_MASK		0x007f
+
+#define RTC_PDN1		0x002c
+#define RTC_PDN1_PWRON_TIME	(1 << 7)
+
+#define RTC_PDN2			0x002e
+#define RTC_PDN2_PWRON_MTH_MASK		0x000f
+#define RTC_PDN2_PWRON_MTH_SHIFT	0
+#define RTC_PDN2_PWRON_ALARM		(1 << 4)
+#define RTC_PDN2_UART_MASK		0x0060
+#define RTC_PDN2_UART_SHIFT		5
+#define RTC_PDN2_PWRON_YEA_MASK		0x7f00
+#define RTC_PDN2_PWRON_YEA_SHIFT	8
+#define RTC_PDN2_PWRON_LOGO		(1 << 15)
+
+#define RTC_MIN_YEAR		1968
+#define RTC_BASE_YEAR		1900
+#define RTC_NUM_YEARS		128
+#define RTC_MIN_YEAR_OFFSET	(RTC_MIN_YEAR - RTC_BASE_YEAR)
+#define RTC_RELPWR_WHEN_XRST	1
+
+struct mt6397_rtc {
+	struct device		*dev;
+	struct rtc_device	*rtc_dev;
+	struct mutex		lock;
+	struct regmap		*regmap;
+	int			irq;
+	u32			addr_base;
+	u32			addr_range;
+};
+
+static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
+{
+	u32 rdata = 0;
+	u32 addr = rtc->addr_base + offset;
+
+	if (offset < rtc->addr_range)
+		regmap_read(rtc->regmap, addr, &rdata);
+
+	return (u16)rdata;
+}
+
+static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
+{
+	u32 addr;
+
+	addr = rtc->addr_base + offset;
+
+	if (offset < rtc->addr_range)
+		regmap_write(rtc->regmap, addr, data);
+}
+
+static void rtc_write_trigger(struct mt6397_rtc *rtc)
+{
+	rtc_write(rtc, RTC_WRTGR, 1);
+	while (rtc_read(rtc, RTC_BBPU) & RTC_BBPU_CBUSY)
+		cpu_relax();
+}
+
+static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
+{
+	struct mt6397_rtc *rtc = data;
+	u16 irqsta, irqen;
+
+	mutex_lock(&rtc->lock);
+	irqsta = rtc_read(rtc, RTC_IRQ_STA);
+	mutex_unlock(&rtc->lock);
+
+	if (irqsta & RTC_IRQ_STA_AL) {
+		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
+		irqen = irqsta & ~RTC_IRQ_EN_AL;
+		rtc_write(rtc, RTC_IRQ_EN, irqen);
+		rtc_write_trigger(rtc);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	unsigned long time;
+	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
+
+	mutex_lock(&rtc->lock);
+	do {
+		tm->tm_sec = rtc_read(rtc, RTC_TC_SEC);
+		tm->tm_min = rtc_read(rtc, RTC_TC_MIN);
+		tm->tm_hour = rtc_read(rtc, RTC_TC_HOU);
+		tm->tm_mday = rtc_read(rtc, RTC_TC_DOM);
+		tm->tm_mon = rtc_read(rtc, RTC_TC_MTH);
+		tm->tm_year = rtc_read(rtc, RTC_TC_YEA);
+	} while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec);
+	mutex_unlock(&rtc->lock);
+
+	tm->tm_year += RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon--;
+	rtc_tm_to_time(tm, &time);
+
+	tm->tm_wday = (time / 86400 + 4) % 7;
+
+	return 0;
+}
+
+static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
+
+	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon++;
+	mutex_lock(&rtc->lock);
+	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
+	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
+	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
+	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
+	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
+	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
+	rtc_write_trigger(rtc);
+	mutex_unlock(&rtc->lock);
+
+	return 0;
+}
+
+static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	struct rtc_time *tm = &alm->time;
+	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
+	u16 irqen, pdn2;
+
+	mutex_lock(&rtc->lock);
+	irqen = rtc_read(rtc, RTC_IRQ_EN);
+	pdn2 = rtc_read(rtc, RTC_PDN2);
+	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
+	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
+	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
+	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
+	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
+	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
+	mutex_unlock(&rtc->lock);
+
+	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
+	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
+
+	tm->tm_year += RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon--;
+
+	return 0;
+}
+
+static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+	struct rtc_time *tm = &alm->time;
+	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
+	u16 irqen;
+
+	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon++;
+
+	if (alm->enabled) {
+		mutex_lock(&rtc->lock);
+		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
+		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
+				RTC_NEW_SPARE3) | tm->tm_mon);
+		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
+				RTC_NEW_SPARE1) | tm->tm_mday);
+		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
+				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
+		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
+		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
+		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
+		rtc_write_trigger(rtc);
+		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
+		rtc_write(rtc, RTC_IRQ_EN, irqen);
+		rtc_write_trigger(rtc);
+		mutex_unlock(&rtc->lock);
+	}
+
+	return 0;
+}
+
+static struct rtc_class_ops mtk_rtc_ops = {
+	.read_time  = mtk_rtc_read_time,
+	.set_time   = mtk_rtc_set_time,
+	.read_alarm = mtk_rtc_read_alarm,
+	.set_alarm  = mtk_rtc_set_alarm,
+};
+
+static int mtk_rtc_probe(struct platform_device *pdev)
+{
+	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
+	struct mt6397_rtc *rtc;
+	u32 reg[2];
+	int ret = 0;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
+	if (ret) {
+		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
+		return -EINVAL;
+	}
+	rtc->addr_base = reg[0];
+	rtc->addr_range = reg[1];
+	rtc->regmap = mt6397_chip->regmap;
+	rtc->dev = &pdev->dev;
+	mutex_init(&rtc->lock);
+
+	platform_set_drvdata(pdev, rtc);
+
+	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
+				&mtk_rtc_ops, THIS_MODULE);
+	if (IS_ERR(rtc->rtc_dev)) {
+		dev_err(&pdev->dev, "register rtc device failed\n");
+		return PTR_ERR(rtc->rtc_dev);
+	}
+
+	rtc->irq = platform_get_irq(pdev, 0);
+	if (rtc->irq < 0) {
+		ret = rtc->irq;
+		goto out_rtc;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
+			rtc_irq_handler_thread, IRQF_ONESHOT,
+			"mt6397-rtc", rtc);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
+			rtc->irq, ret);
+		goto out_rtc;
+	}
+
+	device_init_wakeup(&pdev->dev, 1);
+
+	return 0;
+
+out_rtc:
+	rtc_device_unregister(rtc->rtc_dev);
+	return ret;
+
+}
+
+static int mtk_rtc_remove(struct platform_device *pdev)
+{
+	struct mt6397_rtc *rtc = platform_get_drvdata(pdev);
+
+	rtc_device_unregister(rtc->rtc_dev);
+	return 0;
+}
+
+static const struct of_device_id mt6397_rtc_of_match[] = {
+	{ .compatible = "mediatek,mt6397-rtc", },
+	{ }
+};
+
+static struct platform_driver mtk_rtc_driver = {
+	.driver = {
+		.name = "mt6397-rtc",
+		.of_match_table = mt6397_rtc_of_match,
+	},
+	.probe	= mtk_rtc_probe,
+	.remove = mtk_rtc_remove,
+};
+
+module_platform_driver(mtk_rtc_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Tianping Fang <tianping.fang@mediatek.com>");
+MODULE_DESCRIPTION("RTC Driver for MediaTek MT6397 PMIC");
+MODULE_ALIAS("platform:mt6397-rtc");
-- 
1.8.1.1.dirty

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Andrew Morton]

On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:

> From: Tianping Fang <tianping.fang@mediatek.com>
> 
> Add Mediatek MT63xx RTC driver

There are a couple of checkpatch warnings which should be addressed,
please:

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#150: 
new file mode 100644

WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
#488: FILE: drivers/rtc/rtc-mt6397.c:334:
+	{ .compatible = "mediatek,mt6397-rtc", },




>
> ...
>
> +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> +{
> +	u32 rdata = 0;
> +	u32 addr = rtc->addr_base + offset;
> +
> +	if (offset < rtc->addr_range)
> +		regmap_read(rtc->regmap, addr, &rdata);
> +
> +	return (u16)rdata;
> +}
> +
> +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> +{
> +	u32 addr;
> +
> +	addr = rtc->addr_base + offset;
> +
> +	if (offset < rtc->addr_range)
> +		regmap_write(rtc->regmap, addr, data);
> +}

regmap_read() and regmap_write() can return errors.  There is no
checking for this.

> +static void rtc_write_trigger(struct mt6397_rtc *rtc)
> +{
> +	rtc_write(rtc, RTC_WRTGR, 1);
> +	while (rtc_read(rtc, RTC_BBPU) & RTC_BBPU_CBUSY)
> +		cpu_relax();
> +}
> +
>
> ...
>
> +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen, pdn2;
> +
> +	mutex_lock(&rtc->lock);
> +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> +	pdn2 = rtc_read(rtc, RTC_PDN2);
> +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> +	mutex_unlock(&rtc->lock);
> +
> +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> +
> +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon--;
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen;
> +
> +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon++;
> +
> +	if (alm->enabled) {
> +		mutex_lock(&rtc->lock);
> +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> +				RTC_NEW_SPARE3) | tm->tm_mon);
> +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> +				RTC_NEW_SPARE1) | tm->tm_mday);
> +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> +		rtc_write_trigger(rtc);
> +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> +		rtc_write_trigger(rtc);
> +		mutex_unlock(&rtc->lock);
> +	}
> +
> +	return 0;
> +}

This all looks a bit racy.  Wouldn't it be better if the testing of and
modification of ->enabled and ->pending were protected by the mutex?

>
> ...
>
> +static int mtk_rtc_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> +	struct mt6397_rtc *rtc;
> +	u32 reg[2];
> +	int ret = 0;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> +	if (ret) {
> +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> +		return -EINVAL;
> +	}
> +	rtc->addr_base = reg[0];
> +	rtc->addr_range = reg[1];
> +	rtc->regmap = mt6397_chip->regmap;
> +	rtc->dev = &pdev->dev;
> +	mutex_init(&rtc->lock);
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> +				&mtk_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc->rtc_dev)) {
> +		dev_err(&pdev->dev, "register rtc device failed\n");
> +		return PTR_ERR(rtc->rtc_dev);
> +	}
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0) {
> +		ret = rtc->irq;
> +		goto out_rtc;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +			rtc_irq_handler_thread, IRQF_ONESHOT,
> +			"mt6397-rtc", rtc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> +			rtc->irq, ret);
> +		goto out_rtc;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	return 0;
> +
> +out_rtc:
> +	rtc_device_unregister(rtc->rtc_dev);
> +	return ret;
> +
> +}

It seems strange to request the IRQ after having registered the rtc. 
And possibly racy - I seem to recall another driver having issues with
this recently.

A lot of rtc drivers are requesting the IRQ after registration so
presumably it isn't a huge problem.  But still, wouldn't it be better
to defer registration until after the IRQ has been successfully
obtained?

>
> ...
>

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Andrew,

On Mon, 2015-02-23 at 13:50 -0800, Andrew Morton wrote:
> On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:
> 
> > From: Tianping Fang <tianping.fang@mediatek.com>
> > 
> > Add Mediatek MT63xx RTC driver
> 
> There are a couple of checkpatch warnings which should be addressed,
> please:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #150: 
> new file mode 100644
> 
OK, I will update MAINTAINERS file

> WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
> #488: FILE: drivers/rtc/rtc-mt6397.c:334:
> +	{ .compatible = "mediatek,mt6397-rtc", },
> 
> 
Do you include patch "[PATCH 1/2] dt: bindings: Add Mediatek RTC driver
binding document" in this series ?
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320425.html

I think this warning shouldn't happen if include this patch.


> >
> > ...
> >
> > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> > +{
> > +	u32 addr;
> > +
> > +	addr = rtc->addr_base + offset;
> > +
> > +	if (offset < rtc->addr_range)
> > +		regmap_write(rtc->regmap, addr, data);
> > +}
> 
> regmap_read() and regmap_write() can return errors.  There is no
> checking for this.
Will fix it.

> > ...
> >
> > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen, pdn2;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> > +	pdn2 = rtc_read(rtc, RTC_PDN2);
> > +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> > +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> > +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> > +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> > +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> > +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> > +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen;
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +
> > +	if (alm->enabled) {
> > +		mutex_lock(&rtc->lock);
> > +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> > +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> > +				RTC_NEW_SPARE3) | tm->tm_mon);
> > +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> > +				RTC_NEW_SPARE1) | tm->tm_mday);
> > +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> > +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> > +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> > +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> > +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> > +		rtc_write_trigger(rtc);
> > +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		mutex_unlock(&rtc->lock);
> > +	}
> > +
> > +	return 0;
> > +}
> 
> This all looks a bit racy.  Wouldn't it be better if the testing of and
> modification of ->enabled and ->pending were protected by the mutex?
> 
Will fix it.

> >
> > ...
> >
> > +static int mtk_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +	struct mt6397_rtc *rtc;
> > +	u32 reg[2];
> > +	int ret = 0;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> > +		return -EINVAL;
> > +	}
> > +	rtc->addr_base = reg[0];
> > +	rtc->addr_range = reg[1];
> > +	rtc->regmap = mt6397_chip->regmap;
> > +	rtc->dev = &pdev->dev;
> > +	mutex_init(&rtc->lock);
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > +				&mtk_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc->rtc_dev)) {
> > +		dev_err(&pdev->dev, "register rtc device failed\n");
> > +		return PTR_ERR(rtc->rtc_dev);
> > +	}
> > +
> > +	rtc->irq = platform_get_irq(pdev, 0);
> > +	if (rtc->irq < 0) {
> > +		ret = rtc->irq;
> > +		goto out_rtc;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> > +			rtc_irq_handler_thread, IRQF_ONESHOT,
> > +			"mt6397-rtc", rtc);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> > +			rtc->irq, ret);
> > +		goto out_rtc;
> > +	}
> > +
> > +	device_init_wakeup(&pdev->dev, 1);
> > +
> > +	return 0;
> > +
> > +out_rtc:
> > +	rtc_device_unregister(rtc->rtc_dev);
> > +	return ret;
> > +
> > +}
> 
> It seems strange to request the IRQ after having registered the rtc. 
> And possibly racy - I seem to recall another driver having issues with
> this recently.
> 
> A lot of rtc drivers are requesting the IRQ after registration so
> presumably it isn't a huge problem.  But still, wouldn't it be better
> to defer registration until after the IRQ has been successfully
> obtained?
> 
OK, will fix it.

Eddie

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Andrew Morton]

On Mon, 2 Mar 2015 16:20:36 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:

> > WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
> > #488: FILE: drivers/rtc/rtc-mt6397.c:334:
> > +	{ .compatible = "mediatek,mt6397-rtc", },
> > 
> > 
> Do you include patch "[PATCH 1/2] dt: bindings: Add Mediatek RTC driver
> binding document" in this series ?
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320425.html
> 
> I think this warning shouldn't happen if include this patch.

I can't find that patch on rtc-linux or linux-kernel.  Resend, please?

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi,

On Mon, 2015-02-23 at 13:50 -0800, Andrew Morton wrote:
> On Wed, 28 Jan 2015 17:27:56 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:
> 
> > From: Tianping Fang <tianping.fang@mediatek.com>
> > 
> > Add Mediatek MT63xx RTC driver
> 
> There are a couple of checkpatch warnings which should be addressed,
> please:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #150: 
> new file mode 100644
> 
> WARNING: DT compatible string "mediatek,mt6397-rtc" appears un-documented -- check ./Documentation/devicetree/bindings/
> #488: FILE: drivers/rtc/rtc-mt6397.c:334:
> +	{ .compatible = "mediatek,mt6397-rtc", },
> 
> 
> 
> 
> >
> > ...
> >
> > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > +{
> > +	u32 rdata = 0;
> > +	u32 addr = rtc->addr_base + offset;
> > +
> > +	if (offset < rtc->addr_range)
> > +		regmap_read(rtc->regmap, addr, &rdata);
> > +
> > +	return (u16)rdata;
> > +}
> > +
> > +static void rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> > +{
> > +	u32 addr;
> > +
> > +	addr = rtc->addr_base + offset;
> > +
> > +	if (offset < rtc->addr_range)
> > +		regmap_write(rtc->regmap, addr, data);
> > +}
> 
> regmap_read() and regmap_write() can return errors.  There is no
> checking for this.
> 

I encounter some trouble when I add code to check return value of
regmap_read and regmap_write. Every RTC register access through regmap,
and there are many register read/write in this driver. If I check every
return value, the driver will become ugly. I try to make this driver
clean using following macro.

static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
{
        u32 addr = rtc->addr_base + offset;

        if (offset < rtc->addr_range)
                return regmap_read(rtc->regmap, addr, data);

        return -EINVAL;
}

#define rtc_read(ret, rtc, offset, data)                \
({                                                      \
        ret = __rtc_read(rtc, offset, data);            \
        if (ret < 0)                                    \
                goto rtc_exit;                          \
})                                                      \


And function call rtc_read, rtc_write looks like:

static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
{
        unsigned long time;
        struct mt6397_rtc *rtc = dev_get_drvdata(dev);
        int ret = 0;
        u32 sec;

        mutex_lock(&rtc->lock);
        do {
                rtc_read(ret, rtc, RTC_TC_SEC, &tm->tm_sec);
                rtc_read(ret, rtc, RTC_TC_MIN, &tm->tm_min);
                rtc_read(ret, rtc, RTC_TC_HOU, &tm->tm_hour);
                rtc_read(ret, rtc, RTC_TC_DOM, &tm->tm_mday);
                rtc_read(ret, rtc, RTC_TC_MTH, &tm->tm_mon);
                rtc_read(ret, rtc, RTC_TC_YEA, &tm->tm_year);
                rtc_read(ret, rtc, RTC_TC_SEC, &sec);
        } while (sec < tm->tm_sec);
        mutex_unlock(&rtc->lock);

        tm->tm_year += RTC_MIN_YEAR_OFFSET;
        tm->tm_mon--;
        rtc_tm_to_time(tm, &time);

        tm->tm_wday = (time / 86400 + 4) % 7;

        return 0;

rtc_exit:
        mutex_unlock(&rtc->lock);
        return ret;
}

It's a little tricky, does anyone have good suggestion ?

Eddie

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Sascha Hauer]

Hi Eddie,

On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote:
> > regmap_read() and regmap_write() can return errors.  There is no
> > checking for this.
> > 
> 
> I encounter some trouble when I add code to check return value of
> regmap_read and regmap_write. Every RTC register access through regmap,
> and there are many register read/write in this driver. If I check every
> return value, the driver will become ugly. I try to make this driver
> clean using following macro.
> 
> static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> {
>         u32 addr = rtc->addr_base + offset;
> 
>         if (offset < rtc->addr_range)
>                 return regmap_read(rtc->regmap, addr, data);
> 
>         return -EINVAL;
> }
> 
> #define rtc_read(ret, rtc, offset, data)                \
> ({                                                      \
>         ret = __rtc_read(rtc, offset, data);            \
>         if (ret < 0)                                    \
>                 goto rtc_exit;                          \
> })                                                      \

Hiding a goto (or return) in a macro is a very bad idea.

what you can do is

	ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec);
	ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min);

	if (ret)
		return -EIO;

(Don't return ret in this case though as it might contain different
error codes orred together)

Another possibilty at least for contiguous registers would be
regmap_bulk_read().

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Sascha,

On Fri, 2015-03-13 at 11:57 +0100, Sascha Hauer wrote:
> Hi Eddie,
> 
> On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote:
> > > regmap_read() and regmap_write() can return errors.  There is no
> > > checking for this.
> > > 
> > 
> > I encounter some trouble when I add code to check return value of
> > regmap_read and regmap_write. Every RTC register access through regmap,
> > and there are many register read/write in this driver. If I check every
> > return value, the driver will become ugly. I try to make this driver
> > clean using following macro.
> > 
> > static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> > {
> >         u32 addr = rtc->addr_base + offset;
> > 
> >         if (offset < rtc->addr_range)
> >                 return regmap_read(rtc->regmap, addr, data);
> > 
> >         return -EINVAL;
> > }
> > 
> > #define rtc_read(ret, rtc, offset, data)                \
> > ({                                                      \
> >         ret = __rtc_read(rtc, offset, data);            \
> >         if (ret < 0)                                    \
> >                 goto rtc_exit;                          \
> > })                                                      \
> 
> Hiding a goto (or return) in a macro is a very bad idea.
> 
> what you can do is
> 
> 	ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec);
> 	ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min);
> 
> 	if (ret)
> 		return -EIO;
> 
> (Don't return ret in this case though as it might contain different
> error codes orred together)
> 

OK, I will drop macro, and check regmap_read, regmap_write return value
in each function.

> Another possibilty at least for contiguous registers would be
> regmap_bulk_read().
> 
> Sascha
> 

Contiguous registers access occurs in reading and writing time. I think
Matthias's suggestion is a good way: 

do {
	ret = __mtk_rtc_read_time(rtc, &tm, &sec);
	if (ret < 0)
		goto rtc_exit;
} while (sec < tm->tm_sec);

Eddie

[rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Matthias Brugger]

On 13/03/15 11:29, Eddie Huang wrote:
> 
> I encounter some trouble when I add code to check return value of
> regmap_read and regmap_write. Every RTC register access through regmap,
> and there are many register read/write in this driver. If I check every
> return value, the driver will become ugly. I try to make this driver
> clean using following macro.
> 
> static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> {
>         u32 addr = rtc->addr_base + offset;
> 
>         if (offset < rtc->addr_range)
>                 return regmap_read(rtc->regmap, addr, data);
> 
>         return -EINVAL;
> }
> 
> #define rtc_read(ret, rtc, offset, data)                \
> ({                                                      \
>         ret = __rtc_read(rtc, offset, data);            \
>         if (ret < 0)                                    \
>                 goto rtc_exit;                          \
> })                                                      \
> 

I agree with Sascha on hiding a goto statement in a macro is not a good idea.

> 
> And function call rtc_read, rtc_write looks like:
> 
> static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> {
>         unsigned long time;
>         struct mt6397_rtc *rtc = dev_get_drvdata(dev);
>         int ret = 0;
>         u32 sec;
> 
>         mutex_lock(&rtc->lock);
>         do {
>                 rtc_read(ret, rtc, RTC_TC_SEC, &tm->tm_sec);
>                 rtc_read(ret, rtc, RTC_TC_MIN, &tm->tm_min);
>                 rtc_read(ret, rtc, RTC_TC_HOU, &tm->tm_hour);
>                 rtc_read(ret, rtc, RTC_TC_DOM, &tm->tm_mday);
>                 rtc_read(ret, rtc, RTC_TC_MTH, &tm->tm_mon);
>                 rtc_read(ret, rtc, RTC_TC_YEA, &tm->tm_year);
>                 rtc_read(ret, rtc, RTC_TC_SEC, &sec);
>         } while (sec < tm->tm_sec);

What about introducing
static int __mtk_rtc_read_time(struct mt6397_rtc *rtc, struct rtc_time *tm, u32 *sec)
and hide the checks of return values from regmap_read and the offset check in there. You return the error code or 0.

This way the while loop would look like this:

do {
	ret = __mtk_rtc_read_time(rtc, &tm, &sec);
	if (ret < 0)
		goto rtc_exit;
} while (sec < tm->tm_sec);

Best regards,
Matthias

[PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> From: Tianping Fang <tianping.fang@mediatek.com>
> 
> Add Mediatek MT63xx RTC driver
MT6397?

> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index f15cddf..8ac52d8 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART
>  	   This driver can also be built as a module. If so, the module
>  	   will be called rtc-moxart
>  
> +config RTC_DRV_MT63XX
> +	tristate "Mediatek Real Time Clock driver"
> +	depends on MFD_MT6397
I suggest:

	depends on MFD_MT6397 || COMPILE_TEST

(maybe + any hard dependencies you need for compilation).

> +	help
> +	  This selects the Mediatek(R) RTC driver, you should add support
> +	  for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver.
> +
> +	  If you want to use Mediatek(R) RTC interface, select Y or M here.
> +	  If unsure, Please select N.
Given the dependency above I'd say choosing y here is fine. Instead of
recommending that I'd just drop this line.

> [...]
> +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
rtc_read is a bad name for a driver. There are already 6 functions with
this name in the kernel. Better use a unique prefix.

> [...]
> +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> +{
> +	struct mt6397_rtc *rtc = data;
> +	u16 irqsta, irqen;
> +
> +	mutex_lock(&rtc->lock);
> +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
Do you really need to lock for a single read access?

> +	mutex_unlock(&rtc->lock);
> +
> +	if (irqsta & RTC_IRQ_STA_AL) {
> +		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> +		irqen = irqsta & ~RTC_IRQ_EN_AL;
> +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> +		rtc_write_trigger(rtc);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	unsigned long time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +
> +	mutex_lock(&rtc->lock);
> +	do {
> +		tm->tm_sec = rtc_read(rtc, RTC_TC_SEC);
> +		tm->tm_min = rtc_read(rtc, RTC_TC_MIN);
> +		tm->tm_hour = rtc_read(rtc, RTC_TC_HOU);
> +		tm->tm_mday = rtc_read(rtc, RTC_TC_DOM);
> +		tm->tm_mon = rtc_read(rtc, RTC_TC_MTH);
> +		tm->tm_year = rtc_read(rtc, RTC_TC_YEA);
> +	} while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec);
> +	mutex_unlock(&rtc->lock);
> +
> +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon--;
> +	rtc_tm_to_time(tm, &time);
rtc_tm_to_time is deprecated, better use rtc_tm_to_time64.

> +	tm->tm_wday = (time / 86400 + 4) % 7;
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +
> +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon++;
> +	mutex_lock(&rtc->lock);
> +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
write to it but after you wrote RTC_TC_MIN?

> +	rtc_write_trigger(rtc);
> +	mutex_unlock(&rtc->lock);
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen, pdn2;
> +
> +	mutex_lock(&rtc->lock);
> +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> +	pdn2 = rtc_read(rtc, RTC_PDN2);
> +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> +	mutex_unlock(&rtc->lock);
> +
> +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> +
> +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon--;
> +
> +	return 0;
> +}
> +
> +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> +{
> +	struct rtc_time *tm = &alm->time;
> +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +	u16 irqen;
> +
> +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +	tm->tm_mon++;
> +
> +	if (alm->enabled) {
> +		mutex_lock(&rtc->lock);
> +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> +				RTC_NEW_SPARE3) | tm->tm_mon);
This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register
name? I would have expected:

	(rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon;


> +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> +				RTC_NEW_SPARE1) | tm->tm_mday);
> +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
Is this racy? I.e. if the previous set alarm is

	2015-03-13 14:15:00

and you write

	2015-03.14 17:17:00

is it possible that this triggers an alarm if the update happens at

	2015-03-14 14:15:00

?

> +		rtc_write_trigger(rtc);
> +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> +		rtc_write_trigger(rtc);
> +		mutex_unlock(&rtc->lock);
	} else {
		/* disable alarm here */

> +	}
> +
> +	return 0;
> +}
> +
> +static struct rtc_class_ops mtk_rtc_ops = {
> +	.read_time  = mtk_rtc_read_time,
> +	.set_time   = mtk_rtc_set_time,
> +	.read_alarm = mtk_rtc_read_alarm,
> +	.set_alarm  = mtk_rtc_set_alarm,
> +};
> +
> +static int mtk_rtc_probe(struct platform_device *pdev)
> +{
> +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> +	struct mt6397_rtc *rtc;
> +	u32 reg[2];
> +	int ret = 0;
> +
> +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> +	if (!rtc)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> +	if (ret) {
> +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> +		return -EINVAL;
> +	}
> +	rtc->addr_base = reg[0];
> +	rtc->addr_range = reg[1];
This looks strange, but maybe that's right as you reuse the parent's
regmap.

> +	rtc->regmap = mt6397_chip->regmap;
> +	rtc->dev = &pdev->dev;
> +	mutex_init(&rtc->lock);
> +
> +	platform_set_drvdata(pdev, rtc);
> +
> +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> +				&mtk_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(rtc->rtc_dev)) {
> +		dev_err(&pdev->dev, "register rtc device failed\n");
> +		return PTR_ERR(rtc->rtc_dev);
> +	}
> +
> +	rtc->irq = platform_get_irq(pdev, 0);
> +	if (rtc->irq < 0) {
platform_get_irq(pdev, 0) = 0 should be treated as error, too.

> +		ret = rtc->irq;
> +		goto out_rtc;
> +	}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Uwe,

Thanks your review.

On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-K?nig wrote:
> Hello Eddie,
> 
> On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > From: Tianping Fang <tianping.fang@mediatek.com>
> > 
> > Add Mediatek MT63xx RTC driver
> MT6397?

Yes, it is better to use MT6397

> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index f15cddf..8ac52d8 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART
> >  	   This driver can also be built as a module. If so, the module
> >  	   will be called rtc-moxart
> >  
> > +config RTC_DRV_MT63XX
> > +	tristate "Mediatek Real Time Clock driver"
> > +	depends on MFD_MT6397
> I suggest:
> 
> 	depends on MFD_MT6397 || COMPILE_TEST
> 
> (maybe + any hard dependencies you need for compilation).

OK, will fix it

> 
> > +	help
> > +	  This selects the Mediatek(R) RTC driver, you should add support
> > +	  for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver.
> > +
> > +	  If you want to use Mediatek(R) RTC interface, select Y or M here.
> > +	  If unsure, Please select N.
> Given the dependency above I'd say choosing y here is fine. Instead of
> recommending that I'd just drop this line.

ok, will drop.

> 
> > [...]
> > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> rtc_read is a bad name for a driver. There are already 6 functions with
> this name in the kernel. Better use a unique prefix.

I will use prefix mtk_

> 
> > [...]
> > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > +{
> > +	struct mt6397_rtc *rtc = data;
> > +	u16 irqsta, irqen;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> Do you really need to lock for a single read access?

I think this lock is necessary, because other thread may access rtc
register at the same time, for example, call mtk_rtc_set_alarm to modify
alarm time.

> 
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	if (irqsta & RTC_IRQ_STA_AL) {
> > +		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> > +		irqen = irqsta & ~RTC_IRQ_EN_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	unsigned long time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	mutex_lock(&rtc->lock);
> > +	do {
> > +		tm->tm_sec = rtc_read(rtc, RTC_TC_SEC);
> > +		tm->tm_min = rtc_read(rtc, RTC_TC_MIN);
> > +		tm->tm_hour = rtc_read(rtc, RTC_TC_HOU);
> > +		tm->tm_mday = rtc_read(rtc, RTC_TC_DOM);
> > +		tm->tm_mon = rtc_read(rtc, RTC_TC_MTH);
> > +		tm->tm_year = rtc_read(rtc, RTC_TC_YEA);
> > +	} while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +	rtc_tm_to_time(tm, &time);
> rtc_tm_to_time is deprecated, better use rtc_tm_to_time64.

OK, will change to rtc_tm_to_time64

> 
> > +	tm->tm_wday = (time / 86400 + 4) % 7;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +	mutex_lock(&rtc->lock);
> > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> write to it but after you wrote RTC_TC_MIN?

register value will write to hardware after rtc_write_trigger, so the
racy condition not exist.

> 
> > +	rtc_write_trigger(rtc);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen, pdn2;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> > +	pdn2 = rtc_read(rtc, RTC_PDN2);
> > +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> > +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> > +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> > +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> > +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> > +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> > +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen;
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +
> > +	if (alm->enabled) {
> > +		mutex_lock(&rtc->lock);
> > +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> > +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> > +				RTC_NEW_SPARE3) | tm->tm_mon);
> This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register
> name? I would have expected:
> 
> 	(rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon;

I will remove RTC_NEW_SPARE3. Hardware will return 0 if register bit is
useless. So the bit clear is redundant. 

> 
> > +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> > +				RTC_NEW_SPARE1) | tm->tm_mday);
> > +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> > +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> > +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> > +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> > +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> Is this racy? I.e. if the previous set alarm is
> 
> 	2015-03-13 14:15:00
> 
> and you write
> 
> 	2015-03.14 17:17:00
> 
> is it possible that this triggers an alarm if the update happens at
> 
> 	2015-03-14 14:15:00
> 
> ?
> 
All rtc register value write to RTC hardware after rtc_write_trigger.
Race condition should not happen. 


> > +		rtc_write_trigger(rtc);
> > +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		mutex_unlock(&rtc->lock);
> 	} else {
> 		/* disable alarm here */
> 
OK, should clear RTC_IRQ_EN

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rtc_class_ops mtk_rtc_ops = {
> > +	.read_time  = mtk_rtc_read_time,
> > +	.set_time   = mtk_rtc_set_time,
> > +	.read_alarm = mtk_rtc_read_alarm,
> > +	.set_alarm  = mtk_rtc_set_alarm,
> > +};
> > +
> > +static int mtk_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +	struct mt6397_rtc *rtc;
> > +	u32 reg[2];
> > +	int ret = 0;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> > +		return -EINVAL;
> > +	}
> > +	rtc->addr_base = reg[0];
> > +	rtc->addr_range = reg[1];
> This looks strange, but maybe that's right as you reuse the parent's
> regmap.

According Sascha and Mark Brown's discussion:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323239.html

Address and interrupt will move from device tree to mfd_cell in
mt6397-core.c

> 
> > +	rtc->regmap = mt6397_chip->regmap;
> > +	rtc->dev = &pdev->dev;
> > +	mutex_init(&rtc->lock);
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > +				&mtk_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc->rtc_dev)) {
> > +		dev_err(&pdev->dev, "register rtc device failed\n");
> > +		return PTR_ERR(rtc->rtc_dev);
> > +	}
> > +
> > +	rtc->irq = platform_get_irq(pdev, 0);
> > +	if (rtc->irq < 0) {
> platform_get_irq(pdev, 0) = 0 should be treated as error, too.

OK, will fix it

> 
> > +		ret = rtc->irq;
> > +		goto out_rtc;
> > +	}
> 
> Best regards
> Uwe
> 

[PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-K?nig wrote:
> > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > [...]
> > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > rtc_read is a bad name for a driver. There are already 6 functions with
> > this name in the kernel. Better use a unique prefix.
> 
> I will use prefix mtk_
I would prefer a prefix that is unique to the driver. "mtk_" doesn't
work to distinguish between the rtc and a (say) spi driver. What you
want here is that if someone reports a bug on any mailinglist with a
backtrace you are able to immediately see which driver is affected.

> > > [...]
> > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > +{
> > > +	struct mt6397_rtc *rtc = data;
> > > +	u16 irqsta, irqen;
> > > +
> > > +	mutex_lock(&rtc->lock);
> > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > Do you really need to lock for a single read access?
> 
> I think this lock is necessary, because other thread may access rtc
> register at the same time, for example, call mtk_rtc_set_alarm to modify
> alarm time.
That would be a valid reason if mtk_rtc_set_alarm touched that register
twice in a single critical section and the handler must not read the
value of the first write. Otherwise it should be fine, shouldn't it?

> > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > > +
> > > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > > +	tm->tm_mon++;
> > > +	mutex_lock(&rtc->lock);
> > > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> > write to it but after you wrote RTC_TC_MIN?
> 
> register value will write to hardware after rtc_write_trigger, so the
> racy condition not exist.
Ah, it seems the hardware guys did their job. Nice.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Uwe,

On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-K?nig wrote:
> Hello Eddie,
> 
> On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-K?nig wrote:
> > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > > [...]
> > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > > rtc_read is a bad name for a driver. There are already 6 functions with
> > > this name in the kernel. Better use a unique prefix.
> > 
> > I will use prefix mtk_
> I would prefer a prefix that is unique to the driver. "mtk_" doesn't
> work to distinguish between the rtc and a (say) spi driver. What you
> want here is that if someone reports a bug on any mailinglist with a
> backtrace you are able to immediately see which driver is affected.
> 

My meaning is mtk_rtc_read, mtk_rtc_write.

> > > > [...]
> > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > > +{
> > > > +	struct mt6397_rtc *rtc = data;
> > > > +	u16 irqsta, irqen;
> > > > +
> > > > +	mutex_lock(&rtc->lock);
> > > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > > Do you really need to lock for a single read access?
> > 
> > I think this lock is necessary, because other thread may access rtc
> > register at the same time, for example, call mtk_rtc_set_alarm to modify
> > alarm time.
> That would be a valid reason if mtk_rtc_set_alarm touched that register
> twice in a single critical section and the handler must not read the
> value of the first write. Otherwise it should be fine, shouldn't it?
> 

My original though is if disable alarm in mtk_rtc_set_alarm function,
RTC_IRQ_STA may be affected, this is why I add mutex. After checking
with designer, RTC_IRQ_STA will not be affected. I will remove the
mutex.

> > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > > > +
> > > > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > > > +	tm->tm_mon++;
> > > > +	mutex_lock(&rtc->lock);
> > > > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > > > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > > > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > > > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > > > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > > > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> > > write to it but after you wrote RTC_TC_MIN?
> > 
> > register value will write to hardware after rtc_write_trigger, so the
> > racy condition not exist.
> Ah, it seems the hardware guys did their job. Nice.
> 
> Best regards
> Uwe
> 

[PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Wed, Mar 18, 2015 at 11:27:40AM +0800, Eddie Huang wrote:
> On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-K?nig wrote:
> > On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> > > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-K?nig wrote:
> > > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > > > [...]
> > > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > > > +{
> > > > > +	struct mt6397_rtc *rtc = data;
> > > > > +	u16 irqsta, irqen;
> > > > > +
> > > > > +	mutex_lock(&rtc->lock);
> > > > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > > > Do you really need to lock for a single read access?
> > > 
> > > I think this lock is necessary, because other thread may access rtc
> > > register at the same time, for example, call mtk_rtc_set_alarm to modify
> > > alarm time.
> > That would be a valid reason if mtk_rtc_set_alarm touched that register
> > twice in a single critical section and the handler must not read the
> > value of the first write. Otherwise it should be fine, shouldn't it?
> > 
> 
> My original though is if disable alarm in mtk_rtc_set_alarm function,
> RTC_IRQ_STA may be affected, this is why I add mutex. After checking
> with designer, RTC_IRQ_STA will not be affected. I will remove the
> mutex.
Even if IRQ_STA is changed there is no problem. With the lock the
following can happen:

Either the irq thread comes first:

	thread1			thread2

	lock
	rtc_read
	unlock   ------------>  lock
				dosomething with IRQ_STA
				unlock

or the other thread comes first:

	thread1			thread2

				lock
				dosomething with IRQ_STA
	lock   <-------------	unlock
	rtc_read
	unlock

So thread1 either reads the old or the new value of IRQ_STA.

Without locking you have the same! Either the write in thread2 comes
first or not. And depending on that you either read the new or the old
value respectively.

Of course this assumes that a write is atomic in the sense that if you
update a value from 0x01 to 0x10 there is no point in time a reader can
see 0x00 or 0x11. But this is usually given. Also reading the register
in question must not have side effects that affect the other threads.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |