[PATCH v1 00/16] Add support to MT6323 RTC and its power device

[PATCH v1 00/16] Add support to MT6323 RTC and its power device

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Hi,

The series keeps to extend the capability of BPI-R2 board with MT7623 and
the result can as well bring benefits into the other MediaTek PMICs such
as MT6397 or SoCs.

The series sent across mfd, rtc, pm, dt-binding sub-system is for hoping
to let people have a simple cross-reference to know the exact dependency
and why those patches are being split in that way between each one.

Patch 1-3: Add dt-binding to the related devices newly or already
	   supported.
Patch 4-5: Extend driver with the functionality of MT6323 RTC device.
Patch 6-9 and 12-13: Add a few of trivial fixups, cleanups and
	  improvements.
Patch 10-11: It's a preparation for Patch 14 adding a new driver.
Patch 14: Add a new driver for a power-off driver.
Patch 15-16: Update MAINTAINERS with these new files being added.

	Sean

Sean Wang (16):
  dt-bindings: power: reset: mediatek: add bindings for power device
  dt-bindings: rtc: mediatek: add bindings for PMIC RTC
  dt-bindings: mfd: mediatek: add a description for MT6323 RTC
  mfd: mt6397: add MT6323 RTC support into MT6397 driver
  rtc: mediatek: add MT6323 support to RTC driver
  rtc: mediatek: remove unnecessary parentheses
  rtc: mediatek: replace a poll with regmap_read_poll_timeout
  rtc: mediatek: remove unnecessary irq_dispose_mapping
  rtc: mediatek: convert to use device managed functions
  rtc: mediatek: add devm_of_platform_populate
  rtc: mediatek: move the declaration into a globally visible header
    file
  rtc: mediatek: cleanup header files to include
  rtc: mediatek: update license converting to using SPDX identifiers
  power: reset: mediatek: add a power-off driver using PMIC RTC device
  MAINTAINERS: update entry for ARM/Mediatek RTC DRIVER
  MAINTAINERS: add an entry for MediaTek board level shutdown driver

 Documentation/devicetree/bindings/mfd/mt6397.txt   |   4 +-
 .../bindings/power/reset/mt6397-rtc-poweroff.txt   |  24 ++++
 .../devicetree/bindings/rtc/rtc-mt6397.txt         |  39 ++++++
 MAINTAINERS                                        |   9 ++
 drivers/mfd/mt6397-core.c                          |  23 +++-
 drivers/power/reset/Kconfig                        |   9 ++
 drivers/power/reset/Makefile                       |   1 +
 drivers/power/reset/mt6397-rtc-poweroff.c          | 100 ++++++++++++++
 drivers/rtc/rtc-mt6397.c                           | 145 +++++----------------
 include/linux/rtc/mt6397.h                         |  73 +++++++++++
 10 files changed, 314 insertions(+), 113 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
 create mode 100644 drivers/power/reset/mt6397-rtc-poweroff.c
 create mode 100644 include/linux/rtc/mt6397.h

-- 
2.7.4

[PATCH v1 01/16] dt-bindings: power: reset: mediatek: add bindings for power device

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add device-tree binding for the power device responsible for shutdown a
remote SoC, which is a tiny circuit block present on MediaTek PMIC based
RTC.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../bindings/power/reset/mt6397-rtc-poweroff.txt   | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt

diff --git a/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt b/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
new file mode 100644
index 0000000..75a9d7d
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
@@ -0,0 +1,24 @@
+Device-Tree bindings for Power Device on MediaTek PMIC RTC
+
+The power device is responsible for externally power off or on the remote
+MediaTek SoC through the a tiny circuit block BBPU inside PMIC RTC.
+
+Required parent node:
+- rtc
+	For MediaTek PMIC RTC bindings, see:
+	Documentation/devicetree/bindings/mfd/mt6397.txt
+
+Required properties:
+- compatible: Should be one of follows
+	"mediatek,mt6323-rtc-poweroff": for MT6323 PMIC
+	"mediatek,mt6397-rtc-poweroff": for MT6397 PMIC
+
+Example:
+
+	rtc {
+		compatible = "mediatek,mt6323-rtc";
+
+		power-off {
+			compatible = "mediatek,mt6323-rtc-poweroff";
+		};
+	};
-- 
2.7.4

[PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add device-tree binding for MediaTek PMIC based RTC.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt

diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
new file mode 100644
index 0000000..83ff6be
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
@@ -0,0 +1,39 @@
+Device-Tree bindings for MediaTek PMIC based RTC
+
+MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
+is working as a multi-function device (MFD). And the RTC can be configured and
+set up via PMIC wrapper bus. Which is also common resource shared among the
+other functions present on the PMIC.
+
+For MediaTek PMIC wrapper bus bindings, see:
+Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
+
+Required parent node:
+- pmic
+  For MediaTek PMIC MFD bindings, see:
+  Documentation/devicetree/bindings/mfd/mt6397.txt
+
+Required properties:
+- compatible: Should be one of follows
+	"mediatek,mt6323-rtc": for MT6323 PMIC
+	"mediatek,mt6397-rtc": for MT6397 PMIC
+
+Optional child node:
+- power-off
+  For Power-Off Device for MediaTek PMIC RTC bindings, see:
+  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
+
+Example:
+
+	pmic {
+		compatible = "mediatek,mt6323";
+
+		...
+		rtc {
+			compatible = "mediatek,mt6323-rtc";
+
+			power-off {
+				compatible = "mediatek,mt6323-rtc-poweroff";
+			};
+		};
+};
-- 
2.7.4

[PATCH v1 03/16] dt-bindings: mfd: mediatek: add a description for MT6323 RTC

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add a description for MT6323 RTC and link it to the detailed binding
documentation.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 Documentation/devicetree/bindings/mfd/mt6397.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mfd/mt6397.txt b/Documentation/devicetree/bindings/mfd/mt6397.txt
index d1df77f..11a748d 100644
--- a/Documentation/devicetree/bindings/mfd/mt6397.txt
+++ b/Documentation/devicetree/bindings/mfd/mt6397.txt
@@ -22,8 +22,10 @@ compatible: "mediatek,mt6397" or "mediatek,mt6323"
 Optional subnodes:
 
 - rtc
-	Required properties:
+	Required properties: Should be one of follows
+		- compatible: "mediatek,mt6323-rtc"
 		- compatible: "mediatek,mt6397-rtc"
+	For details, see Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
 - regulators
 	Required properties:
 		- compatible: "mediatek,mt6397-regulator"
-- 
2.7.4

[PATCH v1 04/16] mfd: mt6397: add MT6323 RTC support into MT6397 driver

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add compatible string as "mt6323-rtc" that will make the OF core spawn
child devices for the RTC subnode of that MT6323 MFD node.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/mfd/mt6397-core.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index 77b64bd..f71874a 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014 MediaTek Inc.
+ * Copyright (c) 2014-2018 MediaTek Inc.
  * Author: Flora Fu, MediaTek
  *
  * This program is free software; you can redistribute it and/or modify
@@ -23,6 +23,9 @@
 #include <linux/mfd/mt6397/registers.h>
 #include <linux/mfd/mt6323/registers.h>
 
+#define MT6323_RTC_BASE		0x8000
+#define MT6323_RTC_SIZE		0x3e
+
 #define MT6397_RTC_BASE		0xe000
 #define MT6397_RTC_SIZE		0x3e
 
@@ -30,6 +33,19 @@
 #define MT6391_CID_CODE		0x91
 #define MT6397_CID_CODE		0x97
 
+static const struct resource mt6323_rtc_resources[] = {
+	{
+		.start = MT6323_RTC_BASE,
+		.end   = MT6323_RTC_BASE + MT6323_RTC_SIZE,
+		.flags = IORESOURCE_MEM,
+	},
+	{
+		.start = MT6323_IRQ_STATUS_RTC,
+		.end   = MT6323_IRQ_STATUS_RTC,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
 static const struct resource mt6397_rtc_resources[] = {
 	{
 		.start = MT6397_RTC_BASE,
@@ -55,6 +71,11 @@ static const struct resource mt6397_keys_resources[] = {
 
 static const struct mfd_cell mt6323_devs[] = {
 	{
+		.name = "mt6323-rtc",
+		.num_resources = ARRAY_SIZE(mt6323_rtc_resources),
+		.resources = mt6323_rtc_resources,
+		.of_compatible = "mediatek,mt6323-rtc",
+	}, {
 		.name = "mt6323-regulator",
 		.of_compatible = "mediatek,mt6323-regulator"
 	}, {
-- 
2.7.4

[PATCH v1 05/16] rtc: mediatek: add MT6323 support to RTC driver

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Just to add MT6323 support to the existent RTC driver.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 385f830..0df7ccd 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -398,6 +398,7 @@ static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
 			mt6397_rtc_resume);
 
 static const struct of_device_id mt6397_rtc_of_match[] = {
+	{ .compatible = "mediatek,mt6323-rtc", },
 	{ .compatible = "mediatek,mt6397-rtc", },
 	{ }
 };
-- 
2.7.4

[PATCH v1 06/16] rtc: mediatek: remove unnecessary parentheses

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Remove unnecessary parentheses due to explicit C operator precedence.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 0df7ccd..4411c08 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -106,7 +106,7 @@ static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
 	int ret;
 
 	ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_IRQ_STA, &irqsta);
-	if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
+	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);
-- 
2.7.4

[PATCH v1 07/16] rtc: mediatek: replace a poll with regmap_read_poll_timeout

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Reuse the helper regmap_read_poll_timeout instead to simpify the logic.

Furthermore, the time for a wait in each iteration changed from cpu_relax
to 20us is for matching the usage of the helper, but it wouldn't acctually
break any the existent functionality according to a good test on MT6323
PMIC.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 4411c08..b62eaa8 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -18,6 +18,7 @@
 #include <linux/regmap.h>
 #include <linux/rtc.h>
 #include <linux/irqdomain.h>
+#include <linux/jiffies.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -63,6 +64,9 @@
 #define RTC_NUM_YEARS		128
 #define RTC_MIN_YEAR_OFFSET	(RTC_MIN_YEAR - RTC_BASE_YEAR)
 
+#define MTK_RTC_POLL_DELAY_US	10
+#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
+
 struct mt6397_rtc {
 	struct device		*dev;
 	struct rtc_device	*rtc_dev;
@@ -74,7 +78,6 @@ struct mt6397_rtc {
 
 static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
 {
-	unsigned long timeout = jiffies + HZ;
 	int ret;
 	u32 data;
 
@@ -82,19 +85,13 @@ static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
 	if (ret < 0)
 		return ret;
 
-	while (1) {
-		ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_BBPU,
-				  &data);
-		if (ret < 0)
-			break;
-		if (!(data & RTC_BBPU_CBUSY))
-			break;
-		if (time_after(jiffies, timeout)) {
-			ret = -ETIMEDOUT;
-			break;
-		}
-		cpu_relax();
-	}
+	ret = regmap_read_poll_timeout(rtc->regmap,
+				       rtc->addr_base + RTC_BBPU, data,
+				       !(data & RTC_BBPU_CBUSY),
+				       MTK_RTC_POLL_DELAY_US,
+				       MTK_RTC_POLL_TIMEOUT);
+	if (ret)
+		dev_err(rtc->dev, "failed to write WRTGE: %d\n", ret);
 
 	return ret;
 }
-- 
2.7.4

[PATCH v1 08/16] rtc: mediatek: remove unnecessary irq_dispose_mapping

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

It's unnecessary doing irq_dispose_mapping as a reverse operation for
platform_get_irq.

Ususally, irq_dispose_mapping should be called in error path or module
removal to release the resources for irq_of_parse_and_map requested.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index b62eaa8..cefb83b 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -17,7 +17,6 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
-#include <linux/irqdomain.h>
 #include <linux/jiffies.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
@@ -336,7 +335,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
 			rtc->irq, ret);
-		goto out_dispose_irq;
+		return ret;
 	}
 
 	device_init_wakeup(&pdev->dev, 1);
@@ -353,8 +352,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
 
 out_free_irq:
 	free_irq(rtc->irq, rtc->rtc_dev);
-out_dispose_irq:
-	irq_dispose_mapping(rtc->irq);
+
 	return ret;
 }
 
@@ -364,7 +362,6 @@ static int mtk_rtc_remove(struct platform_device *pdev)
 
 	rtc_device_unregister(rtc->rtc_dev);
 	free_irq(rtc->irq, rtc->rtc_dev);
-	irq_dispose_mapping(rtc->irq);
 
 	return 0;
 }
-- 
2.7.4

[PATCH v1 09/16] rtc: mediatek: convert to use device managed functions

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Use device managed operation to simplify error handling, reduce source
code size, and reduce the likelyhood of bugs, and remove our removal
callback which contains anything already done by device managed functions.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index cefb83b..bfc5d6f 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -14,6 +14,7 @@
 
 #include <linux/delay.h>
 #include <linux/init.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
@@ -328,10 +329,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rtc);
 
-	ret = request_threaded_irq(rtc->irq, NULL,
-				   mtk_rtc_irq_handler_thread,
-				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
-				   "mt6397-rtc", 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);
@@ -340,30 +341,15 @@ static int mtk_rtc_probe(struct platform_device *pdev)
 
 	device_init_wakeup(&pdev->dev, 1);
 
-	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
-					   &mtk_rtc_ops, THIS_MODULE);
+	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "mt6397-rtc",
+						&mtk_rtc_ops, THIS_MODULE);
 	if (IS_ERR(rtc->rtc_dev)) {
 		dev_err(&pdev->dev, "register rtc device failed\n");
 		ret = PTR_ERR(rtc->rtc_dev);
-		goto out_free_irq;
+		return ret;
 	}
 
 	return 0;
