[PATCH v3 0/7] spacemit: introduce P1 PMIC support

[PATCH v3 0/7] spacemit: introduce P1 PMIC support

[Alex Elder]

The SpacemiT P1 is an I2C-controlled PMIC that implements 6 buck
converters and 12 LDOs.  It contains a load switch, ADC channels,
GPIOs, a real-time clock, and a watchdog timer.

This series introduces a multifunction driver for the P1 PMIC as well
as drivers for its regulators and RTC.

This series is available here:
  https://github.com/riscstar/linux/tree/outgoing/pmic-v3

					-Alex
Between version 2 and version 3:
  - Removed "spacemit-pmic.c" and updated "simple-mfd-i2c.c" instead
  - Added an RTC driver, so that the MFD has more than one sub-device
  - Other suggestions were directed at "spacemit-pmic.c"

Here is version 2 of this series:
  https://lore.kernel.org/lkml/20250613210150.1468845-1-elder@riscstar.com/

Between version 1 and version 2:
  - Added Reviewed-by tag from Mark Brown to patch 3
  - Implemented suggestions from Vivian Wang:
      - Fixed a typo in the subject line in patch 1
      - Now use module_i2c_driver() for the PMIC driver in patch 2
      - Added MODULE_ALIAS() for both drivers (patches 2 and 3)
      - Defined and used DRV_NAME in both drivers
      - Added additional Kconfig module help text for both drivers

Here is version 1 of this series:
  https://lore.kernel.org/lkml/20250613210150.1468845-1-elder@riscstar.com/

Alex Elder (7):
  dt-bindings: mfd: add support the SpacemiT P1 PMIC
  mfd: simple-mfd-i2c: add SpacemiT P1 support
  regulator: spacemit: support SpacemiT P1 regulators
  rtc: spacemit: support the SpacemiT P1 RTC
  riscv: dts: spacemit: enable the i2c8 adapter
  riscv: dts: spacemit: define fixed regulators
  riscv: dts: spacemit: define regulator constraints

 .../devicetree/bindings/mfd/spacemit,p1.yaml  |  86 ++++++++++
 .../boot/dts/spacemit/k1-bananapi-f3.dts      | 138 +++++++++++++++
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi  |   7 +
 arch/riscv/boot/dts/spacemit/k1.dtsi          |  11 ++
 drivers/mfd/Kconfig                           |  10 ++
 drivers/mfd/simple-mfd-i2c.c                  |  18 ++
 drivers/regulator/Kconfig                     |  12 ++
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/spacemit-p1.c               | 157 ++++++++++++++++++
 drivers/rtc/Kconfig                           |  10 ++
 drivers/rtc/Makefile                          |   1 +
 drivers/rtc/rtc-p1.c                          | 137 +++++++++++++++
 12 files changed, 588 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
 create mode 100644 drivers/regulator/spacemit-p1.c
 create mode 100644 drivers/rtc/rtc-p1.c


base-commit: 5d4809e25903ab8e74034c1f23c787fd26d52934
-- 
2.45.2

[PATCH v3 1/7] dt-bindings: mfd: add support the SpacemiT P1 PMIC

[Alex Elder]

Enable the SpacemiT P1, which is an I2C-controlled PMIC.  Initially
only the RTC and regulators will be supported.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 .../devicetree/bindings/mfd/spacemit,p1.yaml  | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/spacemit,p1.yaml

diff --git a/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml b/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
new file mode 100644
index 0000000000000..5cc34d4934b54
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/spacemit,p1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT P1 Power Management Integrated Circuit
+
+maintainers:
+  - Troy Mitchell <troymitchell988@gmail.com>
+
+description:
+  P1 is an I2C-controlled PMIC produced by SpacemiT.  It implements six
+  constant-on-time buck converters and twelve low-dropout regulators.
+  It also contains a load switch, watchdog timer, real-time clock, eight
+  12-bit ADC channels, and six GPIOs.  Additional details are available
+  in the "Power Stone/P1" section at the following link.
+    https://developer.spacemit.com/documentation
+
+properties:
+  compatible:
+    const: spacemit,p1
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vin-supply:
+    description: Input supply phandle.
+
+  regulators:
+    type: object
+
+    patternProperties:
+      "^(buck[1-6]|aldo[1-4]|dldo[1-7])$":
+        type: object
+        $ref: /schemas/regulator/regulator.yaml#
+        unevaluatedProperties: false
+
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        pmic@41 {
+            compatible = "spacemit,p1";
+            reg = <0x41>;
+            interrupts = <64>;
+
+            regulators {
+                buck1 {
+                    regulator-name = "buck1";
+                    regulator-min-microvolt = <500000>;
+                    regulator-max-microvolt = <3450000>;
+                    regulator-ramp-delay = <5000>;
+                    regulator-always-on;
+                };
+
+                aldo1 {
+                    regulator-name = "aldo1";
+                    regulator-min-microvolt = <500000>;
+                    regulator-max-microvolt = <3400000>;
+                    regulator-boot-on;
+                };
+
+                dldo1 {
+                    regulator-name = "dldo1";
+                    regulator-min-microvolt = <500000>;
+                    regulator-max-microvolt = <3400000>;
+                    regulator-boot-on;
+                };
+            };
+        };
+    };
-- 
2.45.2

[PATCH v3 2/7] mfd: simple-mfd-i2c: add SpacemiT P1 support

[Alex Elder]

Enable support for the RTC and regulators found in the SpacemiT P1
PMIC.  Support is implemented by the simple I2C MFD driver.

The P1 PMIC is normally implemented with the SpacemiT K1 SoC.  This
PMIC provides 6 buck converters and 12 LDO regulators.  It also
implements a switch, watchdog timer, real-time clock, and more.
Initially its RTC and regulators are supported.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v3: - Support is now implemented in "simple-mfd-i2c.c" by defining a
      "spacemit,p1" match entry with match data defining the PMIC.

 drivers/mfd/Kconfig          | 10 ++++++++++
 drivers/mfd/simple-mfd-i2c.c | 18 ++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6fb3768e3d71c..a83884da6befa 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1182,6 +1182,16 @@ config MFD_QCOM_RPM
 	  Say M here if you want to include support for the Qualcomm RPM as a
 	  module. This will build a module called "qcom_rpm".
 
+config MFD_SPACEMIT_P1
+	tristate "SpacemiT P1 PMIC"
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  This option supports the I2C-based SpacemiT P1 PMIC, which
+	  contains regulators, a power switch, GPIOs, an RTC, and more.
+	  This option is selected when any of the supported sub-devices
+	  is configured.  The basic functionality is implemented by the
+	  simple MFD I2C driver.
+
 config MFD_SPMI_PMIC
 	tristate "Qualcomm SPMI PMICs"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 22159913bea03..026cd92e20ad3 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -93,12 +93,30 @@ static const struct simple_mfd_data maxim_mon_max77705 = {
 	.mfd_cell_size = ARRAY_SIZE(max77705_sensor_cells),
 };
 
