[PATCH 0/3] Add support for nuvoton ma35d1 rtc controller

[PATCH 0/3] Add support for nuvoton ma35d1 rtc controller

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

This patch series adds the rtc driver for the nuvoton ma35d1 ARMv8 SoC.
It includes DT binding documentation, the ma35d1 rtc driver, and device
tree updates.

The ma35d1 rtc controller provides real-time and calendar messaging
capabilities. It supports programmable time tick and alarm match
interrupts. The time and calendar messages are expressed in BCD format.

This rtc driver has been tested on the ma35d1 som board with Linux 6.5-rc2.

Jacky Huang (3):
  dt-bindings: rtc: Document nuvoton ma35d1 rtc driver
  arm64: dts: nuvoton: Add rtc for ma35d1
  rtc: Add driver for nuvoton ma35d1 rtc controller

 .../bindings/rtc/nuvoton,ma35d1-rtc.yaml      |  45 +++
 .../boot/dts/nuvoton/ma35d1-iot-512m.dts      |   4 +
 .../boot/dts/nuvoton/ma35d1-som-256m.dts      |   4 +
 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi       |   8 +
 drivers/rtc/Kconfig                           |  11 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-ma35d1.c                      | 371 ++++++++++++++++++
 7 files changed, 444 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
 create mode 100644 drivers/rtc/rtc-ma35d1.c

-- 
2.34.1

[PATCH 1/3] dt-bindings: rtc: Document nuvoton ma35d1 rtc driver

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

Add documentation to describe nuvoton ma35d1 rtc driver.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 .../bindings/rtc/nuvoton,ma35d1-rtc.yaml      | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml

diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
new file mode 100644
index 000000000000..08c30f3018fb
--- /dev/null
+++ b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rtc/nuvoton,ma35d1-rtc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 Read Time Clock
+
+maintainers:
+  - Min-Jen Chen <mjchen@nuvoton.com>
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,ma35d1-rtc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+    rtc@40410000 {
+        compatible = "nuvoton,ma35d1-rtc";
+        reg = <0x40410000 0x200>;
+        interrupts = <GIC_SPI 5 IRQ_TYPE_EDGE_RISING>;
+        clocks = <&clk RTC_GATE>;
+    };
+
+...
-- 
2.34.1

Re: [PATCH 1/3] dt-bindings: rtc: Document nuvoton ma35d1 rtc driver

[Krzysztof Kozlowski]

On 20/07/2023 03:28, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> Add documentation to describe nuvoton ma35d1 rtc driver.
> 
> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
> ---
>  .../bindings/rtc/nuvoton,ma35d1-rtc.yaml      | 45 +++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
> new file mode 100644
> index 000000000000..08c30f3018fb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/rtc/nuvoton,ma35d1-rtc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 Read Time Clock
> +
> +maintainers:
> +  - Min-Jen Chen <mjchen@nuvoton.com>
> +

Missing ref to rtc.yaml.


> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,ma35d1-rtc
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +
> +additionalProperties: false

And then use unevaluatedProperties: false instead.


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: rtc: Document nuvoton ma35d1 rtc driver

[Jacky Huang]

On 2023/7/20 下午 02:10, Krzysztof Kozlowski wrote:
> On 20/07/2023 03:28, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> Add documentation to describe nuvoton ma35d1 rtc driver.
>>
>> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
>> ---
>>   .../bindings/rtc/nuvoton,ma35d1-rtc.yaml      | 45 +++++++++++++++++++
>>   1 file changed, 45 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
>> new file mode 100644
>> index 000000000000..08c30f3018fb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/rtc/nuvoton,ma35d1-rtc.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/rtc/nuvoton,ma35d1-rtc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Nuvoton MA35D1 Read Time Clock
>> +
>> +maintainers:
>> +  - Min-Jen Chen <mjchen@nuvoton.com>
>> +
> Missing ref to rtc.yaml.
>
>
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - nuvoton,ma35d1-rtc
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +
>> +additionalProperties: false
> And then use unevaluatedProperties: false instead.
>
>
> Best regards,
> Krzysztof
>

Thank you for the advice.
I will fix these two issues in the next version.


Best Regards,
Jacky Huang

[PATCH 2/3] arm64: dts: nuvoton: Add rtc for ma35d1

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