-
-out_free_irq:
-	free_irq(rtc->irq, 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);
-	free_irq(rtc->irq, rtc->rtc_dev);
-
-	return 0;
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -405,7 +391,6 @@ static struct platform_driver mtk_rtc_driver = {
 		.pm = &mt6397_pm_ops,
 	},
 	.probe	= mtk_rtc_probe,
-	.remove = mtk_rtc_remove,
 };
 
 module_platform_driver(mtk_rtc_driver);
-- 
2.7.4

[PATCH v1 10/16] rtc: mediatek: add devm_of_platform_populate

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add devm_of_platform_populate to populate all child nodes according to
the dt-binding.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index bfc5d6f..d133d1f 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -16,6 +16,7 @@
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/of_platform.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
 #include <linux/jiffies.h>
@@ -349,7 +350,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	return 0;
+	return devm_of_platform_populate(&pdev->dev);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.7.4

[PATCH v1 11/16] rtc: mediatek: move the declaration into a globally visible header file

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

This is in preparation for allowing other drivers can share the
declaration, so move the declaration into a globally visible header file.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c   | 53 +---------------------------------
 include/linux/rtc/mt6397.h | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 52 deletions(-)
 create mode 100644 include/linux/rtc/mt6397.h

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index d133d1f..015609d 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -19,63 +19,12 @@
 #include <linux/of_platform.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
-#include <linux/jiffies.h>
 #include <linux/platform_device.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
 #include <linux/io.h>
 #include <linux/mfd/mt6397/core.h>
-
-#define RTC_BBPU		0x0000
-#define RTC_BBPU_CBUSY		BIT(6)
-
-#define RTC_WRTGR		0x003c
-
-#define RTC_IRQ_STA		0x0002
-#define RTC_IRQ_STA_AL		BIT(0)
-#define RTC_IRQ_STA_LP		BIT(3)
-
-#define RTC_IRQ_EN		0x0004
-#define RTC_IRQ_EN_AL		BIT(0)
-#define RTC_IRQ_EN_ONESHOT	BIT(2)
-#define RTC_IRQ_EN_LP		BIT(3)
-#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
-
-#define RTC_AL_MASK		0x0008
-#define RTC_AL_MASK_DOW		BIT(4)
-
-#define RTC_TC_SEC		0x000a
-/* Min, Hour, Dom... register offset to RTC_TC_SEC */
-#define RTC_OFFSET_SEC		0
-#define RTC_OFFSET_MIN		1
-#define RTC_OFFSET_HOUR		2
-#define RTC_OFFSET_DOM		3
-#define RTC_OFFSET_DOW		4
-#define RTC_OFFSET_MTH		5
-#define RTC_OFFSET_YEAR		6
-#define RTC_OFFSET_COUNT	7
-
-#define RTC_AL_SEC		0x0018
-
-#define RTC_PDN2		0x002e
-#define RTC_PDN2_PWRON_ALARM	BIT(4)
-
-#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 MTK_RTC_POLL_DELAY_US	10
-#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
-
-struct mt6397_rtc {
-	struct device		*dev;
-	struct rtc_device	*rtc_dev;
-	struct mutex		lock;
-	struct regmap		*regmap;
-	int			irq;
-	u32			addr_base;
-};
+#include <linux/rtc/mt6397.h>
 
 static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
 {
diff --git a/include/linux/rtc/mt6397.h b/include/linux/rtc/mt6397.h
new file mode 100644
index 0000000..4b19f51
--- /dev/null
+++ b/include/linux/rtc/mt6397.h
@@ -0,0 +1,72 @@
+
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2014-2018 MediaTek Inc.
+ *
+ * Author: Tianping.Fang <tianping.fang@mediatek.com>
+ *	   Sean Wang <sean.wang@mediatek.com>
+ */
+
+#ifndef _LINUX_RTC_MT6397_H_
+#define _LINUX_RTC_MT6397_H_
+
+#include <linux/jiffies.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+
+#define RTC_BBPU		0x0000
+#define RTC_BBPU_CBUSY		BIT(6)
+
+#define RTC_WRTGR		0x003c
+
+#define RTC_IRQ_STA		0x0002
+#define RTC_IRQ_STA_AL		BIT(0)
+#define RTC_IRQ_STA_LP		BIT(3)
+
+#define RTC_IRQ_EN		0x0004
+#define RTC_IRQ_EN_AL		BIT(0)
+#define RTC_IRQ_EN_ONESHOT	BIT(2)
+#define RTC_IRQ_EN_LP		BIT(3)
+#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
+
+#define RTC_AL_MASK		0x0008
+#define RTC_AL_MASK_DOW		BIT(4)
+
+#define RTC_TC_SEC		0x000a
+/* Min, Hour, Dom... register offset to RTC_TC_SEC */
+#define RTC_OFFSET_SEC		0
+#define RTC_OFFSET_MIN		1
+#define RTC_OFFSET_HOUR		2
+#define RTC_OFFSET_DOM		3
+#define RTC_OFFSET_DOW		4
+#define RTC_OFFSET_MTH		5
+#define RTC_OFFSET_YEAR		6
+#define RTC_OFFSET_COUNT	7
+
+#define RTC_AL_SEC		0x0018
+
+#define RTC_PDN2		0x002e
+#define RTC_PDN2_PWRON_ALARM	BIT(4)
+
+#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 MTK_RTC_POLL_DELAY_US	10
+#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
+
+struct mt6397_rtc {
+	struct device		*dev;
+	struct rtc_device	*rtc_dev;
+
+	/* protect registers accessing */
+	struct mutex		lock;
+	struct regmap		*regmap;
+	int			irq;
+	u32			addr_base;
+};
+
+#endif /* _LINUX_RTC_MT6397_H_ */
+
-- 
2.7.4

[PATCH v1 12/16] rtc: mediatek: cleanup header files to include

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Clean up those included header files by removing unreferenced ones,
add missing ones the driver explicitly depends on and finally list
all #includes in alphabetical order.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index 015609d..f5dc70d 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -12,18 +12,15 @@
 * GNU General Public License for more details.
 */
 
-#include <linux/delay.h>
-#include <linux/init.h>
+#include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/mfd/mt6397/core.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_platform.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/rtc.h>
-#include <linux/platform_device.h>
-#include <linux/of_address.h>
-#include <linux/of_irq.h>
-#include <linux/io.h>
-#include <linux/mfd/mt6397/core.h>
 #include <linux/rtc/mt6397.h>
 
 static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
-- 
2.7.4

[PATCH v1 13/16] rtc: mediatek: update license converting to using SPDX identifiers

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Update the license and as well convert to using SPDX identifiers.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/rtc/rtc-mt6397.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
index f5dc70d..ad336a4 100644
--- a/drivers/rtc/rtc-mt6397.c
+++ b/drivers/rtc/rtc-mt6397.c
@@ -1,16 +1,12 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
-* 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.
-*/
+ * MediaTek PMIC RTC driver
+ *
+ * Copyright (C) 2014-2018 MediaTek Inc.
+ *
+ * Author: Tianping.Fang <tianping.fang@mediatek.com>
+ *	   Sean Wang <sean.wang@mediatek.com>
+ */
 
 #include <linux/err.h>
 #include <linux/interrupt.h>
-- 
2.7.4

[PATCH v1 14/16] power: reset: mediatek: add a power-off driver using PMIC RTC device

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

The power device is responsible for externally down or up the power
of the remote MediaTek SoC through the tiny circuit BBPU inside PMIC RTC.

Though it's a part of RTC device, it would be better to be a standalone
driver against existent RTC driver so as to make concentration on works
about power-controlling topic and help gather more improvements while the
subsystem's constantly growing.

Currently, the most basic functionality supported is to just power off the
system by writing to a special bit field in BBPU register after the system
has reached pm_poweroff.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 drivers/power/reset/Kconfig               |   9 +++
 drivers/power/reset/Makefile              |   1 +
 drivers/power/reset/mt6397-rtc-poweroff.c | 100 ++++++++++++++++++++++++++++++
 include/linux/rtc/mt6397.h                |   1 +
 4 files changed, 111 insertions(+)
 create mode 100644 drivers/power/reset/mt6397-rtc-poweroff.c

diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig
index a102e74..0bd4603 100644
--- a/drivers/power/reset/Kconfig
+++ b/drivers/power/reset/Kconfig
@@ -121,6 +121,15 @@ config POWER_RESET_LTC2952
 	  This driver supports an external powerdown trigger and board power
 	  down via the LTC2952. Bindings are made in the device tree.
 
+config POWER_RESET_MT6397_RTC
+	bool "MediaTek MT6397 RTC power-off driver"
+	help
+	  This driver supports turning off a remote MediaTek SoC by
+	  controlling BBPU on MT6397 or MT6323 RTC.
+
+	  Select this if you're building a kernel with your MediaTek SoC
+	  with an equipment with MT6397 or MT6323 PMIC.
+
 config POWER_RESET_QNAP
 	bool "QNAP power-off driver"
 	depends on OF_GPIO && PLAT_ORION
diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile
index dcc92f5..d45099e 100644
--- a/drivers/power/reset/Makefile
+++ b/drivers/power/reset/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_POWER_RESET_GPIO) += gpio-poweroff.o
 obj-$(CONFIG_POWER_RESET_GPIO_RESTART) += gpio-restart.o
 obj-$(CONFIG_POWER_RESET_HISI) += hisi-reboot.o
 obj-$(CONFIG_POWER_RESET_MSM) += msm-poweroff.o
+obj-$(CONFIG_POWER_RESET_MT6397_RTC) += mt6397-rtc-poweroff.o
 obj-$(CONFIG_POWER_RESET_PIIX4_POWEROFF) += piix4-poweroff.o
 obj-$(CONFIG_POWER_RESET_LTC2952) += ltc2952-poweroff.o
 obj-$(CONFIG_POWER_RESET_QNAP) += qnap-poweroff.o