+static const struct regmap_config spacemit_p1_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = 0xaa,
+};
+
+static const struct mfd_cell spacemit_p1_cells[] = {
+	{ .name = "spacemit-p1-regulator", },
+	{ .name = "spacemit-p1-rtc", },
+};
+
+static const struct simple_mfd_data spacemit_p1 = {
+	.regmap_config = &spacemit_p1_regmap_config,
+	.mfd_cell = spacemit_p1_cells,
+	.mfd_cell_size = ARRAY_SIZE(spacemit_p1_cells),
+};
+
 static const struct of_device_id simple_mfd_i2c_of_match[] = {
 	{ .compatible = "kontron,sl28cpld" },
 	{ .compatible = "silergy,sy7636a", .data = &silergy_sy7636a},
 	{ .compatible = "maxim,max5970", .data = &maxim_max5970},
 	{ .compatible = "maxim,max5978", .data = &maxim_max5970},
 	{ .compatible = "maxim,max77705-battery", .data = &maxim_mon_max77705},
+	{ .compatible = "spacemit,p1", .data = &spacemit_p1, },
 	{}
 };
 MODULE_DEVICE_TABLE(of, simple_mfd_i2c_of_match);
-- 
2.45.2

[PATCH v3 3/7] regulator: spacemit: support SpacemiT P1 regulators

[Alex Elder]

Add support for the regulators found in the SpacemiT P1 PMIC.  This
PMIC provides 6 buck converters and 12 LDO regulators.

The PMIC is implemented as a multi-function device.  These regulators
are probed based on this driver being named in a MFD cell in the simple
MFD I2C driver.

Signed-off-by: Alex Elder <elder@riscstar.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/Kconfig       |  12 +++
 drivers/regulator/Makefile      |   1 +
 drivers/regulator/spacemit-p1.c | 157 ++++++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 drivers/regulator/spacemit-p1.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7423954153b07..3e7feeebf8aca 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1395,6 +1395,18 @@ config REGULATOR_SLG51000
 	  The SLG51000 is seven compact and customizable low dropout
 	  regulators.
 
+config REGULATOR_SPACEMIT_P1
+	tristate "SpacemiT P1 regulators"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	select MFD_SPACEMIT_P1
+	default ARCH_SPACEMIT
+	help
+	  Enable support for regulators implemented by the SpacemiT P1
+	  power controller.  The P1 implements 6 high-efficiency buck
+	  converters and 12 programmable LDO regulators.  To compile this
+	  driver as a module, choose M here.  The module will be called
+	  "spacemit-pmic".
+
 config REGULATOR_STM32_BOOSTER
 	tristate "STMicroelectronics STM32 BOOSTER"
 	depends on ARCH_STM32 || COMPILE_TEST
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index be98b29d6675d..278f5b8d1c7d7 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -162,6 +162,7 @@ obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
 obj-$(CONFIG_REGULATOR_SC2731) += sc2731-regulator.o
 obj-$(CONFIG_REGULATOR_SKY81452) += sky81452-regulator.o
 obj-$(CONFIG_REGULATOR_SLG51000) += slg51000-regulator.o
+obj-$(CONFIG_REGULATOR_SPACEMIT_P1) += spacemit-p1.o
 obj-$(CONFIG_REGULATOR_STM32_BOOSTER) += stm32-booster.o
 obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
 obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
new file mode 100644
index 0000000000000..d437e6738ea1e
--- /dev/null
+++ b/drivers/regulator/spacemit-p1.c
@@ -0,0 +1,157 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for regulators found in the SpacemiT P1 PMIC
+ *
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ * Derived from code from SpacemiT.
+ *	Copyright (c) 2023, SPACEMIT Co., Ltd
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/linear_range.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+
+#define MOD_NAME	"spacemit-p1-regulator"
+
+enum p1_regulator_id {
+	P1_BUCK1,
+	P1_BUCK2,
+	P1_BUCK3,
+	P1_BUCK4,
+	P1_BUCK5,
+	P1_BUCK6,
+
+	P1_ALDO1,
+	P1_ALDO2,
+	P1_ALDO3,
+	P1_ALDO4,
+
+	P1_DLDO1,
+	P1_DLDO2,
+	P1_DLDO3,
+	P1_DLDO4,
+	P1_DLDO5,
+	P1_DLDO6,
+	P1_DLDO7,
+};
+
+static const struct regulator_ops p1_regulator_ops = {
+	.list_voltage		= regulator_list_voltage_linear_range,
+	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
+	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
+	.set_voltage_time_sel   = regulator_set_voltage_time_sel,
+	.enable			= regulator_enable_regmap,
+	.disable		= regulator_disable_regmap,
+	.is_enabled		= regulator_is_enabled_regmap,
+};
+
+/* Selector value 255 can be used to disable the buck converter on sleep */
+static const struct linear_range p1_buck_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000, 0, 170, 5000),
+	REGULATOR_LINEAR_RANGE(1375000, 171, 254, 25000),
+};
+
+/* Selector value 0 can be used for suspend */
+static const struct linear_range p1_ldo_ranges[] = {
+	REGULATOR_LINEAR_RANGE(500000, 11, 127, 25000),
+};
+
+/* These define the voltage selector field for buck and LDO regulators */
+#define BUCK_MASK		GENMASK(7, 0)
+#define LDO_MASK		GENMASK(6, 0)
+
+#define P1_ID(_TYPE, _n)	P1_ ## _TYPE ## _n
+#define P1_ENABLE_REG(_off, _n)	((_off) + 3 * ((_n) - 1))
+
+#define P1_REG_DESC(_TYPE, _type, _n, _s, _off, _mask, _nv, _ranges)	\
+	{								\
+		.name			= #_type #_n,			\
+		.supply_name		= _s,				\
+		.of_match		= of_match_ptr(#_type #_n),	\
+		.regulators_node	= of_match_ptr("regulators"),	\
+		.id			= P1_ID(_TYPE, _n),		\
+		.n_voltages		= _nv,				\
+		.ops			= &p1_regulator_ops,		\
+		.owner			= THIS_MODULE,			\
+		.linear_ranges		= _ranges,			\
+		.n_linear_ranges	= ARRAY_SIZE(_ranges),		\
+		.vsel_reg		= P1_ENABLE_REG(_off, _n) + 1,	\
+		.vsel_mask		= _mask,			\
+		.enable_reg		= P1_ENABLE_REG(_off, _n),	\
+		.enable_mask		= BIT(0),			\
+	}
+
+#define P1_BUCK_DESC(_n) \
+	P1_REG_DESC(BUCK, buck, _n, "vcc", 0x47, BUCK_MASK, 254, p1_buck_ranges)
+
+#define P1_ALDO_DESC(_n) \
+	P1_REG_DESC(ALDO, aldo, _n, "vcc", 0x5b, LDO_MASK, 117, p1_ldo_ranges)
+
+#define P1_DLDO_DESC(_n) \
+	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 117, p1_ldo_ranges)
+
+static const struct regulator_desc p1_regulator_desc[] = {
+	P1_BUCK_DESC(1),
+	P1_BUCK_DESC(2),
+	P1_BUCK_DESC(3),
+	P1_BUCK_DESC(4),
+	P1_BUCK_DESC(5),
+	P1_BUCK_DESC(6),
+
+	P1_ALDO_DESC(1),
+	P1_ALDO_DESC(2),
+	P1_ALDO_DESC(3),
+	P1_ALDO_DESC(4),
+
+	P1_DLDO_DESC(1),
+	P1_DLDO_DESC(2),
+	P1_DLDO_DESC(3),
+	P1_DLDO_DESC(4),
+	P1_DLDO_DESC(5),
+	P1_DLDO_DESC(6),
+	P1_DLDO_DESC(7),
+};
+
+static int p1_regulator_probe(struct platform_device *pdev)
+{
+	struct regulator_config config = { };
+	struct device *dev = &pdev->dev;
+	u32 i;
+
+	/*
+	 * The parent device (PMIC) owns the regmap.  Since we don't
+	 * provide one in the config structure, that one will be used.
+	 */
+	config.dev = dev->parent;
+
+	for (i = 0; i < ARRAY_SIZE(p1_regulator_desc); i++) {
+		const struct regulator_desc *desc = &p1_regulator_desc[i];
+		struct regulator_dev *rdev;
+
+		rdev = devm_regulator_register(dev, desc, &config);
+		if (IS_ERR(rdev))
+			return dev_err_probe(dev, PTR_ERR(rdev),
+					     "error registering regulator %s\n",
+					     desc->name);
+	}
+
+	return 0;
+}
+
+static struct platform_driver p1_regulator_driver = {
+	.probe = p1_regulator_probe,
+	.driver = {
+		.name = MOD_NAME,
+	},
+};
+
+module_platform_driver(p1_regulator_driver);
+
+MODULE_DESCRIPTION("SpacemiT P1 regulator driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" MOD_NAME);
-- 
2.45.2

[PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Alex Elder]

Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
only setting and reading the time are supported.

The PMIC is implemented as a multi-function device.  This RTC is
probed based on this driver being named in a MFD cell in the simple
MFD I2C driver.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
v3: - Added this driver to the series, in response to Lee Jones saying
      more than one MFD sub-device was required to be acceptable

 drivers/rtc/Kconfig  |  10 ++++
 drivers/rtc/Makefile |   1 +
 drivers/rtc/rtc-p1.c | 137 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 148 insertions(+)
 create mode 100644 drivers/rtc/rtc-p1.c

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 9aec922613cec..27cff02ba4e66 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -406,6 +406,16 @@ config RTC_DRV_MAX77686
 	  This driver can also be built as a module. If so, the module
 	  will be called rtc-max77686.
 
+config RTC_DRV_P1
+	tristate "SpacemiT P1 RTC"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	select MFD_SPACEMIT_P1
+	default ARCH_SPACEMIT
+	help
+	  Enable support for the RTC function in the SpacemiT P1 PMIC.
+	  This driver can also be built as a module, which will be called
+	  "spacemit-p1-rtc".
+
 config RTC_DRV_NCT3018Y
 	tristate "Nuvoton NCT3018Y"
 	depends on OF
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
index 4619aa2ac4697..f8588426e2ba4 100644
--- a/drivers/rtc/Makefile
+++ b/drivers/rtc/Makefile
@@ -171,6 +171,7 @@ obj-$(CONFIG_RTC_DRV_SD2405AL)	+= rtc-sd2405al.o
 obj-$(CONFIG_RTC_DRV_SD3078)	+= rtc-sd3078.o
 obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
 obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
+obj-$(CONFIG_RTC_DRV_P1)	+= rtc-p1.o
 obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
 obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
 obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
diff --git a/drivers/rtc/rtc-p1.c b/drivers/rtc/rtc-p1.c
new file mode 100644
index 0000000000000..e0d2c0c822142
--- /dev/null
+++ b/drivers/rtc/rtc-p1.c
@@ -0,0 +1,137 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the RTC found in the SpacemiT P1 PMIC
+ *
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ */
+
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/rtc.h>
+
+#define MOD_NAME	"spacemit-p1-rtc"
+
+/* Offset to byte containing the given time unit */
+enum time_unit {
+	tu_second = 0,		/* 0-59 */
+	tu_minute,		/* 0-59 */
+	tu_hour,		/* 0-59 */
+	tu_day,			/* 0-30 (struct tm uses 1-31) */
+	tu_month,		/* 0-11 */
+	tu_year,		/* Years since 2000 (struct tm uses 1900) */
+	tu_count,		/* Last; not a time unit */
+};
+
+/* Consecutive bytes contain seconds, minutes, etc. */
+#define RTC_COUNT_BASE		0x0d
+
+#define RTC_CTRL		0x1d
+#define RTC_EN		BIT(2)
+
+struct p1_rtc {
+	struct regmap *regmap;
+	struct rtc_device *rtc;
+};
+
+static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
+{
+	struct p1_rtc *p1 = dev_get_drvdata(dev);
+	u8 time[tu_count];
+	int ret;
+
+	ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, sizeof(time));
+	if (ret)
+		return ret;
+
+	t->tm_sec = time[tu_second] & GENMASK(5, 0);
+	t->tm_min = time[tu_minute] & GENMASK(5, 0);
+	t->tm_hour = time[tu_hour] & GENMASK(4, 0);
+	t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
+	t->tm_mon = time[tu_month] & GENMASK(3, 0);
+	t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;
+	/* tm_wday, tm_yday, and tm_isdst aren't used */
+
+	return 0;
+}
+
+static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
+{
+	struct p1_rtc *p1 = dev_get_drvdata(dev);
+	u8 time[tu_count];
+	int ret;
+
+	time[tu_second] = t->tm_sec;
+	time[tu_minute] = t->tm_min;
+	time[tu_hour] = t->tm_hour;
+	time[tu_day] = t->tm_mday - 1;
+	time[tu_month] = t->tm_mon;
+	time[tu_year] = t->tm_year - 100;
+
+	/* Disable the RTC to update; re-enable again when done */
+	ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, sizeof(time));
+
+	(void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);
+
+	return ret;
+}
+
+static const struct rtc_class_ops p1_rtc_class_ops = {
+	.read_time = p1_rtc_read_time,
+	.set_time = p1_rtc_set_time,
+};
+
+static int p1_rtc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rtc_device *rtc;
+	struct p1_rtc *p1;
+	int ret;
+
+	p1 = devm_kzalloc(dev, sizeof(*p1), GFP_KERNEL);
+	if (!p1)
+		return -ENOMEM;
+	dev_set_drvdata(dev, p1);
+
+	p1->regmap = dev_get_regmap(dev->parent, NULL);
+	if (!p1->regmap)
+		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
+
+	rtc = devm_rtc_allocate_device(dev);
+	if (IS_ERR(rtc))
+		return dev_err_probe(dev, PTR_ERR(rtc),
+				     "error allocating device\n");
+	p1->rtc = rtc;
+
+	rtc->ops = &p1_rtc_class_ops;
+	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
+	rtc->range_max = RTC_TIMESTAMP_END_2063;
+
+	clear_bit(RTC_FEATURE_ALARM, rtc->features);
+	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
+
+	ret = devm_rtc_register_device(rtc);
+	if (ret)
+		return dev_err_probe(dev, ret, "error registering RTC\n");
+
+	return 0;
+}
+
+static struct platform_driver p1_rtc_driver = {
+	.probe = p1_rtc_probe,
+	.driver = {
+		.name = MOD_NAME,
+	},
+};
+
+module_platform_driver(p1_rtc_driver);
+
+MODULE_DESCRIPTION("SpacemiT P1 RTC driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" MOD_NAME);
-- 
2.45.2

[PATCH v3 5/7] riscv: dts: spacemit: enable the i2c8 adapter

[Alex Elder]

Define properties for the I2C adapter that provides access to the
SpacemiT P1 PMIC.  Enable this adapter on the Banana Pi BPI-F3.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts | 15 +++++++++++++++
 arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi    |  7 +++++++
 arch/riscv/boot/dts/spacemit/k1.dtsi            | 11 +++++++++++
 3 files changed, 33 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index fe22c747c5012..7c9f91c88e01a 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -40,6 +40,21 @@ &emmc {
 	status = "okay";
 };
 
+&i2c8 {
+	pinctrl-0 = <&i2c8_cfg>;
+	pinctrl-names = "default";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	status = "okay";
+
+	pmic@41 {
+		compatible = "spacemit,p1";
+		reg = <0x41>;
+		interrupts = <64>;
+		status = "okay";
+	};
+};
+
 &uart0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart0_2_cfg>;
