[PATCH 0/6] spacemit: introduce P1 PMIC and regulator support

[PATCH 0/6] spacemit: introduce P1 PMIC and regulator support

[Alex Elder]

The SpacemiT P1 is an I2C-controlled PMIC that implements six buck
converters and twelve 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 a driver for its regulators.

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

					-Alex

Alex Elder (6):
  dt-bindings: mfd: add support the SpacmiT P1 PMIC
  mfd: spacemit: add support for SpacemiT PMICs
  regulator: spacemit: support SpacemiT P1 regulators
  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                           |  11 ++
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/spacemit-pmic.c                   |  91 +++++++++++
 drivers/regulator/Kconfig                     |   9 +
 drivers/regulator/Makefile                    |   1 +
 drivers/regulator/spacemit-p1.c               | 154 ++++++++++++++++++
 10 files changed, 509 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
 create mode 100644 drivers/mfd/spacemit-pmic.c
 create mode 100644 drivers/regulator/spacemit-p1.c


base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
-- 
2.45.2

[PATCH 1/6] dt-bindings: mfd: add support the SpacmiT P1 PMIC

[Alex Elder]

Enable the SpacemiT P1, which is an I2C-controlled PMIC.  Initially we
only the 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

Re: [PATCH 1/6] dt-bindings: mfd: add support the SpacmiT P1 PMIC

[Vivian Wang]

Hi Alex,

Before the patch body, there's a typo in the subject line: "SpacmiT" ->
"SpacemiT".

On 6/14/25 05:01, Alex Elder wrote:
> Enable the SpacemiT P1, which is an I2C-controlled PMIC.  Initially we
> only the 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
>
> <snip>
>
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        pmic@41 {
> +            compatible = "spacemit,p1";
> +            reg = <0x41>;
> +            interrupts = <64>;
> +

(Not really a review, just a note.)

I couldn't believe it at first, but it looks like the SpacemiT K1 really
does have PLIC interrupt 64 dedicated to receiving PMIC interrupt [1]
over pin "PMIC_INT_N".

Regards,
Vivian "dramforever" Wang

[1]: https://developer.spacemit.com/documentation?token=AZsXwVrqaisaDGkUqMWcNuMVnPb

> <snip>

Re: [PATCH 1/6] dt-bindings: mfd: add support the SpacmiT P1 PMIC

[Alex Elder]

On 6/19/25 1:03 AM, Vivian Wang wrote:
> Hi Alex,
> 
> Before the patch body, there's a typo in the subject line: "SpacmiT" ->
> "SpacemiT".

Thank you, I will fix that in v2, which I plan to post today.

					-Alex

> On 6/14/25 05:01, Alex Elder wrote:
>> Enable the SpacemiT P1, which is an I2C-controlled PMIC.  Initially we
>> only the 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
>>
>> <snip>
>>
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        pmic@41 {
>> +            compatible = "spacemit,p1";
>> +            reg = <0x41>;
>> +            interrupts = <64>;
>> +
> 
> (Not really a review, just a note.)
> 
> I couldn't believe it at first, but it looks like the SpacemiT K1 really
> does have PLIC interrupt 64 dedicated to receiving PMIC interrupt [1]
> over pin "PMIC_INT_N".
> 
> Regards,
> Vivian "dramforever" Wang
> 
> [1]: https://developer.spacemit.com/documentation?token=AZsXwVrqaisaDGkUqMWcNuMVnPb
> 
>> <snip>
> 

[PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Alex Elder]

Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
but the driver is structured to allow support for others to be added.

The P1 PMIC is controlled by I2C, and is normally implemented with the
SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
regulators.  It also implements a switch, watchdog timer, real-time clock,
and more, but initially we will only support its regulators.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/mfd/Kconfig         | 11 +++++
 drivers/mfd/Makefile        |  1 +
 drivers/mfd/spacemit-pmic.c | 91 +++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+)
 create mode 100644 drivers/mfd/spacemit-pmic.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6fb3768e3d71c..c59ae6cc2dd8d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1182,6 +1182,17 @@ 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_PMIC
+	tristate "SpacemiT PMIC"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on I2C && OF
+	select MFD_CORE
+	select REGMAP_I2C
+	default ARCH_SPACEMIT
+	help
+	  This option enables support for SpacemiT I2C based PMICs.  At
+	  this time only the P1 PMIC (used with the K1 SoC) is supported.
+
 config MFD_SPMI_PMIC
 	tristate "Qualcomm SPMI PMICs"
 	depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 79495f9f3457b..59d1ec8db3a3f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
 obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
 obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
 obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
+obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
 obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
 obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
 obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
new file mode 100644
index 0000000000000..7c3c3e27236da
--- /dev/null
+++ b/drivers/mfd/spacemit-pmic.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ * Derived from code from:
+ *	Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+struct spacemit_pmic_data {
+	const struct regmap_config *regmap_config;
+	const struct mfd_cell *mfd_cells;	/* array */
+	size_t mfd_cell_count;
+};
+
+static const struct regmap_config p1_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= 0xaa,
+};
+
+/* The name field defines the *driver* name that should bind to the device */
+static const struct mfd_cell p1_cells[] = {
+	{
+		.name		= "spacemit-p1-regulator",
+	},
+};
+
+static const struct spacemit_pmic_data p1_pmic_data = {
+	.regmap_config	= &p1_regmap_config,
+	.mfd_cells	= p1_cells,
+	.mfd_cell_count	= ARRAY_SIZE(p1_cells),
+};
+
+static int spacemit_pmic_probe(struct i2c_client *client)
+{
+	const struct spacemit_pmic_data *data;
+	struct device *dev = &client->dev;
+	struct regmap *regmap;
+
+	/* We currently have no need for a device-specific structure */
+	data = of_device_get_match_data(dev);
+	regmap = devm_regmap_init_i2c(client, data->regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "regmap initialization failed");
+
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
+				    data->mfd_cells, data->mfd_cell_count,
+				    NULL, 0, NULL);
+}
+
+static const struct of_device_id spacemit_pmic_match[] = {
+	{
+		.compatible	= "spacemit,p1",
+		.data		= &p1_pmic_data,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, spacemit_pmic_match);
+
+static struct i2c_driver spacemit_pmic_i2c_driver = {
+	.driver = {
+		.name = "spacemit-pmic",
+		.of_match_table = spacemit_pmic_match,
+	},
+	.probe    = spacemit_pmic_probe,
+};
+
+static int __init spacemit_pmic_init(void)
+{
+	return i2c_add_driver(&spacemit_pmic_i2c_driver);
+}
+
+static void __exit spacemit_pmic_exit(void)
+{
+	i2c_del_driver(&spacemit_pmic_i2c_driver);
+}
+
+module_init(spacemit_pmic_init);
+module_exit(spacemit_pmic_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SpacemiT multi-function PMIC driver");
-- 
2.45.2

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Vivian Wang]

On 6/14/25 05:01, Alex Elder wrote:
> <snip>
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6fb3768e3d71c..c59ae6cc2dd8d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1182,6 +1182,17 @@ 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_PMIC
> +	tristate "SpacemiT PMIC"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on I2C && OF
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	default ARCH_SPACEMIT
> +	help
> +	  This option enables support for SpacemiT I2C based PMICs.  At
> +	  this time only the P1 PMIC (used with the K1 SoC) is supported.
> +

Module name?

+	  To compile this driver as a module, choose M here: the
+	  module will be called spacemit-pmic.

>  config MFD_SPMI_PMIC
>  	tristate "Qualcomm SPMI PMICs"
>  	depends on ARCH_QCOM || COMPILE_TEST
>
> <snip>
>
> +static struct i2c_driver spacemit_pmic_i2c_driver = {
> +	.driver = {
> +		.name = "spacemit-pmic",
> +		.of_match_table = spacemit_pmic_match,
> +	},
> +	.probe    = spacemit_pmic_probe,
> +};
> +
> +static int __init spacemit_pmic_init(void)
> +{
> +	return i2c_add_driver(&spacemit_pmic_i2c_driver);
> +}
> +
> +static void __exit spacemit_pmic_exit(void)
> +{
> +	i2c_del_driver(&spacemit_pmic_i2c_driver);
> +}
> +
> +module_init(spacemit_pmic_init);
> +module_exit(spacemit_pmic_exit);
> +

module_i2c_driver

Regards,
Vivian "dramforever" Wang

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SpacemiT multi-function PMIC driver");

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Alex Elder]