diff --git a/drivers/power/reset/mt6397-rtc-poweroff.c b/drivers/power/reset/mt6397-rtc-poweroff.c
new file mode 100644
index 0000000..9b57366
--- /dev/null
+++ b/drivers/power/reset/mt6397-rtc-poweroff.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Power-off using MediaTek PMIC RTC device
+ *
+ * Copyright (C) 2018 MediaTek Inc.
+ *
+ * Author: Sean Wang <sean.wang@mediatek.com>
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc/mt6397.h>
+
+struct mt6397_rtc_powercon {
+	struct device *dev;
+	struct mt6397_rtc *rtc;
+};
+
+static struct mt6397_rtc_powercon *mt_powercon;
+
+static void mt6397_rtc_do_poweroff(void)
+{
+	struct mt6397_rtc_powercon *powercon = mt_powercon;
+	struct mt6397_rtc *rtc = powercon->rtc;
+	unsigned int val;
+	int ret;
+
+	regmap_write(rtc->regmap, rtc->addr_base + RTC_BBPU, RTC_BBPU_KEY);
+	regmap_write(rtc->regmap, rtc->addr_base + RTC_WRTGR, 1);
+
+	ret = regmap_read_poll_timeout(rtc->regmap,
+				       rtc->addr_base + RTC_BBPU, val,
+				       !(val & RTC_BBPU_CBUSY),
+				       MTK_RTC_POLL_DELAY_US,
+				       MTK_RTC_POLL_TIMEOUT);
+	if (ret)
+		dev_err(powercon->dev, "failed to write BBPU: %d\n", ret);
+
+	/* Wait some time until system down, otherwise, notice with a warn */
+	mdelay(1000);
+
+	WARN_ONCE(1, "Unable to poweroff system\n");
+}
+
+static int mt6397_rtc_poweroff_probe(struct platform_device *pdev)
+{
+	struct mt6397_rtc *rtc = dev_get_drvdata(pdev->dev.parent);
+	struct mt6397_rtc_powercon *powercon;
+
+	if (!rtc) {
+		dev_err(&pdev->dev, "Can't find RTC as the parent\n");
+		return -ENODEV;
+	}
+
+	powercon = devm_kzalloc(&pdev->dev, sizeof(*powercon), GFP_KERNEL);
+	if (!powercon)
+		return -ENOMEM;
+
+	powercon->dev = &pdev->dev;
+	powercon->rtc = rtc;
+	mt_powercon = powercon;
+
+	pm_power_off = &mt6397_rtc_do_poweroff;
+
+	return 0;
+}
+
+static int mt6397_rtc_poweroff_remove(struct platform_device *pdev)
+{
+	if (pm_power_off == &mt6397_rtc_do_poweroff)
+		pm_power_off = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id mt6397_rtc_poweroff_dt_match[] = {
+	{ .compatible = "mediatek,mt6323-rtc-poweroff" },
+	{ .compatible = "mediatek,mt6397-rtc-poweroff" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mt6397_rtc_poweroff_dt_match);
+
+static struct platform_driver mt6397_rtc_poweroff_driver = {
+	.probe		= mt6397_rtc_poweroff_probe,
+	.remove		= mt6397_rtc_poweroff_remove,
+	.driver		= {
+		.name	= "mt6397-rtc-poweroff",
+		.of_match_table = mt6397_rtc_poweroff_dt_match,
+	},
+};
+
+module_platform_driver(mt6397_rtc_poweroff_driver);
+
+MODULE_DESCRIPTION("Poweroff driver using MediaTek PMIC RTC");
+MODULE_AUTHOR("Sean Wang <sean.wang@mediatek.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:mt6397-rtc-poweroff");
diff --git a/include/linux/rtc/mt6397.h b/include/linux/rtc/mt6397.h
index 4b19f51..e618974 100644
--- a/include/linux/rtc/mt6397.h
+++ b/include/linux/rtc/mt6397.h
@@ -17,6 +17,7 @@
 
 #define RTC_BBPU		0x0000
 #define RTC_BBPU_CBUSY		BIT(6)
+#define RTC_BBPU_KEY            (0x43 << 8)
 
 #define RTC_WRTGR		0x003c
 
-- 
2.7.4

[PATCH v1 15/16] MAINTAINERS: update entry for ARM/Mediatek RTC DRIVER

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add new files for new dt-bindings and a globally visible header file.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d3c33d7..1c13f29 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1626,9 +1626,11 @@ M:	Sean Wang <sean.wang@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:	Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
 F:	Documentation/devicetree/bindings/rtc/rtc-mt7622.txt
 F:	drivers/rtc/rtc-mt6397.c
 F:	drivers/rtc/rtc-mt7622.c
+F:	include/linux/rtc/mt6397.h
 
 ARM/Mediatek SoC support
 M:	Matthias Brugger <matthias.bgg@gmail.com>
-- 
2.7.4

[PATCH v1 16/16] MAINTAINERS: add an entry for MediaTek board level shutdown driver

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

Add an entry for the MediaTek board level shutdown driver.

Signed-off-by: Sean Wang <sean.wang@mediatek.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1c13f29..863d1e3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8818,6 +8818,13 @@ S:	Maintained
 F:	drivers/net/dsa/mt7530.*
 F:	net/dsa/tag_mtk.c
 
+MEDIATEK BOARD LEVEL SHUTDOWN DRIVERS
+M:	Sean Wang <sean.wang@mediatek.com>
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
+F:	drivers/power/reset/mt6397-rtc-poweroff.c
+
 MEDIATEK JPEG DRIVER
 M:	Rick Chang <rick.chang@mediatek.com>
 M:	Bin Liu <bin.liu@mediatek.com>
-- 
2.7.4

Re: [PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[Alexandre Belloni]

Hi,

On 23/03/2018 at 17:14:59 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add device-tree binding for MediaTek PMIC based RTC.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> 
> diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> new file mode 100644
> index 0000000..83ff6be
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> @@ -0,0 +1,39 @@
> +Device-Tree bindings for MediaTek PMIC based RTC
> +
> +MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
> +is working as a multi-function device (MFD). And the RTC can be configured and
> +set up via PMIC wrapper bus. Which is also common resource shared among the
> +other functions present on the PMIC.
> +
> +For MediaTek PMIC wrapper bus bindings, see:
> +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> +
> +Required parent node:
> +- pmic
> +  For MediaTek PMIC MFD bindings, see:
> +  Documentation/devicetree/bindings/mfd/mt6397.txt
> +
> +Required properties:
> +- compatible: Should be one of follows
> +	"mediatek,mt6323-rtc": for MT6323 PMIC
> +	"mediatek,mt6397-rtc": for MT6397 PMIC
> +
> +Optional child node:
> +- power-off
> +  For Power-Off Device for MediaTek PMIC RTC bindings, see:
> +  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> +
> +Example:
> +
> +	pmic {
> +		compatible = "mediatek,mt6323";
> +
> +		...
> +		rtc {
> +			compatible = "mediatek,mt6323-rtc";
> +
> +			power-off {
> +				compatible = "mediatek,mt6323-rtc-poweroff";
> +			};

I'm pretty sure the whole point of mfd is to avoid having the poweroff
controller under the rtc


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 11/16] rtc: mediatek: move the declaration into a globally visible header file

[Alexandre Belloni]

On 23/03/2018 at 17:15:08 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> This is in preparation for allowing other drivers can share the
> declaration, so move the declaration into a globally visible header file.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/rtc/rtc-mt6397.c   | 53 +---------------------------------
>  include/linux/rtc/mt6397.h | 72 ++++++++++++++++++++++++++++++++++++++++++++++

This should go in include/linux/mfd/

>  2 files changed, 73 insertions(+), 52 deletions(-)
>  create mode 100644 include/linux/rtc/mt6397.h
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index d133d1f..015609d 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -19,63 +19,12 @@
>  #include <linux/of_platform.h>
>  #include <linux/regmap.h>
>  #include <linux/rtc.h>
> -#include <linux/jiffies.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/io.h>
>  #include <linux/mfd/mt6397/core.h>
> -
> -#define RTC_BBPU		0x0000
> -#define RTC_BBPU_CBUSY		BIT(6)
> -
> -#define RTC_WRTGR		0x003c
> -
> -#define RTC_IRQ_STA		0x0002
> -#define RTC_IRQ_STA_AL		BIT(0)
> -#define RTC_IRQ_STA_LP		BIT(3)
> -
> -#define RTC_IRQ_EN		0x0004
> -#define RTC_IRQ_EN_AL		BIT(0)
> -#define RTC_IRQ_EN_ONESHOT	BIT(2)
> -#define RTC_IRQ_EN_LP		BIT(3)
> -#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> -
> -#define RTC_AL_MASK		0x0008
> -#define RTC_AL_MASK_DOW		BIT(4)
> -
> -#define RTC_TC_SEC		0x000a
> -/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> -#define RTC_OFFSET_SEC		0
> -#define RTC_OFFSET_MIN		1
> -#define RTC_OFFSET_HOUR		2
> -#define RTC_OFFSET_DOM		3
> -#define RTC_OFFSET_DOW		4
> -#define RTC_OFFSET_MTH		5
> -#define RTC_OFFSET_YEAR		6
> -#define RTC_OFFSET_COUNT	7
> -
> -#define RTC_AL_SEC		0x0018
> -
> -#define RTC_PDN2		0x002e
> -#define RTC_PDN2_PWRON_ALARM	BIT(4)
> -
> -#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 MTK_RTC_POLL_DELAY_US	10
> -#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
> -
> -struct mt6397_rtc {
> -	struct device		*dev;
> -	struct rtc_device	*rtc_dev;
> -	struct mutex		lock;
> -	struct regmap		*regmap;
> -	int			irq;
> -	u32			addr_base;
> -};
> +#include <linux/rtc/mt6397.h>
>  
>  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
>  {
> diff --git a/include/linux/rtc/mt6397.h b/include/linux/rtc/mt6397.h
> new file mode 100644
> index 0000000..4b19f51
> --- /dev/null
> +++ b/include/linux/rtc/mt6397.h
> @@ -0,0 +1,72 @@
> +

Unnecessary empty line

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2014-2018 MediaTek Inc.
> + *
> + * Author: Tianping.Fang <tianping.fang@mediatek.com>
> + *	   Sean Wang <sean.wang@mediatek.com>
> + */
> +
> +#ifndef _LINUX_RTC_MT6397_H_
> +#define _LINUX_RTC_MT6397_H_
> +
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +#define RTC_BBPU		0x0000
> +#define RTC_BBPU_CBUSY		BIT(6)
> +
> +#define RTC_WRTGR		0x003c
> +
> +#define RTC_IRQ_STA		0x0002
> +#define RTC_IRQ_STA_AL		BIT(0)
> +#define RTC_IRQ_STA_LP		BIT(3)
> +
> +#define RTC_IRQ_EN		0x0004
> +#define RTC_IRQ_EN_AL		BIT(0)
> +#define RTC_IRQ_EN_ONESHOT	BIT(2)
> +#define RTC_IRQ_EN_LP		BIT(3)
> +#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> +
> +#define RTC_AL_MASK		0x0008
> +#define RTC_AL_MASK_DOW		BIT(4)
> +
> +#define RTC_TC_SEC		0x000a
> +/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> +#define RTC_OFFSET_SEC		0
> +#define RTC_OFFSET_MIN		1
> +#define RTC_OFFSET_HOUR		2
> +#define RTC_OFFSET_DOM		3
> +#define RTC_OFFSET_DOW		4
> +#define RTC_OFFSET_MTH		5
> +#define RTC_OFFSET_YEAR		6
> +#define RTC_OFFSET_COUNT	7
> +
> +#define RTC_AL_SEC		0x0018
> +
> +#define RTC_PDN2		0x002e
> +#define RTC_PDN2_PWRON_ALARM	BIT(4)
> +
> +#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 MTK_RTC_POLL_DELAY_US	10
> +#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
> +
> +struct mt6397_rtc {
> +	struct device		*dev;
> +	struct rtc_device	*rtc_dev;
> +
> +	/* protect registers accessing */
> +	struct mutex		lock;
> +	struct regmap		*regmap;
> +	int			irq;
> +	u32			addr_base;
> +};
> +
> +#endif /* _LINUX_RTC_MT6397_H_ */
> +
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 05/16] rtc: mediatek: add MT6323 support to RTC driver

[Alexandre Belloni]

Hi,

The subject line should be rtc: mt6397: (to differentiate with rtc-mt7622)

On 23/03/2018 at 17:15:02 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Just to add MT6323 support to the existent RTC driver.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/rtc/rtc-mt6397.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index 385f830..0df7ccd 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -398,6 +398,7 @@ static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
>  			mt6397_rtc_resume);
>  
>  static const struct of_device_id mt6397_rtc_of_match[] = {
> +	{ .compatible = "mediatek,mt6323-rtc", },
>  	{ .compatible = "mediatek,mt6397-rtc", },
>  	{ }
>  };
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[Alexandre Belloni]

On 23/03/2018 at 10:41:18 +0100, Alexandre Belloni wrote:
> Hi,
> 
> On 23/03/2018 at 17:14:59 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > Add device-tree binding for MediaTek PMIC based RTC.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > new file mode 100644
> > index 0000000..83ff6be
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > @@ -0,0 +1,39 @@
> > +Device-Tree bindings for MediaTek PMIC based RTC
> > +
> > +MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
> > +is working as a multi-function device (MFD). And the RTC can be configured and
> > +set up via PMIC wrapper bus. Which is also common resource shared among the
> > +other functions present on the PMIC.
> > +
> > +For MediaTek PMIC wrapper bus bindings, see:
> > +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> > +
> > +Required parent node:
> > +- pmic
> > +  For MediaTek PMIC MFD bindings, see:
> > +  Documentation/devicetree/bindings/mfd/mt6397.txt
> > +
> > +Required properties:
> > +- compatible: Should be one of follows
> > +	"mediatek,mt6323-rtc": for MT6323 PMIC
> > +	"mediatek,mt6397-rtc": for MT6397 PMIC
> > +
> > +Optional child node:
> > +- power-off
> > +  For Power-Off Device for MediaTek PMIC RTC bindings, see:
> > +  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > +
> > +Example:
> > +
> > +	pmic {
> > +		compatible = "mediatek,mt6323";
> > +
> > +		...
> > +		rtc {
> > +			compatible = "mediatek,mt6323-rtc";
> > +
> > +			power-off {
> > +				compatible = "mediatek,mt6323-rtc-poweroff";
> > +			};
> 
> I'm pretty sure the whole point of mfd is to avoid having the poweroff
> controller under the rtc
> 

BTW, I think it is enough to have that documented in only one file (the
MFD one is enough)

> 
> -- 
> Alexandre Belloni, Bootlin (formerly Free Electrons)
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 06/16] rtc: mediatek: remove unnecessary parentheses

[Alexandre Belloni]

On 23/03/2018 at 17:15:03 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Remove unnecessary parentheses due to explicit C operator precedence.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/rtc/rtc-mt6397.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index 0df7ccd..4411c08 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -106,7 +106,7 @@ static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
>  	int ret;
>  
>  	ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_IRQ_STA, &irqsta);
> -	if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> +	if (ret >= 0 && irqsta & RTC_IRQ_STA_AL) {

I don't think this makes the code particularly clearer.

>  		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
>  		irqen = irqsta & ~RTC_IRQ_EN_AL;
>  		mutex_lock(&rtc->lock);
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 08/16] rtc: mediatek: remove unnecessary irq_dispose_mapping

[Alexandre Belloni]

On 23/03/2018 at 17:15:05 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> It's unnecessary doing irq_dispose_mapping as a reverse operation for
> platform_get_irq.
> 
> Ususally, irq_dispose_mapping should be called in error path or module
> removal to release the resources for irq_of_parse_and_map requested.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/rtc/rtc-mt6397.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index b62eaa8..cefb83b 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -17,7 +17,6 @@
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  #include <linux/rtc.h>
> -#include <linux/irqdomain.h>
>  #include <linux/jiffies.h>
>  #include <linux/platform_device.h>
>  #include <linux/of_address.h>
> @@ -336,7 +335,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
>  			rtc->irq, ret);
> -		goto out_dispose_irq;
> +		return ret;
>  	}
>  
>  	device_init_wakeup(&pdev->dev, 1);
> @@ -353,8 +352,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  
>  out_free_irq:
>  	free_irq(rtc->irq, rtc->rtc_dev);
> -out_dispose_irq:
> -	irq_dispose_mapping(rtc->irq);
> +

Don't you still have a irq_create_mapping?

>  	return ret;
>  }
>  
> @@ -364,7 +362,6 @@ static int mtk_rtc_remove(struct platform_device *pdev)
>  
>  	rtc_device_unregister(rtc->rtc_dev);
>  	free_irq(rtc->irq, rtc->rtc_dev);
> -	irq_dispose_mapping(rtc->irq);
>  
>  	return 0;
>  }
> -- 
> 2.7.4
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 09/16] rtc: mediatek: convert to use device managed functions