diff --git a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
index 283663647a86f..9d6d4503fe751 100644
--- a/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1-pinctrl.dtsi
@@ -11,6 +11,13 @@
 #define K1_GPIO(x)	(x / 32) (x % 32)
 
 &pinctrl {
+	i2c8_cfg: i2c8-cfg {
+		i2c8-0-pins {
+			pinmux = <K1_PADCONF(93, 0)>,	/* PWR_SCL */
+				 <K1_PADCONF(94, 0)>;	/* PWR_SDA */
+		};
+	};
+
 	uart0_2_cfg: uart0-2-cfg {
 		uart0-2-pins {
 			pinmux = <K1_PADCONF(68, 2)>,
diff --git a/arch/riscv/boot/dts/spacemit/k1.dtsi b/arch/riscv/boot/dts/spacemit/k1.dtsi
index 14097f1f6f447..a85239e8e430b 100644
--- a/arch/riscv/boot/dts/spacemit/k1.dtsi
+++ b/arch/riscv/boot/dts/spacemit/k1.dtsi
@@ -483,6 +483,17 @@ gpio: gpio@d4019000 {
 				      <&pinctrl 3 0 96 32>;
 		};
 
+		i2c8: i2c@d401d800 {
+			compatible = "spacemit,k1-i2c";
+			reg = <0x0 0xd401d800 0x0 0x38>;
+			interrupts = <19>;
+			clocks = <&syscon_apbc CLK_TWSI8>,
+				 <&syscon_apbc CLK_TWSI8_BUS>;
+			clock-names = "func", "bus";
+			clock-frequency = <400000>;
+			status = "disabled";
+		};
+
 		pinctrl: pinctrl@d401e000 {
 			compatible = "spacemit,k1-pinctrl";
 			reg = <0x0 0xd401e000 0x0 0x400>;
-- 
2.45.2

[PATCH v3 6/7] riscv: dts: spacemit: define fixed regulators

[Alex Elder]

Define the DC power input and the 4v power as fixed supplies in the
Banana Pi BPI-F3.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 .../boot/dts/spacemit/k1-bananapi-f3.dts      | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 7c9f91c88e01a..a1c184b814262 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -28,6 +28,25 @@ led1 {
 			default-state = "on";
 		};
 	};
+
+	reg_dc_in: dc-in-12v {
+		compatible = "regulator-fixed";
+		regulator-name = "dc_in_12v";
+		regulator-min-microvolt = <12000000>;
+		regulator-max-microvolt = <12000000>;
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
+	reg_vcc_4v: vcc-4v {
+		compatible = "regulator-fixed";
+		regulator-name = "vcc_4v";
+		regulator-min-microvolt = <4000000>;
+		regulator-max-microvolt = <4000000>;
+		regulator-boot-on;
+		regulator-always-on;
+		vin-supply = <&reg_dc_in>;
+	};
 };
 
 &emmc {
-- 
2.45.2

[PATCH v3 7/7] riscv: dts: spacemit: define regulator constraints

[Alex Elder]

Define basic constraints for the regulators in the SpacemiT P1 PMIC,
as implemented in the Banana Pi BPI-F3.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 .../boot/dts/spacemit/k1-bananapi-f3.dts      | 104 ++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index a1c184b814262..83907cc1d5ccf 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -70,7 +70,111 @@ pmic@41 {
 		compatible = "spacemit,p1";
 		reg = <0x41>;
 		interrupts = <64>;
+		vin-supply = <&reg_vcc_4v>;
 		status = "okay";
+
+		regulators {
+			buck1 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck2 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck3 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <1800000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck4 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck5 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			buck6 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3450000>;
+				regulator-ramp-delay = <5000>;
+				regulator-always-on;
+			};
+
+			aldo1 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+			};
+
+			aldo2 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			aldo3 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			aldo4 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo1 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-boot-on;
+			};
+
+			dldo2 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo3 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo4 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-always-on;
+			};
+
+			dldo5 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+
+			dldo6 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+				regulator-always-on;
+			};
+
+			dldo7 {
+				regulator-min-microvolt = <500000>;
+				regulator-max-microvolt = <3400000>;
+			};
+		};
 	};
 };
 
-- 
2.45.2

Re: [PATCH v3 2/7] mfd: simple-mfd-i2c: add SpacemiT P1 support

[kernel test robot]

Hi Alex,

kernel test robot noticed the following build errors:

[auto build test ERROR on 5d4809e25903ab8e74034c1f23c787fd26d52934]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Elder/dt-bindings-mfd-add-support-the-SpacemiT-P1-PMIC/20250622-113200
base:   5d4809e25903ab8e74034c1f23c787fd26d52934
patch link:    https://lore.kernel.org/r/20250622032941.3768912-3-elder%40riscstar.com
patch subject: [PATCH v3 2/7] mfd: simple-mfd-i2c: add SpacemiT P1 support
config: i386-randconfig-017-20250623 (https://download.01.org/0day-ci/archive/20250623/202506231720.IakJqbvG-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250623/202506231720.IakJqbvG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506231720.IakJqbvG-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: __devm_regmap_init_i2c
   >>> referenced by simple-mfd-i2c.c:47 (drivers/mfd/simple-mfd-i2c.c:47)
   >>>               drivers/mfd/simple-mfd-i2c.o:(simple_mfd_i2c_probe) in archive vmlinux.a
   >>> referenced by simple-mfd-i2c.c:47 (drivers/mfd/simple-mfd-i2c.c:47)
   >>>               drivers/mfd/simple-mfd-i2c.o:(simple_mfd_i2c_probe) in archive vmlinux.a

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MFD_SIMPLE_MFD_I2C
   Depends on [m]: HAS_IOMEM [=y] && I2C [=m]
   Selected by [y]:
   - MFD_SPACEMIT_P1 [=y] && HAS_IOMEM [=y]
   Selected by [m]:
   - JOYSTICK_SENSEHAT [=m] && INPUT_JOYSTICK [=y] && INPUT [=y] && I2C [=m] && HAS_IOMEM [=y]
   - MFD_MAX77705 [=m] && HAS_IOMEM [=y] && I2C [=m]
   - MFD_SY7636A [=m] && HAS_IOMEM [=y] && I2C [=m]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 2/7] mfd: simple-mfd-i2c: add SpacemiT P1 support

[kernel test robot]

Hi Alex,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5d4809e25903ab8e74034c1f23c787fd26d52934]