On 6/19/25 12:46 AM, Vivian Wang wrote:
> On 6/14/25 05:01, Alex Elder wrote:
>> <snip>
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 6fb3768e3d71c..c59ae6cc2dd8d 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1182,6 +1182,17 @@ 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_PMIC
>> +	tristate "SpacemiT PMIC"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on I2C && OF
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  This option enables support for SpacemiT I2C based PMICs.  At
>> +	  this time only the P1 PMIC (used with the K1 SoC) is supported.
>> +
> 
> Module name?
> 
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called spacemit-pmic.

I will add a comment like this to the help text.

> 
>>   config MFD_SPMI_PMIC
>>   	tristate "Qualcomm SPMI PMICs"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>>
>> <snip>
>>
>> +static struct i2c_driver spacemit_pmic_i2c_driver = {
>> +	.driver = {
>> +		.name = "spacemit-pmic",
>> +		.of_match_table = spacemit_pmic_match,
>> +	},
>> +	.probe    = spacemit_pmic_probe,
>> +};
>> +
>> +static int __init spacemit_pmic_init(void)
>> +{
>> +	return i2c_add_driver(&spacemit_pmic_i2c_driver);
>> +}
>> +
>> +static void __exit spacemit_pmic_exit(void)
>> +{
>> +	i2c_del_driver(&spacemit_pmic_i2c_driver);
>> +}
>> +
>> +module_init(spacemit_pmic_init);
>> +module_exit(spacemit_pmic_exit);
>> +
> 
> module_i2c_driver

You're right.  Thanks for the review.  I also added
MODULE_ALIAS() for this driver, and defined MOD_NAME.

					-Alex
> 
> Regards,
> Vivian "dramforever" Wang
> 
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("SpacemiT multi-function PMIC driver");
> 

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Lee Jones]

On Fri, 13 Jun 2025, Alex Elder wrote:

> Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
> but the driver is structured to allow support for others to be added.
> 
> The P1 PMIC is controlled by I2C, and is normally implemented with the
> SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO

six or 12.  Please pick a format and remain consistent.

> regulators.  It also implements a switch, watchdog timer, real-time clock,
> and more, but initially we will only support its regulators.

You have to provide support for more than one device for this to be
accepted into MFD.

> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  drivers/mfd/Kconfig         | 11 +++++
>  drivers/mfd/Makefile        |  1 +
>  drivers/mfd/spacemit-pmic.c | 91 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 103 insertions(+)
>  create mode 100644 drivers/mfd/spacemit-pmic.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6fb3768e3d71c..c59ae6cc2dd8d 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1182,6 +1182,17 @@ 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_PMIC
> +	tristate "SpacemiT PMIC"
> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on I2C && OF
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	default ARCH_SPACEMIT
> +	help
> +	  This option enables support for SpacemiT I2C based PMICs.  At
> +	  this time only the P1 PMIC (used with the K1 SoC) is supported.
> +
>  config MFD_SPMI_PMIC
>  	tristate "Qualcomm SPMI PMICs"
>  	depends on ARCH_QCOM || COMPILE_TEST
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 79495f9f3457b..59d1ec8db3a3f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
>  obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>  obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>  obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
> +obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
>  obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>  obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>  obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
> diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
> new file mode 100644
> index 0000000000000..7c3c3e27236da
> --- /dev/null
> +++ b/drivers/mfd/spacemit-pmic.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> + * Derived from code from:
> + *	Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +struct spacemit_pmic_data {

s/data/ddata/

> +	const struct regmap_config *regmap_config;
> +	const struct mfd_cell *mfd_cells;	/* array */

Hmm ... this is a red flag.  Let's see.

> +	size_t mfd_cell_count;
> +};
> +
> +static const struct regmap_config p1_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +	.max_register	= 0xaa,
> +};
> +
> +/* The name field defines the *driver* name that should bind to the device */

This comment is superfluous.