[Alexandre Belloni]

On 23/03/2018 at 17:15:06 +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Use device managed operation to simplify error handling, reduce source
> code size, and reduce the likelyhood of bugs, and remove our removal
> callback which contains anything already done by device managed functions.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/rtc/rtc-mt6397.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> index cefb83b..bfc5d6f 100644
> --- a/drivers/rtc/rtc-mt6397.c
> +++ b/drivers/rtc/rtc-mt6397.c
> @@ -14,6 +14,7 @@
>  
>  #include <linux/delay.h>
>  #include <linux/init.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  #include <linux/rtc.h>
> @@ -328,10 +329,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, rtc);
>  
> -	ret = request_threaded_irq(rtc->irq, NULL,
> -				   mtk_rtc_irq_handler_thread,
> -				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> -				   "mt6397-rtc", 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);
> @@ -340,30 +341,15 @@ static int mtk_rtc_probe(struct platform_device *pdev)
>  
>  	device_init_wakeup(&pdev->dev, 1);
>  
> -	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> -					   &mtk_rtc_ops, THIS_MODULE);
> +	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "mt6397-rtc",
> +						&mtk_rtc_ops, THIS_MODULE);

You should probably switch to devm_rtc_allocate_device() and
rtc_register_device instead of devm_rtc_device_register.