url:    https://github.com/intel-lab-lkp/linux/commits/Alex-Elder/dt-bindings-mfd-add-support-the-SpacemiT-P1-PMIC/20250622-113200
base:   5d4809e25903ab8e74034c1f23c787fd26d52934
patch link:    https://lore.kernel.org/r/20250622032941.3768912-3-elder%40riscstar.com
patch subject: [PATCH v3 2/7] mfd: simple-mfd-i2c: add SpacemiT P1 support
config: i386-kismet-CONFIG_MFD_SIMPLE_MFD_I2C-CONFIG_MFD_SPACEMIT_P1-0-0 (https://download.01.org/0day-ci/archive/20250623/202506231727.FiPSVMzv-lkp@intel.com/config)
reproduce: (https://download.01.org/0day-ci/archive/20250623/202506231727.FiPSVMzv-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506231727.FiPSVMzv-lkp@intel.com/

kismet warnings: (new ones prefixed by >>)
>> kismet: WARNING: unmet direct dependencies detected for MFD_SIMPLE_MFD_I2C when selected by MFD_SPACEMIT_P1
   WARNING: unmet direct dependencies detected for MFD_SIMPLE_MFD_I2C
     Depends on [n]: HAS_IOMEM [=y] && I2C [=n]
     Selected by [y]:
     - MFD_SPACEMIT_P1 [=y] && HAS_IOMEM [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Mateusz Jończyk]

W dniu 22.06.2025 o 05:29, Alex Elder pisze:
> Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
> only setting and reading the time are supported.
>
> The PMIC is implemented as a multi-function device.  This RTC is
> probed based on this driver being named in a MFD cell in the simple
> MFD I2C driver.
>
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v3: - Added this driver to the series, in response to Lee Jones saying
>        more than one MFD sub-device was required to be acceptable
[snip]
> +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> +	u8 time[tu_count];
> +	int ret;
> +
> +	ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, sizeof(time));
> +	if (ret)
> +		return ret;

Hello,

Are you sure that you read the time parts consistently here? I mean:
is there a risk that the clock is updating below you - so that for
example during the transition

12:59:59 -> 13:00:00

you are going to read 12:00:00 or 12:59:00?

If so, you could for example read the time in a loop until you get
two same readouts.

> +
> +	t->tm_sec = time[tu_second] & GENMASK(5, 0);
> +	t->tm_min = time[tu_minute] & GENMASK(5, 0);
> +	t->tm_hour = time[tu_hour] & GENMASK(4, 0);
> +	t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
> +	t->tm_mon = time[tu_month] & GENMASK(3, 0);
> +	t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;

Is it necessary to use the bitmasks here? Are the higher bits undefined
in hardware? If so, is it necessary to mask them while writing the time
in p1_rtc_set_time()?

> +	/* tm_wday, tm_yday, and tm_isdst aren't used */
> +
> +	return 0;
> +}
> +
> +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> +	u8 time[tu_count];
> +	int ret;
> +
> +	time[tu_second] = t->tm_sec;
> +	time[tu_minute] = t->tm_min;
> +	time[tu_hour] = t->tm_hour;
> +	time[tu_day] = t->tm_mday - 1;
> +	time[tu_month] = t->tm_mon;
> +	time[tu_year] = t->tm_year - 100;
> +
> +	/* Disable the RTC to update; re-enable again when done */
> +	ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, sizeof(time));
> +
> +	(void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);
Perhaps you shouldn't ignore any errors here - failing to restart
the clock should make p1_rtc_set_time() return an error code.
> +
> +	return ret;
> +}
> +

Greetings,

Mateusz

Re: [PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Alexandre Belloni]

Hello,

On 21/06/2025 22:29:36-0500, Alex Elder wrote:
> Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
> only setting and reading the time are supported.
> 
> The PMIC is implemented as a multi-function device.  This RTC is
> probed based on this driver being named in a MFD cell in the simple
> MFD I2C driver.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
> v3: - Added this driver to the series, in response to Lee Jones saying
>       more than one MFD sub-device was required to be acceptable
> 
>  drivers/rtc/Kconfig  |  10 ++++
>  drivers/rtc/Makefile |   1 +
>  drivers/rtc/rtc-p1.c | 137 +++++++++++++++++++++++++++++++++++++++++++

We need something more descriptive than p1 here

>  3 files changed, 148 insertions(+)
>  create mode 100644 drivers/rtc/rtc-p1.c
> 
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 9aec922613cec..27cff02ba4e66 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -406,6 +406,16 @@ config RTC_DRV_MAX77686
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-max77686.
>  
> +config RTC_DRV_P1

Ditto

> +	tristate "SpacemiT P1 RTC"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	select MFD_SPACEMIT_P1
> +	default ARCH_SPACEMIT
> +	help
> +	  Enable support for the RTC function in the SpacemiT P1 PMIC.
> +	  This driver can also be built as a module, which will be called
> +	  "spacemit-p1-rtc".
> +
>  config RTC_DRV_NCT3018Y
>  	tristate "Nuvoton NCT3018Y"
>  	depends on OF
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 4619aa2ac4697..f8588426e2ba4 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -171,6 +171,7 @@ obj-$(CONFIG_RTC_DRV_SD2405AL)	+= rtc-sd2405al.o
>  obj-$(CONFIG_RTC_DRV_SD3078)	+= rtc-sd3078.o
>  obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
>  obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
> +obj-$(CONFIG_RTC_DRV_P1)	+= rtc-p1.o
>  obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
>  obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
>  obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
> diff --git a/drivers/rtc/rtc-p1.c b/drivers/rtc/rtc-p1.c
> new file mode 100644
> index 0000000000000..e0d2c0c822142
> --- /dev/null
> +++ b/drivers/rtc/rtc-p1.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the RTC found in the SpacemiT P1 PMIC
> + *
> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/rtc.h>
> +
> +#define MOD_NAME	"spacemit-p1-rtc"
> +
> +/* Offset to byte containing the given time unit */
> +enum time_unit {
> +	tu_second = 0,		/* 0-59 */
> +	tu_minute,		/* 0-59 */
> +	tu_hour,		/* 0-59 */
> +	tu_day,			/* 0-30 (struct tm uses 1-31) */
> +	tu_month,		/* 0-11 */
> +	tu_year,		/* Years since 2000 (struct tm uses 1900) */
> +	tu_count,		/* Last; not a time unit */
> +};

I'm not sure this enum actually brings anything

> +
> +/* Consecutive bytes contain seconds, minutes, etc. */
> +#define RTC_COUNT_BASE		0x0d
> +
> +#define RTC_CTRL		0x1d
> +#define RTC_EN		BIT(2)
> +
> +struct p1_rtc {
> +	struct regmap *regmap;
> +	struct rtc_device *rtc;
> +};
> +
> +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> +	u8 time[tu_count];
> +	int ret;
> +
> +	ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, sizeof(time));
> +	if (ret)
> +		return ret;
> +
> +	t->tm_sec = time[tu_second] & GENMASK(5, 0);
> +	t->tm_min = time[tu_minute] & GENMASK(5, 0);
> +	t->tm_hour = time[tu_hour] & GENMASK(4, 0);
> +	t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
> +	t->tm_mon = time[tu_month] & GENMASK(3, 0);
> +	t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;
> +	/* tm_wday, tm_yday, and tm_isdst aren't used */
> +
> +	return 0;
> +}
> +
> +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
> +{
> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> +	u8 time[tu_count];
> +	int ret;
> +
> +	time[tu_second] = t->tm_sec;
> +	time[tu_minute] = t->tm_min;
> +	time[tu_hour] = t->tm_hour;
> +	time[tu_day] = t->tm_mday - 1;
> +	time[tu_month] = t->tm_mon;
> +	time[tu_year] = t->tm_year - 100;
> +
> +	/* Disable the RTC to update; re-enable again when done */
> +	ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, sizeof(time));
> +
> +	(void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);

Don't you care whether the RTC has been reenabled?

> +
> +	return ret;
> +}
> +
> +static const struct rtc_class_ops p1_rtc_class_ops = {
> +	.read_time = p1_rtc_read_time,
> +	.set_time = p1_rtc_set_time,
> +};
> +
> +static int p1_rtc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rtc_device *rtc;
> +	struct p1_rtc *p1;
> +	int ret;
> +
> +	p1 = devm_kzalloc(dev, sizeof(*p1), GFP_KERNEL);
> +	if (!p1)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, p1);
> +
> +	p1->regmap = dev_get_regmap(dev->parent, NULL);
> +	if (!p1->regmap)
> +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
> +
> +	rtc = devm_rtc_allocate_device(dev);
> +	if (IS_ERR(rtc))
> +		return dev_err_probe(dev, PTR_ERR(rtc),
> +				     "error allocating device\n");
> +	p1->rtc = rtc;
> +
> +	rtc->ops = &p1_rtc_class_ops;
> +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	rtc->range_max = RTC_TIMESTAMP_END_2063;
> +
> +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
> +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> +
> +	ret = devm_rtc_register_device(rtc);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "error registering RTC\n");

