[PATCH v8 0/2] Raspberry Pi 5 RTC driver

[PATCH v8 0/2] Raspberry Pi 5 RTC driver

[Sander Speetjens]

The Raspberry Pi 5 has a battery-backed RTC located
on the DA9091 PMIC accessed via the I2C bus[1].
Access to the RTC is provided exclusively through the VideoCore firmware
mailbox interface (not via a direct I2C register interface).

As stated by jamesh on the Rasperry Pi forum [2],
"There are no plans to release a public datasheet for the DA9091."
So we are stuck with the firmware interface for now.
PhilE also states that "The firmware needs full control of the DA9091,
sometimes with extremely low latency,
otherwise there's a risk the supply could just cut the power."

This series adds:
  - a DT property for the charging voltage of the RTC battery
  - the RTC driver that talks to the firmware

The driver and binding are based on (and largely taken from) the
downstream Raspberry Pi kernel. The main changes for upstream are
the addition of a dt binding property for setting the charging voltage and
binding the driver from the raspberrypi-firmware driver instead of from
the DT.

The driver supports time read/write, alarm, and optional trickle
charging voltage configuration via the device-tree.

Testing
- Hardware: Raspberry Pi 5 Model B revC
- Kernel: v7.2
- dt_binding_check: passed
- Basic RTC operations (read/write time, alarm) verified

[1] https://www.raspberrypi.com/news/introducing-raspberry-pi-5/
[2] https://forums.raspberrypi.com/viewtopic.php?t=359073

Sander Speetjens (2):
  dt-bindings: rtc: Add property for Raspberry Pi 5 RTC
  rtc: Add Raspberry Pi 5 RTC driver

 .../arm/bcm/raspberrypi,bcm2835-firmware.yaml |   6 +
 drivers/firmware/raspberrypi.c                |  23 ++
 drivers/rtc/Kconfig                           |  12 +
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-raspberrypi.c                 | 279 ++++++++++++++++++
 5 files changed, 321 insertions(+)
 create mode 100644 drivers/rtc/rtc-raspberrypi.c


base-commit: 93f51579e7df248780214094418f205253383cc5
-- 
2.55.0