>  	if (IS_ERR(rtc->rtc_dev)) {
>  		dev_err(&pdev->dev, "register rtc device failed\n");
>  		ret = PTR_ERR(rtc->rtc_dev);
> -		goto out_free_irq;
> +		return ret;

ret doesn't seem necessary anymore here.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 05/16] rtc: mediatek: add MT6323 support to RTC driver

[Sean Wang]

On Fri, 2018-03-23 at 11:01 +0100, Alexandre Belloni wrote:
> Hi,
> 
> The subject line should be rtc: mt6397: (to differentiate with rtc-mt7622)
> 

Sure, I will change subject line into rtc: mt6397: along with the other
related patches.

> On 23/03/2018 at 17:15:02 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > Just to add MT6323 support to the existent RTC driver.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  drivers/rtc/rtc-mt6397.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > index 385f830..0df7ccd 100644
> > --- a/drivers/rtc/rtc-mt6397.c
> > +++ b/drivers/rtc/rtc-mt6397.c
> > @@ -398,6 +398,7 @@ static SIMPLE_DEV_PM_OPS(mt6397_pm_ops, mt6397_rtc_suspend,
> >  			mt6397_rtc_resume);
> >  
> >  static const struct of_device_id mt6397_rtc_of_match[] = {
> > +	{ .compatible = "mediatek,mt6323-rtc", },
> >  	{ .compatible = "mediatek,mt6397-rtc", },
> >  	{ }
> >  };
> > -- 
> > 2.7.4
> > 
> 

Re: [PATCH v1 06/16] rtc: mediatek: remove unnecessary parentheses

[Sean Wang]

On Fri, 2018-03-23 at 11:21 +0100, Alexandre Belloni wrote:
> On 23/03/2018 at 17:15:03 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > Remove unnecessary parentheses due to explicit C operator precedence.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  drivers/rtc/rtc-mt6397.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > index 0df7ccd..4411c08 100644
> > --- a/drivers/rtc/rtc-mt6397.c
> > +++ b/drivers/rtc/rtc-mt6397.c
> > @@ -106,7 +106,7 @@ static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
> >  	int ret;
> >  
> >  	ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_IRQ_STA, &irqsta);
> > -	if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> > +	if (ret >= 0 && irqsta & RTC_IRQ_STA_AL) {
> 
> I don't think this makes the code particularly clearer.
> 

But it is still a one of check items in checkpatch

CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'ret >= 0'
#126: FILE: drivers/rtc/rtc-xxx.c:109:
+       if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {


or we still want to keep it in parentheses around here?

> >  		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> >  		irqen = irqsta & ~RTC_IRQ_EN_AL;
> >  		mutex_lock(&rtc->lock);
> > -- 
> > 2.7.4
> > 
> 

Re: [PATCH v1 11/16] rtc: mediatek: move the declaration into a globally visible header file

[Sean Wang]

On Fri, 2018-03-23 at 10:57 +0100, Alexandre Belloni wrote:
> On 23/03/2018 at 17:15:08 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > This is in preparation for allowing other drivers can share the
> > declaration, so move the declaration into a globally visible header file.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  drivers/rtc/rtc-mt6397.c   | 53 +---------------------------------
> >  include/linux/rtc/mt6397.h | 72 ++++++++++++++++++++++++++++++++++++++++++++++
> 
> This should go in include/linux/mfd/

include/linux/mfd/mt6397 is present, so include/linux/mfd/mt6397/rtc.h
seems a nice place to keep these declarations for the pmic rtc.

> >  2 files changed, 73 insertions(+), 52 deletions(-)
> >  create mode 100644 include/linux/rtc/mt6397.h
> > 
> > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > index d133d1f..015609d 100644
> > --- a/drivers/rtc/rtc-mt6397.c
> > +++ b/drivers/rtc/rtc-mt6397.c
> > @@ -19,63 +19,12 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/regmap.h>
> >  #include <linux/rtc.h>
> > -#include <linux/jiffies.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/io.h>
> >  #include <linux/mfd/mt6397/core.h>
> > -
> > -#define RTC_BBPU		0x0000
> > -#define RTC_BBPU_CBUSY		BIT(6)
> > -
> > -#define RTC_WRTGR		0x003c
> > -
> > -#define RTC_IRQ_STA		0x0002
> > -#define RTC_IRQ_STA_AL		BIT(0)
> > -#define RTC_IRQ_STA_LP		BIT(3)
> > -
> > -#define RTC_IRQ_EN		0x0004
> > -#define RTC_IRQ_EN_AL		BIT(0)
> > -#define RTC_IRQ_EN_ONESHOT	BIT(2)
> > -#define RTC_IRQ_EN_LP		BIT(3)
> > -#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> > -
> > -#define RTC_AL_MASK		0x0008
> > -#define RTC_AL_MASK_DOW		BIT(4)
> > -
> > -#define RTC_TC_SEC		0x000a
> > -/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> > -#define RTC_OFFSET_SEC		0
> > -#define RTC_OFFSET_MIN		1
> > -#define RTC_OFFSET_HOUR		2
> > -#define RTC_OFFSET_DOM		3
> > -#define RTC_OFFSET_DOW		4
> > -#define RTC_OFFSET_MTH		5
> > -#define RTC_OFFSET_YEAR		6
> > -#define RTC_OFFSET_COUNT	7
> > -
> > -#define RTC_AL_SEC		0x0018
> > -
> > -#define RTC_PDN2		0x002e
> > -#define RTC_PDN2_PWRON_ALARM	BIT(4)
> > -
> > -#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 MTK_RTC_POLL_DELAY_US	10
> > -#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
> > -
> > -struct mt6397_rtc {
> > -	struct device		*dev;
> > -	struct rtc_device	*rtc_dev;
> > -	struct mutex		lock;
> > -	struct regmap		*regmap;
> > -	int			irq;
> > -	u32			addr_base;
> > -};
> > +#include <linux/rtc/mt6397.h>
> >  
> >  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> >  {
> > diff --git a/include/linux/rtc/mt6397.h b/include/linux/rtc/mt6397.h
> > new file mode 100644
> > index 0000000..4b19f51
> > --- /dev/null
> > +++ b/include/linux/rtc/mt6397.h
> > @@ -0,0 +1,72 @@
> > +
> 
> Unnecessary empty line
> 

will remove the unnecessary empty line

> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2014-2018 MediaTek Inc.
> > + *
> > + * Author: Tianping.Fang <tianping.fang@mediatek.com>
> > + *	   Sean Wang <sean.wang@mediatek.com>
> > + */
> > +
> > +#ifndef _LINUX_RTC_MT6397_H_
> > +#define _LINUX_RTC_MT6397_H_
> > +
> > +#include <linux/jiffies.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/rtc.h>
> > +
> > +#define RTC_BBPU		0x0000
> > +#define RTC_BBPU_CBUSY		BIT(6)
> > +
> > +#define RTC_WRTGR		0x003c
> > +
> > +#define RTC_IRQ_STA		0x0002
> > +#define RTC_IRQ_STA_AL		BIT(0)
> > +#define RTC_IRQ_STA_LP		BIT(3)
> > +
> > +#define RTC_IRQ_EN		0x0004
> > +#define RTC_IRQ_EN_AL		BIT(0)
> > +#define RTC_IRQ_EN_ONESHOT	BIT(2)
> > +#define RTC_IRQ_EN_LP		BIT(3)
> > +#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> > +
> > +#define RTC_AL_MASK		0x0008
> > +#define RTC_AL_MASK_DOW		BIT(4)
> > +
> > +#define RTC_TC_SEC		0x000a
> > +/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> > +#define RTC_OFFSET_SEC		0
> > +#define RTC_OFFSET_MIN		1
> > +#define RTC_OFFSET_HOUR		2
> > +#define RTC_OFFSET_DOM		3
> > +#define RTC_OFFSET_DOW		4
> > +#define RTC_OFFSET_MTH		5
> > +#define RTC_OFFSET_YEAR		6
> > +#define RTC_OFFSET_COUNT	7
> > +
> > +#define RTC_AL_SEC		0x0018
> > +
> > +#define RTC_PDN2		0x002e
> > +#define RTC_PDN2_PWRON_ALARM	BIT(4)
> > +
> > +#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 MTK_RTC_POLL_DELAY_US	10
> > +#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
> > +
> > +struct mt6397_rtc {
> > +	struct device		*dev;
> > +	struct rtc_device	*rtc_dev;
> > +
> > +	/* protect registers accessing */
> > +	struct mutex		lock;
> > +	struct regmap		*regmap;
> > +	int			irq;
> > +	u32			addr_base;
> > +};
> > +
> > +#endif /* _LINUX_RTC_MT6397_H_ */
> > +
> > -- 
> > 2.7.4
> > 
> 

Re: [PATCH v1 06/16] rtc: mediatek: remove unnecessary parentheses

[Alexandre Belloni]

On 24/03/2018 at 15:14:12 +0800, Sean Wang wrote:
> On Fri, 2018-03-23 at 11:21 +0100, Alexandre Belloni wrote:
> > On 23/03/2018 at 17:15:03 +0800, sean.wang@mediatek.com wrote:
> > > From: Sean Wang <sean.wang@mediatek.com>
> > > 
> > > Remove unnecessary parentheses due to explicit C operator precedence.
> > > 
> > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > > ---
> > >  drivers/rtc/rtc-mt6397.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > > index 0df7ccd..4411c08 100644
> > > --- a/drivers/rtc/rtc-mt6397.c
> > > +++ b/drivers/rtc/rtc-mt6397.c
> > > @@ -106,7 +106,7 @@ static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
> > >  	int ret;
> > >  
> > >  	ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_IRQ_STA, &irqsta);
> > > -	if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> > > +	if (ret >= 0 && irqsta & RTC_IRQ_STA_AL) {
> > 
> > I don't think this makes the code particularly clearer.
> > 
> 
> But it is still a one of check items in checkpatch
> 
> CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'ret >= 0'
> #126: FILE: drivers/rtc/rtc-xxx.c:109:
> +       if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> 
> 
> or we still want to keep it in parentheses around here?
> 

Yeah, this is a matter of taste, I would keep the parentheses.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 11/16] rtc: mediatek: move the declaration into a globally visible header file

[Alexandre Belloni]

On 24/03/2018 at 15:31:49 +0800, Sean Wang wrote:
> On Fri, 2018-03-23 at 10:57 +0100, Alexandre Belloni wrote:
> > On 23/03/2018 at 17:15:08 +0800, sean.wang@mediatek.com wrote:
> > > From: Sean Wang <sean.wang@mediatek.com>
> > > 
> > > This is in preparation for allowing other drivers can share the
> > > declaration, so move the declaration into a globally visible header file.
> > > 
> > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > > ---
> > >  drivers/rtc/rtc-mt6397.c   | 53 +---------------------------------
> > >  include/linux/rtc/mt6397.h | 72 ++++++++++++++++++++++++++++++++++++++++++++++
> > 
> > This should go in include/linux/mfd/
> 
> include/linux/mfd/mt6397 is present, so include/linux/mfd/mt6397/rtc.h
> seems a nice place to keep these declarations for the pmic rtc.
> 

Yes, this seems good to me.

> > >  2 files changed, 73 insertions(+), 52 deletions(-)
> > >  create mode 100644 include/linux/rtc/mt6397.h
> > > 
> > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > > index d133d1f..015609d 100644
> > > --- a/drivers/rtc/rtc-mt6397.c
> > > +++ b/drivers/rtc/rtc-mt6397.c
> > > @@ -19,63 +19,12 @@
> > >  #include <linux/of_platform.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/rtc.h>
> > > -#include <linux/jiffies.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/of_address.h>
> > >  #include <linux/of_irq.h>
> > >  #include <linux/io.h>
> > >  #include <linux/mfd/mt6397/core.h>
> > > -
> > > -#define RTC_BBPU		0x0000
> > > -#define RTC_BBPU_CBUSY		BIT(6)
> > > -
> > > -#define RTC_WRTGR		0x003c
> > > -
> > > -#define RTC_IRQ_STA		0x0002
> > > -#define RTC_IRQ_STA_AL		BIT(0)
> > > -#define RTC_IRQ_STA_LP		BIT(3)
> > > -
> > > -#define RTC_IRQ_EN		0x0004
> > > -#define RTC_IRQ_EN_AL		BIT(0)
> > > -#define RTC_IRQ_EN_ONESHOT	BIT(2)
> > > -#define RTC_IRQ_EN_LP		BIT(3)
> > > -#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> > > -
> > > -#define RTC_AL_MASK		0x0008
> > > -#define RTC_AL_MASK_DOW		BIT(4)
> > > -
> > > -#define RTC_TC_SEC		0x000a
> > > -/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> > > -#define RTC_OFFSET_SEC		0
> > > -#define RTC_OFFSET_MIN		1
> > > -#define RTC_OFFSET_HOUR		2
> > > -#define RTC_OFFSET_DOM		3
> > > -#define RTC_OFFSET_DOW		4
> > > -#define RTC_OFFSET_MTH		5
> > > -#define RTC_OFFSET_YEAR		6
> > > -#define RTC_OFFSET_COUNT	7
> > > -
> > > -#define RTC_AL_SEC		0x0018
> > > -
> > > -#define RTC_PDN2		0x002e
> > > -#define RTC_PDN2_PWRON_ALARM	BIT(4)
> > > -
> > > -#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 MTK_RTC_POLL_DELAY_US	10
> > > -#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
> > > -
> > > -struct mt6397_rtc {
> > > -	struct device		*dev;
> > > -	struct rtc_device	*rtc_dev;
> > > -	struct mutex		lock;
> > > -	struct regmap		*regmap;
> > > -	int			irq;
> > > -	u32			addr_base;
> > > -};
> > > +#include <linux/rtc/mt6397.h>
> > >  
> > >  static int mtk_rtc_write_trigger(struct mt6397_rtc *rtc)
> > >  {
> > > diff --git a/include/linux/rtc/mt6397.h b/include/linux/rtc/mt6397.h
> > > new file mode 100644
> > > index 0000000..4b19f51
> > > --- /dev/null
> > > +++ b/include/linux/rtc/mt6397.h
> > > @@ -0,0 +1,72 @@
> > > +
> > 
> > Unnecessary empty line
> > 
> 
> will remove the unnecessary empty line
> 
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2014-2018 MediaTek Inc.
> > > + *
> > > + * Author: Tianping.Fang <tianping.fang@mediatek.com>
> > > + *	   Sean Wang <sean.wang@mediatek.com>
> > > + */
> > > +
> > > +#ifndef _LINUX_RTC_MT6397_H_
> > > +#define _LINUX_RTC_MT6397_H_
> > > +
> > > +#include <linux/jiffies.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/rtc.h>
> > > +
> > > +#define RTC_BBPU		0x0000
> > > +#define RTC_BBPU_CBUSY		BIT(6)
> > > +
> > > +#define RTC_WRTGR		0x003c
> > > +
> > > +#define RTC_IRQ_STA		0x0002
> > > +#define RTC_IRQ_STA_AL		BIT(0)
> > > +#define RTC_IRQ_STA_LP		BIT(3)
> > > +
> > > +#define RTC_IRQ_EN		0x0004
> > > +#define RTC_IRQ_EN_AL		BIT(0)
> > > +#define RTC_IRQ_EN_ONESHOT	BIT(2)
> > > +#define RTC_IRQ_EN_LP		BIT(3)
> > > +#define RTC_IRQ_EN_ONESHOT_AL	(RTC_IRQ_EN_ONESHOT | RTC_IRQ_EN_AL)
> > > +
> > > +#define RTC_AL_MASK		0x0008
> > > +#define RTC_AL_MASK_DOW		BIT(4)
> > > +
> > > +#define RTC_TC_SEC		0x000a
> > > +/* Min, Hour, Dom... register offset to RTC_TC_SEC */
> > > +#define RTC_OFFSET_SEC		0
> > > +#define RTC_OFFSET_MIN		1
> > > +#define RTC_OFFSET_HOUR		2
> > > +#define RTC_OFFSET_DOM		3
> > > +#define RTC_OFFSET_DOW		4
> > > +#define RTC_OFFSET_MTH		5
> > > +#define RTC_OFFSET_YEAR		6
> > > +#define RTC_OFFSET_COUNT	7
> > > +
> > > +#define RTC_AL_SEC		0x0018
> > > +
> > > +#define RTC_PDN2		0x002e
> > > +#define RTC_PDN2_PWRON_ALARM	BIT(4)
> > > +
> > > +#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 MTK_RTC_POLL_DELAY_US	10
> > > +#define MTK_RTC_POLL_TIMEOUT	(jiffies_to_usecs(HZ))
> > > +
> > > +struct mt6397_rtc {
> > > +	struct device		*dev;
> > > +	struct rtc_device	*rtc_dev;
> > > +
> > > +	/* protect registers accessing */
> > > +	struct mutex		lock;
> > > +	struct regmap		*regmap;
> > > +	int			irq;
> > > +	u32			addr_base;
> > > +};
> > > +
> > > +#endif /* _LINUX_RTC_MT6397_H_ */
> > > +
> > > -- 
> > > 2.7.4
> > > 
> > 
> 
> 

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 06/16] rtc: mediatek: remove unnecessary parentheses

[Sean Wang]

On Sat, 2018-03-24 at 19:53 +0100, Alexandre Belloni wrote:
> On 24/03/2018 at 15:14:12 +0800, Sean Wang wrote:
> > On Fri, 2018-03-23 at 11:21 +0100, Alexandre Belloni wrote:
> > > On 23/03/2018 at 17:15:03 +0800, sean.wang@mediatek.com wrote:
> > > > From: Sean Wang <sean.wang@mediatek.com>
> > > > 
> > > > Remove unnecessary parentheses due to explicit C operator precedence.
> > > > 
> > > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > > > ---
> > > >  drivers/rtc/rtc-mt6397.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > > > index 0df7ccd..4411c08 100644
> > > > --- a/drivers/rtc/rtc-mt6397.c
> > > > +++ b/drivers/rtc/rtc-mt6397.c
> > > > @@ -106,7 +106,7 @@ static irqreturn_t mtk_rtc_irq_handler_thread(int irq, void *data)
> > > >  	int ret;
> > > >  
> > > >  	ret = regmap_read(rtc->regmap, rtc->addr_base + RTC_IRQ_STA, &irqsta);
> > > > -	if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> > > > +	if (ret >= 0 && irqsta & RTC_IRQ_STA_AL) {
> > > 
> > > I don't think this makes the code particularly clearer.
> > > 
> > 
> > But it is still a one of check items in checkpatch
> > 
> > CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'ret >= 0'
> > #126: FILE: drivers/rtc/rtc-xxx.c:109:
> > +       if ((ret >= 0) && (irqsta & RTC_IRQ_STA_AL)) {
> > 
> > 
> > or we still want to keep it in parentheses around here?
> > 
> 
> Yeah, this is a matter of taste, I would keep the parentheses.

okay, lets keep the parentheses

> 
> 

Re: [PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[Sean Wang]

On Fri, 2018-03-23 at 11:15 +0100, Alexandre Belloni wrote:
> On 23/03/2018 at 10:41:18 +0100, Alexandre Belloni wrote:
> > Hi,
> > 
> > On 23/03/2018 at 17:14:59 +0800, sean.wang@mediatek.com wrote:
> > > From: Sean Wang <sean.wang@mediatek.com>
> > > 
> > > Add device-tree binding for MediaTek PMIC based RTC.
> > > 
> > > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > > ---
> > >  .../devicetree/bindings/rtc/rtc-mt6397.txt         | 39 ++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > > new file mode 100644
> > > index 0000000..83ff6be
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/rtc/rtc-mt6397.txt
> > > @@ -0,0 +1,39 @@
> > > +Device-Tree bindings for MediaTek PMIC based RTC
> > > +
> > > +MediaTek PMIC based RTC is an independent function of MediaTek PMIC which
> > > +is working as a multi-function device (MFD). And the RTC can be configured and
> > > +set up via PMIC wrapper bus. Which is also common resource shared among the
> > > +other functions present on the PMIC.
> > > +
> > > +For MediaTek PMIC wrapper bus bindings, see:
> > > +Documentation/devicetree/bindings/soc/mediatek/pwrap.txt
> > > +
> > > +Required parent node:
> > > +- pmic
> > > +  For MediaTek PMIC MFD bindings, see:
> > > +  Documentation/devicetree/bindings/mfd/mt6397.txt
> > > +
> > > +Required properties:
> > > +- compatible: Should be one of follows
> > > +	"mediatek,mt6323-rtc": for MT6323 PMIC
> > > +	"mediatek,mt6397-rtc": for MT6397 PMIC
> > > +
> > > +Optional child node:
> > > +- power-off
> > > +  For Power-Off Device for MediaTek PMIC RTC bindings, see:
> > > +  Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > > +
> > > +Example:
> > > +
> > > +	pmic {
> > > +		compatible = "mediatek,mt6323";
> > > +
> > > +		...
> > > +		rtc {
> > > +			compatible = "mediatek,mt6323-rtc";
> > > +
> > > +			power-off {
> > > +				compatible = "mediatek,mt6323-rtc-poweroff";
> > > +			};
> > 
> > I'm pretty sure the whole point of mfd is to avoid having the poweroff
> > controller under the rtc
> > 
> 
> BTW, I think it is enough to have that documented in only one file (the
> MFD one is enough)
> 

just reply both replies in the same mail

1.) the power-off device is a part of rtc, use the same registers rtc
has and thus it is put as child nodes under the node rtc to reflect the
reality of characteristics the rtc has.

Or am I wrong for a certain aspect in these opinions?

2) the other sub-functions for the same pmic already created its own
dt-binding document belonged to its corresponding subsystem. Don't we
really want to follow it them all?

> > 
> > -- 
> > Alexandre Belloni, Bootlin (formerly Free Electrons)
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
> 

Re: [PATCH v1 11/16] rtc: mediatek: move the declaration into a globally visible header file

[Fabio Estevam]

On Fri, Mar 23, 2018 at 6:15 AM,  <sean.wang@mediatek.com> wrote:

> --- /dev/null
> +++ b/include/linux/rtc/mt6397.h
> @@ -0,0 +1,72 @@
> +
> +// SPDX-License-Identifier: GPL-2.0

According to Documentation/process/license-rules.rst the SPDX notation
for C header file should be:

/* SPDX-License-Identifier: GPL-2.0 */

Re: [PATCH v1 11/16] rtc: mediatek: move the declaration into a globally visible header file

[Sean Wang]

On Sat, 2018-03-24 at 17:00 -0300, Fabio Estevam wrote:
> On Fri, Mar 23, 2018 at 6:15 AM,  <sean.wang@mediatek.com> wrote:
> 
> > --- /dev/null
> > +++ b/include/linux/rtc/mt6397.h
> > @@ -0,0 +1,72 @@
> > +
> > +// SPDX-License-Identifier: GPL-2.0
> 
> According to Documentation/process/license-rules.rst the SPDX notation
> for C header file should be:
> 
> /* SPDX-License-Identifier: GPL-2.0 */


Thank you, I'll have an improvement according to the license rule.

Re: [PATCH v1 12/16] rtc: mediatek: cleanup header files to include

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6558 bytes --]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v4.16-rc6 next-20180323]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/Add-support-to-MT6323-RTC-and-its-power-device/20180325-104529
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: x86_64-randconfig-x004-201812 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers//rtc/rtc-mt6397.c: In function 'mtk_rtc_probe':
>> drivers//rtc/rtc-mt6397.c:270:13: error: implicit declaration of function 'irq_create_mapping'; did you mean 'qid_has_mapping'? [-Werror=implicit-function-declaration]
     rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
                ^~~~~~~~~~~~~~~~~~
                qid_has_mapping
   Cyclomatic Complexity 1 include/linux/err.h:PTR_ERR
   Cyclomatic Complexity 1 include/linux/err.h:IS_ERR
   Cyclomatic Complexity 1 include/linux/math64.h:div_s64_rem
   Cyclomatic Complexity 1 include/linux/math64.h:div_s64
   Cyclomatic Complexity 3 include/linux/ktime.h:ktime_compare
   Cyclomatic Complexity 1 include/linux/ktime.h:ktime_add_us
   Cyclomatic Complexity 1 include/linux/device.h:devm_kzalloc
   Cyclomatic Complexity 1 include/linux/pm_wakeup.h:device_set_wakeup_capable
   Cyclomatic Complexity 1 include/linux/pm_wakeup.h:device_set_wakeup_enable
   Cyclomatic Complexity 1 include/linux/pm_wakeup.h:device_init_wakeup
   Cyclomatic Complexity 1 include/linux/device.h:dev_get_drvdata
   Cyclomatic Complexity 1 include/linux/device.h:dev_set_drvdata
   Cyclomatic Complexity 1 include/linux/platform_device.h:platform_set_drvdata
   Cyclomatic Complexity 1 include/linux/of_platform.h:devm_of_platform_populate
   Cyclomatic Complexity 1 drivers//rtc/rtc-mt6397.c:mtk_rtc_driver_init
   Cyclomatic Complexity 5 drivers//rtc/rtc-mt6397.c:mtk_rtc_probe
   Cyclomatic Complexity 11 drivers//rtc/rtc-mt6397.c:mtk_rtc_write_trigger
   Cyclomatic Complexity 6 drivers//rtc/rtc-mt6397.c:mtk_rtc_set_alarm
   Cyclomatic Complexity 2 drivers//rtc/rtc-mt6397.c:mtk_rtc_set_time
   Cyclomatic Complexity 4 drivers//rtc/rtc-mt6397.c:mtk_rtc_read_alarm
   Cyclomatic Complexity 2 drivers//rtc/rtc-mt6397.c:__mtk_rtc_read_time
   Cyclomatic Complexity 3 drivers//rtc/rtc-mt6397.c:mtk_rtc_read_time
   Cyclomatic Complexity 4 drivers//rtc/rtc-mt6397.c:mtk_rtc_irq_handler_thread
   Cyclomatic Complexity 1 drivers//rtc/rtc-mt6397.c:mtk_rtc_driver_exit
   cc1: some warnings being treated as errors

vim +270 drivers//rtc/rtc-mt6397.c

fc2979118 Tianping Fang  2015-05-06  254  
fc2979118 Tianping Fang  2015-05-06  255  static int mtk_rtc_probe(struct platform_device *pdev)
fc2979118 Tianping Fang  2015-05-06  256  {
fc2979118 Tianping Fang  2015-05-06  257  	struct resource *res;
fc2979118 Tianping Fang  2015-05-06  258  	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
fc2979118 Tianping Fang  2015-05-06  259  	struct mt6397_rtc *rtc;
fc2979118 Tianping Fang  2015-05-06  260  	int ret;
fc2979118 Tianping Fang  2015-05-06  261  
fc2979118 Tianping Fang  2015-05-06  262  	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
fc2979118 Tianping Fang  2015-05-06  263  	if (!rtc)
fc2979118 Tianping Fang  2015-05-06  264  		return -ENOMEM;
fc2979118 Tianping Fang  2015-05-06  265  
fc2979118 Tianping Fang  2015-05-06  266  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
fc2979118 Tianping Fang  2015-05-06  267  	rtc->addr_base = res->start;
fc2979118 Tianping Fang  2015-05-06  268  
fc2979118 Tianping Fang  2015-05-06  269  	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
fc2979118 Tianping Fang  2015-05-06 @270  	rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
fc2979118 Tianping Fang  2015-05-06  271  	if (rtc->irq <= 0)
fc2979118 Tianping Fang  2015-05-06  272  		return -EINVAL;
fc2979118 Tianping Fang  2015-05-06  273  
fc2979118 Tianping Fang  2015-05-06  274  	rtc->regmap = mt6397_chip->regmap;
fc2979118 Tianping Fang  2015-05-06  275  	rtc->dev = &pdev->dev;
fc2979118 Tianping Fang  2015-05-06  276  	mutex_init(&rtc->lock);
fc2979118 Tianping Fang  2015-05-06  277  
fc2979118 Tianping Fang  2015-05-06  278  	platform_set_drvdata(pdev, rtc);
fc2979118 Tianping Fang  2015-05-06  279  
66c231b40 Sean Wang      2018-03-23  280  	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
fc2979118 Tianping Fang  2015-05-06  281  					mtk_rtc_irq_handler_thread,
fc2979118 Tianping Fang  2015-05-06  282  					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
fc2979118 Tianping Fang  2015-05-06  283  					"mt6397-rtc", rtc);
fc2979118 Tianping Fang  2015-05-06  284  	if (ret) {
fc2979118 Tianping Fang  2015-05-06  285  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
fc2979118 Tianping Fang  2015-05-06  286  			rtc->irq, ret);
63044753b Sean Wang      2018-03-23  287  		return ret;
fc2979118 Tianping Fang  2015-05-06  288  	}
fc2979118 Tianping Fang  2015-05-06  289  
baeca4495 Wei-Ning Huang 2015-07-02  290  	device_init_wakeup(&pdev->dev, 1);
baeca4495 Wei-Ning Huang 2015-07-02  291  
66c231b40 Sean Wang      2018-03-23  292  	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "mt6397-rtc",
fc2979118 Tianping Fang  2015-05-06  293  						&mtk_rtc_ops, THIS_MODULE);
fc2979118 Tianping Fang  2015-05-06  294  	if (IS_ERR(rtc->rtc_dev)) {
fc2979118 Tianping Fang  2015-05-06  295  		dev_err(&pdev->dev, "register rtc device failed\n");
fc2979118 Tianping Fang  2015-05-06  296  		ret = PTR_ERR(rtc->rtc_dev);
fc2979118 Tianping Fang  2015-05-06  297  		return ret;
fc2979118 Tianping Fang  2015-05-06  298  	}
fc2979118 Tianping Fang  2015-05-06  299  
89a68f3c0 Sean Wang      2018-03-23  300  	return devm_of_platform_populate(&pdev->dev);
fc2979118 Tianping Fang  2015-05-06  301  }
fc2979118 Tianping Fang  2015-05-06  302  