> +static const struct mfd_cell p1_cells[] = {
> +	{
> +		.name		= "spacemit-p1-regulator",

This spacing is wonky.  Take a look at all the other drivers here.

Also, you probably want to use MFD_CELL_NAME().

One is not enough.

> +	},
> +};
> +
> +static const struct spacemit_pmic_data p1_pmic_data = {
> +	.regmap_config	= &p1_regmap_config,
> +	.mfd_cells	= p1_cells,
> +	.mfd_cell_count	= ARRAY_SIZE(p1_cells),
> +};
> +
> +static int spacemit_pmic_probe(struct i2c_client *client)
> +{
> +	const struct spacemit_pmic_data *data;
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +
> +	/* We currently have no need for a device-specific structure */

Then why are we adding one?

> +	data = of_device_get_match_data(dev);
> +	regmap = devm_regmap_init_i2c(client, data->regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "regmap initialization failed");
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> +				    data->mfd_cells, data->mfd_cell_count,
> +				    NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id spacemit_pmic_match[] = {
> +	{
> +		.compatible	= "spacemit,p1",
> +		.data		= &p1_pmic_data,

Ah, now I see.

We do not allow one data from registration mechanism (MFD) to be piped
through another (OF).  If you have to match platform data to device (you
don't), then pass through identifiers and match on those in a switch()
statement instead.

> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, spacemit_pmic_match);
> +
> +static struct i2c_driver spacemit_pmic_i2c_driver = {
> +	.driver = {
> +		.name = "spacemit-pmic",
> +		.of_match_table = spacemit_pmic_match,
> +	},
> +	.probe    = spacemit_pmic_probe,

Remove these odd tabs please.

> +};
> +
> +static int __init spacemit_pmic_init(void)
> +{
> +	return i2c_add_driver(&spacemit_pmic_i2c_driver);
> +}
> +
> +static void __exit spacemit_pmic_exit(void)
> +{
> +	i2c_del_driver(&spacemit_pmic_i2c_driver);
> +}
> +

Remove this line.

> +module_init(spacemit_pmic_init);
> +module_exit(spacemit_pmic_exit);

Are you sure there isn't some boiler plate to do all of this?

Ah ha:

  module_i2c_driver()

> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SpacemiT multi-function PMIC driver");
> -- 
> 2.45.2
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Lee Jones]

On Thu, 19 Jun 2025, Lee Jones wrote:

> On Fri, 13 Jun 2025, Alex Elder wrote:
> 
> > Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
> > but the driver is structured to allow support for others to be added.
> > 
> > The P1 PMIC is controlled by I2C, and is normally implemented with the
> > SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
> 
> six or 12.  Please pick a format and remain consistent.
> 
> > regulators.  It also implements a switch, watchdog timer, real-time clock,
> > and more, but initially we will only support its regulators.
> 
> You have to provide support for more than one device for this to be
> accepted into MFD.
> 
> > Signed-off-by: Alex Elder <elder@riscstar.com>
> > ---
> >  drivers/mfd/Kconfig         | 11 +++++
> >  drivers/mfd/Makefile        |  1 +
> >  drivers/mfd/spacemit-pmic.c | 91 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 103 insertions(+)
> >  create mode 100644 drivers/mfd/spacemit-pmic.c

Right now, it looks like all you need is:

drivers/mfd/simple-mfd-i2c.c

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Alex Elder]

On 6/19/25 9:41 AM, Lee Jones wrote:
> On Thu, 19 Jun 2025, Lee Jones wrote:
> 
>> On Fri, 13 Jun 2025, Alex Elder wrote:
>>
>>> Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
>>> but the driver is structured to allow support for others to be added.
>>>
>>> The P1 PMIC is controlled by I2C, and is normally implemented with the
>>> SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
>>
>> six or 12.  Please pick a format and remain consistent.
>>
>>> regulators.  It also implements a switch, watchdog timer, real-time clock,
>>> and more, but initially we will only support its regulators.
>>
>> You have to provide support for more than one device for this to be
>> accepted into MFD.
>>
>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>> ---
>>>   drivers/mfd/Kconfig         | 11 +++++
>>>   drivers/mfd/Makefile        |  1 +
>>>   drivers/mfd/spacemit-pmic.c | 91 +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 103 insertions(+)
>>>   create mode 100644 drivers/mfd/spacemit-pmic.c
> 
> Right now, it looks like all you need is:
> 
> drivers/mfd/simple-mfd-i2c.c

Now *that* is a good suggestion.  I'll look at that now and will use
that if I can.

Thanks.

					-Alex

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Alex Elder]

On 6/19/25 9:40 AM, Lee Jones wrote:
> On Fri, 13 Jun 2025, Alex Elder wrote:
> 
>> Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
>> but the driver is structured to allow support for others to be added.
>>
>> The P1 PMIC is controlled by I2C, and is normally implemented with the
>> SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
> 
> six or 12.  Please pick a format and remain consistent.

"Numbers smaller than ten should be spelled out."

But I'll use 6 and 12.

>> regulators.  It also implements a switch, watchdog timer, real-time clock,
>> and more, but initially we will only support its regulators.
> 
> You have to provide support for more than one device for this to be
> accepted into MFD.

OK.  I'm looking at the other device functions to see if I
can pick the easiest one.

>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>>   drivers/mfd/Kconfig         | 11 +++++
>>   drivers/mfd/Makefile        |  1 +
>>   drivers/mfd/spacemit-pmic.c | 91 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 103 insertions(+)
>>   create mode 100644 drivers/mfd/spacemit-pmic.c
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 6fb3768e3d71c..c59ae6cc2dd8d 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1182,6 +1182,17 @@ 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_PMIC
>> +	tristate "SpacemiT PMIC"
>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on I2C && OF
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  This option enables support for SpacemiT I2C based PMICs.  At
>> +	  this time only the P1 PMIC (used with the K1 SoC) is supported.
>> +
>>   config MFD_SPMI_PMIC
>>   	tristate "Qualcomm SPMI PMICs"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 79495f9f3457b..59d1ec8db3a3f 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
>>   obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
>>   obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
>>   obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
>> +obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
>>   obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
>>   obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
>>   obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
>> diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
>> new file mode 100644
>> index 0000000000000..7c3c3e27236da
>> --- /dev/null
>> +++ b/drivers/mfd/spacemit-pmic.c
>> @@ -0,0 +1,91 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
>> + * Derived from code from:
>> + *	Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +
>> +struct spacemit_pmic_data {
> 
> s/data/ddata/

I hadn't noticed that convention.  I'll use it.

>> +	const struct regmap_config *regmap_config;
>> +	const struct mfd_cell *mfd_cells;	/* array */
> 
> Hmm ... this is a red flag.  Let's see.
> 
>> +	size_t mfd_cell_count;
>> +};
>> +
>> +static const struct regmap_config p1_regmap_config = {
>> +	.reg_bits	= 8,
>> +	.val_bits	= 8,
>> +	.max_register	= 0xaa,
>> +};
>> +
>> +/* The name field defines the *driver* name that should bind to the device */
> 
> This comment is superfluous.