This message is unnecessary, there are no silent error path in
devm_rtc_register_device

> +
> +	return 0;
> +}
> +
> +static struct platform_driver p1_rtc_driver = {
> +	.probe = p1_rtc_probe,
> +	.driver = {
> +		.name = MOD_NAME,
> +	},
> +};
> +
> +module_platform_driver(p1_rtc_driver);
> +
> +MODULE_DESCRIPTION("SpacemiT P1 RTC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" MOD_NAME);

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Alex Elder]

On 6/23/25 1:54 PM, Mateusz Jończyk wrote:
> W dniu 22.06.2025 o 05:29, Alex Elder pisze:
>> Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
>> only setting and reading the time are supported.
>>
>> The PMIC is implemented as a multi-function device.  This RTC is
>> probed based on this driver being named in a MFD cell in the simple
>> MFD I2C driver.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v3: - Added this driver to the series, in response to Lee Jones saying
>>        more than one MFD sub-device was required to be acceptable
> [snip]
>> +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
>> +{
>> +    struct p1_rtc *p1 = dev_get_drvdata(dev);
>> +    u8 time[tu_count];
>> +    int ret;
>> +
>> +    ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, 
>> sizeof(time));
>> +    if (ret)
>> +        return ret;
> 
> Hello,
> 
> Are you sure that you read the time parts consistently here? I mean:
> is there a risk that the clock is updating below you - so that for
> example during the transition

The documentation says this:

   If need to read the calendar value currently recorded inside
   the PMIC, need to read RTC_COUNT_S first. At the same time,
   this operation will latch all current calendar values to the
   registers RTC_COUNT_S ~ RTC_COUNT_Y.

The RTC_COUNT_S register is the first one read in the I2C
bulk request.  So I concluded that the request would record
a consistent timestamp in all 6 registers.

That said, the downstream code did a loop as you describe.  I
will ask the vendor about this.


Related:  I disabled the RTC while updating (which is what the
downstream code did).  But based on what the documentation says
I shouldn't need to do that either.

   If need to reset the current calendar value, need to
   configure each calendar value (RTC_COUNT_S ~ RTC_COUNT_Y)
   in order. After finally writing the register RTC_COUNT_Y,
   PMIC will update the calendar value configured by the
   current user to the internal timer of the RTC module, and
   so on.

Here too, the last register written in the bulk request is
RTC_COUNT_Y, so again I think a consistent value stored in
the 6 registers will be updated at once.

Either way, I'll make an inquiry about both of these.  I'll
explain what I learned in the next version of this series,
and will update the code accordingly.

> 12:59:59 -> 13:00:00
> 
> you are going to read 12:00:00 or 12:59:00?
> 
> If so, you could for example read the time in a loop until you get
> two same readouts.
> 
>> +
>> +    t->tm_sec = time[tu_second] & GENMASK(5, 0);
>> +    t->tm_min = time[tu_minute] & GENMASK(5, 0);
>> +    t->tm_hour = time[tu_hour] & GENMASK(4, 0);
>> +    t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
>> +    t->tm_mon = time[tu_month] & GENMASK(3, 0);
>> +    t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;
> 
> Is it necessary to use the bitmasks here? Are the higher bits undefined
> in hardware? If so, is it necessary to mask them while writing the time
> in p1_rtc_set_time()?

It is probably not necessary to do the bitmask.  But the upper
bits are marked "reserved" and so I the correct thing to do is
to explicitly ignore them (by masking them off).  Is there a
reason you'd rather they not be masked?

There should be no need to mask them in the set_time() function.
The core code verifies all values are in range before it calls
rtc_class_ops->set_time().  So the upper bits should all be zero
by the time p1_rtc_set_time() is called.

>> +    /* tm_wday, tm_yday, and tm_isdst aren't used */
>> +
>> +    return 0;
>> +}
>> +
>> +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
>> +{
>> +    struct p1_rtc *p1 = dev_get_drvdata(dev);
>> +    u8 time[tu_count];
>> +    int ret;
>> +
>> +    time[tu_second] = t->tm_sec;
>> +    time[tu_minute] = t->tm_min;
>> +    time[tu_hour] = t->tm_hour;
>> +    time[tu_day] = t->tm_mday - 1;
>> +    time[tu_month] = t->tm_mon;
>> +    time[tu_year] = t->tm_year - 100;
>> +
>> +    /* Disable the RTC to update; re-enable again when done */
>> +    ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, 
>> sizeof(time));
>> +
>> +    (void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);

> Perhaps you shouldn't ignore any errors here - failing to restart
> the clock should make p1_rtc_set_time() return an error code.

I intentionally return any error from the bulk write call.

But you're right:  if that doesn't have an error and re-enabling
the RTC does, that error should be returned.

I'm hoping there's no need to disable the clock after all,
and that will simplify things here.  But either way, I'll
take your suggestion (but still return the first error if
more than one occurs).

Thanks a lot for the review.

					-Alex

>> +
>> +    return ret;
>> +}
>> +
> 
> Greetings,
> 
> Mateusz
> 

Re: [PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Alex Elder]

On 6/23/25 2:14 PM, Alexandre Belloni wrote:
> Hello,
> 
> On 21/06/2025 22:29:36-0500, Alex Elder wrote:
>> Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
>> only setting and reading the time are supported.
>>
>> The PMIC is implemented as a multi-function device.  This RTC is
>> probed based on this driver being named in a MFD cell in the simple
>> MFD I2C driver.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>> v3: - Added this driver to the series, in response to Lee Jones saying
>>        more than one MFD sub-device was required to be acceptable
>>
>>   drivers/rtc/Kconfig  |  10 ++++
>>   drivers/rtc/Makefile |   1 +
>>   drivers/rtc/rtc-p1.c | 137 +++++++++++++++++++++++++++++++++++++++++++
> 
> We need something more descriptive than p1 here

Are you referring to the chip itself, or do you want a longer
file name?   Do you prefer "rtc-spacemit-p1.c" or something?