:::::: The code at line 270 was first introduced by commit
:::::: fc2979118f3f5193475cb53d5df7bdaa7e358a42 rtc: mediatek: Add MT6397 RTC driver

:::::: TO: Tianping Fang <tianping.fang@mediatek.com>
:::::: CC: Alexandre Belloni <alexandre.belloni@free-electrons.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30527 bytes --]

Re: [PATCH v1 12/16] rtc: mediatek: cleanup header files to include

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5005 bytes --]

Hi Sean,

I love your patch! Yet something to improve:

[auto build test ERROR on abelloni/rtc-next]
[also build test ERROR on v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/Add-support-to-MT6323-RTC-and-its-power-device/20180325-104529
base:   https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux.git rtc-next
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All errors (new ones prefixed by >>):

   drivers//rtc/rtc-mt6397.c: In function 'mtk_rtc_probe':
>> drivers//rtc/rtc-mt6397.c:270:13: error: implicit declaration of function 'irq_create_mapping'; did you mean 'page_mapping'? [-Werror=implicit-function-declaration]
     rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
                ^~~~~~~~~~~~~~~~~~
                page_mapping
   cc1: some warnings being treated as errors

vim +270 drivers//rtc/rtc-mt6397.c

fc2979118 Tianping Fang  2015-05-06  254  
fc2979118 Tianping Fang  2015-05-06  255  static int mtk_rtc_probe(struct platform_device *pdev)
fc2979118 Tianping Fang  2015-05-06  256  {
fc2979118 Tianping Fang  2015-05-06  257  	struct resource *res;
fc2979118 Tianping Fang  2015-05-06  258  	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
fc2979118 Tianping Fang  2015-05-06  259  	struct mt6397_rtc *rtc;
fc2979118 Tianping Fang  2015-05-06  260  	int ret;
fc2979118 Tianping Fang  2015-05-06  261  
fc2979118 Tianping Fang  2015-05-06  262  	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
fc2979118 Tianping Fang  2015-05-06  263  	if (!rtc)
fc2979118 Tianping Fang  2015-05-06  264  		return -ENOMEM;
fc2979118 Tianping Fang  2015-05-06  265  
fc2979118 Tianping Fang  2015-05-06  266  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
fc2979118 Tianping Fang  2015-05-06  267  	rtc->addr_base = res->start;
fc2979118 Tianping Fang  2015-05-06  268  
fc2979118 Tianping Fang  2015-05-06  269  	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
fc2979118 Tianping Fang  2015-05-06 @270  	rtc->irq = irq_create_mapping(mt6397_chip->irq_domain, res->start);
fc2979118 Tianping Fang  2015-05-06  271  	if (rtc->irq <= 0)
fc2979118 Tianping Fang  2015-05-06  272  		return -EINVAL;
fc2979118 Tianping Fang  2015-05-06  273  
fc2979118 Tianping Fang  2015-05-06  274  	rtc->regmap = mt6397_chip->regmap;
fc2979118 Tianping Fang  2015-05-06  275  	rtc->dev = &pdev->dev;
fc2979118 Tianping Fang  2015-05-06  276  	mutex_init(&rtc->lock);
fc2979118 Tianping Fang  2015-05-06  277  
fc2979118 Tianping Fang  2015-05-06  278  	platform_set_drvdata(pdev, rtc);
fc2979118 Tianping Fang  2015-05-06  279  
66c231b40 Sean Wang      2018-03-23  280  	ret = devm_request_threaded_irq(&pdev->dev, rtc->irq, NULL,
fc2979118 Tianping Fang  2015-05-06  281  					mtk_rtc_irq_handler_thread,
fc2979118 Tianping Fang  2015-05-06  282  					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
fc2979118 Tianping Fang  2015-05-06  283  					"mt6397-rtc", rtc);
fc2979118 Tianping Fang  2015-05-06  284  	if (ret) {
fc2979118 Tianping Fang  2015-05-06  285  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
fc2979118 Tianping Fang  2015-05-06  286  			rtc->irq, ret);
63044753b Sean Wang      2018-03-23  287  		return ret;
fc2979118 Tianping Fang  2015-05-06  288  	}
fc2979118 Tianping Fang  2015-05-06  289  
baeca4495 Wei-Ning Huang 2015-07-02  290  	device_init_wakeup(&pdev->dev, 1);
baeca4495 Wei-Ning Huang 2015-07-02  291  
66c231b40 Sean Wang      2018-03-23  292  	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "mt6397-rtc",
fc2979118 Tianping Fang  2015-05-06  293  						&mtk_rtc_ops, THIS_MODULE);
fc2979118 Tianping Fang  2015-05-06  294  	if (IS_ERR(rtc->rtc_dev)) {
fc2979118 Tianping Fang  2015-05-06  295  		dev_err(&pdev->dev, "register rtc device failed\n");
fc2979118 Tianping Fang  2015-05-06  296  		ret = PTR_ERR(rtc->rtc_dev);
fc2979118 Tianping Fang  2015-05-06  297  		return ret;
fc2979118 Tianping Fang  2015-05-06  298  	}
fc2979118 Tianping Fang  2015-05-06  299  
89a68f3c0 Sean Wang      2018-03-23  300  	return devm_of_platform_populate(&pdev->dev);
fc2979118 Tianping Fang  2015-05-06  301  }
fc2979118 Tianping Fang  2015-05-06  302  