I'll delete it.

I was expecting the driver to recognize the device, not
the device specifying what driver to use, but I guess
I'm used to the DT model.

>> +static const struct mfd_cell p1_cells[] = {
>> +	{
>> +		.name		= "spacemit-p1-regulator",
> 
> This spacing is wonky.  Take a look at all the other drivers here.
> 
> Also, you probably want to use MFD_CELL_NAME().

Yes, I see that does what I want.

> One is not enough.
> 
>> +	},
>> +};
>> +
>> +static const struct spacemit_pmic_data p1_pmic_data = {
>> +	.regmap_config	= &p1_regmap_config,
>> +	.mfd_cells	= p1_cells,
>> +	.mfd_cell_count	= ARRAY_SIZE(p1_cells),
>> +};
>> +
>> +static int spacemit_pmic_probe(struct i2c_client *client)
>> +{
>> +	const struct spacemit_pmic_data *data;
>> +	struct device *dev = &client->dev;
>> +	struct regmap *regmap;
>> +
>> +	/* We currently have no need for a device-specific structure */
> 
> Then why are we adding one?

I don't understand, but it might be moot once I add support
for another (sub)device.

>> +	data = of_device_get_match_data(dev);
>> +	regmap = devm_regmap_init_i2c(client, data->regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return dev_err_probe(dev, PTR_ERR(regmap),
>> +				     "regmap initialization failed");
>> +
>> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
>> +				    data->mfd_cells, data->mfd_cell_count,
>> +				    NULL, 0, NULL);
>> +}
>> +
>> +static const struct of_device_id spacemit_pmic_match[] = {
>> +	{
>> +		.compatible	= "spacemit,p1",
>> +		.data		= &p1_pmic_data,
> 
> Ah, now I see.
> 
> We do not allow one data from registration mechanism (MFD) to be piped
> through another (OF).  If you have to match platform data to device (you
> don't), then pass through identifiers and match on those in a switch()
> statement instead.

I haven't done an MFD driver before and it took some time
to get this working.  I'll tell you what led me to it.

I used code posted by Troy Mitchell (plus downstream) as a
starting point.
   https://lore.kernel.org/lkml/20241230-k1-p1-v1-0-aa4e02b9f993@gmail.com/

Krzysztof Kozlowski made this comment on Troy's DT binding:
   Drop compatible, regulators are not re-usable blocks.

So my goal was to have the PMIC regulators get bound to a
driver without specifying a DT compatible string, and I
found this worked.

You say I don't need to match platform data to device, but
if I did I would pass through identifiers.  Can you refer
me to an example of code that correctly does what I should
be doing instead?

One other comment/question:
   This driver is structured as if it could support a different
   PMIC (in addition to P1).  Should I *not* do that, and simply
   make a source file hard-coded for this one PMIC?

>> +	},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, spacemit_pmic_match);
>> +
>> +static struct i2c_driver spacemit_pmic_i2c_driver = {
>> +	.driver = {
>> +		.name = "spacemit-pmic",
>> +		.of_match_table = spacemit_pmic_match,
>> +	},
>> +	.probe    = spacemit_pmic_probe,
> 
> Remove these odd tabs please.

OK.

>> +};
>> +
>> +static int __init spacemit_pmic_init(void)
>> +{
>> +	return i2c_add_driver(&spacemit_pmic_i2c_driver);
>> +}
>> +
>> +static void __exit spacemit_pmic_exit(void)
>> +{
>> +	i2c_del_driver(&spacemit_pmic_i2c_driver);
>> +}
>> +
> 
> Remove this line.

Sure.

>> +module_init(spacemit_pmic_init);
>> +module_exit(spacemit_pmic_exit);
> 
> Are you sure there isn't some boiler plate to do all of this?
> 
> Ah ha:
> 
>    module_i2c_driver()

Thanks for Googling that for me.  And thank you very much
for the review.

					-Alex


>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("SpacemiT multi-function PMIC driver");
>> -- 
>> 2.45.2
>>
> 

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Lee Jones]

On Fri, 20 Jun 2025, Alex Elder wrote:

> On 6/19/25 9:40 AM, Lee Jones wrote:
> > On Fri, 13 Jun 2025, Alex Elder wrote:
> > 
> > > Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
> > > but the driver is structured to allow support for others to be added.
> > > 
> > > The P1 PMIC is controlled by I2C, and is normally implemented with the
> > > SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
> > 
> > six or 12.  Please pick a format and remain consistent.
> 
> "Numbers smaller than ten should be spelled out."

Never heard of that before Googling it.  Formal writing is odd. :)