Add rtc controller support to the dtsi of ma35d1 SoC and
enable rtc on SOM and IoT boards.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts | 4 ++++
 arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts | 4 ++++
 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi         | 8 ++++++++
 3 files changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
index b89e2be6abae..b3be4331abcf 100644
--- a/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts
@@ -54,3 +54,7 @@ &clk {
 			   "integer",
 			   "integer";
 };
+
+&rtc {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
index a1ebddecb7f8..9858788a589c 100644
--- a/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts
@@ -54,3 +54,7 @@ &clk {
 			   "integer",
 			   "integer";
 };
+
+&rtc {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
index 781cdae566a0..394395bfd3ae 100644
--- a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
+++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi
@@ -95,6 +95,14 @@ clk: clock-controller@40460200 {
 			clocks = <&clk_hxt>;
 		};
 
+		rtc: rtc@40410000 {
+			compatible = "nuvoton,ma35d1-rtc";
+			reg = <0x0 0x40410000 0x0 0x200>;
+			interrupts = <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk RTC_GATE>;
+			status = "disabled";
+		};
+
 		uart0: serial@40700000 {
 			compatible = "nuvoton,ma35d1-uart";
 			reg = <0x0 0x40700000 0x0 0x100>;
-- 
2.34.1

[PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller

[Jacky Huang]

From: Jacky Huang <ychuang3@nuvoton.com>

The ma35d1 rtc controller provides real-time and calendar messaging
capabilities. It supports programmable time tick and alarm match
interrupts. The time and calendar messages are expressed in BCD format.
This driver supports the built-in rtc controller of the ma35d1. It
enables setting and reading the rtc time and configuring and reading
the rtc alarm.

Signed-off-by: Jacky Huang <ychuang3@nuvoton.com>
---
 drivers/rtc/Kconfig      |  11 ++
 drivers/rtc/Makefile     |   1 +
 drivers/rtc/rtc-ma35d1.c | 371 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 383 insertions(+)
 create mode 100644 drivers/rtc/rtc-ma35d1.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05f4b2d66290..95ddd8f616f4 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1929,6 +1929,17 @@ config RTC_DRV_TI_K3
 	  This driver can also be built as a module, if so, the module
 	  will be called "rtc-ti-k3".
 
+config RTC_DRV_MA35D1
+	tristate "Nuvoton MA35D1 RTC"
+	depends on ARCH_MA35 || COMPILE_TEST
+	select REGMAP_MMIO
+	help
+	   If you say yes here you get support for the Nuvoton MA35D1
+	   On-Chip Real Time Clock.
+
+	   This driver can also be built as a module, if so, the module
+	   will be called "rtc-ma35d1".
+
 comment "HID Sensor RTC drivers"
 
 config RTC_DRV_HID_SENSOR_TIME
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index fd209883ee2e..763c9cb5dde1 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -88,6 +88,7 @@ obj-$(CONFIG_RTC_DRV_M41T94)	+= rtc-m41t94.o
 obj-$(CONFIG_RTC_DRV_M48T35)	+= rtc-m48t35.o
 obj-$(CONFIG_RTC_DRV_M48T59)	+= rtc-m48t59.o
 obj-$(CONFIG_RTC_DRV_M48T86)	+= rtc-m48t86.o
+obj-$(CONFIG_RTC_DRV_MA35D1)	+= rtc-ma35d1.o
 obj-$(CONFIG_RTC_DRV_MAX6900)	+= rtc-max6900.o
 obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
 obj-$(CONFIG_RTC_DRV_MAX6916)	+= rtc-max6916.o
diff --git a/drivers/rtc/rtc-ma35d1.c b/drivers/rtc/rtc-ma35d1.c
new file mode 100644
index 000000000000..48c991a8f669
--- /dev/null
+++ b/drivers/rtc/rtc-ma35d1.c
@@ -0,0 +1,371 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RTC driver for Nuvoton MA35D1
+ *
+ * Copyright (C) 2023 Nuvoton Technology Corp.
+ */
+
+#include <linux/bcd.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+
+/* MA35D1 RTC Control Registers */
+#define MA35_REG_RTC_INIT	0x00
+#define MA35_REG_RTC_SINFASTS	0x04
+#define MA35_REG_RTC_FREQADJ	0x08
+#define MA35_REG_RTC_TIME	0x0c
+#define MA35_REG_RTC_CAL	0x10
+#define MA35_REG_RTC_CLKFMT	0x14
+#define MA35_REG_RTC_WEEKDAY	0x18
+#define MA35_REG_RTC_TALM	0x1c
+#define MA35_REG_RTC_CALM	0x20
+#define MA35_REG_RTC_LEAPYEAR	0x24
+#define MA35_REG_RTC_INTEN	0x28
+#define MA35_REG_RTC_INTSTS	0x2c
+#define MA35_REG_RTC_TICK	0x30
+
+/* register MA35_REG_RTC_INIT */
+#define RTC_INIT_ACTIVE		BIT(0)
+#define RTC_INIT_MAGIC_CODE	0xa5eb1357
+
+/* register MA35_REG_RTC_CLKFMT */
+#define RTC_CLKFMT_24HEN	BIT(0)
+#define RTC_CLKFMT_DCOMPEN	BIT(16)
+
+/* register MA35_REG_RTC_INTEN */
+#define RTC_INTEN_ALMIEN	BIT(0)
+#define RTC_INTEN_TICKIEN	BIT(1)
+#define RTC_INTEN_CLKFIEN	BIT(24)
+#define RTC_INTEN_CLKSTIEN	BIT(25)
+
+/* register MA35_REG_RTC_INTSTS */
+#define RTC_INTSTS_ALMIF	BIT(0)
+#define RTC_INTSTS_TICKIF	BIT(1)
+#define RTC_INTSTS_CLKFIF	BIT(24)
+#define RTC_INTSTS_CLKSTIF	BIT(25)
+
+#define RTC_INIT_TIMEOUT	250
+
+struct ma35_rtc {
+	int irq_num;
+	void __iomem *rtc_reg;
+	struct rtc_device *rtcdev;
+};
+
+struct ma35_bcd_time {
+	int bcd_sec;
+	int bcd_min;
+	int bcd_hour;
+	int bcd_mday;
+	int bcd_mon;
+	int bcd_year;
+};
+
+static u32 rtc_reg_read(struct ma35_rtc *p, u32 offset)
+{
+	return __raw_readl(p->rtc_reg + offset);
+}
+
+static inline void rtc_reg_write(struct ma35_rtc *p, u32 offset, u32 value)
+{
+	__raw_writel(value, p->rtc_reg + offset);
+}
+
+static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
+{
+	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
+	unsigned long events = 0, rtc_irq;
+
+	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
+
+	if (rtc_irq & RTC_INTSTS_ALMIF) {
+		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
+		events |= RTC_AF | RTC_IRQF;
+	}
+
+	if (rtc_irq & RTC_INTSTS_TICKIF) {
+		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
+		events |= RTC_UF | RTC_IRQF;
+	}
+
+	rtc_update_irq(rtc->rtcdev, 1, events);
+
+	return IRQ_HANDLED;
+}
+
+static int ma35d1_rtc_init(struct ma35_rtc *ma35_rtc, u32 ms_timeout)
+{
+	const unsigned long timeout = jiffies + msecs_to_jiffies(ms_timeout);
+
+	do {
+		if (rtc_reg_read(ma35_rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACTIVE)
+			return 0;
+
+		rtc_reg_write(ma35_rtc, MA35_REG_RTC_INIT, RTC_INIT_MAGIC_CODE);
+
+		mdelay(1);
+
+	} while (time_before(jiffies, timeout));
+
+	return -ETIMEDOUT;
+}
+
+static int ma35d1_rtc_bcd2bin(u32 time, u32 cal, u32 wday, struct rtc_time *tm)
+{
+	tm->tm_mday	= bcd2bin(cal >> 0);
+	tm->tm_mon	= bcd2bin(cal >> 8);
+	tm->tm_mon	= tm->tm_mon - 1;
+	tm->tm_year	= bcd2bin(cal >> 16) + 100;
+
+	tm->tm_sec	= bcd2bin(time >> 0);
+	tm->tm_min	= bcd2bin(time >> 8);
+	tm->tm_hour	= bcd2bin(time >> 16);
+
+	tm->tm_wday = wday;
+
+	return rtc_valid_tm(tm);
+}
+
+static int ma35d1_rtc_alarm_bcd2bin(u32 talm, u32 calm, struct rtc_time *tm)
+{
+	tm->tm_mday	= bcd2bin(calm >> 0);
+	tm->tm_mon	= bcd2bin(calm >> 8);
+	tm->tm_mon	= tm->tm_mon - 1;
+	tm->tm_year	= bcd2bin(calm >> 16) + 100;
+
+	tm->tm_sec	= bcd2bin(talm >> 0);
+	tm->tm_min	= bcd2bin(talm >> 8);
+	tm->tm_hour	= bcd2bin(talm >> 16);
+
+	return rtc_valid_tm(tm);
+}
+
+static void ma35d1_rtc_bin2bcd(struct device *dev, struct rtc_time *settm,
+			       struct ma35_bcd_time *gettm)
+{
+	gettm->bcd_mday = bin2bcd(settm->tm_mday) << 0;
+	gettm->bcd_mon  = bin2bcd((settm->tm_mon + 1)) << 8;
+
+	if (settm->tm_year < 100) {
+		dev_warn(dev, "The year will be between 1970-1999, right?\n");
+		gettm->bcd_year = bin2bcd(settm->tm_year) << 16;
+	} else {
+		gettm->bcd_year = bin2bcd(settm->tm_year - 100) << 16;
+	}
+
+	gettm->bcd_sec  = bin2bcd(settm->tm_sec) << 0;
+	gettm->bcd_min  = bin2bcd(settm->tm_min) << 8;
+	gettm->bcd_hour = bin2bcd(settm->tm_hour) << 16;
+}
+
+static int ma35d1_alarm_irq_enable(struct device *dev, u32 enabled)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	u32 reg_ien;
+
+	reg_ien = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
+
+	if (enabled)
+		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien | RTC_INTEN_ALMIEN);
+	else
+		rtc_reg_write(rtc, MA35_REG_RTC_INTEN, reg_ien & ~RTC_INTEN_ALMIEN);
+
+	return 0;
+}
+
+static int ma35d1_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	u32 time, cal, wday;
+
+	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
+	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
+	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
+
+	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
+}
+
+static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	struct ma35_bcd_time gettm;
+	u32 val;
+
+	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
+
+	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
+	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
+
+	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
+	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
+
+	val = tm->tm_wday;
+	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
+
+	return 0;
+}
+
+static int ma35d1_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	u32 talm, calm;
+
+	talm = rtc_reg_read(rtc, MA35_REG_RTC_TALM);
+	calm  = rtc_reg_read(rtc, MA35_REG_RTC_CALM);
+
+	return ma35d1_rtc_alarm_bcd2bin(talm, calm, &alrm->time);
+}
+
+static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
+{
+	struct ma35_rtc *rtc = dev_get_drvdata(dev);
+	struct ma35_bcd_time tm;
+	unsigned long val;
+
+	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
+
+	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
+	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
+
+	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
+	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
+
+	return 0;
+}
+
+static const struct rtc_class_ops ma35d1_rtc_ops = {
+	.read_time = ma35d1_rtc_read_time,
+	.set_time = ma35d1_rtc_set_time,
+	.read_alarm = ma35d1_rtc_read_alarm,
+	.set_alarm = ma35d1_rtc_set_alarm,
+	.alarm_irq_enable = ma35d1_alarm_irq_enable,
+};
+
+static int ma35d1_rtc_probe(struct platform_device *pdev)
+{
+	struct ma35_rtc *ma35_rtc;
+	struct clk *clk;
+	u32 regval;
+	int err;
+
+	ma35_rtc = devm_kzalloc(&pdev->dev, sizeof(struct ma35_rtc),
+								GFP_KERNEL);
+	if (!ma35_rtc)
+		return -ENOMEM;
+
+	ma35_rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ma35_rtc->rtc_reg))
+		return PTR_ERR(ma35_rtc->rtc_reg);
+
+	clk = of_clk_get(pdev->dev.of_node, 0);
+	if (IS_ERR(clk)) {
+		err = PTR_ERR(clk);
+		dev_err(&pdev->dev, "failed to get core clk: %d\n", err);
+		return -ENOENT;
+	}
+	err = clk_prepare_enable(clk);
+	if (err)
+		return -ENOENT;
+
+	platform_set_drvdata(pdev, ma35_rtc);
+
+	ma35_rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
+						    &ma35d1_rtc_ops, THIS_MODULE);
+	if (IS_ERR(ma35_rtc->rtcdev)) {
+		dev_err(&pdev->dev, "rtc device register failed\n");
+		return PTR_ERR(ma35_rtc->rtcdev);
+	}
+
+	err = ma35d1_rtc_init(ma35_rtc, RTC_INIT_TIMEOUT);
+	if (err)
+		return err;
+
+	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_CLKFMT);
+	regval |= RTC_CLKFMT_24HEN;
+	rtc_reg_write(ma35_rtc, MA35_REG_RTC_CLKFMT, regval);
+
+	ma35_rtc->irq_num = platform_get_irq(pdev, 0);
+
+	if (devm_request_irq(&pdev->dev, ma35_rtc->irq_num, ma35d1_rtc_interrupt,
+			     IRQF_NO_SUSPEND, "ma35d1rtc", ma35_rtc)) {
+		dev_err(&pdev->dev, "ma35d1 RTC request irq failed\n");
+		return -EBUSY;
+	}
+
+	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
+	regval |= RTC_INTEN_TICKIEN;
+	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
+
+	device_init_wakeup(&pdev->dev, true);
+
+	return 0;
+}
+
+static int __exit ma35d1_rtc_remove(struct platform_device *pdev)
+{
+	device_init_wakeup(&pdev->dev, false);
+
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev);
+	u32 regval;
+
+	if (device_may_wakeup(&pdev->dev))
+		enable_irq_wake(ma35_rtc->irq_num);
+
+	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
+	regval &= ~RTC_INTEN_TICKIEN;
+	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
+
+	return 0;
+}
+
+static int ma35d1_rtc_resume(struct platform_device *pdev)
+{
+	struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev);
+	u32 regval;
+
+	if (device_may_wakeup(&pdev->dev))
+		disable_irq_wake(ma35_rtc->irq_num);
+
+	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
+	regval |= RTC_INTEN_TICKIEN;
+	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
+
+	return 0;
+}
+
+static const struct of_device_id ma35d1_rtc_of_match[] = {
+	{ .compatible = "nuvoton,ma35d1-rtc", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
+
+static struct platform_driver ma35d1_rtc_driver = {
+	.remove     = __exit_p(ma35d1_rtc_remove),
+	.suspend    = ma35d1_rtc_suspend,
+	.resume     = ma35d1_rtc_resume,
+	.probe      = ma35d1_rtc_probe,
+	.driver		= {
+		.name	= "rtc-ma35d1",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(ma35d1_rtc_of_match),
+	},
+};
+
+module_platform_driver(ma35d1_rtc_driver);
+
+MODULE_AUTHOR("Min-Jen Chen <mjchen@nuvoton.com>");
+MODULE_DESCRIPTION("MA35D1 RTC driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller

[Krzysztof Kozlowski]

On 20/07/2023 03:28, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@nuvoton.com>
> 
> The ma35d1 rtc controller provides real-time and calendar messaging
> capabilities. It supports programmable time tick and alarm match
> interrupts. The time and calendar messages are expressed in BCD format.
> This driver supports the built-in rtc controller of the ma35d1. It
> enables setting and reading the rtc time and configuring and reading
> the rtc alarm.
> 

...

> +static int ma35d1_rtc_probe(struct platform_device *pdev)
> +{
> +	struct ma35_rtc *ma35_rtc;
> +	struct clk *clk;
> +	u32 regval;
> +	int err;
> +
> +	ma35_rtc = devm_kzalloc(&pdev->dev, sizeof(struct ma35_rtc),

sizeof(*)

> +								GFP_KERNEL);
> +	if (!ma35_rtc)
> +		return -ENOMEM;
> +
> +	ma35_rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ma35_rtc->rtc_reg))
> +		return PTR_ERR(ma35_rtc->rtc_reg);
> +
> +	clk = of_clk_get(pdev->dev.of_node, 0);
> +	if (IS_ERR(clk)) {
> +		err = PTR_ERR(clk);
> +		dev_err(&pdev->dev, "failed to get core clk: %d\n", err);
> +		return -ENOENT;

return dev_err_probe

> +	}
> +	err = clk_prepare_enable(clk);
> +	if (err)
> +		return -ENOENT;
> +
> +	platform_set_drvdata(pdev, ma35_rtc);
> +
> +	ma35_rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
> +						    &ma35d1_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(ma35_rtc->rtcdev)) {
> +		dev_err(&pdev->dev, "rtc device register failed\n");
> +		return PTR_ERR(ma35_rtc->rtcdev);
> +	}
> +
> +	err = ma35d1_rtc_init(ma35_rtc, RTC_INIT_TIMEOUT);
> +	if (err)
> +		return err;
> +
> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_CLKFMT);
> +	regval |= RTC_CLKFMT_24HEN;
> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_CLKFMT, regval);
> +
> +	ma35_rtc->irq_num = platform_get_irq(pdev, 0);
> +
> +	if (devm_request_irq(&pdev->dev, ma35_rtc->irq_num, ma35d1_rtc_interrupt,
> +			     IRQF_NO_SUSPEND, "ma35d1rtc", ma35_rtc)) {
> +		dev_err(&pdev->dev, "ma35d1 RTC request irq failed\n");
> +		return -EBUSY;

return dev_err_probe

> +	}
> +
> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
> +	regval |= RTC_INTEN_TICKIEN;
> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
> +
> +	device_init_wakeup(&pdev->dev, true);
> +
> +	return 0;
> +}
> +
> +static int __exit ma35d1_rtc_remove(struct platform_device *pdev)