:::::: The code at line 270 was first introduced by commit
:::::: fc2979118f3f5193475cb53d5df7bdaa7e358a42 rtc: mediatek: Add MT6397 RTC driver

:::::: TO: Tianping Fang <tianping.fang@mediatek.com>
:::::: CC: Alexandre Belloni <alexandre.belloni@free-electrons.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51610 bytes --]

Re: [PATCH v1 08/16] rtc: mediatek: remove unnecessary irq_dispose_mapping

[Sean Wang]

On Fri, 2018-03-23 at 11:38 +0100, Alexandre Belloni wrote:
> On 23/03/2018 at 17:15:05 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > It's unnecessary doing irq_dispose_mapping as a reverse operation for
> > platform_get_irq.
> > 
> > Ususally, irq_dispose_mapping should be called in error path or module
> > removal to release the resources for irq_of_parse_and_map requested.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  drivers/rtc/rtc-mt6397.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > index b62eaa8..cefb83b 100644
> > --- a/drivers/rtc/rtc-mt6397.c
> > +++ b/drivers/rtc/rtc-mt6397.c
> > @@ -17,7 +17,6 @@
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> >  #include <linux/rtc.h>
> > -#include <linux/irqdomain.h>
> >  #include <linux/jiffies.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/of_address.h>
> > @@ -336,7 +335,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "Failed to request alarm IRQ: %d: %d\n",
> >  			rtc->irq, ret);
> > -		goto out_dispose_irq;
> > +		return ret;
> >  	}
> >  
> >  	device_init_wakeup(&pdev->dev, 1);
> > @@ -353,8 +352,7 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> >  
> >  out_free_irq:
> >  	free_irq(rtc->irq, rtc->rtc_dev);
> > -out_dispose_irq:
> > -	irq_dispose_mapping(rtc->irq);
> > +
> 
> Don't you still have a irq_create_mapping?
> 

Sorry for that I didn't mention in the beginning that the series must
depend on another patch [1]. With the patch, the job irq_create_mapping
had been moved from rtc to mfd, so here it should be better to cleanup
up irq_dispose_mapping in all paths.

[1] https://patchwork.kernel.org/patch/9954643/

> >  	return ret;
> >  }
> >  
> > @@ -364,7 +362,6 @@ static int mtk_rtc_remove(struct platform_device *pdev)
> >  
> >  	rtc_device_unregister(rtc->rtc_dev);
> >  	free_irq(rtc->irq, rtc->rtc_dev);
> > -	irq_dispose_mapping(rtc->irq);
> >  
> >  	return 0;
> >  }
> > -- 
> > 2.7.4
> > 
> 

Re: [PATCH v1 09/16] rtc: mediatek: convert to use device managed functions

[Sean Wang]

On Fri, 2018-03-23 at 11:50 +0100, Alexandre Belloni wrote:
> On 23/03/2018 at 17:15:06 +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > Use device managed operation to simplify error handling, reduce source
> > code size, and reduce the likelyhood of bugs, and remove our removal
> > callback which contains anything already done by device managed functions.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  drivers/rtc/rtc-mt6397.c | 31 ++++++++-----------------------
> >  1 file changed, 8 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/rtc/rtc-mt6397.c b/drivers/rtc/rtc-mt6397.c
> > index cefb83b..bfc5d6f 100644
> > --- a/drivers/rtc/rtc-mt6397.c
> > +++ b/drivers/rtc/rtc-mt6397.c
> > @@ -14,6 +14,7 @@
> >  
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > +#include <linux/interrupt.h>
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> >  #include <linux/rtc.h>
> > @@ -328,10 +329,10 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> >  
> >  	platform_set_drvdata(pdev, rtc);
> >  
> > -	ret = request_threaded_irq(rtc->irq, NULL,
> > -				   mtk_rtc_irq_handler_thread,
> > -				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> > -				   "mt6397-rtc", 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);
> > @@ -340,30 +341,15 @@ static int mtk_rtc_probe(struct platform_device *pdev)
> >  
> >  	device_init_wakeup(&pdev->dev, 1);
> >  
> > -	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > -					   &mtk_rtc_ops, THIS_MODULE);
> > +	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "mt6397-rtc",
> > +						&mtk_rtc_ops, THIS_MODULE);
> 
> You should probably switch to devm_rtc_allocate_device() and
> rtc_register_device instead of devm_rtc_device_register.
> 

Just would like to know something details

It seems you just encourage me to switch into the new registration
method and currently devm_rtc_device_register I used for the driver
shouldn't cause any harm. right?