>>   3 files changed, 148 insertions(+)
>>   create mode 100644 drivers/rtc/rtc-p1.c
>>
>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>> index 9aec922613cec..27cff02ba4e66 100644
>> --- a/drivers/rtc/Kconfig
>> +++ b/drivers/rtc/Kconfig
>> @@ -406,6 +406,16 @@ config RTC_DRV_MAX77686
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called rtc-max77686.
>>   
>> +config RTC_DRV_P1
> 
> Ditto
> 
>> +	tristate "SpacemiT P1 RTC"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	select MFD_SPACEMIT_P1
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  Enable support for the RTC function in the SpacemiT P1 PMIC.
>> +	  This driver can also be built as a module, which will be called
>> +	  "spacemit-p1-rtc".
>> +
>>   config RTC_DRV_NCT3018Y
>>   	tristate "Nuvoton NCT3018Y"
>>   	depends on OF
>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>> index 4619aa2ac4697..f8588426e2ba4 100644
>> --- a/drivers/rtc/Makefile
>> +++ b/drivers/rtc/Makefile
>> @@ -171,6 +171,7 @@ obj-$(CONFIG_RTC_DRV_SD2405AL)	+= rtc-sd2405al.o
>>   obj-$(CONFIG_RTC_DRV_SD3078)	+= rtc-sd3078.o
>>   obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
>>   obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
>> +obj-$(CONFIG_RTC_DRV_P1)	+= rtc-p1.o
>>   obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
>>   obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
>>   obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
>> diff --git a/drivers/rtc/rtc-p1.c b/drivers/rtc/rtc-p1.c
>> new file mode 100644
>> index 0000000000000..e0d2c0c822142
>> --- /dev/null
>> +++ b/drivers/rtc/rtc-p1.c
>> @@ -0,0 +1,137 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for the RTC found in the SpacemiT P1 PMIC
>> + *
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
>> + */
>> +
>> +#include <linux/bits.h>
>> +#include <linux/device.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/rtc.h>
>> +
>> +#define MOD_NAME	"spacemit-p1-rtc"
>> +
>> +/* Offset to byte containing the given time unit */
>> +enum time_unit {
>> +	tu_second = 0,		/* 0-59 */
>> +	tu_minute,		/* 0-59 */
>> +	tu_hour,		/* 0-59 */
>> +	tu_day,			/* 0-30 (struct tm uses 1-31) */
>> +	tu_month,		/* 0-11 */
>> +	tu_year,		/* Years since 2000 (struct tm uses 1900) */
>> +	tu_count,		/* Last; not a time unit */
>> +};
> 
> I'm not sure this enum actually brings anything

It's just defining the sequence of register values
symbolically.  Do you prefer #defines?

It doesn't matter much to me, I just want to know what
you'd prefer.

> 
>> +
>> +/* Consecutive bytes contain seconds, minutes, etc. */
>> +#define RTC_COUNT_BASE		0x0d
>> +
>> +#define RTC_CTRL		0x1d
>> +#define RTC_EN		BIT(2)
>> +
>> +struct p1_rtc {
>> +	struct regmap *regmap;
>> +	struct rtc_device *rtc;
>> +};
>> +
>> +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
>> +{
>> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
>> +	u8 time[tu_count];
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, sizeof(time));
>> +	if (ret)
>> +		return ret;
>> +
>> +	t->tm_sec = time[tu_second] & GENMASK(5, 0);
>> +	t->tm_min = time[tu_minute] & GENMASK(5, 0);
>> +	t->tm_hour = time[tu_hour] & GENMASK(4, 0);
>> +	t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
>> +	t->tm_mon = time[tu_month] & GENMASK(3, 0);
>> +	t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;
>> +	/* tm_wday, tm_yday, and tm_isdst aren't used */
>> +
>> +	return 0;
>> +}
>> +
>> +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
>> +{
>> +	struct p1_rtc *p1 = dev_get_drvdata(dev);
>> +	u8 time[tu_count];
>> +	int ret;
>> +
>> +	time[tu_second] = t->tm_sec;
>> +	time[tu_minute] = t->tm_min;
>> +	time[tu_hour] = t->tm_hour;
>> +	time[tu_day] = t->tm_mday - 1;
>> +	time[tu_month] = t->tm_mon;
>> +	time[tu_year] = t->tm_year - 100;
>> +
>> +	/* Disable the RTC to update; re-enable again when done */
>> +	ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, sizeof(time));
>> +
>> +	(void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);
> 
> Don't you care whether the RTC has been reenabled?

Yes, Mateusz pointed this out.  I'll fix this.

I hope that disabling and re-enabling isn't require,
which makes the error possibilities a lot simpler.

Otherwise it's not clear how best to recover from
an error re-enabling the RTC (but yes, if no error
occurs in the bulk write, an error when re-enabling
will be returned).

>> +
>> +	return ret;
>> +}
>> +
>> +static const struct rtc_class_ops p1_rtc_class_ops = {
>> +	.read_time = p1_rtc_read_time,
>> +	.set_time = p1_rtc_set_time,
>> +};
>> +
>> +static int p1_rtc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct rtc_device *rtc;
>> +	struct p1_rtc *p1;
>> +	int ret;
>> +
>> +	p1 = devm_kzalloc(dev, sizeof(*p1), GFP_KERNEL);
>> +	if (!p1)
>> +		return -ENOMEM;
>> +	dev_set_drvdata(dev, p1);
>> +
>> +	p1->regmap = dev_get_regmap(dev->parent, NULL);
>> +	if (!p1->regmap)
>> +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
>> +
>> +	rtc = devm_rtc_allocate_device(dev);
>> +	if (IS_ERR(rtc))
>> +		return dev_err_probe(dev, PTR_ERR(rtc),
>> +				     "error allocating device\n");
>> +	p1->rtc = rtc;
>> +
>> +	rtc->ops = &p1_rtc_class_ops;
>> +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
>> +	rtc->range_max = RTC_TIMESTAMP_END_2063;
>> +
>> +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
>> +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
>> +
>> +	ret = devm_rtc_register_device(rtc);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "error registering RTC\n");
> 
> This message is unnecessary, there are no silent error path in
> devm_rtc_register_device

Awesome.  I'll just return what devm_rtc_regsister_device()
returns.

Thanks a lot.

					-Alex

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver p1_rtc_driver = {
>> +	.probe = p1_rtc_probe,
>> +	.driver = {
>> +		.name = MOD_NAME,
>> +	},
>> +};
>> +
>> +module_platform_driver(p1_rtc_driver);
>> +
>> +MODULE_DESCRIPTION("SpacemiT P1 RTC driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" MOD_NAME);
> 

Re: [PATCH v3 4/7] rtc: spacemit: support the SpacemiT P1 RTC

[Alexandre Belloni]

On 23/06/2025 15:03:25-0500, Alex Elder wrote:
> On 6/23/25 2:14 PM, Alexandre Belloni wrote:
> > Hello,
> > 
> > On 21/06/2025 22:29:36-0500, Alex Elder wrote:
> > > Add support for the RTC found in the SpacemiT P1 PMIC.  Initially
> > > only setting and reading the time are supported.
> > > 
> > > The PMIC is implemented as a multi-function device.  This RTC is
> > > probed based on this driver being named in a MFD cell in the simple
> > > MFD I2C driver.
> > > 
> > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > ---
> > > v3: - Added this driver to the series, in response to Lee Jones saying
> > >        more than one MFD sub-device was required to be acceptable
> > > 
> > >   drivers/rtc/Kconfig  |  10 ++++
> > >   drivers/rtc/Makefile |   1 +
> > >   drivers/rtc/rtc-p1.c | 137 +++++++++++++++++++++++++++++++++++++++++++
> > 
> > We need something more descriptive than p1 here
> 
> Are you referring to the chip itself, or do you want a longer
> file name?   Do you prefer "rtc-spacemit-p1.c" or something?

Yes, this would be better, I feel like p1 is too short and is going to conflict
later on.