It's not an exit.

> +{
> +	device_init_wakeup(&pdev->dev, false);
> +
> +	platform_set_drvdata(pdev, NULL);

Just drop remove. You don't do anything useful here.


> +
> +	return 0;
> +}
> +
> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev);
> +	u32 regval;
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		enable_irq_wake(ma35_rtc->irq_num);
> +
> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
> +	regval &= ~RTC_INTEN_TICKIEN;
> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
> +
> +	return 0;
> +}
> +
> +static int ma35d1_rtc_resume(struct platform_device *pdev)
> +{
> +	struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev);
> +	u32 regval;
> +
> +	if (device_may_wakeup(&pdev->dev))
> +		disable_irq_wake(ma35_rtc->irq_num);
> +
> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
> +	regval |= RTC_INTEN_TICKIEN;
> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ma35d1_rtc_of_match[] = {
> +	{ .compatible = "nuvoton,ma35d1-rtc", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
> +
> +static struct platform_driver ma35d1_rtc_driver = {
> +	.remove     = __exit_p(ma35d1_rtc_remove),
> +	.suspend    = ma35d1_rtc_suspend,
> +	.resume     = ma35d1_rtc_resume,
> +	.probe      = ma35d1_rtc_probe,
> +	.driver		= {
> +		.name	= "rtc-ma35d1",
> +		.owner	= THIS_MODULE,

??? No.

> +		.of_match_table = of_match_ptr(ma35d1_rtc_of_match),

Drop of_match_ptr. Didn't you get such comment before? Your other
submission also had the same bug...

Actually, most of these comments you already received for your other
drivers, so it would be great if we did not have to repeat it for every
new driver from you.

Best regards,
Krzysztof

Re: [PATCH 3/3] rtc: Add driver for nuvoton ma35d1 rtc controller

[Jacky Huang]

Dear Krzysztof,


On 2023/7/20 下午 02:14, Krzysztof Kozlowski wrote:
> On 20/07/2023 03:28, Jacky Huang wrote:
>> From: Jacky Huang <ychuang3@nuvoton.com>
>>
>> The ma35d1 rtc controller provides real-time and calendar messaging
>> capabilities. It supports programmable time tick and alarm match
>> interrupts. The time and calendar messages are expressed in BCD format.
>> This driver supports the built-in rtc controller of the ma35d1. It
>> enables setting and reading the rtc time and configuring and reading
>> the rtc alarm.
>>
> ...
>
>> +static int ma35d1_rtc_probe(struct platform_device *pdev)
>> +{
>> +	struct ma35_rtc *ma35_rtc;
>> +	struct clk *clk;
>> +	u32 regval;
>> +	int err;
>> +
>> +	ma35_rtc = devm_kzalloc(&pdev->dev, sizeof(struct ma35_rtc),
> sizeof(*)

I will modify this as
ma35_rtc = devm_kzalloc(&pdev->dev, sizeof(*ma35_rtc),

>> +								GFP_KERNEL);
>> +	if (!ma35_rtc)
>> +		return -ENOMEM;
>> +
>> +	ma35_rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(ma35_rtc->rtc_reg))
>> +		return PTR_ERR(ma35_rtc->rtc_reg);
>> +
>> +	clk = of_clk_get(pdev->dev.of_node, 0);
>> +	if (IS_ERR(clk)) {
>> +		err = PTR_ERR(clk);
>> +		dev_err(&pdev->dev, "failed to get core clk: %d\n", err);
>> +		return -ENOENT;
> return dev_err_probe

I will replace these with

return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc 
clock\n");

>
>> +	}
>> +	err = clk_prepare_enable(clk);
>> +	if (err)
>> +		return -ENOENT;
>> +
>> +	platform_set_drvdata(pdev, ma35_rtc);
>> +
>> +	ma35_rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
>> +						    &ma35d1_rtc_ops, THIS_MODULE);
>> +	if (IS_ERR(ma35_rtc->rtcdev)) {
>> +		dev_err(&pdev->dev, "rtc device register failed\n");
>> +		return PTR_ERR(ma35_rtc->rtcdev);
>> +	}
>> +
>> +	err = ma35d1_rtc_init(ma35_rtc, RTC_INIT_TIMEOUT);
>> +	if (err)
>> +		return err;
>> +
>> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_CLKFMT);
>> +	regval |= RTC_CLKFMT_24HEN;
>> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_CLKFMT, regval);
>> +
>> +	ma35_rtc->irq_num = platform_get_irq(pdev, 0);
>> +
>> +	if (devm_request_irq(&pdev->dev, ma35_rtc->irq_num, ma35d1_rtc_interrupt,
>> +			     IRQF_NO_SUSPEND, "ma35d1rtc", ma35_rtc)) {
>> +		dev_err(&pdev->dev, "ma35d1 RTC request irq failed\n");
>> +		return -EBUSY;
> return dev_err_probe
>
>> +	}
>> +
>> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
>> +	regval |= RTC_INTEN_TICKIEN;
>> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
>> +
>> +	device_init_wakeup(&pdev->dev, true);
>> +
>> +	return 0;
>> +}
>> +
>> +static int __exit ma35d1_rtc_remove(struct platform_device *pdev)
> It's not an exit.
>
>> +{
>> +	device_init_wakeup(&pdev->dev, false);
>> +
>> +	platform_set_drvdata(pdev, NULL);
> Just drop remove. You don't do anything useful here.

I will drop 'ma35d1_rtc_remove'.
>
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
>> +	struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev);
>> +	u32 regval;
>> +
>> +	if (device_may_wakeup(&pdev->dev))
>> +		enable_irq_wake(ma35_rtc->irq_num);
>> +
>> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
>> +	regval &= ~RTC_INTEN_TICKIEN;
>> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_rtc_resume(struct platform_device *pdev)
>> +{
>> +	struct ma35_rtc *ma35_rtc = platform_get_drvdata(pdev);
>> +	u32 regval;
>> +
>> +	if (device_may_wakeup(&pdev->dev))
>> +		disable_irq_wake(ma35_rtc->irq_num);
>> +
>> +	regval = rtc_reg_read(ma35_rtc, MA35_REG_RTC_INTEN);
>> +	regval |= RTC_INTEN_TICKIEN;
>> +	rtc_reg_write(ma35_rtc, MA35_REG_RTC_INTEN, regval);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id ma35d1_rtc_of_match[] = {
>> +	{ .compatible = "nuvoton,ma35d1-rtc", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
>> +
>> +static struct platform_driver ma35d1_rtc_driver = {
>> +	.remove     = __exit_p(ma35d1_rtc_remove),
>> +	.suspend    = ma35d1_rtc_suspend,
>> +	.resume     = ma35d1_rtc_resume,
>> +	.probe      = ma35d1_rtc_probe,
>> +	.driver		= {
>> +		.name	= "rtc-ma35d1",
>> +		.owner	= THIS_MODULE,
> ??? No.

I will drop this line.

>> +		.of_match_table = of_match_ptr(ma35d1_rtc_of_match),

I will modify it as

.of_match_table = ma35d1_rtc_of_match,



> Drop of_match_ptr. Didn't you get such comment before? Your other
> submission also had the same bug...
>
> Actually, most of these comments you already received for your other
> drivers, so it would be great if we did not have to repeat it for every
> new driver from you.
>
> Best regards,
> Krzysztof
>

I will be more careful in reviewing code to avoid making the
same mistakes again. Thank you for your guidance.,


Best Regards,
Jacky Huang