[PATCH v2 0/3] Add Mediatek SoC RTC driver

[PATCH v2 0/3] Add Mediatek SoC 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 4.0-rc1, and Sascha's PMIC mfd patch[2]. Test ok 
on MT8173 platform.

[1] https://lkml.org/lkml/2015/1/23/325
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/324985.html

Change in v2:
1. Move RTC address and interrupt to mt6397-core.c, and register 
   these resource in mfd_cell.
   http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323239.html
2. Remove dt-binding document due to register resouce in mfd_cell, not from 
   device tree.
3. Update MAINTAINER file to add Mediatek RTC mainainter.
4. Add prefix mtk_ to some internal functions.
5. Fix racy condition
6. Check return value of regmap_read and regmap_write
7. Remove some unnecessary register readback, clear, then write.
8. Add disable alarm in mtk_rtc_set_alarm function

v1:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/320424.html

Eddie Huang (2):
  mfd: provide RTC resource in MT6397 MFD
  MAINTAINERS: add Mediatek RTC driver

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

 MAINTAINERS               |   7 +
 drivers/mfd/mt6397-core.c |  15 ++
 drivers/rtc/Kconfig       |  10 +
 drivers/rtc/Makefile      |   1 +
 drivers/rtc/rtc-mt6397.c  | 454 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 487 insertions(+)
 create mode 100644 drivers/rtc/rtc-mt6397.c

-- 
1.8.1.1.dirty

[PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD

[Eddie Huang]

Provide MT6397 RTC interrupt, base address, and register in
MT6397 MFD.

Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 drivers/mfd/mt6397-core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index b61c4eb..ef431d8 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -21,9 +21,24 @@
 #include <linux/mfd/mt6397/core.h>
 #include <linux/mfd/mt6397/registers.h>
 
+static const struct resource mt6397_rtc_resources[] = {
+	{
+		.start = 0xe000,
+		.end   = 0xe03e,
+		.flags  = IORESOURCE_MEM,
+	},
+	{
+		.start = MT6397_IRQ_RTC,
+		.end   = MT6397_IRQ_RTC,
+		.flags  = IORESOURCE_IRQ,
+	},
+};
+
 static const struct mfd_cell mt6397_devs[] = {
 	{
 		.name = "mt6397-rtc",
+		.num_resources = ARRAY_SIZE(mt6397_rtc_resources),
+		.resources = mt6397_rtc_resources,
 		.of_compatible = "mediatek,mt6397-rtc",
 	}, {
 		.name = "mt6397-regulator",
-- 
1.8.1.1.dirty

[PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Eddie Huang]

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

Add Mediatek MT6397 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 | 454 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 465 insertions(+)
 create mode 100644 drivers/rtc/rtc-mt6397.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index b5b5c3d..dcb94af 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1510,6 +1510,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_MT6397
+	tristate "Mediatek Real Time Clock driver"
+	depends on MFD_MT6397 || COMPILE_TEST
+	help
+	  This selects the Mediatek(R) RTC driver. RTC is part of Mediatek
+	  MT6397 PMIC. You should enable MT6397 PMIC MFD before select
+	  Mediatek(R) RTC driver.
+
+	  If you want to use Mediatek(R) RTC interface, select Y or M here.
+
 config RTC_DRV_XGENE
 	tristate "APM X-Gene RTC"
 	depends on HAS_IOMEM
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 69c8706..85c8bea 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -153,3 +153,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_MT6397)	+= rtc-mt6397.o
diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
new file mode 100644
index 0000000..130480d
--- /dev/null
+++ b/drivers/rtc/rtc-mt6397.c
@@ -0,0 +1,454 @@
+/*
+* 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/of_irq.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_IRQ_STA_AL		(1 << 0)
+#define RTC_IRQ_STA_LP		(1 << 3)
+
+#define RTC_AL_MASK		0x0008
+#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_AL_HOU_MASK		0x001f
+
+#define RTC_AL_DOM		0x001e
+#define RTC_AL_DOM_MASK		0x001f
+
+#define RTC_AL_MTH		0x0022
+#define RTC_AL_MTH_MASK		0x000f
+
+#define RTC_AL_YEA		0x0024
+#define RTC_AL_YEA_MASK		0x007f
+
+#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 int mtk_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;
+}
+
+static int mtk_rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
+{
+	u32 addr;
+
+	addr = rtc->addr_base + offset;
+
+	if (offset < rtc->addr_range)
+		return regmap_write(rtc->regmap, addr, data);
+
+	return -EINVAL;
+}
+
+static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
+{
+	int ret;
+	u32 data;
+
+	ret = mtk_rtc_write(rtc, RTC_WRTGR, 1);
+	if (ret < 0)
+		goto exit;
+
+	ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
+	if (ret < 0)
+		goto exit;
+
+	while (data & RTC_BBPU_CBUSY) {
+		cpu_relax();
+		ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
+		if (ret < 0)
+			goto exit;
+	}
+
+exit:
+	return ret;
+}
+
+static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
+{
+	struct mt6397_rtc *rtc = data;
+	u32 irqsta, irqen;
+	int ret;
+
+	ret = mtk_rtc_read(rtc, RTC_IRQ_STA, &irqsta);
+
+	if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
+		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
+		irqen = irqsta & ~RTC_IRQ_EN_AL;
+		mutex_lock(&rtc->lock);
+		if (mtk_rtc_write(rtc, RTC_IRQ_EN, irqen) < 0)
+			mtk_rtc_write_trigger(rtc);
+		mutex_unlock(&rtc->lock);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int __mtk_rtc_read_time(struct mt6397_rtc *rtc,
+				struct rtc_time *tm, int *sec)
+{
+	int ret;
+
+	mutex_lock(&rtc->lock);
+	ret = mtk_rtc_read(rtc, RTC_TC_SEC, &tm->tm_sec);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_read(rtc, RTC_TC_MIN, &tm->tm_min);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_read(rtc, RTC_TC_HOU, &tm->tm_hour);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_read(rtc, RTC_TC_DOM, &tm->tm_mday);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_read(rtc, RTC_TC_MTH, &tm->tm_mon);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_read(rtc, RTC_TC_YEA, &tm->tm_year);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_read(rtc, RTC_TC_SEC, sec);
+
+exit:
+	mutex_unlock(&rtc->lock);
+	return ret;
+}
+
+static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	time64_t time;
+	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
+	int sec, ret;
+
+	do {
+		ret = __mtk_rtc_read_time(rtc, tm, &sec);
+		if (ret < 0)
+			goto exit;
+	} while (sec < tm->tm_sec);
+
+	tm->tm_year += RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon--;
+	time = rtc_tm_to_time64(tm);
+
+	tm->tm_wday = (time / 86400 + 4) % 7;
+
+exit:
+	return ret;
+}
+
+static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
+	int ret;
+
+	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon++;
+
+	mutex_lock(&rtc->lock);
+	ret = mtk_rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
+	if (ret < 0)
+		goto exit;
+	ret = mtk_rtc_write_trigger(rtc);
+
+exit:
+	mutex_unlock(&rtc->lock);
+	return ret;
+}
+
+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);
+	u32 irqen, pdn2;
+	int ret;
+
+	mutex_lock(&rtc->lock);
+	ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_PDN2, &pdn2);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_AL_SEC, &tm->tm_sec);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_AL_MIN, &tm->tm_min);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_AL_HOU, &tm->tm_hour);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_AL_DOM, &tm->tm_mday);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_AL_MTH, &tm->tm_mon);
+	if (ret < 0)
+		goto err_exit;
+	ret = mtk_rtc_read(rtc, RTC_AL_YEA, &tm->tm_year);
+	if (ret < 0)
+		goto err_exit;
+
+	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
+	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
+	mutex_unlock(&rtc->lock);
+
+	tm->tm_year += RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon--;
+
+	return 0;
+err_exit:
+	mutex_unlock(&rtc->lock);
+	return ret;
+}
+
+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);
+	u32 irqen;
+	int ret;
+
+	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
+	tm->tm_mon++;
+
+	mutex_lock(&rtc->lock);
+	if (alm->enabled) {
+		ret = mtk_rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write(rtc, RTC_AL_MTH, tm->tm_mon);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write(rtc, RTC_AL_DOM, tm->tm_mday);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write(rtc, RTC_AL_HOU, tm->tm_hour);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write_trigger(rtc);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
+		if (ret < 0)
+			goto exit;
+		irqen |= RTC_IRQ_EN_ONESHOT_AL;
+		ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen);
+		if (ret < 0)
+			goto exit;
+		ret = mtk_rtc_write_trigger(rtc);
+		if (ret < 0)
+			goto exit;
+	} else {
+		ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
+		if (ret < 0)
+			goto exit;
+		irqen &= ~RTC_IRQ_EN_ONESHOT_AL;
+		ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen);
+		if (ret < 0)
+			goto exit;
+	}
+
+exit:
+	mutex_unlock(&rtc->lock);
+	return ret;
+}
+
+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 resource *res;
+	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
+	struct mt6397_rtc *rtc;
+	int ret = 0;
+
+	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
+	if (!rtc)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	rtc->addr_base = res->start;
+	rtc->addr_range = res->end - res->start;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
+	if (rtc->irq <= 0)
+		goto out_rtc;
+
+	rtc->regmap = mt6397_chip->regmap;
+	rtc->dev = &pdev->dev;
+	mutex_init(&rtc->lock);
+
+	platform_set_drvdata(pdev, rtc);
+
+	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
+			mtk_rtc_irq_handler_thread,
+			IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+			"mt6397-rtc", rtc);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
+			rtc->irq, ret);
+		goto out_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);
+	}
+
+	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

[PATCH v2 3/3] MAINTAINERS: add Mediatek RTC driver

[Eddie Huang]

Add Mediatek RTC driver to maintainer entry.

Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ddc5a8c..ca27a69 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1202,6 +1202,13 @@ W:	http://www.digriz.org.uk/ts78xx/kernel
 S:	Maintained
 F:	arch/arm/mach-orion5x/ts78xx-*
 
+ARM/Mediatek RTC DRIVER
+M:	Eddie Huang <eddie.huang@mediatek.com>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+L:	linux-mediatek@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	drivers/rtc/rtc-mt*
+
 ARM/Mediatek SoC support
 M:	Matthias Brugger <matthias.bgg@gmail.com>
 L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
-- 
1.8.1.1.dirty

Re: [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD

[Lee Jones]

On Wed, 18 Mar 2015, Eddie Huang wrote:

> Provide MT6397 RTC interrupt, base address, and register in
> MT6397 MFD.
> 
> Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> ---
>  drivers/mfd/mt6397-core.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index b61c4eb..ef431d8 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -21,9 +21,24 @@
>  #include <linux/mfd/mt6397/core.h>
>  #include <linux/mfd/mt6397/registers.h>
>  
> +static const struct resource mt6397_rtc_resources[] = {
> +	{
> +		.start = 0xe000,
> +		.end   = 0xe03e,

I'd like to see these defined please.

> +		.flags  = IORESOURCE_MEM,
> +	},
> +	{
> +		.start = MT6397_IRQ_RTC,
> +		.end   = MT6397_IRQ_RTC,
> +		.flags  = IORESOURCE_IRQ,
> +	},
> +};
> +
>  static const struct mfd_cell mt6397_devs[] = {
>  	{
>  		.name = "mt6397-rtc",
> +		.num_resources = ARRAY_SIZE(mt6397_rtc_resources),
> +		.resources = mt6397_rtc_resources,
>  		.of_compatible = "mediatek,mt6397-rtc",
>  	}, {
>  		.name = "mt6397-regulator",

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Dmitry Torokhov]

Hi Eddie,


On Tue, Mar 17, 2015 at 10:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> +static int mtk_rtc_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> +       struct mt6397_rtc *rtc;
> +       int ret = 0;
> +
> +       rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> +       if (!rtc)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       rtc->addr_base = res->start;
> +       rtc->addr_range = res->end - res->start;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
> +       if (rtc->irq <= 0)
> +               goto out_rtc;
> +
> +       rtc->regmap = mt6397_chip->regmap;
> +       rtc->dev = &pdev->dev;
> +       mutex_init(&rtc->lock);
> +
> +       platform_set_drvdata(pdev, rtc);
> +
> +       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +                       mtk_rtc_irq_handler_thread,
> +                       IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> +                       "mt6397-rtc", rtc);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> +                       rtc->irq, ret);
> +               goto out_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);
> +       }
> +
> +       device_init_wakeup(&pdev->dev, 1);
> +
> +       return 0;
> +
> +out_rtc:
> +       rtc_device_unregister(rtc->rtc_dev);

This is wrong. Whenever you jump to this label the RTC device has not
been registered yet.

> +       return ret;
> +
> +}
> +

Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Tomasz Figa]

Hi Eddie,

Please see my comments inline.

On Wed, Mar 18, 2015 at 2:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> From: Tianping Fang <tianping.fang@mediatek.com>
>
> Add Mediatek MT6397 RTC driver

[snip]

> +#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_IRQ_STA_AL         (1 << 0)
> +#define RTC_IRQ_STA_LP         (1 << 3)

nit: Could you use BIT() macro for definitions of single bits? (+
further occurrences in the patch)

> +
> +#define RTC_AL_MASK            0x0008
> +#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

[snip]

> +
> +static int mtk_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;
> +}
> +
> +static int mtk_rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> +{
> +       u32 addr;
> +
> +       addr = rtc->addr_base + offset;
> +
> +       if (offset < rtc->addr_range)
> +               return regmap_write(rtc->regmap, addr, data);
> +
> +       return -EINVAL;
> +}

Do you actually need these wrappers? Could you use regmap_write() and
_read() directly? This would also enable you to use
regmap_update_bits() instead of implicit read, modify and write.

> +
> +static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> +{
> +       int ret;
> +       u32 data;
> +
> +       ret = mtk_rtc_write(rtc, RTC_WRTGR, 1);
> +       if (ret < 0)
> +               goto exit;
> +
> +       ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
> +       if (ret < 0)
> +               goto exit;
> +
> +       while (data & RTC_BBPU_CBUSY) {
> +               cpu_relax();
> +               ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
> +               if (ret < 0)
> +                       goto exit;
> +       }

The initial read and the loop could be folded into a do {} while loop?
Also it would be safer to have a timeout here.

> +
> +exit:
> +       return ret;
> +}
> +
> +static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
> +{
> +       struct mt6397_rtc *rtc = data;
> +       u32 irqsta, irqen;
> +       int ret;
> +
> +       ret = mtk_rtc_read(rtc, RTC_IRQ_STA, &irqsta);
> +
> +       if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> +               rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> +               irqen = irqsta & ~RTC_IRQ_EN_AL;
> +               mutex_lock(&rtc->lock);
> +               if (mtk_rtc_write(rtc, RTC_IRQ_EN, irqen) < 0)
> +                       mtk_rtc_write_trigger(rtc);
> +               mutex_unlock(&rtc->lock);
> +
> +               return IRQ_HANDLED;
> +       }
> +
> +       return IRQ_NONE;
> +}
> +
> +static int __mtk_rtc_read_time(struct mt6397_rtc *rtc,
> +                               struct rtc_time *tm, int *sec)
> +{
> +       int ret;
> +
> +       mutex_lock(&rtc->lock);
> +       ret = mtk_rtc_read(rtc, RTC_TC_SEC, &tm->tm_sec);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_read(rtc, RTC_TC_MIN, &tm->tm_min);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_read(rtc, RTC_TC_HOU, &tm->tm_hour);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_read(rtc, RTC_TC_DOM, &tm->tm_mday);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_read(rtc, RTC_TC_MTH, &tm->tm_mon);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_read(rtc, RTC_TC_YEA, &tm->tm_year);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_read(rtc, RTC_TC_SEC, sec);

Would the hardware allow this to be merged into single burst transfer
reading all the registers into a buffer, so then you could just copy
the values from that buffer into target struct instead of issuing
multiple reads one by one?

Also shouldn't the unused bits be masked out?

> +
> +exit:
> +       mutex_unlock(&rtc->lock);
> +       return ret;
> +}
> +
> +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +       time64_t time;
> +       struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +       int sec, ret;
> +
> +       do {
> +               ret = __mtk_rtc_read_time(rtc, tm, &sec);
> +               if (ret < 0)
> +                       goto exit;
> +       } while (sec < tm->tm_sec);

Shouldn't this be while (sec > tm->tm_sec)?

> +
> +       tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +       tm->tm_mon--;

Could you add a comment explaining why this is decremented?

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

Could you add a comment, or even better, an inline function with a
comment, explaining this calculation?

> +
> +exit:
> +       return ret;
> +}
> +
> +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +       struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> +       int ret;
> +
> +       tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +       tm->tm_mon++;
> +
> +       mutex_lock(&rtc->lock);
> +       ret = mtk_rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> +       if (ret < 0)
> +               goto exit;
> +       ret = mtk_rtc_write_trigger(rtc);
> +
> +exit:
> +       mutex_unlock(&rtc->lock);
> +       return ret;
> +}
> +
> +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);
> +       u32 irqen, pdn2;
> +       int ret;
> +
> +       mutex_lock(&rtc->lock);
> +       ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_PDN2, &pdn2);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_AL_SEC, &tm->tm_sec);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_AL_MIN, &tm->tm_min);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_AL_HOU, &tm->tm_hour);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_AL_DOM, &tm->tm_mday);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_AL_MTH, &tm->tm_mon);
> +       if (ret < 0)
> +               goto err_exit;
> +       ret = mtk_rtc_read(rtc, RTC_AL_YEA, &tm->tm_year);
> +       if (ret < 0)
> +               goto err_exit;

Similarly to _read_time(), could this be changed into a single burst read?

> +
> +       alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> +       alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> +       mutex_unlock(&rtc->lock);
> +
> +       tm->tm_year += RTC_MIN_YEAR_OFFSET;
> +       tm->tm_mon--;
> +
> +       return 0;
> +err_exit:
> +       mutex_unlock(&rtc->lock);
> +       return ret;
> +}
> +
> +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);
> +       u32 irqen;
> +       int ret;
> +
> +       tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> +       tm->tm_mon++;
> +
> +       mutex_lock(&rtc->lock);
> +       if (alm->enabled) {

Is this possible that an alarm was already set? Is it okay to keep it
enabled while changing the alarm time to new one?

> +               ret = mtk_rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write(rtc, RTC_AL_MTH, tm->tm_mon);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write(rtc, RTC_AL_DOM, tm->tm_mday);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write(rtc, RTC_AL_HOU, tm->tm_hour);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_write_trigger(rtc);
> +               if (ret < 0)
> +                       goto exit;
> +               ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
> +               if (ret < 0)
> +                       goto exit;
> +               irqen |= RTC_IRQ_EN_ONESHOT_AL;
> +               ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen);
> +               if (ret < 0)
> +                       goto exit;

regmap_update_bits() could be used instead of the read, modify and write above.

> +               ret = mtk_rtc_write_trigger(rtc);
> +               if (ret < 0)
> +                       goto exit;
> +       } else {
> +               ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
> +               if (ret < 0)
> +                       goto exit;
> +               irqen &= ~RTC_IRQ_EN_ONESHOT_AL;
> +               ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen);
> +               if (ret < 0)
> +                       goto exit;

Ditto.

> +       }
> +
> +exit:
> +       mutex_unlock(&rtc->lock);
> +       return ret;
> +}
> +
> +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 resource *res;
> +       struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> +       struct mt6397_rtc *rtc;
> +       int ret = 0;
> +
> +       rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> +       if (!rtc)
> +               return -ENOMEM;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       rtc->addr_base = res->start;
> +       rtc->addr_range = res->end - res->start;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +       rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
> +       if (rtc->irq <= 0)
> +               goto out_rtc;

Just return an error code here directly. Which one is actually a good
question. Looks like existing code is using -EINVAL or -ENXIO. Any
ideas?

> +
> +       rtc->regmap = mt6397_chip->regmap;
> +       rtc->dev = &pdev->dev;q
> +       mutex_init(&rtc->lock);
> +
> +       platform_set_drvdata(pdev, rtc);
> +
> +       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> +                       mtk_rtc_irq_handler_thread,
> +                       IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> +                       "mt6397-rtc", rtc);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> +                       rtc->irq, ret);
> +               goto out_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);
> +       }
> +
> +       device_init_wakeup(&pdev->dev, 1);
> +
> +       return 0;
> +
> +out_rtc:
> +       rtc_device_unregister(rtc->rtc_dev);

All references to this label are actually before rtc_device_register()
is even called. The proper thing to do here is to dispose the created
IRQ mapping.

> +       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);

What about the IRQ mapping created in probe?

Best regards,
Tomasz

Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Eddie Huang]

Hi Dmitry,

On Fri, 2015-03-20 at 22:25 -0700, Dmitry Torokhov wrote:
> Hi Eddie,
> 
> 
> On Tue, Mar 17, 2015 at 10:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> > +static int mtk_rtc_probe(struct platform_device *pdev)
> > +{
> > +       struct resource *res;
> > +       struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +       struct mt6397_rtc *rtc;
> > +       int ret = 0;
> > +
> > +       rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +       if (!rtc)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       rtc->addr_base = res->start;
> > +       rtc->addr_range = res->end - res->start;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
> > +       if (rtc->irq <= 0)
> > +               goto out_rtc;
> > +
> > +       rtc->regmap = mt6397_chip->regmap;
> > +       rtc->dev = &pdev->dev;
> > +       mutex_init(&rtc->lock);
> > +
> > +       platform_set_drvdata(pdev, rtc);
> > +
> > +       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> > +                       mtk_rtc_irq_handler_thread,
> > +                       IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > +                       "mt6397-rtc", rtc);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> > +                       rtc->irq, ret);
> > +               goto out_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);
> > +       }
> > +
> > +       device_init_wakeup(&pdev->dev, 1);
> > +
> > +       return 0;
> > +
> > +out_rtc:
> > +       rtc_device_unregister(rtc->rtc_dev);
> 
> This is wrong. Whenever you jump to this label the RTC device has not
> been registered yet.

Oops, will fix in next round.

Eddie

Re: [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD

[Eddie Huang]

Hi Lee,

On Wed, 2015-03-18 at 07:30 +0000, Lee Jones wrote:
> On Wed, 18 Mar 2015, Eddie Huang wrote:
> 
> > Provide MT6397 RTC interrupt, base address, and register in
> > MT6397 MFD.
> > 
> > Signed-off-by: Eddie Huang <eddie.huang@mediatek.com>
> > ---
> >  drivers/mfd/mt6397-core.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> > index b61c4eb..ef431d8 100644
> > --- a/drivers/mfd/mt6397-core.c
> > +++ b/drivers/mfd/mt6397-core.c
> > @@ -21,9 +21,24 @@
> >  #include <linux/mfd/mt6397/core.h>
> >  #include <linux/mfd/mt6397/registers.h>
> >  
> > +static const struct resource mt6397_rtc_resources[] = {
> > +	{
> > +		.start = 0xe000,
> > +		.end   = 0xe03e,
> 
> I'd like to see these defined please.
> 
Sure, will change to use define next round.

Eddie

Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Eddie Huang]

Hi Tomasz,

On Mon, 2015-03-30 at 16:41 +0900, Tomasz Figa wrote:
> Hi Eddie,
> 
> Please see my comments inline.
> 
> On Wed, Mar 18, 2015 at 2:45 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:
> > From: Tianping Fang <tianping.fang@mediatek.com>
> >
> > Add Mediatek MT6397 RTC driver
> 
> [snip]
> 
> > +#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_IRQ_STA_AL         (1 << 0)
> > +#define RTC_IRQ_STA_LP         (1 << 3)
> 
> nit: Could you use BIT() macro for definitions of single bits? (+
> further occurrences in the patch)
Will fix it.

> 
> [snip]
> 
> > +
> > +static int mtk_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;
> > +}
> > +
> > +static int mtk_rtc_write(struct mt6397_rtc *rtc, u32 offset, u32 data)
> > +{
> > +       u32 addr;
> > +
> > +       addr = rtc->addr_base + offset;
> > +
> > +       if (offset < rtc->addr_range)
> > +               return regmap_write(rtc->regmap, addr, data);
> > +
> > +       return -EINVAL;
> > +}
> 
> Do you actually need these wrappers? Could you use regmap_write() and
> _read() directly? This would also enable you to use
> regmap_update_bits() instead of implicit read, modify and write.
These wrappers used to check register range. But I think the check is
redundant, I will bypass the check and use regmap API directly.

> 
> > +
> > +static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> > +{
> > +       int ret;
> > +       u32 data;
> > +
> > +       ret = mtk_rtc_write(rtc, RTC_WRTGR, 1);
> > +       if (ret < 0)
> > +               goto exit;
> > +
> > +       ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
> > +       if (ret < 0)
> > +               goto exit;
> > +
> > +       while (data & RTC_BBPU_CBUSY) {
> > +               cpu_relax();
> > +               ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
> > +               if (ret < 0)
> > +                       goto exit;
> > +       }
> 
> The initial read and the loop could be folded into a do {} while loop?
> Also it would be safer to have a timeout here.
Because I need to check return value, so not put initial read in do { }.
Indeed, it is safer to add timeout here.


> > +
> > +static int __mtk_rtc_read_time(struct mt6397_rtc *rtc,
> > +                               struct rtc_time *tm, int *sec)
> > +{
> > +       int ret;
> > +
> > +       mutex_lock(&rtc->lock);
> > +       ret = mtk_rtc_read(rtc, RTC_TC_SEC, &tm->tm_sec);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_read(rtc, RTC_TC_MIN, &tm->tm_min);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_read(rtc, RTC_TC_HOU, &tm->tm_hour);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_read(rtc, RTC_TC_DOM, &tm->tm_mday);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_read(rtc, RTC_TC_MTH, &tm->tm_mon);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_read(rtc, RTC_TC_YEA, &tm->tm_year);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_read(rtc, RTC_TC_SEC, sec);
> 
> Would the hardware allow this to be merged into single burst transfer
> reading all the registers into a buffer, so then you could just copy
> the values from that buffer into target struct instead of issuing
> multiple reads one by one?
OK, Sascha already mentioned this before, I think I should change to use
single burst reading.

> 
> Also shouldn't the unused bits be masked out?
Hardware return zero in unused bits. So I think it not necessary to add
mask.

> 
> > +
> > +exit:
> > +       mutex_unlock(&rtc->lock);
> > +       return ret;
> > +}
> > +
> > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +       time64_t time;
> > +       struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +       int sec, ret;
> > +
> > +       do {
> > +               ret = __mtk_rtc_read_time(rtc, tm, &sec);
> > +               if (ret < 0)
> > +                       goto exit;
> > +       } while (sec < tm->tm_sec);
> 
> Shouldn't this be while (sec > tm->tm_sec)?
No, it should keep it as is, this is used to check whether second
overflow (from 59 to 0). If yes, read time again.

> 
> > +
> > +       tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +       tm->tm_mon--;
> 
> Could you add a comment explaining why this is decremented?
Year register only have 7bits, use RTC_MIN_YEAR_OFFSET to reduce bit
usage. Minus the offset before write to register and add back the offset
after read register back. And month register start from 1, but tm_mon
start from zero. I will add comment.

> 
> > +       time = rtc_tm_to_time64(tm);
> > +
> > +       tm->tm_wday = (time / 86400 + 4) % 7;
> 
> Could you add a comment, or even better, an inline function with a
> comment, explaining this calculation?
rtc_tm_to_time64 function return time base on 01-01-1970 00:00:00.
This base time is Thursday. I will add comment

> 
> > +
> > +exit:
> > +       return ret;
> > +}
> > +
> > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +       struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +       int ret;
> > +
> > +       tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +       tm->tm_mon++;
> > +
> > +       mutex_lock(&rtc->lock);
> > +       ret = mtk_rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > +       if (ret < 0)
> > +               goto exit;
> > +       ret = mtk_rtc_write_trigger(rtc);
> > +
> > +exit:
> > +       mutex_unlock(&rtc->lock);
> > +       return ret;
> > +}
> > +
> > +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);
> > +       u32 irqen, pdn2;
> > +       int ret;
> > +
> > +       mutex_lock(&rtc->lock);
> > +       ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_PDN2, &pdn2);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_AL_SEC, &tm->tm_sec);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_AL_MIN, &tm->tm_min);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_AL_HOU, &tm->tm_hour);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_AL_DOM, &tm->tm_mday);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_AL_MTH, &tm->tm_mon);
> > +       if (ret < 0)
> > +               goto err_exit;
> > +       ret = mtk_rtc_read(rtc, RTC_AL_YEA, &tm->tm_year);
> > +       if (ret < 0)
> > +               goto err_exit;
> 
> Similarly to _read_time(), could this be changed into a single burst read?
will change API.

> 
> > +
> > +       alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> > +       alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> > +       mutex_unlock(&rtc->lock);
> > +
> > +       tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +       tm->tm_mon--;
> > +
> > +       return 0;
> > +err_exit:
> > +       mutex_unlock(&rtc->lock);
> > +       return ret;
> > +}
> > +
> > +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);
> > +       u32 irqen;
> > +       int ret;
> > +
> > +       tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +       tm->tm_mon++;
> > +
> > +       mutex_lock(&rtc->lock);
> > +       if (alm->enabled) {
> 
> Is this possible that an alarm was already set? Is it okay to keep it
> enabled while changing the alarm time to new one?
It's ok because all alarm time register set to hardware after call
mtk_rtc_write_trigger.

> 
> > +               ret = mtk_rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write(rtc, RTC_AL_MTH, tm->tm_mon);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write(rtc, RTC_AL_DOM, tm->tm_mday);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write(rtc, RTC_AL_HOU, tm->tm_hour);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_write_trigger(rtc);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               irqen |= RTC_IRQ_EN_ONESHOT_AL;
> > +               ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +               if (ret < 0)
> > +                       goto exit;
> 
> regmap_update_bits() could be used instead of the read, modify and write above.
I will check how to use this api.
> 
> > +               ret = mtk_rtc_write_trigger(rtc);
> > +               if (ret < 0)
> > +                       goto exit;
> > +       } else {
> > +               ret = mtk_rtc_read(rtc, RTC_IRQ_EN, &irqen);
> > +               if (ret < 0)
> > +                       goto exit;
> > +               irqen &= ~RTC_IRQ_EN_ONESHOT_AL;
> > +               ret = mtk_rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +               if (ret < 0)
> > +                       goto exit;
> 
> Ditto.
> 
> > +       }
> > +
> > +exit:
> > +       mutex_unlock(&rtc->lock);
> > +       return ret;
> > +}
> > +
> > +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 resource *res;
> > +       struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +       struct mt6397_rtc *rtc;
> > +       int ret = 0;
> > +
> > +       rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +       if (!rtc)
> > +               return -ENOMEM;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       rtc->addr_base = res->start;
> > +       rtc->addr_range = res->end - res->start;
> > +
> > +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +       rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
> > +       if (rtc->irq <= 0)
> > +               goto out_rtc;
> 
> Just return an error code here directly. Which one is actually a good
> question. Looks like existing code is using -EINVAL or -ENXIO. Any
> ideas?
I tend to use -EINVAL
> 
> > +
> > +       rtc->regmap = mt6397_chip->regmap;
> > +       rtc->dev = &pdev->dev;q
> > +       mutex_init(&rtc->lock);
> > +
> > +       platform_set_drvdata(pdev, rtc);
> > +
> > +       ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
> > +                       mtk_rtc_irq_handler_thread,
> > +                       IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > +                       "mt6397-rtc", rtc);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> > +                       rtc->irq, ret);
> > +               goto out_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);
> > +       }
> > +
> > +       device_init_wakeup(&pdev->dev, 1);
> > +
> > +       return 0;
> > +
> > +out_rtc:
> > +       rtc_device_unregister(rtc->rtc_dev);
> 
> All references to this label are actually before rtc_device_register()
> is even called. The proper thing to do here is to dispose the created
> IRQ mapping.
OK, will call irq_dispose_mapping and free_irq

> 
> > +       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);
> 
> What about the IRQ mapping created in probe?
OK, will call irq_dispose_mapping and free_irq
> 

Eddie

Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Tomasz Figa]

Hi Eddie,

Please see my response inline.

On Tue, Mar 31, 2015 at 6:44 PM, Eddie Huang <eddie.huang@mediatek.com> wrote:

[snip]

>> > +       ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
>> > +       if (ret < 0)
>> > +               goto exit;
>> > +
>> > +       while (data & RTC_BBPU_CBUSY) {
>> > +               cpu_relax();
>> > +               ret = mtk_rtc_read(rtc, RTC_BBPU, &data);
>> > +               if (ret < 0)
>> > +                       goto exit;
>> > +       }
>>
>> The initial read and the loop could be folded into a do {} while loop?
>> Also it would be safer to have a timeout here.
> Because I need to check return value, so not put initial read in do { }.

Hmm, inside the loop you also check return value. Considering the fact
that cpu_relax() doesn't do anything interesting besides issuing a
memory barrier (and probably could be omitted here) I don't see why
this couldn't be made a do {} while loop. (Obviously this is a bit of
bikeshedding, but by the way of other changes this could be changed as
well.)

[snip]

>>
>> Also shouldn't the unused bits be masked out?
> Hardware return zero in unused bits. So I think it not necessary to add
> mask.
>

OK. Thanks for explaining this.

>>
>> > +
>> > +exit:
>> > +       mutex_unlock(&rtc->lock);
>> > +       return ret;
>> > +}
>> > +
>> > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> > +{
>> > +       time64_t time;
>> > +       struct mt6397_rtc *rtc = dev_get_drvdata(dev);
>> > +       int sec, ret;
>> > +
>> > +       do {
>> > +               ret = __mtk_rtc_read_time(rtc, tm, &sec);
>> > +               if (ret < 0)
>> > +                       goto exit;
>> > +       } while (sec < tm->tm_sec);
>>
>> Shouldn't this be while (sec > tm->tm_sec)?
> No, it should keep it as is, this is used to check whether second
> overflow (from 59 to 0). If yes, read time again.
>

Ah, right, of course, an overlooking on my side. Thanks for clarifying this.

[snip]

>> > +       mutex_lock(&rtc->lock);
>> > +       if (alm->enabled) {
>>
>> Is this possible that an alarm was already set? Is it okay to keep it
>> enabled while changing the alarm time to new one?
> It's ok because all alarm time register set to hardware after call
> mtk_rtc_write_trigger.
>

Fair enough. Thanks for explanation. Could you maybe add a comment
here saying that the new alarm setting will be committed after calling
mtk_rtc_write_trigger()?

[snip]

>> > +       res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> > +       rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
>> > +       if (rtc->irq <= 0)
>> > +               goto out_rtc;
>>
>> Just return an error code here directly. Which one is actually a good
>> question. Looks like existing code is using -EINVAL or -ENXIO. Any
>> ideas?
> I tend to use -EINVAL

SGTM.

[snip]

>> > +
>> > +out_rtc:
>> > +       rtc_device_unregister(rtc->rtc_dev);
>>
>> All references to this label are actually before rtc_device_register()
>> is even called. The proper thing to do here is to dispose the created
>> IRQ mapping.
> OK, will call irq_dispose_mapping and free_irq
>

OK, thanks. Please note that this will also mean changing
devm_request_threaded_irq() to normal request_threaded_irq().

Still, now as I think of it, I'm not sure if this driver is the right
place to call irq_create_mapping(). Instead, shouldn't the parent MFD
driver create the mapping and pass the final virtual IRQ number to
this driver through resources?

Lee, could you comment on this, please?

Best regards,
Tomasz

Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver

[Andrew Morton]

On Mon, 30 Mar 2015 16:15:50 +0800 Eddie Huang <eddie.huang@mediatek.com> wrote:

> > > +out_rtc:
> > > +       rtc_device_unregister(rtc->rtc_dev);
> > 
> > This is wrong. Whenever you jump to this label the RTC device has not
> > been registered yet.
> 
> Oops, will fix in next round.

Please ensure that Uwe's review comments (starting at
https://lkml.org/lkml/2015/3/17/494) have been addressed.