> 
> > >   3 files changed, 148 insertions(+)
> > >   create mode 100644 drivers/rtc/rtc-p1.c
> > > 
> > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > > index 9aec922613cec..27cff02ba4e66 100644
> > > --- a/drivers/rtc/Kconfig
> > > +++ b/drivers/rtc/Kconfig
> > > @@ -406,6 +406,16 @@ config RTC_DRV_MAX77686
> > >   	  This driver can also be built as a module. If so, the module
> > >   	  will be called rtc-max77686.
> > > +config RTC_DRV_P1
> > 
> > Ditto
> > 
> > > +	tristate "SpacemiT P1 RTC"
> > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > > +	select MFD_SPACEMIT_P1
> > > +	default ARCH_SPACEMIT
> > > +	help
> > > +	  Enable support for the RTC function in the SpacemiT P1 PMIC.
> > > +	  This driver can also be built as a module, which will be called
> > > +	  "spacemit-p1-rtc".
> > > +
> > >   config RTC_DRV_NCT3018Y
> > >   	tristate "Nuvoton NCT3018Y"
> > >   	depends on OF
> > > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> > > index 4619aa2ac4697..f8588426e2ba4 100644
> > > --- a/drivers/rtc/Makefile
> > > +++ b/drivers/rtc/Makefile
> > > @@ -171,6 +171,7 @@ obj-$(CONFIG_RTC_DRV_SD2405AL)	+= rtc-sd2405al.o
> > >   obj-$(CONFIG_RTC_DRV_SD3078)	+= rtc-sd3078.o
> > >   obj-$(CONFIG_RTC_DRV_SH)	+= rtc-sh.o
> > >   obj-$(CONFIG_RTC_DRV_SNVS)	+= rtc-snvs.o
> > > +obj-$(CONFIG_RTC_DRV_P1)	+= rtc-p1.o
> > >   obj-$(CONFIG_RTC_DRV_SPEAR)	+= rtc-spear.o
> > >   obj-$(CONFIG_RTC_DRV_STARFIRE)	+= rtc-starfire.o
> > >   obj-$(CONFIG_RTC_DRV_STK17TA8)	+= rtc-stk17ta8.o
> > > diff --git a/drivers/rtc/rtc-p1.c b/drivers/rtc/rtc-p1.c
> > > new file mode 100644
> > > index 0000000000000..e0d2c0c822142
> > > --- /dev/null
> > > +++ b/drivers/rtc/rtc-p1.c
> > > @@ -0,0 +1,137 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Driver for the RTC found in the SpacemiT P1 PMIC
> > > + *
> > > + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> > > + */
> > > +
> > > +#include <linux/bits.h>
> > > +#include <linux/device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/rtc.h>
> > > +
> > > +#define MOD_NAME	"spacemit-p1-rtc"
> > > +
> > > +/* Offset to byte containing the given time unit */
> > > +enum time_unit {
> > > +	tu_second = 0,		/* 0-59 */
> > > +	tu_minute,		/* 0-59 */
> > > +	tu_hour,		/* 0-59 */
> > > +	tu_day,			/* 0-30 (struct tm uses 1-31) */
> > > +	tu_month,		/* 0-11 */
> > > +	tu_year,		/* Years since 2000 (struct tm uses 1900) */
> > > +	tu_count,		/* Last; not a time unit */
> > > +};
> > 
> > I'm not sure this enum actually brings anything
> 
> It's just defining the sequence of register values
> symbolically.  Do you prefer #defines?
> 
> It doesn't matter much to me, I just want to know what
> you'd prefer.

Most of the drivers simply use the index number directly as the tm_* members are
descriptive enough.

> 
> > 
> > > +
> > > +/* Consecutive bytes contain seconds, minutes, etc. */
> > > +#define RTC_COUNT_BASE		0x0d
> > > +
> > > +#define RTC_CTRL		0x1d
> > > +#define RTC_EN		BIT(2)
> > > +
> > > +struct p1_rtc {
> > > +	struct regmap *regmap;
> > > +	struct rtc_device *rtc;
> > > +};
> > > +
> > > +static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
> > > +{
> > > +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> > > +	u8 time[tu_count];
> > > +	int ret;
> > > +
> > > +	ret = regmap_bulk_read(p1->regmap, RTC_COUNT_BASE, &time, sizeof(time));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	t->tm_sec = time[tu_second] & GENMASK(5, 0);
> > > +	t->tm_min = time[tu_minute] & GENMASK(5, 0);
> > > +	t->tm_hour = time[tu_hour] & GENMASK(4, 0);
> > > +	t->tm_mday = (time[tu_day] & GENMASK(4, 0)) + 1;
> > > +	t->tm_mon = time[tu_month] & GENMASK(3, 0);
> > > +	t->tm_year = (time[tu_year] & GENMASK(5, 0)) + 100;
> > > +	/* tm_wday, tm_yday, and tm_isdst aren't used */
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int p1_rtc_set_time(struct device *dev, struct rtc_time *t)
> > > +{
> > > +	struct p1_rtc *p1 = dev_get_drvdata(dev);
> > > +	u8 time[tu_count];
> > > +	int ret;
> > > +
> > > +	time[tu_second] = t->tm_sec;
> > > +	time[tu_minute] = t->tm_min;
> > > +	time[tu_hour] = t->tm_hour;
> > > +	time[tu_day] = t->tm_mday - 1;
> > > +	time[tu_month] = t->tm_mon;
> > > +	time[tu_year] = t->tm_year - 100;
> > > +
> > > +	/* Disable the RTC to update; re-enable again when done */
> > > +	ret = regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, 0);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = regmap_bulk_write(p1->regmap, RTC_COUNT_BASE, time, sizeof(time));
> > > +
> > > +	(void)regmap_update_bits(p1->regmap, RTC_CTRL, RTC_EN, RTC_EN);
> > 
> > Don't you care whether the RTC has been reenabled?
> 
> Yes, Mateusz pointed this out.  I'll fix this.
> 
> I hope that disabling and re-enabling isn't require,
> which makes the error possibilities a lot simpler.
> 
> Otherwise it's not clear how best to recover from
> an error re-enabling the RTC (but yes, if no error
> occurs in the bulk write, an error when re-enabling
> will be returned).
> 
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct rtc_class_ops p1_rtc_class_ops = {
> > > +	.read_time = p1_rtc_read_time,
> > > +	.set_time = p1_rtc_set_time,
> > > +};
> > > +
> > > +static int p1_rtc_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct rtc_device *rtc;
> > > +	struct p1_rtc *p1;
> > > +	int ret;
> > > +
> > > +	p1 = devm_kzalloc(dev, sizeof(*p1), GFP_KERNEL);
> > > +	if (!p1)
> > > +		return -ENOMEM;
> > > +	dev_set_drvdata(dev, p1);
> > > +
> > > +	p1->regmap = dev_get_regmap(dev->parent, NULL);
> > > +	if (!p1->regmap)
> > > +		return dev_err_probe(dev, -ENODEV, "failed to get regmap\n");
> > > +
> > > +	rtc = devm_rtc_allocate_device(dev);
> > > +	if (IS_ERR(rtc))
> > > +		return dev_err_probe(dev, PTR_ERR(rtc),
> > > +				     "error allocating device\n");
> > > +	p1->rtc = rtc;
> > > +
> > > +	rtc->ops = &p1_rtc_class_ops;
> > > +	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > > +	rtc->range_max = RTC_TIMESTAMP_END_2063;
> > > +
> > > +	clear_bit(RTC_FEATURE_ALARM, rtc->features);
> > > +	clear_bit(RTC_FEATURE_UPDATE_INTERRUPT, rtc->features);
> > > +
> > > +	ret = devm_rtc_register_device(rtc);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "error registering RTC\n");
> > 
> > This message is unnecessary, there are no silent error path in
> > devm_rtc_register_device
> 
> Awesome.  I'll just return what devm_rtc_regsister_device()
> returns.
> 
> Thanks a lot.
> 
> 					-Alex
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver p1_rtc_driver = {
> > > +	.probe = p1_rtc_probe,
> > > +	.driver = {
> > > +		.name = MOD_NAME,
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(p1_rtc_driver);
> > > +
> > > +MODULE_DESCRIPTION("SpacemiT P1 RTC driver");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:" MOD_NAME);
> > 
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com