[PATCH v8 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[Sander Speetjens]

The Raspberry Pi 5 has a built-in RTC that can be battery-backed
via the VBAT pin. When a rechargeable coin cell is used, the
hardware requires a trickle-charge voltage to keep the battery
topped up.

Expose the trickle-voltage-millivolt DT property so the driver can
program the correct charge voltage for board specific configurations with
the attached battery.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>
---
V7 -> V8: No changes

V6 -> V7: No changes

V5 -> V6: No changes

V4 -> V5: As discussed with Conor Dooley, the reff to uint32 is removed
and a PR is made on dt-schema adding millivolt to property-units.yaml

V3 -> V4: Add a reff to uint32

V2 -> V3: Fixed the firmware property naming

V1 -> V2: As discussed with Krzysztof Kozlowski, this property is folded into
the existing raspberrypi,bcm2835-firmware properties.

 .../bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml      | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
index 983ea80eaec9..dc71fd57834e 100644
--- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
@@ -133,6 +133,11 @@ properties:
     required:
       - compatible
 
+  trickle-voltage-millivolt:
+    $ref: /schemas/rtc/rtc.yaml#/properties/trickle-voltage-millivolt
+    minimum: 1300
+    maximum: 5000
+
 required:
   - compatible
   - mboxes
@@ -144,6 +149,7 @@ examples:
     firmware {
         compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
         mboxes = <&mailbox>;
+        trickle-voltage-millivolt = <3300>;
 
         firmware_clocks: clocks {
             compatible = "raspberrypi,firmware-clocks";
-- 
2.55.0

[PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Sander Speetjens]

Upstreaming the downstream Raspberry Pi 5 RTC driver.
This driver supports the custom DA9091,
which is accessed through the firmware mailbox.

Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
Signed-off-by: Dom Cobley <popcornmix@gmail.com>
Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>
---
V7 -> V8:
- Fix whitespacing errors
- Use of_machine_is_compatible instead of strncmp

V6 -> V7:
- Move from u32 array to struct
- Fix encapsulation as mentioned by sashiko-bot

V5 -> V6:
- Add MODULE_ALIAS, as mentioned by sashiko-bot
- Encapsulate the mailbox data in le32_to_cpu or cpu_to_le32, as
  mentioned by sashiko-bot. For good practice

V4 -> V5: No changes

V3 -> V4:
- Fix Kconfig depends that disallows having Raspberrypi
firmware built as a module while the rtc driver is built in
- Remove sysfs
- Add limits from firmware properties and check in rpi_rtc_set_charge_voltage

V2 -> V3:
- Move platform check to firmware and abort before registering
a platform device if on another platform
- Add a dependency on the Raspberry Pi firmware to Kconfig
- Check return value of devm_device_init_wakeup
- Fix property name

V1 -> V2:
Instead of the original driver, which was directly bound to the device tree.
This driver is bound to the Raspberry Pi firmware device by creating
a child device in the firmware driver probe function.
The child device is then bound to this driver,
which uses the firmware mailbox to access the RTC.

A couple of minor changes have been made to the driver since it was originally written, including:
- Checking if the model is a Raspberry Pi 5, as the RTC is only present on that model.
- Using the new devm_rpi_firmware_get() and devm_init_wakeup() helper to get the firmware device and avoid leaking memory.
- Using millivolts instead of microvolts for the trickle charge voltage, to match the RTC standard.
- Instead of setting the trickle charge voltage to 0 to disable trickle charging, the property is now optional. If the property is not present, trickle charging is disabled, per the RTC standard.
- Renaming the driver dt match compatible to "raspberrypi,firmware-rtc" to match the other firmware bindings.
- Renaming the driver name to "raspberrypi-rtc" to match the other firmware drivers.


 drivers/firmware/raspberrypi.c |  23 +++
 drivers/rtc/Kconfig            |  12 ++
 drivers/rtc/Makefile           |   1 +
 drivers/rtc/rtc-raspberrypi.c  | 279 +++++++++++++++++++++++++++++++++
 4 files changed, 315 insertions(+)
 create mode 100644 drivers/rtc/rtc-raspberrypi.c

diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 0aa322e9a2e7..b5ee6d7a8951 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -24,6 +24,7 @@
 
 static struct platform_device *rpi_hwmon;
 static struct platform_device *rpi_clk;
+static struct platform_device *rpi_rtc;
 
 struct rpi_firmware {
 	struct mbox_client cl;
@@ -231,6 +232,25 @@ static void rpi_register_clk_driver(struct device *dev)
 						-1, NULL, 0);
 }
 
+static void rpi_register_rtc_driver(struct device *dev)
+{
+	struct device_node *firmware;
+
+	// Check if our model is a Raspberry Pi 5, as the RTC is only present on that model.
+	if (!of_machine_is_compatible("brcm,bcm2712"))
+		return;
+
+	firmware = of_get_compatible_child(dev->of_node,
+					   "raspberrypi,firmware-rtc");
+	if (firmware) {
+		of_node_put(firmware);
+		return;
+	}
+
+	rpi_rtc = platform_device_register_data(dev, "raspberrypi-rtc",
+						-1, NULL, 0);
+}
+
 unsigned int rpi_firmware_clk_get_max_rate(struct rpi_firmware *fw, unsigned int id)
 {
 	struct rpi_firmware_clk_rate_request msg =
@@ -305,6 +325,7 @@ static int rpi_firmware_probe(struct platform_device *pdev)
 	rpi_firmware_print_firmware_revision(fw);
 	rpi_register_hwmon_driver(dev, fw);
 	rpi_register_clk_driver(dev);
+	rpi_register_rtc_driver(dev);
 
 	return 0;
 }
@@ -327,6 +348,8 @@ static void rpi_firmware_remove(struct platform_device *pdev)
 	rpi_hwmon = NULL;
 	platform_device_unregister(rpi_clk);
 	rpi_clk = NULL;
+	platform_device_unregister(rpi_rtc);
+	rpi_rtc = NULL;
 
 	rpi_firmware_put(fw);
 }
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 05b9233b9418..4dbe20bcace8 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1999,6 +1999,18 @@ config RTC_DRV_R7301
 	   This driver can also be built as a module. If so, the module
 	   will be called rtc-r7301.
 
+config RTC_DRV_RPI
+        tristate "Raspberry Pi RTC"
+	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
+        depends on ARCH_BRCMSTB || COMPILE_TEST
+        default ARCH_BRCMSTB
+        help
+          If you say yes here you get support for the RTC found on
+          Raspberry Pi devices.
+
+          This driver can also be built as a module. If so, the module
+          will be called rtc-raspberrypi.
+
 config RTC_DRV_STM32
 	tristate "STM32 RTC"
 	select REGMAP_MMIO
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 0347645b021f..46f1a0fc2416 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -144,6 +144,7 @@ obj-$(CONFIG_RTC_DRV_PS3)	+= rtc-ps3.o
 obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
 obj-$(CONFIG_RTC_DRV_R7301)	+= rtc-r7301.o
 obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
+obj-$(CONFIG_RTC_DRV_RPI)	+= rtc-raspberrypi.o
 obj-$(CONFIG_RTC_DRV_RC5T583)	+= rtc-rc5t583.o
 obj-$(CONFIG_RTC_DRV_RC5T619)	+= rtc-rc5t619.o
 obj-$(CONFIG_RTC_DRV_RK808)	+= rtc-rk808.o
diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
new file mode 100644
index 000000000000..3b77a0cef1d3
--- /dev/null
+++ b/drivers/rtc/rtc-raspberrypi.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+/**
+ * rtc-raspberrypi.c
+ *
+ * RTC driver using firmware mailbox
+ * Supports battery backed RTC and wake alarms
+ *
+ * Based on rtc-meson-vrtc by Neil Armstrong
+ *
+ * Copyright (c) 2023, Raspberry Pi Ltd.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rtc.h>
+#include <linux/of.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define RPI_FIRMWARE_GET_RTC_REG 0x00030087
+#define RPI_FIRMWARE_SET_RTC_REG 0x00038087
+
+enum {
+	RTC_TIME,
+	RTC_ALARM,
+	RTC_ALARM_PENDING,
+	RTC_ALARM_ENABLE,
+	RTC_BBAT_CHG_VOLTS,
+	RTC_BBAT_CHG_VOLTS_MIN,
+	RTC_BBAT_CHG_VOLTS_MAX,
+	RTC_BBAT_VOLTS
+};
+
+struct rpi_rtc_data {
+	struct rtc_device *rtc;
+	struct rpi_firmware *fw;
+	u32 bbat_vchg_millivolts;
+	u32 bbat_vchg_min_millivolts;
+	u32 bbat_vchg_max_millivolts;
+};
+
+struct rpi_rtc_reg {
+	__le32 reg;
+	__le32 val;
+} __packed;
+
+static int rpi_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_TIME)
+	};
+	int err;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+				    &data, sizeof(data));
+	rtc_time64_to_tm(le32_to_cpu(data.val), tm);
+	return err;
+}
+
+static int rpi_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_TIME),
+		.val = cpu_to_le32(rtc_tm_to_time64(tm))
+	};
+
+	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				     &data, sizeof(data));
+}
+
+static int rpi_rtc_alarm_irq_is_enabled(struct device *dev, unsigned char *enabled)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_ALARM_ENABLE)
+	};
+	s32 err = 0;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+				    &data, sizeof(data));
+	*enabled = le32_to_cpu(data.val) & 0x1;
+	return err;
+}
+
+static int rpi_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_ALARM_ENABLE),
+		.val = cpu_to_le32(enabled)
+	};
+
+	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				     &data, sizeof(data));
+}
+
+static int rpi_rtc_alarm_clear_pending(struct device *dev)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_ALARM_PENDING),
+		.val = cpu_to_le32(1)
+	};
+
+	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				     &data, sizeof(data));
+}
+
+static int rpi_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_ALARM)
+	};
+	s32 err = 0;
+
+	err = rpi_rtc_alarm_irq_is_enabled(dev, &alarm->enabled);
+	if (!err)
+		err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+					    &data, sizeof(data));
+	rtc_time64_to_tm(le32_to_cpu(data.val), &alarm->time);
+
+	return err;
+}
+
+static int rpi_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_ALARM),
+		.val = cpu_to_le32(rtc_tm_to_time64(&alarm->time))
+	};
+	int err;
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				    &data, sizeof(data));
+
+	if (err == 0)
+		err = rpi_rtc_alarm_irq_enable(dev, alarm->enabled);
+
+	return err;
+}
+
+static const struct rtc_class_ops rpi_rtc_ops = {
+	.read_time = rpi_rtc_read_time,
+	.set_time = rpi_rtc_set_time,
+	.read_alarm = rpi_rtc_read_alarm,
+	.set_alarm = rpi_rtc_set_alarm,
+	.alarm_irq_enable = rpi_rtc_alarm_irq_enable,
+};
+
+static void rpi_rtc_set_limits(struct device *dev)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_BBAT_CHG_VOLTS_MIN)
+	};
+
+	int err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+					&data, sizeof(data));
+	if (err == 0)
+		vrtc->bbat_vchg_min_millivolts = le32_to_cpu(data.val) / 1000U;
+
+	data.reg = cpu_to_le32(RTC_BBAT_CHG_VOLTS_MAX);
+	data.val = cpu_to_le32(0);
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
+				    &data, sizeof(data));
+	if (err == 0)
+		vrtc->bbat_vchg_max_millivolts = le32_to_cpu(data.val) / 1000U;
+}
+
+static int rpi_rtc_set_charge_voltage(struct device *dev)
+{
+	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
+	struct rpi_rtc_reg data = {
+		.reg = cpu_to_le32(RTC_BBAT_CHG_VOLTS),
+		.val = cpu_to_le32(vrtc->bbat_vchg_millivolts * 1000U)
+	};
+	int err;
+
+	if (vrtc->bbat_vchg_millivolts != 0 &&
+	    (vrtc->bbat_vchg_millivolts < vrtc->bbat_vchg_min_millivolts ||
+	    vrtc->bbat_vchg_millivolts > vrtc->bbat_vchg_max_millivolts)) {
+		dev_warn(dev, "trickle charge voltage %umV is outside of the supported range (%umV - %umV)\n",
+			 vrtc->bbat_vchg_millivolts,
+			 vrtc->bbat_vchg_min_millivolts,
+			 vrtc->bbat_vchg_max_millivolts);
+		return -EINVAL;
+	}
+
+	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
+				    &data, sizeof(data));
+
+	if (err)
+		dev_err(dev, "failed to set trickle charge voltage to %umV: %d\n",
+			vrtc->bbat_vchg_millivolts, err);
+	else if (vrtc->bbat_vchg_millivolts)
+		dev_info(dev, "trickle charging enabled at %umV\n",
+			 vrtc->bbat_vchg_millivolts);
+
+	return err;
+}
+
+static int rpi_rtc_probe(struct platform_device *pdev)
+{
+	struct rpi_rtc_data *vrtc;
+	struct device *dev = &pdev->dev;
+	struct rpi_firmware *firmware;
+	int ret;
+
+	// Get the firmware device from the parent device
+	firmware = devm_rpi_firmware_get(dev, dev->parent->of_node);
+
+	if (!firmware)
+		return -EPROBE_DEFER;
+
+	vrtc = devm_kzalloc(dev, sizeof(*vrtc), GFP_KERNEL);
+	if (!vrtc)
+		return -ENOMEM;
+
+	vrtc->fw = firmware;
+
+	ret = devm_device_init_wakeup(dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, vrtc);
+
+	vrtc->rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(vrtc->rtc))
+		return PTR_ERR(vrtc->rtc);
+
+	vrtc->rtc->range_max = U32_MAX;           /* 2106-02-07 */
+
+	set_bit(RTC_FEATURE_ALARM_WAKEUP_ONLY, vrtc->rtc->features);
+	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, vrtc->rtc->features);
+
+	vrtc->rtc->ops = &rpi_rtc_ops;
+
+	rpi_rtc_alarm_clear_pending(dev);
+
+	/*
+	 * The trickle-voltage property lives on the parent firmware node
+	 * because we deliberately do not create a DT child node just to
+	 * instantiate this driver (see DT maintainer guidance).
+	 * The firmware driver registers us as a platform device at runtime.
+	 */
+	vrtc->bbat_vchg_millivolts = 0;
+	of_property_read_u32(dev->parent->of_node, "trickle-voltage-millivolt",
+			     &vrtc->bbat_vchg_millivolts);
+
+	rpi_rtc_set_limits(dev);
+	rpi_rtc_set_charge_voltage(dev);
+
+	return devm_rtc_register_device(vrtc->rtc);
+}
+
+static const struct of_device_id rpi_rtc_dt_match[] = {
+	{ .compatible = "raspberrypi,firmware-rtc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpi_rtc_dt_match);
+
+static struct platform_driver rpi_rtc_driver = {
+	.driver = {
+		.name = "raspberrypi-rtc",
+		.of_match_table = of_match_ptr(rpi_rtc_dt_match),
+	},
+	.probe = rpi_rtc_probe
+};
+
+module_platform_driver(rpi_rtc_driver);
+
+MODULE_AUTHOR("Jonathan Bell <jonathan@raspberrypi.com>");
+MODULE_AUTHOR("Sander Speetjens <sander.speetjens@gmail.com>");
+MODULE_DESCRIPTION("Raspberry Pi RTC driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:raspberrypi-rtc");
-- 
2.55.0

Re: [PATCH v8 1/2] dt-bindings: rtc: Add property for Raspberry Pi 5 RTC

[sashiko-bot]

> The Raspberry Pi 5 has a built-in RTC that can be battery-backed
> via the VBAT pin. When a rechargeable coin cell is used, the
> hardware requires a trickle-charge voltage to keep the battery
> topped up.
> 
> Expose the trickle-voltage-millivolt DT property so the driver can
> program the correct charge voltage for board specific configurations with
> the attached battery.
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930095203.492949-1-sander.speetjens@gmail.com?part=1

Re: [PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[sashiko-bot]

> Upstreaming the downstream Raspberry Pi 5 RTC driver.
> This driver supports the custom DA9091,
> which is accessed through the firmware mailbox.
> 
> Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930095203.492949-1-sander.speetjens@gmail.com?part=2

Re: [PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Stefan Wahren]

Hi Sander,

Am 30.09.26 um 11:52 schrieb Sander Speetjens:
> Upstreaming the downstream Raspberry Pi 5 RTC driver.
> This driver supports the custom DA9091,
> which is accessed through the firmware mailbox.
>
> Signed-off-by: Jonathan Bell <jonathan@raspberrypi.com>
> Signed-off-by: Dom Cobley <popcornmix@gmail.com>
> Signed-off-by: Sander Speetjens <sander.speetjens@gmail.com>
> ---
> V7 -> V8:
> - Fix whitespacing errors
> - Use of_machine_is_compatible instead of strncmp
>
> V6 -> V7:
> - Move from u32 array to struct
> - Fix encapsulation as mentioned by sashiko-bot
>
> V5 -> V6:
> - Add MODULE_ALIAS, as mentioned by sashiko-bot
> - Encapsulate the mailbox data in le32_to_cpu or cpu_to_le32, as
>    mentioned by sashiko-bot. For good practice
>
> V4 -> V5: No changes
>
> V3 -> V4:
> - Fix Kconfig depends that disallows having Raspberrypi
> firmware built as a module while the rtc driver is built in
> - Remove sysfs
> - Add limits from firmware properties and check in rpi_rtc_set_charge_voltage
>
> V2 -> V3:
> - Move platform check to firmware and abort before registering
> a platform device if on another platform
> - Add a dependency on the Raspberry Pi firmware to Kconfig
> - Check return value of devm_device_init_wakeup
> - Fix property name
>
> V1 -> V2:
> Instead of the original driver, which was directly bound to the device tree.
> This driver is bound to the Raspberry Pi firmware device by creating
> a child device in the firmware driver probe function.
> The child device is then bound to this driver,
> which uses the firmware mailbox to access the RTC.
>
> A couple of minor changes have been made to the driver since it was originally written, including:
> - Checking if the model is a Raspberry Pi 5, as the RTC is only present on that model.
> - Using the new devm_rpi_firmware_get() and devm_init_wakeup() helper to get the firmware device and avoid leaking memory.
> - Using millivolts instead of microvolts for the trickle charge voltage, to match the RTC standard.
> - Instead of setting the trickle charge voltage to 0 to disable trickle charging, the property is now optional. If the property is not present, trickle charging is disabled, per the RTC standard.
> - Renaming the driver dt match compatible to "raspberrypi,firmware-rtc" to match the other firmware bindings.
> - Renaming the driver name to "raspberrypi-rtc" to match the other firmware drivers.
>
>
>   drivers/firmware/raspberrypi.c |  23 +++
>   drivers/rtc/Kconfig            |  12 ++
>   drivers/rtc/Makefile           |   1 +
>   drivers/rtc/rtc-raspberrypi.c  | 279 +++++++++++++++++++++++++++++++++
>   4 files changed, 315 insertions(+)
>   create mode 100644 drivers/rtc/rtc-raspberrypi.c
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 0aa322e9a2e7..b5ee6d7a8951 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -24,6 +24,7 @@
>   
>   static struct platform_device *rpi_hwmon;
>   static struct platform_device *rpi_clk;
> +static struct platform_device *rpi_rtc;
>   
>   struct rpi_firmware {
>   	struct mbox_client cl;
> @@ -231,6 +232,25 @@ static void rpi_register_clk_driver(struct device *dev)
>   						-1, NULL, 0);
>   }
>   
> +static void rpi_register_rtc_driver(struct device *dev)
> +{
> +	struct device_node *firmware;
> +
> +	// Check if our model is a Raspberry Pi 5, as the RTC is only present on that model.
> +	if (!of_machine_is_compatible("brcm,bcm2712"))
> +		return;
I don't like the comment, because it doesn't check for Raspberry Pi 5, 
the code checks for a BCM2712 SoC which could also be on a CM5 or a 
Raspberry Pi 500+
> +
> +	firmware = of_get_compatible_child(dev->of_node,
> +					   "raspberrypi,firmware-rtc");
> +	if (firmware) {
> +		of_node_put(firmware);
> +		return;
> +	}
> +
> +	rpi_rtc = platform_device_register_data(dev, "raspberrypi-rtc",
> +						-1, NULL, 0);
> +}
> +
>   unsigned int rpi_firmware_clk_get_max_rate(struct rpi_firmware *fw, unsigned int id)
>   {
>   	struct rpi_firmware_clk_rate_request msg =
> @@ -305,6 +325,7 @@ static int rpi_firmware_probe(struct platform_device *pdev)
>   	rpi_firmware_print_firmware_revision(fw);
>   	rpi_register_hwmon_driver(dev, fw);
>   	rpi_register_clk_driver(dev);
> +	rpi_register_rtc_driver(dev);
>   
>   	return 0;
>   }
> @@ -327,6 +348,8 @@ static void rpi_firmware_remove(struct platform_device *pdev)
>   	rpi_hwmon = NULL;
>   	platform_device_unregister(rpi_clk);
>   	rpi_clk = NULL;
> +	platform_device_unregister(rpi_rtc);
> +	rpi_rtc = NULL;
>   
>   	rpi_firmware_put(fw);
>   }
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 05b9233b9418..4dbe20bcace8 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1999,6 +1999,18 @@ config RTC_DRV_R7301
>   	   This driver can also be built as a module. If so, the module
>   	   will be called rtc-r7301.
>   
> +config RTC_DRV_RPI
> +        tristate "Raspberry Pi RTC"
> +	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> +        depends on ARCH_BRCMSTB || COMPILE_TEST
> +        default ARCH_BRCMSTB
> +        help
> +          If you say yes here you get support for the RTC found on
> +          Raspberry Pi devices.
> +
> +          This driver can also be built as a module. If so, the module
> +          will be called rtc-raspberrypi.
> +
AFAIK we should try to use tabs here instead of pure spaces
>   config RTC_DRV_STM32
>   	tristate "STM32 RTC"
>   	select REGMAP_MMIO
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 0347645b021f..46f1a0fc2416 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -144,6 +144,7 @@ obj-$(CONFIG_RTC_DRV_PS3)	+= rtc-ps3.o
>   obj-$(CONFIG_RTC_DRV_PXA)	+= rtc-pxa.o
>   obj-$(CONFIG_RTC_DRV_R7301)	+= rtc-r7301.o
>   obj-$(CONFIG_RTC_DRV_R9701)	+= rtc-r9701.o
> +obj-$(CONFIG_RTC_DRV_RPI)	+= rtc-raspberrypi.o
>   obj-$(CONFIG_RTC_DRV_RC5T583)	+= rtc-rc5t583.o
>   obj-$(CONFIG_RTC_DRV_RC5T619)	+= rtc-rc5t619.o
>   obj-$(CONFIG_RTC_DRV_RK808)	+= rtc-rk808.o
> diff --git a/drivers/rtc/rtc-raspberrypi.c b/drivers/rtc/rtc-raspberrypi.c
> new file mode 100644
> index 000000000000..3b77a0cef1d3
> --- /dev/null
> +++ b/drivers/rtc/rtc-raspberrypi.c
> @@ -0,0 +1,279 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/**
> + * rtc-raspberrypi.c
> + *
> + * RTC driver using firmware mailbox
> + * Supports battery backed RTC and wake alarms
> + *
> + * Based on rtc-meson-vrtc by Neil Armstrong
> + *
> + * Copyright (c) 2023, Raspberry Pi Ltd.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/rtc.h>
> +#include <linux/of.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define RPI_FIRMWARE_GET_RTC_REG 0x00030087
> +#define RPI_FIRMWARE_SET_RTC_REG 0x00038087
Was there a specific reason to not include these defines to 
include/soc/bcm2835/raspberry-pi-firmware.h as all the others firmware 
tags?
> +
> +enum {
> +	RTC_TIME,
> +	RTC_ALARM,
> +	RTC_ALARM_PENDING,
> +	RTC_ALARM_ENABLE,
> +	RTC_BBAT_CHG_VOLTS,
> +	RTC_BBAT_CHG_VOLTS_MIN,
> +	RTC_BBAT_CHG_VOLTS_MAX,
> +	RTC_BBAT_VOLTS
> +};
Hm, an enum suggests that we simply can add / remove items, but that's 
not the case. The Raspberry Pi firmware defines the values.
> +
> +struct rpi_rtc_data {
> +	struct rtc_device *rtc;
> +	struct rpi_firmware *fw;
> +	u32 bbat_vchg_millivolts;
> +	u32 bbat_vchg_min_millivolts;
> +	u32 bbat_vchg_max_millivolts;
> +};
> +
> +struct rpi_rtc_reg {
> +	__le32 reg;
> +	__le32 val;
> +} __packed;
> +
> +static int rpi_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_TIME)
> +	};
> +	int err;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +				    &data, sizeof(data));
> +	rtc_time64_to_tm(le32_to_cpu(data.val), tm);
> +	return err;
> +}
> +
> +static int rpi_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_TIME),
> +		.val = cpu_to_le32(rtc_tm_to_time64(tm))
> +	};
> +
> +	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				     &data, sizeof(data));
> +}
> +
> +static int rpi_rtc_alarm_irq_is_enabled(struct device *dev, unsigned char *enabled)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_ALARM_ENABLE)
> +	};
> +	s32 err = 0;
Please use int and no need to initialize
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +				    &data, sizeof(data));
> +	*enabled = le32_to_cpu(data.val) & 0x1;
> +	return err;
> +}
> +
> +static int rpi_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_ALARM_ENABLE),
> +		.val = cpu_to_le32(enabled)
> +	};
> +
> +	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				     &data, sizeof(data));
> +}
> +
> +static int rpi_rtc_alarm_clear_pending(struct device *dev)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_ALARM_PENDING),
> +		.val = cpu_to_le32(1)
> +	};
> +
> +	return rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				     &data, sizeof(data));
> +}
> +
> +static int rpi_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_ALARM)
> +	};
> +	s32 err = 0;
Please use int and no need to initialize
> +
> +	err = rpi_rtc_alarm_irq_is_enabled(dev, &alarm->enabled);
> +	if (!err)
> +		err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +					    &data, sizeof(data));
> +	rtc_time64_to_tm(le32_to_cpu(data.val), &alarm->time);
> +
> +	return err;
> +}
> +
> +static int rpi_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_ALARM),
> +		.val = cpu_to_le32(rtc_tm_to_time64(&alarm->time))
> +	};
> +	int err;
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				    &data, sizeof(data));
> +
> +	if (err == 0)
if (!err)
> +		err = rpi_rtc_alarm_irq_enable(dev, alarm->enabled);
> +
> +	return err;
> +}
> +
> +static const struct rtc_class_ops rpi_rtc_ops = {
> +	.read_time = rpi_rtc_read_time,
> +	.set_time = rpi_rtc_set_time,
> +	.read_alarm = rpi_rtc_read_alarm,
> +	.set_alarm = rpi_rtc_set_alarm,
> +	.alarm_irq_enable = rpi_rtc_alarm_irq_enable,
> +};
> +
> +static void rpi_rtc_set_limits(struct device *dev)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_BBAT_CHG_VOLTS_MIN)
> +	};
> +
> +	int err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +					&data, sizeof(data));
> +	if (err == 0)
if (!err)
> +		vrtc->bbat_vchg_min_millivolts = le32_to_cpu(data.val) / 1000U;
> +
> +	data.reg = cpu_to_le32(RTC_BBAT_CHG_VOLTS_MAX);
> +	data.val = cpu_to_le32(0);
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_GET_RTC_REG,
> +				    &data, sizeof(data));
> +	if (err == 0)
> +		vrtc->bbat_vchg_max_millivolts = le32_to_cpu(data.val) / 1000U;
> +}
> +
> +static int rpi_rtc_set_charge_voltage(struct device *dev)
> +{
> +	struct rpi_rtc_data *vrtc = dev_get_drvdata(dev);
> +	struct rpi_rtc_reg data = {
> +		.reg = cpu_to_le32(RTC_BBAT_CHG_VOLTS),
> +		.val = cpu_to_le32(vrtc->bbat_vchg_millivolts * 1000U)
> +	};
> +	int err;
> +
> +	if (vrtc->bbat_vchg_millivolts != 0 &&
> +	    (vrtc->bbat_vchg_millivolts < vrtc->bbat_vchg_min_millivolts ||
> +	    vrtc->bbat_vchg_millivolts > vrtc->bbat_vchg_max_millivolts)) {
> +		dev_warn(dev, "trickle charge voltage %umV is outside of the supported range (%umV - %umV)\n",
> +			 vrtc->bbat_vchg_millivolts,
> +			 vrtc->bbat_vchg_min_millivolts,
> +			 vrtc->bbat_vchg_max_millivolts);
In case rpi_rtc_set_limits() fails, both limit would be initialized with 
0 and this always fail. Maybe we should dev_warn to rpi_rtc_set_limits()?
> +		return -EINVAL;
> +	}
> +
> +	err = rpi_firmware_property(vrtc->fw, RPI_FIRMWARE_SET_RTC_REG,
> +				    &data, sizeof(data));
> +
> +	if (err)
> +		dev_err(dev, "failed to set trickle charge voltage to %umV: %d\n",
> +			vrtc->bbat_vchg_millivolts, err);
> +	else if (vrtc->bbat_vchg_millivolts)
> +		dev_info(dev, "trickle charging enabled at %umV\n",
> +			 vrtc->bbat_vchg_millivolts);
> +
> +	return err;
> +}
> +
> +static int rpi_rtc_probe(struct platform_device *pdev)
> +{
> +	struct rpi_rtc_data *vrtc;
> +	struct device *dev = &pdev->dev;
> +	struct rpi_firmware *firmware;
> +	int ret;
> +
> +	// Get the firmware device from the parent device
> +	firmware = devm_rpi_firmware_get(dev, dev->parent->of_node);
> +
> +	if (!firmware)
> +		return -EPROBE_DEFER;
> +
> +	vrtc = devm_kzalloc(dev, sizeof(*vrtc), GFP_KERNEL);
> +	if (!vrtc)
> +		return -ENOMEM;
> +
> +	vrtc->fw = firmware;
> +
> +	ret = devm_device_init_wakeup(dev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, vrtc);
> +
> +	vrtc->rtc = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(vrtc->rtc))
> +		return PTR_ERR(vrtc->rtc);
> +
> +	vrtc->rtc->range_max = U32_MAX;           /* 2106-02-07 */
> +
> +	set_bit(RTC_FEATURE_ALARM_WAKEUP_ONLY, vrtc->rtc->features);
> +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, vrtc->rtc->features);
> +
> +	vrtc->rtc->ops = &rpi_rtc_ops;
> +
> +	rpi_rtc_alarm_clear_pending(dev);
> +
> +	/*
> +	 * The trickle-voltage property lives on the parent firmware node
> +	 * because we deliberately do not create a DT child node just to
> +	 * instantiate this driver (see DT maintainer guidance).
> +	 * The firmware driver registers us as a platform device at runtime.
> +	 */
> +	vrtc->bbat_vchg_millivolts = 0;
> +	of_property_read_u32(dev->parent->of_node, "trickle-voltage-millivolt",
> +			     &vrtc->bbat_vchg_millivolts);
> +
> +	rpi_rtc_set_limits(dev);
> +	rpi_rtc_set_charge_voltage(dev);
Just to be sure, both calls are optional and not critical for the 
drivers function?
Why does rpi_rtc_set_charge_voltage have a return value at all?
> +
> +	return devm_rtc_register_device(vrtc->rtc);
> +}
> +
> +static const struct of_device_id rpi_rtc_dt_match[] = {
> +	{ .compatible = "raspberrypi,firmware-rtc" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rpi_rtc_dt_match);
> +
> +static struct platform_driver rpi_rtc_driver = {
> +	.driver = {
> +		.name = "raspberrypi-rtc",
> +		.of_match_table = of_match_ptr(rpi_rtc_dt_match),
> +	},
> +	.probe = rpi_rtc_probe
> +};
> +
> +module_platform_driver(rpi_rtc_driver);
> +
> +MODULE_AUTHOR("Jonathan Bell <jonathan@raspberrypi.com>");
> +MODULE_AUTHOR("Sander Speetjens <sander.speetjens@gmail.com>");
> +MODULE_DESCRIPTION("Raspberry Pi RTC driver");
> +MODULE_LICENSE("GPL");
According to SPDX header this is a license "Dual BSD/GPL" ?
> +MODULE_ALIAS("platform:raspberrypi-rtc");
Is this really necessary for module autoloading?

Best regards

Re: [PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Sander Speetjens]

Hi Stefan,

>> +    // Check if our model is a Raspberry Pi 5, as the RTC is only 
>> present on that model.
>> +    if (!of_machine_is_compatible("brcm,bcm2712"))
>> +        return;
> I don't like the comment, because it doesn't check for Raspberry Pi 5, 
> the code checks for a BCM2712 SoC which could also be on a CM5 or a 
> Raspberry Pi 500+ 
I changed it to BCM2712, but isn't RPi 5 the generation/platform name 
and RPi 5b the specific board?

>> +#define RPI_FIRMWARE_GET_RTC_REG 0x00030087
>> +#define RPI_FIRMWARE_SET_RTC_REG 0x00038087
> Was there a specific reason to not include these defines to 
> include/soc/bcm2835/raspberry-pi-firmware.h as all the others firmware 
> tags? 
No specific reason, this is how Raspberry Pi originally did it, I moved 
them to raspberrypi-firmware.h

>> +
>> +enum {
>> +    RTC_TIME,
>> +    RTC_ALARM,
>> +    RTC_ALARM_PENDING,
>> +    RTC_ALARM_ENABLE,
>> +    RTC_BBAT_CHG_VOLTS,
>> +    RTC_BBAT_CHG_VOLTS_MIN,
>> +    RTC_BBAT_CHG_VOLTS_MAX,
>> +    RTC_BBAT_VOLTS
>> +};
> Hm, an enum suggests that we simply can add / remove items, but that's 
> not the case. The Raspberry Pi firmware defines the values.
Should I also move those to raspberrypi-firmware.h or is this not what 
you imply?

> In case rpi_rtc_set_limits() fails, both limit would be initialized 
> with 0 and this always fail. Maybe we should dev_warn to 
> rpi_rtc_set_limits()?

I added a warning for failing to set the min and max values.

> Just to be sure, both calls are optional and not critical for the 
> drivers function?
> Why does rpi_rtc_set_charge_voltage have a return value at all?
I removed the return value

>> +MODULE_ALIAS("platform:raspberrypi-rtc");
> Is this really necessary for module autoloading?
I'm not sure, it was used in the previous versions when using the 
platform device register on the register_clk driver before it had a 
custom dt node.

Kind regards,

Sander Speetjens

Re: [PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Stefan Wahren]

Am 30.09.26 um 19:29 schrieb Sander Speetjens:
> Hi Stefan,
>
>>> +    // Check if our model is a Raspberry Pi 5, as the RTC is only 
>>> present on that model.
>>> +    if (!of_machine_is_compatible("brcm,bcm2712"))
>>> +        return;
>> I don't like the comment, because it doesn't check for Raspberry Pi 
>> 5, the code checks for a BCM2712 SoC which could also be on a CM5 or 
>> a Raspberry Pi 500+ 
> I changed it to BCM2712, but isn't RPi 5 the generation/platform name 
> and RPi 5b the specific board?
The specific model name for the Raspberry Pi 5 board is "Raspberry Pi 5" 
and it's devicetree compatible is "raspberrypi,5-model-b". Both doesn't 
have anything to do with the generation. "brcm,bcm2712" is the used 
System on chip (SoC), which is common for all board of the 5th generation.

So you can write something like
// Check if our Raspberry Pi board is from the 5th gen ...
>
>>> +#define RPI_FIRMWARE_GET_RTC_REG 0x00030087
>>> +#define RPI_FIRMWARE_SET_RTC_REG 0x00038087
>> Was there a specific reason to not include these defines to 
>> include/soc/bcm2835/raspberry-pi-firmware.h as all the others 
>> firmware tags? 
> No specific reason, this is how Raspberry Pi originally did it, I 
> moved them to raspberrypi-firmware.h
>
>>> +
>>> +enum {
>>> +    RTC_TIME,
>>> +    RTC_ALARM,
>>> +    RTC_ALARM_PENDING,
>>> +    RTC_ALARM_ENABLE,
>>> +    RTC_BBAT_CHG_VOLTS,
>>> +    RTC_BBAT_CHG_VOLTS_MIN,
>>> +    RTC_BBAT_CHG_VOLTS_MAX,
>>> +    RTC_BBAT_VOLTS
>>> +};
>> Hm, an enum suggests that we simply can add / remove items, but 
>> that's not the case. The Raspberry Pi firmware defines the values.
> Should I also move those to raspberrypi-firmware.h or is this not what 
> you imply?
No this wasn't my implication. One way to make this ABI more explicit 
would be to use #define. I think there is no need to move to 
raspberrypi-firmware.
>
>> In case rpi_rtc_set_limits() fails, both limit would be initialized 
>> with 0 and this always fail. Maybe we should dev_warn to 
>> rpi_rtc_set_limits()?
>
> I added a warning for failing to set the min and max values.
>
>> Just to be sure, both calls are optional and not critical for the 
>> drivers function?
>> Why does rpi_rtc_set_charge_voltage have a return value at all?
> I removed the return value
>
>>> +MODULE_ALIAS("platform:raspberrypi-rtc");
>> Is this really necessary for module autoloading?
> I'm not sure, it was used in the previous versions when using the 
> platform device register on the register_clk driver before it had a 
> custom dt node.
You can test by removing the line and compile it as a module. In case 
the RTC driver is still automatically loaded, we can drop it.
>
>
> Kind regards,
>
> Sander Speetjens

Re: [PATCH v8 2/2] rtc: Add Raspberry Pi 5 RTC driver

[Sander Speetjens]

>>>> +MODULE_ALIAS("platform:raspberrypi-rtc");
>>> Is this really necessary for module autoloading?
>> I'm not sure, it was used in the previous versions when using the 
>> platform device register on the register_clk driver before it had a 
>> custom dt node.
> You can test by removing the line and compile it as a module. In case 
> the RTC driver is still automatically loaded, we can drop it.
There is no DT node for the RTC. The firmware driver registers us as a 
platform device at runtime (we deliberately avoid a child node per DT 
maintainer guidance). Therefore the platform alias is still needed for 
module auto-loading.