> But I'll use 6 and 12.
> 
> > > regulators.  It also implements a switch, watchdog timer, real-time clock,
> > > and more, but initially we will only support its regulators.
> > 
> > You have to provide support for more than one device for this to be
> > accepted into MFD.
> 
> OK.  I'm looking at the other device functions to see if I
> can pick the easiest one.
> 
> > > Signed-off-by: Alex Elder <elder@riscstar.com>
> > > ---
> > >   drivers/mfd/Kconfig         | 11 +++++
> > >   drivers/mfd/Makefile        |  1 +
> > >   drivers/mfd/spacemit-pmic.c | 91 +++++++++++++++++++++++++++++++++++++
> > >   3 files changed, 103 insertions(+)
> > >   create mode 100644 drivers/mfd/spacemit-pmic.c
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 6fb3768e3d71c..c59ae6cc2dd8d 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1182,6 +1182,17 @@ 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_PMIC
> > > +	tristate "SpacemiT PMIC"
> > > +	depends on ARCH_SPACEMIT || COMPILE_TEST
> > > +	depends on I2C && OF
> > > +	select MFD_CORE
> > > +	select REGMAP_I2C
> > > +	default ARCH_SPACEMIT
> > > +	help
> > > +	  This option enables support for SpacemiT I2C based PMICs.  At
> > > +	  this time only the P1 PMIC (used with the K1 SoC) is supported.
> > > +
> > >   config MFD_SPMI_PMIC
> > >   	tristate "Qualcomm SPMI PMICs"
> > >   	depends on ARCH_QCOM || COMPILE_TEST
> > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > > index 79495f9f3457b..59d1ec8db3a3f 100644
> > > --- a/drivers/mfd/Makefile
> > > +++ b/drivers/mfd/Makefile
> > > @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_SUN4I_GPADC)	+= sun4i-gpadc.o
> > >   obj-$(CONFIG_MFD_STM32_LPTIMER)	+= stm32-lptimer.o
> > >   obj-$(CONFIG_MFD_STM32_TIMERS) 	+= stm32-timers.o
> > >   obj-$(CONFIG_MFD_MXS_LRADC)     += mxs-lradc.o
> > > +obj-$(CONFIG_MFD_SPACEMIT_PMIC)	+= spacemit-pmic.o
> > >   obj-$(CONFIG_MFD_SC27XX_PMIC)	+= sprd-sc27xx-spi.o
> > >   obj-$(CONFIG_RAVE_SP_CORE)	+= rave-sp.o
> > >   obj-$(CONFIG_MFD_ROHM_BD71828)	+= rohm-bd71828.o
> > > diff --git a/drivers/mfd/spacemit-pmic.c b/drivers/mfd/spacemit-pmic.c
> > > new file mode 100644
> > > index 0000000000000..7c3c3e27236da
> > > --- /dev/null
> > > +++ b/drivers/mfd/spacemit-pmic.c
> > > @@ -0,0 +1,91 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> > > + * Derived from code from:
> > > + *	Copyright (C) 2024 Troy Mitchell <troymitchell988@gmail.com>
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regmap.h>
> > > +#include <linux/types.h>
> > > +
> > > +struct spacemit_pmic_data {
> > 
> > s/data/ddata/
> 
> I hadn't noticed that convention.  I'll use it.
> 
> > > +	const struct regmap_config *regmap_config;
> > > +	const struct mfd_cell *mfd_cells;	/* array */
> > 
> > Hmm ... this is a red flag.  Let's see.
> > 
> > > +	size_t mfd_cell_count;
> > > +};
> > > +
> > > +static const struct regmap_config p1_regmap_config = {
> > > +	.reg_bits	= 8,
> > > +	.val_bits	= 8,
> > > +	.max_register	= 0xaa,
> > > +};
> > > +
> > > +/* The name field defines the *driver* name that should bind to the device */
> > 
> > This comment is superfluous.
> 
> I'll delete it.
> 
> I was expecting the driver to recognize the device, not
> the device specifying what driver to use, but I guess
> I'm used to the DT model.

Even in DT, the *driver* compatible is specified.

  .driver.of_match_table->compatible

> > > +static const struct mfd_cell p1_cells[] = {
> > > +	{
> > > +		.name		= "spacemit-p1-regulator",
> > 
> > This spacing is wonky.  Take a look at all the other drivers here.
> > 
> > Also, you probably want to use MFD_CELL_NAME().
> 
> Yes, I see that does what I want.
> 
> > One is not enough.
> > 
> > > +	},
> > > +};
> > > +
> > > +static const struct spacemit_pmic_data p1_pmic_data = {
> > > +	.regmap_config	= &p1_regmap_config,
> > > +	.mfd_cells	= p1_cells,
> > > +	.mfd_cell_count	= ARRAY_SIZE(p1_cells),
> > > +};
> > > +
> > > +static int spacemit_pmic_probe(struct i2c_client *client)
> > > +{
> > > +	const struct spacemit_pmic_data *data;
> > > +	struct device *dev = &client->dev;
> > > +	struct regmap *regmap;
> > > +
> > > +	/* We currently have no need for a device-specific structure */
> > 
> > Then why are we adding one?
> 
> I don't understand, but it might be moot once I add support
> for another (sub)device.

There are 2 rules in play here:

  - Only add what you need, when you need it
  - MFDs must contain more than 1 device

... and you're right.  The second rule moots the first here.

> > > +	data = of_device_get_match_data(dev);
> > > +	regmap = devm_regmap_init_i2c(client, data->regmap_config);
> > > +	if (IS_ERR(regmap))
> > > +		return dev_err_probe(dev, PTR_ERR(regmap),
> > > +				     "regmap initialization failed");
> > > +
> > > +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> > > +				    data->mfd_cells, data->mfd_cell_count,
> > > +				    NULL, 0, NULL);
> > > +}
> > > +
> > > +static const struct of_device_id spacemit_pmic_match[] = {
> > > +	{
> > > +		.compatible	= "spacemit,p1",
> > > +		.data		= &p1_pmic_data,
> > 
> > Ah, now I see.
> > 
> > We do not allow one data from registration mechanism (MFD) to be piped
> > through another (OF).  If you have to match platform data to device (you
> > don't), then pass through identifiers and match on those in a switch()
> > statement instead.
> 
> I haven't done an MFD driver before and it took some time
> to get this working.  I'll tell you what led me to it.
> 
> I used code posted by Troy Mitchell (plus downstream) as a
> starting point.
>   https://lore.kernel.org/lkml/20241230-k1-p1-v1-0-aa4e02b9f993@gmail.com/
> 
> Krzysztof Kozlowski made this comment on Troy's DT binding:
>   Drop compatible, regulators are not re-usable blocks.
> 
> So my goal was to have the PMIC regulators get bound to a
> driver without specifying a DT compatible string, and I
> found this worked.
> 
> You say I don't need to match platform data to device, but
> if I did I would pass through identifiers.  Can you refer
> me to an example of code that correctly does what I should
> be doing instead?

git grep -A5 compatible -- drivers/mfd | grep -E "\.data = .*[A-Z]+"

Those identifiers are usually matched in a swtich() statement.

> One other comment/question:
>   This driver is structured as if it could support a different
>   PMIC (in addition to P1).  Should I *not* do that, and simply
>   make a source file hard-coded for this one PMIC?

This comes back to the "add only what you need, when you need it" rule.

> > > +	},
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, spacemit_pmic_match);
> > > +
> > > +static struct i2c_driver spacemit_pmic_i2c_driver = {
> > > +	.driver = {
> > > +		.name = "spacemit-pmic",
> > > +		.of_match_table = spacemit_pmic_match,
> > > +	},
> > > +	.probe    = spacemit_pmic_probe,
> > 
> > Remove these odd tabs please.
> 
> OK.
> 
> > > +};
> > > +
> > > +static int __init spacemit_pmic_init(void)
> > > +{
> > > +	return i2c_add_driver(&spacemit_pmic_i2c_driver);
> > > +}
> > > +
> > > +static void __exit spacemit_pmic_exit(void)
> > > +{
> > > +	i2c_del_driver(&spacemit_pmic_i2c_driver);
> > > +}
> > > +
> > 
> > Remove this line.
> 
> Sure.
> 
> > > +module_init(spacemit_pmic_init);
> > > +module_exit(spacemit_pmic_exit);
> > 
> > Are you sure there isn't some boiler plate to do all of this?
> > 
> > Ah ha:
> > 
> >    module_i2c_driver()
> 
> Thanks for Googling that for me.  And thank you very much
> for the review.

`git grep` is my bestie! =:-)

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Alex Elder]

On 6/25/25 3:21 AM, Lee Jones wrote:
> On Fri, 20 Jun 2025, Alex Elder wrote:
> 
>> On 6/19/25 9:40 AM, Lee Jones wrote:
>>> On Fri, 13 Jun 2025, Alex Elder wrote:
>>>
>>>> Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
>>>> but the driver is structured to allow support for others to be added.
>>>>
>>>> The P1 PMIC is controlled by I2C, and is normally implemented with the
>>>> SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
>>>
>>> six or 12.  Please pick a format and remain consistent.
>>
>> "Numbers smaller than ten should be spelled out."
> 
> Never heard of that before Googling it.  Formal writing is odd. :)
> 
>> But I'll use 6 and 12.

. . .

>>>> +/* The name field defines the *driver* name that should bind to the device */
>>>
>>> This comment is superfluous.
>>
>> I'll delete it.
>>
>> I was expecting the driver to recognize the device, not
>> the device specifying what driver to use, but I guess
>> I'm used to the DT model.
> 
> Even in DT, the *driver* compatible is specified.
> 
>    .driver.of_match_table->compatible

I guess I just interpret that differently than you do.  I think
of the device compatible string as saying "this is what I am,"
much like a VID/PID in USB or PCI.

Then the driver's of_match table says "if a device claims to
be compatible with any of these it should be bound to me."

Meanwhile, the MFD device method has the device (cell) saying
"I should be bound to the driver having this name."

>>>> +	/* We currently have no need for a device-specific structure */
>>>
>>> Then why are we adding one?
>>
>> I don't understand, but it might be moot once I add support
>> for another (sub)device.
> 
> There are 2 rules in play here:
> 
>    - Only add what you need, when you need it
>    - MFDs must contain more than 1 device
> 
> ... and you're right.  The second rule moots the first here.

What the comment meant to say is "we have no need to kzalloc()
any special structure here" as most drivers do. Simply adding
the set of MFDs defined by the cells is enough.  The same is
true in "simple-mfd-i2c.c".

But this entire source file is gone now, so it's moot for that
reason.

. . .

>>>> +static const struct of_device_id spacemit_pmic_match[] = {
>>>> +	{
>>>> +		.compatible	= "spacemit,p1",
>>>> +		.data		= &p1_pmic_data,
>>>
>>> Ah, now I see.
>>>
>>> We do not allow one data from registration mechanism (MFD) to be piped
>>> through another (OF).  If you have to match platform data to device (you
>>> don't), then pass through identifiers and match on those in a switch()
>>> statement instead.
>>
>> I haven't done an MFD driver before and it took some time
>> to get this working.  I'll tell you what led me to it.
>>
>> I used code posted by Troy Mitchell (plus downstream) as a
>> starting point.
>>    https://lore.kernel.org/lkml/20241230-k1-p1-v1-0-aa4e02b9f993@gmail.com/
>>
>> Krzysztof Kozlowski made this comment on Troy's DT binding:
>>    Drop compatible, regulators are not re-usable blocks.
>>
>> So my goal was to have the PMIC regulators get bound to a
>> driver without specifying a DT compatible string, and I
>> found this worked.
>>
>> You say I don't need to match platform data to device, but
>> if I did I would pass through identifiers.  Can you refer
>> me to an example of code that correctly does what I should
>> be doing instead?
> 
> git grep -A5 compatible -- drivers/mfd | grep -E "\.data = .*[A-Z]+"
> 
> Those identifiers are usually matched in a swtich() statement.

OK now I see what you you're talking about.  But these
compatible strings (and data) are for the PMIC.  I was
trying to avoid using compatible strings for the *regulators*,
based on Krzysztof's comment.  And in the process I learned
that the MFD cell needs to specify the name of a driver,
not a compatible string.

>> One other comment/question:
>>    This driver is structured as if it could support a different
>>    PMIC (in addition to P1).  Should I *not* do that, and simply
>>    make a source file hard-coded for this one PMIC?
> 
> This comes back to the "add only what you need, when you need it" rule.

Yes, and I agree with that rule.  Thanks for your clarifications.

Using simple-mfd-i2c.c is much better.  I was surprised (and I guess
pleased) to see that it was almost *identical* to what I came up with.

					-Alex
. . .

>>>> +module_init(spacemit_pmic_init);
>>>> +module_exit(spacemit_pmic_exit);
>>>
>>> Are you sure there isn't some boiler plate to do all of this?
>>>
>>> Ah ha:
>>>
>>>     module_i2c_driver()
>>
>> Thanks for Googling that for me.  And thank you very much
>> for the review.
> 
> `git grep` is my bestie! =:-)
> 

Re: [PATCH 2/6] mfd: spacemit: add support for SpacemiT PMICs

[Lee Jones]

On Wed, 25 Jun 2025, Alex Elder wrote:

> On 6/25/25 3:21 AM, Lee Jones wrote:
> > On Fri, 20 Jun 2025, Alex Elder wrote:
> > 
> > > On 6/19/25 9:40 AM, Lee Jones wrote:
> > > > On Fri, 13 Jun 2025, Alex Elder wrote:
> > > > 
> > > > > Add support for SpacemiT PMICs. Initially only the P1 PMIC is supported
> > > > > but the driver is structured to allow support for others to be added.
> > > > > 
> > > > > The P1 PMIC is controlled by I2C, and is normally implemented with the
> > > > > SpacemiT K1 SoC.  This PMIC provides six buck converters and 12 LDO
> > > > 
> > > > six or 12.  Please pick a format and remain consistent.
> > > 
> > > "Numbers smaller than ten should be spelled out."
> > 
> > Never heard of that before Googling it.  Formal writing is odd. :)
> > 
> > > But I'll use 6 and 12.
> 
> . . .
> 
> > > > > +/* The name field defines the *driver* name that should bind to the device */
> > > > 
> > > > This comment is superfluous.
> > > 
> > > I'll delete it.
> > > 
> > > I was expecting the driver to recognize the device, not
> > > the device specifying what driver to use, but I guess
> > > I'm used to the DT model.
> > 
> > Even in DT, the *driver* compatible is specified.
> > 
> >    .driver.of_match_table->compatible
> 
> I guess I just interpret that differently than you do.  I think
> of the device compatible string as saying "this is what I am,"
> much like a VID/PID in USB or PCI.
> 
> Then the driver's of_match table says "if a device claims to
> be compatible with any of these it should be bound to me."
> 
> Meanwhile, the MFD device method has the device (cell) saying
> "I should be bound to the driver having this name."

In all cases that I'm aware of (platform code, DT, ACPI, etc), and as
far back as I can remember, the platform devices specify some predefined
data (IDs or strings) that is associated with (hard-coded directly into
the driver in fact) the driver it wishes to be bound to.  This
pre-defined identifier is stored in the driver's data structure:

struct device_driver {
        const char              	*name;
        const struct of_device_id       *of_match_table;
        const struct acpi_device_id     *acpi_match_table;
};

All of these are statically hard-coded items which a device can specify
in order to be bound to the driver.

> > > > > +	/* We currently have no need for a device-specific structure */
> > > > 
> > > > Then why are we adding one?
> > > 
> > > I don't understand, but it might be moot once I add support
> > > for another (sub)device.
> > 
> > There are 2 rules in play here:
> > 
> >    - Only add what you need, when you need it
> >    - MFDs must contain more than 1 device
> > 
> > ... and you're right.  The second rule moots the first here.
> 
> What the comment meant to say is "we have no need to kzalloc()
> any special structure here" as most drivers do. Simply adding
> the set of MFDs defined by the cells is enough.  The same is
> true in "simple-mfd-i2c.c".

I see.  Driver data is not compulsory.  There are plenty of drivers
which refrain from storing data for the child to make use of.

> But this entire source file is gone now, so it's moot for that
> reason.
> 
> . . .
> 
> > > > > +static const struct of_device_id spacemit_pmic_match[] = {
> > > > > +	{
> > > > > +		.compatible	= "spacemit,p1",
> > > > > +		.data		= &p1_pmic_data,
> > > > 
> > > > Ah, now I see.
> > > > 
> > > > We do not allow one data from registration mechanism (MFD) to be piped
> > > > through another (OF).  If you have to match platform data to device (you
> > > > don't), then pass through identifiers and match on those in a switch()
> > > > statement instead.
> > > 
> > > I haven't done an MFD driver before and it took some time
> > > to get this working.  I'll tell you what led me to it.
> > > 
> > > I used code posted by Troy Mitchell (plus downstream) as a
> > > starting point.
> > >    https://lore.kernel.org/lkml/20241230-k1-p1-v1-0-aa4e02b9f993@gmail.com/
> > > 
> > > Krzysztof Kozlowski made this comment on Troy's DT binding:
> > >    Drop compatible, regulators are not re-usable blocks.
> > > 
> > > So my goal was to have the PMIC regulators get bound to a
> > > driver without specifying a DT compatible string, and I
> > > found this worked.
> > > 
> > > You say I don't need to match platform data to device, but
> > > if I did I would pass through identifiers.  Can you refer
> > > me to an example of code that correctly does what I should
> > > be doing instead?
> > 
> > git grep -A5 compatible -- drivers/mfd | grep -E "\.data = .*[A-Z]+"
> > 
> > Those identifiers are usually matched in a swtich() statement.
> 
> OK now I see what you you're talking about.  But these
> compatible strings (and data) are for the PMIC.  I was
> trying to avoid using compatible strings for the *regulators*,
> based on Krzysztof's comment.  And in the process I learned
> that the MFD cell needs to specify the name of a driver,
> not a compatible string.

That's correct.  The compatible attribute is voluntary.

> > > One other comment/question:
> > >    This driver is structured as if it could support a different
> > >    PMIC (in addition to P1).  Should I *not* do that, and simply
> > >    make a source file hard-coded for this one PMIC?
> > 
> > This comes back to the "add only what you need, when you need it" rule.
> 
> Yes, and I agree with that rule.  Thanks for your clarifications.
> 
> Using simple-mfd-i2c.c is much better.  I was surprised (and I guess
> pleased) to see that it was almost *identical* to what I came up with.

*thumbs-up*

-- 
Lee Jones [李琼斯]

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

[Alex Elder]

Add support for the regulators found in the SpacemiT P1 PMIC.  This
PMIC provides six 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 P1
PMIC driver.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/regulator/Kconfig       |   9 ++
 drivers/regulator/Makefile      |   1 +
 drivers/regulator/spacemit-p1.c | 154 ++++++++++++++++++++++++++++++++
 3 files changed, 164 insertions(+)
 create mode 100644 drivers/regulator/spacemit-p1.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 6d8988387da45..7bb7b8fad24f2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1384,6 +1384,15 @@ 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
+	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.
+
 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 c0bc7a0f4e670..c58aecadd466e 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -161,6 +161,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..35c7b4a36e3ee
--- /dev/null
+++ b/drivers/regulator/spacemit-p1.c
@@ -0,0 +1,154 @@
+// 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>
+
+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 = "spacemit-p1-regulator",
+	},
+};
+
+module_platform_driver(p1_regulator_driver);
+
+MODULE_DESCRIPTION("SpacemiT P1 regulator driver");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Re: [PATCH 3/6] regulator: spacemit: support SpacemiT P1 regulators

[Mark Brown]

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

On Fri, Jun 13, 2025 at 04:01:46PM -0500, Alex Elder wrote:
> Add support for the regulators found in the SpacemiT P1 PMIC.  This
> PMIC provides six buck converters and 12 LDO regulators.

Reviewed-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/6] regulator: spacemit: support SpacemiT P1 regulators

[Vivian Wang]

On 6/14/25 05:01, Alex Elder wrote:
> <snip>
>
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 6d8988387da45..7bb7b8fad24f2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1384,6 +1384,15 @@ 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
> +	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.
Needs module name in help text, as is the case with spacemit-pmic.
> +
>  config REGULATOR_STM32_BOOSTER
>  	tristate "STMicroelectronics STM32 BOOSTER"
>  	depends on ARCH_STM32 || COMPILE_TEST
>
> <snip>
>
> +static struct platform_driver p1_regulator_driver = {
> +	.probe = p1_regulator_probe,
> +	.driver = {
> +		.name = "spacemit-p1-regulator",
> +	},
> +};
> +
> +module_platform_driver(p1_regulator_driver);
> +
> +MODULE_DESCRIPTION("SpacemiT P1 regulator driver");
> +MODULE_LICENSE("GPL");

If this driver is compiled as a module, it needs to be found by modalias
so the driver auto-loads after spacemit-pmic registers the regulator
device, so you need:

+MODULE_ALIAS("platform:spacemit-p1-regulator");

Also, consider extracting the name to a macro:

#define DRV_NAME "spacemit-p1-regulator"

Also, consider naming this consistently: "spacemit-p1", or
"spacemit-p1-regulator"?

Regards,
Vivian "dramforever" Wang

Re: [PATCH 3/6] regulator: spacemit: support SpacemiT P1 regulators

[Alex Elder]

On 6/19/25 1:15 AM, Vivian Wang wrote:
> On 6/14/25 05:01, Alex Elder wrote:
>> <snip>
>>
>> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
>> index 6d8988387da45..7bb7b8fad24f2 100644
>> --- a/drivers/regulator/Kconfig
>> +++ b/drivers/regulator/Kconfig
>> @@ -1384,6 +1384,15 @@ 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
>> +	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.
> Needs module name in help text, as is the case with spacemit-pmic.

I will add this text.

>> +
>>   config REGULATOR_STM32_BOOSTER
>>   	tristate "STMicroelectronics STM32 BOOSTER"
>>   	depends on ARCH_STM32 || COMPILE_TEST
>>
>> <snip>
>>
>> +static struct platform_driver p1_regulator_driver = {
>> +	.probe = p1_regulator_probe,
>> +	.driver = {
>> +		.name = "spacemit-p1-regulator",
>> +	},
>> +};
>> +
>> +module_platform_driver(p1_regulator_driver);
>> +
>> +MODULE_DESCRIPTION("SpacemiT P1 regulator driver");
>> +MODULE_LICENSE("GPL");
> 
> If this driver is compiled as a module, it needs to be found by modalias
> so the driver auto-loads after spacemit-pmic registers the regulator
> device, so you need:
> 
> +MODULE_ALIAS("platform:spacemit-p1-regulator");
> 
> Also, consider extracting the name to a macro:
> 
> #define DRV_NAME "spacemit-p1-regulator"

I will implement both of these suggestions (and will do so in
the PMIC driver as well).

> Also, consider naming this consistently: "spacemit-p1", or
> "spacemit-p1-regulator"?

Let me see if I understand your comment, by explaining the
naming I used.

The PMIC driver could support a different PMIC.  Its OF
match table specifies a compatible string with matching
data, and the data describes attributes of the P1 PMIC.
So that driver uses MOD_NAME "spacemit-pmic".

This driver describes specifically the regulators found
in the P1 PMIC, so it uses "spacemit-p1-regulator" as
its MOD_NAME.

You might still be right; but does this change what you
are suggesting?

Thanks.

					-Alex

> 
> Regards,
> Vivian "dramforever" Wang
> 

Re: [PATCH 3/6] regulator: spacemit: support SpacemiT P1 regulators

[Vivian Wang]

On 6/19/25 21:23, Alex Elder wrote:
> On 6/19/25 1:15 AM, Vivian Wang wrote:
>> Also, consider naming this consistently: "spacemit-p1", or
>> "spacemit-p1-regulator"?
>
> Let me see if I understand your comment, by explaining the
> naming I used.
>
> The PMIC driver could support a different PMIC.  Its OF
> match table specifies a compatible string with matching
> data, and the data describes attributes of the P1 PMIC.
> So that driver uses MOD_NAME "spacemit-pmic".
>
> This driver describes specifically the regulators found
> in the P1 PMIC, so it uses "spacemit-p1-regulator" as
> its MOD_NAME.
>
> You might still be right; but does this change what you
> are suggesting?

Oh sorry it was simpler than that. It's just I've noted that this 
regulator module file is called "spacemit-p1":

> +obj-$(CONFIG_REGULATOR_SPACEMIT_P1) += spacemit-p1.o

... but the MOD_NAME is "spacemit-p1-regulator", and I was wondering if 
it made sense to rename the module to also "spacemit-p1-regulator". In 
addition to consistency, modules are free to have all sorts of names in 
Linux, but the names have to be unique, so if this is only the regulator 
driver part, the name should reflect that.

Vivian "dramforever" Wang

[PATCH 4/6] 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 816ef1bc358ec..f5d454937d300 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -30,6 +30,21 @@ led1 {
 	};
 };
 
+&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 c0f8c5fca975d..34e843bd275fa 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 5/6] 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 f5d454937d300..8003c8173a2aa 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>;
+	};
 };
 
 &i2c8 {
-- 
2.45.2

[PATCH 6/6] 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 8003c8173a2aa..0f9a7f7a6c8b6 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -60,7 +60,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