* [PATCH v2 0/3] Add Mediatek SoC RTC driver
@ 2015-03-18 5:45 Eddie Huang
2015-03-18 5:45 ` [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD Eddie Huang
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Eddie Huang @ 2015-03-18 5:45 UTC (permalink / raw)
To: Lee Jones, Alessandro Zummo, Matthias Brugger
Cc: srv_heupstream, Samuel Ortiz, Andrew Morton, David S. Miller,
Greg KH, Mauro Carvalho Chehab, Joe Perches, Tejun Heo,
Jingoo Han, linux-kernel, rtc-linux, linux-arm-kernel,
linux-mediatek, Uwe Kleine-König
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD
2015-03-18 5:45 [PATCH v2 0/3] Add Mediatek SoC RTC driver Eddie Huang
@ 2015-03-18 5:45 ` Eddie Huang
2015-03-18 7:30 ` Lee Jones
2015-03-18 5:45 ` [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver Eddie Huang
2015-03-18 5:45 ` [PATCH v2 3/3] MAINTAINERS: add Mediatek " Eddie Huang
2 siblings, 1 reply; 12+ messages in thread
From: Eddie Huang @ 2015-03-18 5:45 UTC (permalink / raw)
To: Lee Jones, Alessandro Zummo, Matthias Brugger
Cc: srv_heupstream, Samuel Ortiz, Andrew Morton, David S. Miller,
Greg KH, Mauro Carvalho Chehab, Joe Perches, Tejun Heo,
Jingoo Han, linux-kernel, rtc-linux, linux-arm-kernel,
linux-mediatek, Uwe Kleine-König, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-18 5:45 [PATCH v2 0/3] Add Mediatek SoC RTC driver Eddie Huang
2015-03-18 5:45 ` [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD Eddie Huang
@ 2015-03-18 5:45 ` Eddie Huang
2015-03-21 5:25 ` Dmitry Torokhov
2015-03-30 7:41 ` Tomasz Figa
2015-03-18 5:45 ` [PATCH v2 3/3] MAINTAINERS: add Mediatek " Eddie Huang
2 siblings, 2 replies; 12+ messages in thread
From: Eddie Huang @ 2015-03-18 5:45 UTC (permalink / raw)
To: Lee Jones, Alessandro Zummo, Matthias Brugger
Cc: srv_heupstream, Samuel Ortiz, Andrew Morton, David S. Miller,
Greg KH, Mauro Carvalho Chehab, Joe Perches, Tejun Heo,
Jingoo Han, linux-kernel, rtc-linux, linux-arm-kernel,
linux-mediatek, Uwe Kleine-König, Tianping Fang,
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 3/3] MAINTAINERS: add Mediatek RTC driver
2015-03-18 5:45 [PATCH v2 0/3] Add Mediatek SoC RTC driver Eddie Huang
2015-03-18 5:45 ` [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD Eddie Huang
2015-03-18 5:45 ` [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver Eddie Huang
@ 2015-03-18 5:45 ` Eddie Huang
2 siblings, 0 replies; 12+ messages in thread
From: Eddie Huang @ 2015-03-18 5:45 UTC (permalink / raw)
To: Lee Jones, Alessandro Zummo, Matthias Brugger
Cc: srv_heupstream, Samuel Ortiz, Andrew Morton, David S. Miller,
Greg KH, Mauro Carvalho Chehab, Joe Perches, Tejun Heo,
Jingoo Han, linux-kernel, rtc-linux, linux-arm-kernel,
linux-mediatek, Uwe Kleine-König, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD
2015-03-18 5:45 ` [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD Eddie Huang
@ 2015-03-18 7:30 ` Lee Jones
2015-03-30 8:17 ` Eddie Huang
0 siblings, 1 reply; 12+ messages in thread
From: Lee Jones @ 2015-03-18 7:30 UTC (permalink / raw)
To: Eddie Huang
Cc: Alessandro Zummo, Matthias Brugger, srv_heupstream, Samuel Ortiz,
Andrew Morton, David S. Miller, Greg KH, Mauro Carvalho Chehab,
Joe Perches, Tejun Heo, Jingoo Han, linux-kernel, rtc-linux,
linux-arm-kernel, linux-mediatek, Uwe Kleine-König
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-18 5:45 ` [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver Eddie Huang
@ 2015-03-21 5:25 ` Dmitry Torokhov
2015-03-30 8:15 ` Eddie Huang
2015-03-30 7:41 ` Tomasz Figa
1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Torokhov @ 2015-03-21 5:25 UTC (permalink / raw)
To: Eddie Huang
Cc: Lee Jones, Alessandro Zummo, Matthias Brugger, srv_heupstream,
Samuel Ortiz, Andrew Morton, David S. Miller, Greg KH,
Mauro Carvalho Chehab, Joe Perches, Tejun Heo, Jingoo Han, lkml,
rtc-linux, linux-arm-kernel, linux-mediatek,
Uwe Kleine-König, Tianping Fang
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;
> +
> +}
> +
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-18 5:45 ` [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver Eddie Huang
2015-03-21 5:25 ` Dmitry Torokhov
@ 2015-03-30 7:41 ` Tomasz Figa
2015-03-31 9:44 ` Eddie Huang
1 sibling, 1 reply; 12+ messages in thread
From: Tomasz Figa @ 2015-03-30 7:41 UTC (permalink / raw)
To: Eddie Huang
Cc: Lee Jones, Alessandro Zummo, Matthias Brugger, Samuel Ortiz,
srv_heupstream, Mauro Carvalho Chehab, rtc-linux, Greg KH,
Jingoo Han, linux-kernel, Tianping Fang, Tejun Heo,
linux-mediatek, Uwe Kleine-König, Joe Perches,
Andrew Morton, David S. Miller, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-21 5:25 ` Dmitry Torokhov
@ 2015-03-30 8:15 ` Eddie Huang
2015-03-31 22:01 ` Andrew Morton
0 siblings, 1 reply; 12+ messages in thread
From: Eddie Huang @ 2015-03-30 8:15 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Lee Jones, Alessandro Zummo, Matthias Brugger, srv_heupstream,
Samuel Ortiz, Andrew Morton, David S. Miller, Greg KH,
Mauro Carvalho Chehab, Joe Perches, Tejun Heo, Jingoo Han, lkml,
rtc-linux, linux-arm-kernel, linux-mediatek,
Uwe Kleine-König, Tianping Fang
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD
2015-03-18 7:30 ` Lee Jones
@ 2015-03-30 8:17 ` Eddie Huang
0 siblings, 0 replies; 12+ messages in thread
From: Eddie Huang @ 2015-03-30 8:17 UTC (permalink / raw)
To: Lee Jones
Cc: Alessandro Zummo, Matthias Brugger, srv_heupstream, Samuel Ortiz,
Andrew Morton, David S. Miller, Greg KH, Mauro Carvalho Chehab,
Joe Perches, Tejun Heo, Jingoo Han, linux-kernel, rtc-linux,
linux-arm-kernel, linux-mediatek, Uwe Kleine-König
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-30 7:41 ` Tomasz Figa
@ 2015-03-31 9:44 ` Eddie Huang
2015-03-31 11:01 ` Tomasz Figa
0 siblings, 1 reply; 12+ messages in thread
From: Eddie Huang @ 2015-03-31 9:44 UTC (permalink / raw)
To: Tomasz Figa
Cc: Lee Jones, Alessandro Zummo, Matthias Brugger, Samuel Ortiz,
srv_heupstream, Mauro Carvalho Chehab, rtc-linux, Greg KH,
Jingoo Han, linux-kernel, Tianping Fang, Tejun Heo,
linux-mediatek, Uwe Kleine-König, Joe Perches,
Andrew Morton, David S. Miller, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-31 9:44 ` Eddie Huang
@ 2015-03-31 11:01 ` Tomasz Figa
0 siblings, 0 replies; 12+ messages in thread
From: Tomasz Figa @ 2015-03-31 11:01 UTC (permalink / raw)
To: Eddie Huang, Lee Jones
Cc: Alessandro Zummo, Matthias Brugger, Samuel Ortiz, srv_heupstream,
Mauro Carvalho Chehab, rtc-linux, Greg KH, Jingoo Han,
linux-kernel, Tianping Fang, Tejun Heo, linux-mediatek,
Uwe Kleine-König, Joe Perches, Andrew Morton,
David S. Miller, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver
2015-03-30 8:15 ` Eddie Huang
@ 2015-03-31 22:01 ` Andrew Morton
0 siblings, 0 replies; 12+ messages in thread
From: Andrew Morton @ 2015-03-31 22:01 UTC (permalink / raw)
To: Eddie Huang
Cc: Dmitry Torokhov, Lee Jones, Alessandro Zummo, Matthias Brugger,
srv_heupstream, Samuel Ortiz, David S. Miller, Greg KH,
Mauro Carvalho Chehab, Joe Perches, Tejun Heo, Jingoo Han, lkml,
rtc-linux, linux-arm-kernel, linux-mediatek,
Uwe Kleine-König, Tianping Fang
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-03-31 22:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-18 5:45 [PATCH v2 0/3] Add Mediatek SoC RTC driver Eddie Huang
2015-03-18 5:45 ` [PATCH v2 1/3] mfd: provide RTC resource in MT6397 MFD Eddie Huang
2015-03-18 7:30 ` Lee Jones
2015-03-30 8:17 ` Eddie Huang
2015-03-18 5:45 ` [PATCH v2 2/3] rtc: mediatek: Add MT6397 RTC driver Eddie Huang
2015-03-21 5:25 ` Dmitry Torokhov
2015-03-30 8:15 ` Eddie Huang
2015-03-31 22:01 ` Andrew Morton
2015-03-30 7:41 ` Tomasz Figa
2015-03-31 9:44 ` Eddie Huang
2015-03-31 11:01 ` Tomasz Figa
2015-03-18 5:45 ` [PATCH v2 3/3] MAINTAINERS: add Mediatek " Eddie Huang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).