> >  	if (IS_ERR(rtc->rtc_dev)) {
> >  		dev_err(&pdev->dev, "register rtc device failed\n");
> >  		ret = PTR_ERR(rtc->rtc_dev);
> > -		goto out_free_irq;
> > +		return ret;
> 
> ret doesn't seem necessary anymore here.


okay, it'll be removed

> 
> 

Re: [PATCH v1 01/16] dt-bindings: power: reset: mediatek: add bindings for power device

[Rob Herring]

On Fri, Mar 23, 2018 at 05:14:58PM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add device-tree binding for the power device responsible for shutdown a
> remote SoC, which is a tiny circuit block present on MediaTek PMIC based
> RTC.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  .../bindings/power/reset/mt6397-rtc-poweroff.txt   | 24 ++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt b/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> new file mode 100644
> index 0000000..75a9d7d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> @@ -0,0 +1,24 @@
> +Device-Tree bindings for Power Device on MediaTek PMIC RTC
> +
> +The power device is responsible for externally power off or on the remote
> +MediaTek SoC through the a tiny circuit block BBPU inside PMIC RTC.
> +
> +Required parent node:
> +- rtc
> +	For MediaTek PMIC RTC bindings, see:
> +	Documentation/devicetree/bindings/mfd/mt6397.txt
> +
> +Required properties:
> +- compatible: Should be one of follows
> +	"mediatek,mt6323-rtc-poweroff": for MT6323 PMIC
> +	"mediatek,mt6397-rtc-poweroff": for MT6397 PMIC
> +
> +Example:
> +
> +	rtc {
> +		compatible = "mediatek,mt6323-rtc";
> +
> +		power-off {
> +			compatible = "mediatek,mt6323-rtc-poweroff";
> +		};

There's no need for this node. The OS can register a device in the rtc 
driver.

> +	};
> -- 
> 2.7.4
> 

Re: [PATCH v1 03/16] dt-bindings: mfd: mediatek: add a description for MT6323 RTC

[Rob Herring]

On Fri, Mar 23, 2018 at 05:15:00PM +0800, sean.wang@mediatek.com wrote:
> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add a description for MT6323 RTC and link it to the detailed binding
> documentation.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mfd/mt6397.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v1 01/16] dt-bindings: power: reset: mediatek: add bindings for power device

[Sean Wang]

On Mon, 2018-03-26 at 17:24 -0500, Rob Herring wrote:
> On Fri, Mar 23, 2018 at 05:14:58PM +0800, sean.wang@mediatek.com wrote:
> > From: Sean Wang <sean.wang@mediatek.com>
> > 
> > Add device-tree binding for the power device responsible for shutdown a
> > remote SoC, which is a tiny circuit block present on MediaTek PMIC based
> > RTC.
> > 
> > Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> > ---
> >  .../bindings/power/reset/mt6397-rtc-poweroff.txt   | 24 ++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt b/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > new file mode 100644
> > index 0000000..75a9d7d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/reset/mt6397-rtc-poweroff.txt
> > @@ -0,0 +1,24 @@
> > +Device-Tree bindings for Power Device on MediaTek PMIC RTC
> > +
> > +The power device is responsible for externally power off or on the remote
> > +MediaTek SoC through the a tiny circuit block BBPU inside PMIC RTC.
> > +
> > +Required parent node:
> > +- rtc
> > +	For MediaTek PMIC RTC bindings, see:
> > +	Documentation/devicetree/bindings/mfd/mt6397.txt
> > +
> > +Required properties:
> > +- compatible: Should be one of follows
> > +	"mediatek,mt6323-rtc-poweroff": for MT6323 PMIC
> > +	"mediatek,mt6397-rtc-poweroff": for MT6397 PMIC
> > +
> > +Example:
> > +
> > +	rtc {
> > +		compatible = "mediatek,mt6323-rtc";
> > +
> > +		power-off {
> > +			compatible = "mediatek,mt6323-rtc-poweroff";
> > +		};
> 
> There's no need for this node. The OS can register a device in the rtc 
> driver.
> 

It seems a good way.

I will remove the really simple dt-binding and use a
platform_device_register_simple api embedded in rtc driver to
register the power-off device.

Hope I do not misconception your points addressed here.

> > +	};
> > -- 
> > 2.7.4
> > 

Re: [PATCH v1 09/16] rtc: mediatek: convert to use device managed functions

[Alexandre Belloni]

On 26/03/2018 at 12:07:45 +0800, Sean Wang wrote:
> > > -	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > > -					   &mtk_rtc_ops, THIS_MODULE);
> > > +	rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "mt6397-rtc",
> > > +						&mtk_rtc_ops, THIS_MODULE);
> > 
> > You should probably switch to devm_rtc_allocate_device() and
> > rtc_register_device instead of devm_rtc_device_register.
> > 
> 
> Just would like to know something details
> 
> It seems you just encourage me to switch into the new registration
> method and currently devm_rtc_device_register I used for the driver
> shouldn't cause any harm. right?
> 

It will work but it will have to be converted to rtc_register_device
later anyway.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[Alexandre Belloni]

On 25/03/2018 at 03:36:28 +0800, Sean Wang wrote:
> just reply both replies in the same mail
> 
> 1.) the power-off device is a part of rtc, use the same registers rtc
> has and thus it is put as child nodes under the node rtc to reflect the
> reality of characteristics the rtc has.
> 
> Or am I wrong for a certain aspect in these opinions?
> 

My point is that it is also part of the PMIC so it may as well be
registers from the mfd driver which already registers a bunch of devices
instead of doing unusual stuff from the rtc driver.

mt6397_rtc->regmap is mt6397_chip->regmap anyway. You have the added
benefit that if the RTC driver probe fails for some reason, you may
still be able to probe the reset driver.

I don't tink there is any benefit having it as a child of the rtc
device.

> 2) the other sub-functions for the same pmic already created its own
> dt-binding document belonged to its corresponding subsystem. Don't we
> really want to follow it them all?
> 

Ok, that's fine.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[Sean Wang]

On Tue, 2018-03-27 at 17:18 +0200, Alexandre Belloni wrote:
> On 25/03/2018 at 03:36:28 +0800, Sean Wang wrote:
> > just reply both replies in the same mail
> > 
> > 1.) the power-off device is a part of rtc, use the same registers rtc
> > has and thus it is put as child nodes under the node rtc to reflect the
> > reality of characteristics the rtc has.
> > 
> > Or am I wrong for a certain aspect in these opinions?
> > 
> 
> My point is that it is also part of the PMIC so it may as well be
> registers from the mfd driver which already registers a bunch of devices
> instead of doing unusual stuff from the rtc driver.
> 
> mt6397_rtc->regmap is mt6397_chip->regmap anyway. You have the added
> benefit that if the RTC driver probe fails for some reason, you may
> still be able to probe the reset driver.
> 
> I don't tink there is any benefit having it as a child of the rtc
> device.
> 


really thanks! it's an optional solution I thought it 's fine and worth
doing

but so far I cannot fully make sure of whether mfd can accept two
devices holding overlay IORESOURCE_MEM.

Or do you like Rob's suggestion in [1] ? By which, I tend to embed a
sub-device with platform_device_register_data api in the rtc probe()
instead of treating it as a dt node under rtc node, but which seems
something a bit violates your preferences :(

Just confirm to know which way I should step into before I produce next
version.

[1]
http://lists.infradead.org/pipermail/linux-mediatek/2018-March/012576.html

> > 2) the other sub-functions for the same pmic already created its own
> > dt-binding document belonged to its corresponding subsystem. Don't we
> > really want to follow it them all?
> > 
> 
> Ok, that's fine.
> 

Re: [PATCH v1 02/16] dt-bindings: rtc: mediatek: add bindings for PMIC RTC

[Alexandre Belloni]

On 28/03/2018 at 11:53:17 +0800, Sean Wang wrote:
> On Tue, 2018-03-27 at 17:18 +0200, Alexandre Belloni wrote:
> > On 25/03/2018 at 03:36:28 +0800, Sean Wang wrote:
> > > just reply both replies in the same mail
> > > 
> > > 1.) the power-off device is a part of rtc, use the same registers rtc
> > > has and thus it is put as child nodes under the node rtc to reflect the
> > > reality of characteristics the rtc has.
> > > 
> > > Or am I wrong for a certain aspect in these opinions?
> > > 
> > 
> > My point is that it is also part of the PMIC so it may as well be
> > registers from the mfd driver which already registers a bunch of devices
> > instead of doing unusual stuff from the rtc driver.
> > 
> > mt6397_rtc->regmap is mt6397_chip->regmap anyway. You have the added
> > benefit that if the RTC driver probe fails for some reason, you may
> > still be able to probe the reset driver.
> > 
> > I don't tink there is any benefit having it as a child of the rtc
> > device.
> > 
> 
> 
> really thanks! it's an optional solution I thought it 's fine and worth
> doing
> 
> but so far I cannot fully make sure of whether mfd can accept two
> devices holding overlay IORESOURCE_MEM.
> 

There is no overlay because you are using a regmap which handles
concurrency for you.

What your patch is doing is:

struct mt6397_rtc *rtc = dev_get_drvdata(pdev->dev.parent);

then you use rtc->regmap

But in the rtc driver, you have:

struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
struct mt6397_rtc *rtc;

rtc->regmap = mt6397_chip->regmap;

So there is no benefit from being the child of the rtc, you could just
do the following in your reset driver:

struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);

and then use mt6397_chip->regmap.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v1 03/16] dt-bindings: mfd: mediatek: add a description for MT6323 RTC

[Lee Jones]

On Fri, 23 Mar 2018, sean.wang@mediatek.com wrote:

> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add a description for MT6323 RTC and link it to the detailed binding
> documentation.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  Documentation/devicetree/bindings/mfd/mt6397.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 04/16] mfd: mt6397: add MT6323 RTC support into MT6397 driver

[Lee Jones]

On Fri, 23 Mar 2018, sean.wang@mediatek.com wrote:

> From: Sean Wang <sean.wang@mediatek.com>
> 
> Add compatible string as "mt6323-rtc" that will make the OF core spawn
> child devices for the RTC subnode of that MT6323 MFD node.
> 
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> ---
>  drivers/mfd/mt6397-core.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
> index 77b64bd..f71874a 100644
> --- a/drivers/mfd/mt6397-core.c
> +++ b/drivers/mfd/mt6397-core.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2014 MediaTek Inc.
> + * Copyright (c) 2014-2018 MediaTek Inc.
>   * Author: Flora Fu, MediaTek
>   *
>   * This program is free software; you can redistribute it and/or modify
> @@ -23,6 +23,9 @@
>  #include <linux/mfd/mt6397/registers.h>
>  #include <linux/mfd/mt6323/registers.h>
>  
> +#define MT6323_RTC_BASE		0x8000
> +#define MT6323_RTC_SIZE		0x3e
> +
>  #define MT6397_RTC_BASE		0xe000
>  #define MT6397_RTC_SIZE		0x3e
>  
> @@ -30,6 +33,19 @@
>  #define MT6391_CID_CODE		0x91
>  #define MT6397_CID_CODE		0x97
>  
> +static const struct resource mt6323_rtc_resources[] = {
> +	{
> +		.start = MT6323_RTC_BASE,
> +		.end   = MT6323_RTC_BASE + MT6323_RTC_SIZE,
> +		.flags = IORESOURCE_MEM,
> +	},
> +	{
> +		.start = MT6323_IRQ_STATUS_RTC,
> +		.end   = MT6323_IRQ_STATUS_RTC,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};

Please use the DEFINE_RES_* helpers instead.

Defined in: include/linux/ioport.h

>  static const struct resource mt6397_rtc_resources[] = {
>  	{
>  		.start = MT6397_RTC_BASE,
> @@ -55,6 +71,11 @@ static const struct resource mt6397_keys_resources[] = {
>  
>  static const struct mfd_cell mt6323_devs[] = {
>  	{
> +		.name = "mt6323-rtc",
> +		.num_resources = ARRAY_SIZE(mt6323_rtc_resources),
> +		.resources = mt6323_rtc_resources,
> +		.of_compatible = "mediatek,mt6323-rtc",
> +	}, {
>  		.name = "mt6323-regulator",
>  		.of_compatible = "mediatek,mt6323-regulator"
>  	}, {

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog