[PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[Guodong Xu]

This series fixes hardware voltage constraints and enables flexible power
tree configurations for the SpacemiT P1 PMIC.

In v2, rebased to Spacemit SoC's k1/dt-for-next and added power tree
definition for K1 Milkv Jupiter.

Patch 1, n_voltages is corrected to match hardware register widths, as the
previous values prevented regulators from reaching higher operational
voltages (e.g., 3.3V on LDOs).

Patch 2-4, hardcoded supply assumptions are replaced with explicit
devicetree properties. PMIC supply connections are board-design decisions.
Moving this to DT allows supporting varied topologies without driver
modifications.

Note: Patch 3 introduces a bisect breakage by transitioning to
pin-specific supply names. Probe failures will occur on existing boards
until Patch 4 updates the corresponding DTS file.

Changes in v2:
- Patch 2: dt-bindings, remove providers from the example dts.
- Patch 4: Added the pmic supply properties for K1 Milkv Jupiter.
           Updated the commit message accordingly.
- Link to v1: https://lore.kernel.org/r/20260122-spacemit-p1-v1-0-309be27fbff9@riscstar.com

Signed-off-by: Guodong Xu <guodong@riscstar.com>
---
Guodong Xu (4):
      regulator: spacemit-p1: Fix n_voltages for BUCK and LDO regulators
      dt-bindings: mfd: spacemit,p1: Add individual regulator supply properties
      regulator: spacemit-p1: Update supply names
      riscv: dts: spacemit: Update PMIC supply properties for BPI-F3 and Jupiter

 .../devicetree/bindings/mfd/spacemit,p1.yaml       | 49 +++++++++++++++++++++-
 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts    | 12 +++++-
 arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts  | 12 +++++-
 drivers/regulator/spacemit-p1.c                    | 25 ++++++-----
 4 files changed, 81 insertions(+), 17 deletions(-)
---
base-commit: 5164e95565d3fd508ca8a95351323f5716dfb695
change-id: 20260122-spacemit-p1-ae596efe885f

Best regards,
-- 
Guodong Xu <guodong@riscstar.com>

[PATCH v2 1/4] regulator: spacemit-p1: Fix n_voltages for BUCK and LDO regulators

[Guodong Xu]

Higher voltage settings were unusable due to incorrect n_voltages values
causing registration failures. For example, setting aldo4 to 3.3V failed
with -EINVAL because the required selector (123) exceeded the allowed
range (n_voltages=117).

Fix by aligning n_voltages with the hardware register widths per the P1
datasheet [1]:
- BUCK: 255 (was 254), allows selectors 0-254, selector 255 is reserved
- LDO: 128 (was 117), allows selectors 0-127, selectors 0-10 are for
  suspend mode, valid operational range is 11-127

This enables the full voltage range supported by the hardware.

Fixes: 8b84d712ad84 ("regulator: spacemit: support SpacemiT P1 regulators")
Link: https://developer.spacemit.com/documentation [1]
Signed-off-by: Guodong Xu <guodong@riscstar.com>
---
v2: No change.
---
 drivers/regulator/spacemit-p1.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
index 2bf9137e12b1..2b585ba01a93 100644
--- a/drivers/regulator/spacemit-p1.c
+++ b/drivers/regulator/spacemit-p1.c
@@ -87,13 +87,13 @@ static const struct linear_range p1_ldo_ranges[] = {
 	}
 
 #define P1_BUCK_DESC(_n) \
-	P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 254, p1_buck_ranges)
+	P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
 
 #define P1_ALDO_DESC(_n) \
-	P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 117, p1_ldo_ranges)
+	P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
 
 #define P1_DLDO_DESC(_n) \
-	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 117, p1_ldo_ranges)
+	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
 
 static const struct regulator_desc p1_regulator_desc[] = {
 	P1_BUCK_DESC(1),

-- 
2.43.0

[PATCH v2 2/4] dt-bindings: mfd: spacemit,p1: Add individual regulator supply properties

[Guodong Xu]

Add supply properties that match the P1 PMIC's actual hardware topology
where each buck converter has its own VIN pin and LDO groups share
common input pins. Supply names are defined according to the pinout
names in the P1 datasheet.

This allows different boards to describe their actual power tree
connections in devicetree rather than hardcoding supply relationships
in the driver.

Signed-off-by: Guodong Xu <guodong@riscstar.com>
---
v2: Remove providers from the dts example.
    Pass the 'make dt_binding_check' test.
---
 .../devicetree/bindings/mfd/spacemit,p1.yaml       | 49 +++++++++++++++++++++-
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml b/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
index c6593ac6ef6a..c67b1c6e4e4f 100644
--- a/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
+++ b/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
@@ -27,8 +27,41 @@ properties:
   interrupts:
     maxItems: 1
 
-  vin-supply:
-    description: Input supply phandle.
+  vin1-supply:
+    description:
+      Power supply for BUCK1. Required if BUCK1 is defined.
+
+  vin2-supply:
+    description:
+      Power supply for BUCK2. Required if BUCK2 is defined.
+
+  vin3-supply:
+    description:
+      Power supply for BUCK3. Required if BUCK3 is defined.
+
+  vin4-supply:
+    description:
+      Power supply for BUCK4. Required if BUCK4 is defined.
+
+  vin5-supply:
+    description:
+      Power supply for BUCK5. Required if BUCK5 is defined.
+
+  vin6-supply:
+    description:
+      Power supply for BUCK6. Required if BUCK6 is defined.
+
+  aldoin-supply:
+    description:
+      Power supply for ALDO1-4. Required if any are defined.
+
+  dldoin1-supply:
+    description:
+      Power supply for DLDO1-4. Required if any are defined.
+
+  dldoin2-supply:
+    description:
+      Power supply for DLDO5-7. Required if any are defined.
 
   regulators:
     type: object
@@ -58,6 +91,10 @@ examples:
             compatible = "spacemit,p1";
             reg = <0x41>;
             interrupts = <64>;
+            vin1-supply = <&reg_vcc_5v>;
+            vin5-supply = <&reg_vcc_5v>;
+            aldoin-supply = <&reg_vcc_5v>;
+            dldoin1-supply = <&buck5>;
 
             regulators {
                 buck1 {
@@ -68,6 +105,14 @@ examples:
                     regulator-always-on;
                 };
 
+                buck5: buck5 {
+                    regulator-name = "buck5";
+                    regulator-min-microvolt = <500000>;
+                    regulator-max-microvolt = <3450000>;
+                    regulator-ramp-delay = <5000>;
+                    regulator-always-on;
+                };
+
                 aldo1 {
                     regulator-name = "aldo1";
                     regulator-min-microvolt = <500000>;

-- 
2.43.0

[PATCH v2 3/4] regulator: spacemit-p1: Update supply names

[Guodong Xu]

Update supply names to match the P1 PMIC's actual hardware pinout where
each buck has an individual VIN pin (vin1-vin6) and LDO groups have
dedicated input pins (aldoin, dldoin1, dldoin2).

The supply is a board design decision and should not be hardcoded to any
existing power source. This allows boards to specify their actual power
tree topology in devicetree.

Signed-off-by: Guodong Xu <guodong@riscstar.com>
---
v2: No change.
---
 drivers/regulator/spacemit-p1.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
index 2b585ba01a93..57e6e00a73fa 100644
--- a/drivers/regulator/spacemit-p1.c
+++ b/drivers/regulator/spacemit-p1.c
@@ -87,13 +87,16 @@ static const struct linear_range p1_ldo_ranges[] = {
 	}
 
 #define P1_BUCK_DESC(_n) \
-	P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
+	P1_REG_DESC(BUCK, buck, _n, "vin" #_n, 0x47, BUCK_MASK, 255, p1_buck_ranges)
 
 #define P1_ALDO_DESC(_n) \
-	P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
+	P1_REG_DESC(ALDO, aldo, _n, "aldoin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
 
-#define P1_DLDO_DESC(_n) \
-	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
+#define P1_DLDO1_DESC(_n) \
+	P1_REG_DESC(DLDO, dldo, _n, "dldoin1", 0x67, LDO_MASK, 128, p1_ldo_ranges)
+
+#define P1_DLDO2_DESC(_n) \
+	P1_REG_DESC(DLDO, dldo, _n, "dldoin2", 0x67, LDO_MASK, 128, p1_ldo_ranges)
 
 static const struct regulator_desc p1_regulator_desc[] = {
 	P1_BUCK_DESC(1),
@@ -108,13 +111,13 @@ static const struct regulator_desc p1_regulator_desc[] = {
 	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),
+	P1_DLDO1_DESC(1),
+	P1_DLDO1_DESC(2),
+	P1_DLDO1_DESC(3),
+	P1_DLDO1_DESC(4),
+	P1_DLDO2_DESC(5),
+	P1_DLDO2_DESC(6),
+	P1_DLDO2_DESC(7),
 };
 
 static int p1_regulator_probe(struct platform_device *pdev)

-- 
2.43.0

[PATCH v2 4/4] riscv: dts: spacemit: Update PMIC supply properties for BPI-F3 and Jupiter

[Guodong Xu]

Update individual supply properties in pmic "spacemit,p1" node to specify
the board's power tree topology for BananaPi F3 and Milk-V Jupiter.

Previously these relationships were hardcoded in the driver; now they
are explicitly defined in the devicetree per the updated binding
document spacemit,p1.yaml.

Signed-off-by: Guodong Xu <guodong@riscstar.com>
---
v2: Added the pmic supply properties for K1 Milkv Jupiter.
    Updated the commit message accordingly.
---
 arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts   | 12 ++++++++++--
 arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts | 12 ++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
index 5971605754b3..444c3b1e6f44 100644
--- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
@@ -190,7 +190,15 @@ pmic@41 {
 		compatible = "spacemit,p1";
 		reg = <0x41>;
 		interrupts = <64>;
-		vin-supply = <&reg_vcc_4v>;
+		vin1-supply = <&reg_vcc_4v>;
+		vin2-supply = <&reg_vcc_4v>;
+		vin3-supply = <&reg_vcc_4v>;
+		vin4-supply = <&reg_vcc_4v>;
+		vin5-supply = <&reg_vcc_4v>;
+		vin6-supply = <&reg_vcc_4v>;
+		aldoin-supply = <&reg_vcc_4v>;
+		dldoin1-supply = <&buck5>;
+		dldoin2-supply = <&buck5>;
 
 		regulators {
 			buck1 {
@@ -221,7 +229,7 @@ buck4 {
 				regulator-always-on;
 			};
 
-			buck5 {
+			buck5: buck5 {
 				regulator-min-microvolt = <500000>;
 				regulator-max-microvolt = <3450000>;
 				regulator-ramp-delay = <5000>;
diff --git a/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts b/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts
index 800a112d5d70..e2702a781734 100644
--- a/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts
+++ b/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts
@@ -100,7 +100,15 @@ pmic@41 {
 		compatible = "spacemit,p1";
 		reg = <0x41>;
 		interrupts = <64>;
-		vin-supply = <&reg_vcc_4v>;
+		vin1-supply = <&reg_vcc_4v>;
+		vin2-supply = <&reg_vcc_4v>;
+		vin3-supply = <&reg_vcc_4v>;
+		vin4-supply = <&reg_vcc_4v>;
+		vin5-supply = <&reg_vcc_4v>;
+		vin6-supply = <&reg_vcc_4v>;
+		aldoin-supply = <&reg_vcc_4v>;
+		dldoin1-supply = <&buck5>;
+		dldoin2-supply = <&buck5>;
 
 		regulators {
 			buck1 {
@@ -131,7 +139,7 @@ buck4 {
 				regulator-always-on;
 			};
 
-			buck5 {
+			buck5: buck5 {
 				regulator-min-microvolt = <500000>;
 				regulator-max-microvolt = <3450000>;
 				regulator-ramp-delay = <5000>;

-- 
2.43.0

Re: [PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[Vivian Wang]

On 1/24/26 08:20, Guodong Xu wrote:
> [...]
>
> Note: Patch 3 introduces a bisect breakage by transitioning to
> pin-specific supply names. Probe failures will occur on existing boards
> until Patch 4 updates the corresponding DTS file.

Ouch, that's not a bisect breakage, that's an *ABI breakage*. And AFAICT
this is still not okay in 2026,
see Documentation/devicetree/bindings/ABI.rst

So the bindings would need to be changed to accept both the new and old way.

Driver-wise, at a cursory look from someone not familiar with the
regulator stuff, maybe we can make it compatible with old DTS by adding
the new names as aliases ({devm_,}regulator_register_supply_alias?) as
"vin" or "buck5", if we see the old vin-supply definitions?

Re: [PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[Guodong Xu]

On Sat, Jan 24, 2026 at 2:25 PM Vivian Wang <wangruikang@iscas.ac.cn> wrote:
>
>
> On 1/24/26 08:20, Guodong Xu wrote:
> > [...]
> >
> > Note: Patch 3 introduces a bisect breakage by transitioning to
> > pin-specific supply names. Probe failures will occur on existing boards
> > until Patch 4 updates the corresponding DTS file.
>
> Ouch, that's not a bisect breakage, that's an *ABI breakage*. And AFAICT
> this is still not okay in 2026,
> see Documentation/devicetree/bindings/ABI.rst
>
> So the bindings would need to be changed to accept both the new and old way.

Ideally yes. However, considering this ABI change's actual effect, the two
K1 boards (BPI-F3 and Jupiter) in the kernel get their power settings
from boot firmware as well, and the types of peripherals enabled in the .dts
files are very limited, the probe failure of the pmic regulator doesn't
affect much. So, I think this breakage is acceptable.

>
> Driver-wise, at a cursory look from someone not familiar with the
> regulator stuff, maybe we can make it compatible with old DTS by adding
> the new names as aliases ({devm_,}regulator_register_supply_alias?) as
> "vin" or "buck5", if we see the old vin-supply definitions?
>

We can do that of course. My hesitation is, however, it makes the driver take
extra code which may not be needed once all .dts files have been updated. The
driver code will be left there forever.

BR,
Guodong Xu

Re: [PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[Guodong Xu]

On Sun, Jan 25, 2026 at 12:18 PM Guodong Xu <guodong@riscstar.com> wrote:
>
> On Sat, Jan 24, 2026 at 2:25 PM Vivian Wang <wangruikang@iscas.ac.cn> wrote:
> >
> >
> > On 1/24/26 08:20, Guodong Xu wrote:
> > > [...]
> > >
> > > Note: Patch 3 introduces a bisect breakage by transitioning to
> > > pin-specific supply names. Probe failures will occur on existing boards
> > > until Patch 4 updates the corresponding DTS file.
> >
> > Ouch, that's not a bisect breakage, that's an *ABI breakage*. And AFAICT
> > this is still not okay in 2026,
> > see Documentation/devicetree/bindings/ABI.rst
> >
> > So the bindings would need to be changed to accept both the new and old way.
>
> Ideally yes. However, considering this ABI change's actual effect, the two
> K1 boards (BPI-F3 and Jupiter) in the kernel get their power settings
> from boot firmware as well, and the types of peripherals enabled in the .dts
> files are very limited, the probe failure of the pmic regulator doesn't
> affect much. So, I think this breakage is acceptable.
>
> >
> > Driver-wise, at a cursory look from someone not familiar with the
> > regulator stuff, maybe we can make it compatible with old DTS by adding
> > the new names as aliases ({devm_,}regulator_register_supply_alias?) as
> > "vin" or "buck5", if we see the old vin-supply definitions?
> >
>
> We can do that of course. My hesitation is, however, it makes the driver take
> extra code which may not be needed once all .dts files have been updated. The
> driver code will be left there forever.
>

Mark gave his opinion in v1 review [1], please allow me to partially quote
here: "(it's an ABI change so shouldn't really happen, but perhaps there are
few enough users for everyone to coordinate and it's what you all prefer)."

I do expect to collect more ideas before I decide whether and what to do in
v3, or maybe v3 is not required.

Link: https://lore.kernel.org/all/2e2c2754-fd3e-4fd3-aae4-d7af63e3b528@sirena.org.uk/
[1]

> BR,
> Guodong Xu

Re: [PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[Yixun Lan]

Hi Guodong,

On 12:27 Sun 25 Jan     , Guodong Xu wrote:
> On Sun, Jan 25, 2026 at 12:18 PM Guodong Xu <guodong@riscstar.com> wrote:
> >
> > On Sat, Jan 24, 2026 at 2:25 PM Vivian Wang <wangruikang@iscas.ac.cn> wrote:
> > >
> > >
> > > On 1/24/26 08:20, Guodong Xu wrote:
> > > > [...]
> > > >
> > > > Note: Patch 3 introduces a bisect breakage by transitioning to
> > > > pin-specific supply names. Probe failures will occur on existing boards
> > > > until Patch 4 updates the corresponding DTS file.
> > >
> > > Ouch, that's not a bisect breakage, that's an *ABI breakage*. And AFAICT
> > > this is still not okay in 2026,
> > > see Documentation/devicetree/bindings/ABI.rst
> > >
> > > So the bindings would need to be changed to accept both the new and old way.
> >
> > Ideally yes. However, considering this ABI change's actual effect, the two
> > K1 boards (BPI-F3 and Jupiter) in the kernel get their power settings
> > from boot firmware as well, and the types of peripherals enabled in the .dts
> > files are very limited, the probe failure of the pmic regulator doesn't
> > affect much. So, I think this breakage is acceptable.
> >
> > >
> > > Driver-wise, at a cursory look from someone not familiar with the
> > > regulator stuff, maybe we can make it compatible with old DTS by adding
> > > the new names as aliases ({devm_,}regulator_register_supply_alias?) as
> > > "vin" or "buck5", if we see the old vin-supply definitions?
> > >
> >
> > We can do that of course. My hesitation is, however, it makes the driver take
> > extra code which may not be needed once all .dts files have been updated. The
> > driver code will be left there forever.
> >
> 
> Mark gave his opinion in v1 review [1], please allow me to partially quote
> here: "(it's an ABI change so shouldn't really happen, but perhaps there are
> few enough users for everyone to coordinate and it's what you all prefer)."
> 
> I do expect to collect more ideas before I decide whether and what to do in
> v3, or maybe v3 is not required.
> 
As I checked the dts tree (DT queued for v6.20), although we introduced the
regulator of P1/PMIC, but there is no consumers so far, so in real life, we
shouldn't break anything. In this case, I'd suggest we just give up for doing
the ABI backward compatible work which should simplify our life..

> Link: https://lore.kernel.org/all/2e2c2754-fd3e-4fd3-aae4-d7af63e3b528@sirena.org.uk/
> [1]
> 
> > BR,
> > Guodong Xu
> 

-- 
Yixun Lan (dlan)

Re: [PATCH v2 0/4] regulator: spacemit-p1: Fix voltage ranges and support board power tree

[Vivian Wang]

On 1/25/26 19:03, Yixun Lan wrote:
> Hi Guodong,
>
> On 12:27 Sun 25 Jan     , Guodong Xu wrote:
>> On Sun, Jan 25, 2026 at 12:18 PM Guodong Xu <guodong@riscstar.com> wrote:
>>> On Sat, Jan 24, 2026 at 2:25 PM Vivian Wang <wangruikang@iscas.ac.cn> wrote:
>>>>
>>>> On 1/24/26 08:20, Guodong Xu wrote:
>>>>> [...]
>>>>>
>>>>> Note: Patch 3 introduces a bisect breakage by transitioning to
>>>>> pin-specific supply names. Probe failures will occur on existing boards
>>>>> until Patch 4 updates the corresponding DTS file.
>>>> Ouch, that's not a bisect breakage, that's an *ABI breakage*. And AFAICT
>>>> this is still not okay in 2026,
>>>> see Documentation/devicetree/bindings/ABI.rst
>>>>
>>>> So the bindings would need to be changed to accept both the new and old way.
>>> Ideally yes. However, considering this ABI change's actual effect, the two
>>> K1 boards (BPI-F3 and Jupiter) in the kernel get their power settings
>>> from boot firmware as well, and the types of peripherals enabled in the .dts
>>> files are very limited, the probe failure of the pmic regulator doesn't
>>> affect much. So, I think this breakage is acceptable.
>>>
>>>> Driver-wise, at a cursory look from someone not familiar with the
>>>> regulator stuff, maybe we can make it compatible with old DTS by adding
>>>> the new names as aliases ({devm_,}regulator_register_supply_alias?) as
>>>> "vin" or "buck5", if we see the old vin-supply definitions?
>>>>
>>> We can do that of course. My hesitation is, however, it makes the driver take
>>> extra code which may not be needed once all .dts files have been updated. The
>>> driver code will be left there forever.
>>>
>> Mark gave his opinion in v1 review [1], please allow me to partially quote
>> here: "(it's an ABI change so shouldn't really happen, but perhaps there are
>> few enough users for everyone to coordinate and it's what you all prefer)."
>>
>> I do expect to collect more ideas before I decide whether and what to do in
>> v3, or maybe v3 is not required.
>>
> As I checked the dts tree (DT queued for v6.20), although we introduced the
> regulator of P1/PMIC, but there is no consumers so far, so in real life, we
> shouldn't break anything. In this case, I'd suggest we just give up for doing
> the ABI backward compatible work which should simplify our life..

Having checked again, I agree that this is not that big of a problem.
The breakage with old DT is limited to an otherwise harmless error on
boot that doesn't affect functionality since there are no users.

Vivian "dramforever" Wang.

Re: [PATCH v2 1/4] regulator: spacemit-p1: Fix n_voltages for BUCK and LDO regulators

[Alex Elder]

On 1/23/26 6:20 PM, Guodong Xu wrote:
> Higher voltage settings were unusable due to incorrect n_voltages values
> causing registration failures. For example, setting aldo4 to 3.3V failed
> with -EINVAL because the required selector (123) exceeded the allowed
> range (n_voltages=117).
> 
> Fix by aligning n_voltages with the hardware register widths per the P1
> datasheet [1]:
> - BUCK: 255 (was 254), allows selectors 0-254, selector 255 is reserved
> - LDO: 128 (was 117), allows selectors 0-127, selectors 0-10 are for
>    suspend mode, valid operational range is 11-127
> 
> This enables the full voltage range supported by the hardware.
> 
> Fixes: 8b84d712ad84 ("regulator: spacemit: support SpacemiT P1 regulators")
> Link: https://developer.spacemit.com/documentation [1]
> Signed-off-by: Guodong Xu <guodong@riscstar.com>
> ---
> v2: No change.
> ---
>   drivers/regulator/spacemit-p1.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
> index 2bf9137e12b1..2b585ba01a93 100644
> --- a/drivers/regulator/spacemit-p1.c
> +++ b/drivers/regulator/spacemit-p1.c
> @@ -87,13 +87,13 @@ static const struct linear_range p1_ldo_ranges[] = {
>   	}
>   
>   #define P1_BUCK_DESC(_n) \
> -	P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 254, p1_buck_ranges)
> +	P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)

This is correct.  There are 255 possible ranges, 0..254, and
255 is an illegal value.

I think this bug is an artifact of a change I made while
chasing an issue during development, and I neglected to
change it back.

Technically this is a bug fix but it doesn't matter because
this voltage value (255 represents 3.450 volts) was not
required.

>   #define P1_ALDO_DESC(_n) \
> -	P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 117, p1_ldo_ranges)
> +	P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)

I would say this is not correct.

The valid range of values in this register is 0xd-0x1f (11-127),
which is 117 values; 0xd represents 0.500V and 0x1f represents
3.400V.

Technically, all other values represent 0.5v (and could therefore
be considered valid), but I believe those should never be used
and intentionally considered them invalid.  If 0.5V is desired,
0xd should be used.

Do you disagree with this?
>   #define P1_DLDO_DESC(_n) \
> -	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 117, p1_ldo_ranges)
> +	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>   
>   static const struct regulator_desc p1_regulator_desc[] = {
>   	P1_BUCK_DESC(1),
> 

I have exactly the same comment about this change to the
number of supported values.

					-Alex

Re: [PATCH v2 2/4] dt-bindings: mfd: spacemit,p1: Add individual regulator supply properties

[Alex Elder]

On 1/23/26 6:20 PM, Guodong Xu wrote:
> Add supply properties that match the P1 PMIC's actual hardware topology
> where each buck converter has its own VIN pin and LDO groups share
> common input pins. Supply names are defined according to the pinout
> names in the P1 datasheet.
> 
> This allows different boards to describe their actual power tree
> connections in devicetree rather than hardcoding supply relationships
> in the driver.
> 
> Signed-off-by: Guodong Xu <guodong@riscstar.com>

Your additions match what I see in the data sheet.
This looks good, thank you.

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
> v2: Remove providers from the dts example.
>      Pass the 'make dt_binding_check' test.
> ---
>   .../devicetree/bindings/mfd/spacemit,p1.yaml       | 49 +++++++++++++++++++++-
>   1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml b/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
> index c6593ac6ef6a..c67b1c6e4e4f 100644
> --- a/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
> +++ b/Documentation/devicetree/bindings/mfd/spacemit,p1.yaml
> @@ -27,8 +27,41 @@ properties:
>     interrupts:
>       maxItems: 1
>   
> -  vin-supply:
> -    description: Input supply phandle.
> +  vin1-supply:
> +    description:
> +      Power supply for BUCK1. Required if BUCK1 is defined.
> +
> +  vin2-supply:
> +    description:
> +      Power supply for BUCK2. Required if BUCK2 is defined.
> +
> +  vin3-supply:
> +    description:
> +      Power supply for BUCK3. Required if BUCK3 is defined.
> +
> +  vin4-supply:
> +    description:
> +      Power supply for BUCK4. Required if BUCK4 is defined.
> +
> +  vin5-supply:
> +    description:
> +      Power supply for BUCK5. Required if BUCK5 is defined.
> +
> +  vin6-supply:
> +    description:
> +      Power supply for BUCK6. Required if BUCK6 is defined.
> +
> +  aldoin-supply:
> +    description:
> +      Power supply for ALDO1-4. Required if any are defined.
> +
> +  dldoin1-supply:
> +    description:
> +      Power supply for DLDO1-4. Required if any are defined.
> +
> +  dldoin2-supply:
> +    description:
> +      Power supply for DLDO5-7. Required if any are defined.
>   
>     regulators:
>       type: object
> @@ -58,6 +91,10 @@ examples:
>               compatible = "spacemit,p1";
>               reg = <0x41>;
>               interrupts = <64>;
> +            vin1-supply = <&reg_vcc_5v>;
> +            vin5-supply = <&reg_vcc_5v>;
> +            aldoin-supply = <&reg_vcc_5v>;
> +            dldoin1-supply = <&buck5>;
>   
>               regulators {
>                   buck1 {
> @@ -68,6 +105,14 @@ examples:
>                       regulator-always-on;
>                   };
>   
> +                buck5: buck5 {
> +                    regulator-name = "buck5";
> +                    regulator-min-microvolt = <500000>;
> +                    regulator-max-microvolt = <3450000>;
> +                    regulator-ramp-delay = <5000>;
> +                    regulator-always-on;
> +                };
> +
>                   aldo1 {
>                       regulator-name = "aldo1";
>                       regulator-min-microvolt = <500000>;
> 

Re: [PATCH v2 3/4] regulator: spacemit-p1: Update supply names

[Alex Elder]

On 1/23/26 6:20 PM, Guodong Xu wrote:
> Update supply names to match the P1 PMIC's actual hardware pinout where
> each buck has an individual VIN pin (vin1-vin6) and LDO groups have
> dedicated input pins (aldoin, dldoin1, dldoin2).
> 
> The supply is a board design decision and should not be hardcoded to any
> existing power source. This allows boards to specify their actual power
> tree topology in devicetree.
> 
> Signed-off-by: Guodong Xu <guodong@riscstar.com>

These are good changes but I have a suggestion on the way
you define the DLDO descriptors.  I might be mistaken but
I think you should make this change.

Aside from that:

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
> v2: No change.
> ---
>   drivers/regulator/spacemit-p1.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
> index 2b585ba01a93..57e6e00a73fa 100644
> --- a/drivers/regulator/spacemit-p1.c
> +++ b/drivers/regulator/spacemit-p1.c
> @@ -87,13 +87,16 @@ static const struct linear_range p1_ldo_ranges[] = {
>   	}
>   
>   #define P1_BUCK_DESC(_n) \
> -	P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
> +	P1_REG_DESC(BUCK, buck, _n, "vin" #_n, 0x47, BUCK_MASK, 255, p1_buck_ranges)

That was a simple change...

>   #define P1_ALDO_DESC(_n) \
> -	P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
> +	P1_REG_DESC(ALDO, aldo, _n, "aldoin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)

As stated before, I believe the 128 should be 117 here.  (If
you change the earlier patch, make sure the change to 128
doesn't persist here.)  Same comment for the DLDO regulators.

> -#define P1_DLDO_DESC(_n) \
> -	P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
> +#define P1_DLDO1_DESC(_n) \
> +	P1_REG_DESC(DLDO, dldo, _n, "dldoin1", 0x67, LDO_MASK, 128, p1_ldo_ranges)

Why can't you use _n here like you did for P1_BUCK_DESC() above?

> +
> +#define P1_DLDO2_DESC(_n) \
> +	P1_REG_DESC(DLDO, dldo, _n, "dldoin2", 0x67, LDO_MASK, 128, p1_ldo_ranges)

So this is generalizing the input, which is good.  The use
of "buck5" here was a Banana Pi BPI-F3 design and but it
doesn't have to be that way.

>   static const struct regulator_desc p1_regulator_desc[] = {
>   	P1_BUCK_DESC(1),
> @@ -108,13 +111,13 @@ static const struct regulator_desc p1_regulator_desc[] = {
>   	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),
> +	P1_DLDO1_DESC(1),
> +	P1_DLDO1_DESC(2),
> +	P1_DLDO1_DESC(3),
> +	P1_DLDO1_DESC(4),
> +	P1_DLDO2_DESC(5),
> +	P1_DLDO2_DESC(6),
> +	P1_DLDO2_DESC(7),
>   };
>   
>   static int p1_regulator_probe(struct platform_device *pdev)
> 

Re: [PATCH v2 4/4] riscv: dts: spacemit: Update PMIC supply properties for BPI-F3 and Jupiter

[Alex Elder]

On 1/23/26 6:20 PM, Guodong Xu wrote:
> Update individual supply properties in pmic "spacemit,p1" node to specify
> the board's power tree topology for BananaPi F3 and Milk-V Jupiter.
> 
> Previously these relationships were hardcoded in the driver; now they
> are explicitly defined in the devicetree per the updated binding
> document spacemit,p1.yaml.
> 
> Signed-off-by: Guodong Xu <guodong@riscstar.com>

I acknowledge that you've made an ABI change, and I should
have done a better job of describing this the first time
around.  (And reviewers might have caught that!)

I had some of the same thoughts about supporting just
"vin_supply" for older systems.

However I agree with the conclusion you and Vivian
came to, which is that practically speaking it isn't
likely to be a problem.  Boards should use either
old DTB and software or new DTB and software, not
a mix.

Reviewed-by: Alex Elder <elder@riscstar.com>


> ---
> v2: Added the pmic supply properties for K1 Milkv Jupiter.
>      Updated the commit message accordingly.
> ---
>   arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts   | 12 ++++++++++--
>   arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts | 12 ++++++++++--
>   2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> index 5971605754b3..444c3b1e6f44 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> +++ b/arch/riscv/boot/dts/spacemit/k1-bananapi-f3.dts
> @@ -190,7 +190,15 @@ pmic@41 {
>   		compatible = "spacemit,p1";
>   		reg = <0x41>;
>   		interrupts = <64>;
> -		vin-supply = <&reg_vcc_4v>;
> +		vin1-supply = <&reg_vcc_4v>;
> +		vin2-supply = <&reg_vcc_4v>;
> +		vin3-supply = <&reg_vcc_4v>;
> +		vin4-supply = <&reg_vcc_4v>;
> +		vin5-supply = <&reg_vcc_4v>;
> +		vin6-supply = <&reg_vcc_4v>;
> +		aldoin-supply = <&reg_vcc_4v>;
> +		dldoin1-supply = <&buck5>;
> +		dldoin2-supply = <&buck5>;
>   
>   		regulators {
>   			buck1 {
> @@ -221,7 +229,7 @@ buck4 {
>   				regulator-always-on;
>   			};
>   
> -			buck5 {
> +			buck5: buck5 {
>   				regulator-min-microvolt = <500000>;
>   				regulator-max-microvolt = <3450000>;
>   				regulator-ramp-delay = <5000>;
> diff --git a/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts b/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts
> index 800a112d5d70..e2702a781734 100644
> --- a/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts
> +++ b/arch/riscv/boot/dts/spacemit/k1-milkv-jupiter.dts
> @@ -100,7 +100,15 @@ pmic@41 {
>   		compatible = "spacemit,p1";
>   		reg = <0x41>;
>   		interrupts = <64>;
> -		vin-supply = <&reg_vcc_4v>;
> +		vin1-supply = <&reg_vcc_4v>;
> +		vin2-supply = <&reg_vcc_4v>;
> +		vin3-supply = <&reg_vcc_4v>;
> +		vin4-supply = <&reg_vcc_4v>;
> +		vin5-supply = <&reg_vcc_4v>;
> +		vin6-supply = <&reg_vcc_4v>;
> +		aldoin-supply = <&reg_vcc_4v>;
> +		dldoin1-supply = <&buck5>;
> +		dldoin2-supply = <&buck5>;
>   
>   		regulators {
>   			buck1 {
> @@ -131,7 +139,7 @@ buck4 {
>   				regulator-always-on;
>   			};
>   
> -			buck5 {
> +			buck5: buck5 {
>   				regulator-min-microvolt = <500000>;
>   				regulator-max-microvolt = <3450000>;
>   				regulator-ramp-delay = <5000>;
> 

Re: [PATCH v2 3/4] regulator: spacemit-p1: Update supply names

[Guodong Xu]

Hi, Alex

On Wed, Jan 28, 2026 at 9:29 PM Alex Elder <elder@riscstar.com> wrote:
>
> On 1/23/26 6:20 PM, Guodong Xu wrote:
> > Update supply names to match the P1 PMIC's actual hardware pinout where
> > each buck has an individual VIN pin (vin1-vin6) and LDO groups have
> > dedicated input pins (aldoin, dldoin1, dldoin2).
> >
> > The supply is a board design decision and should not be hardcoded to any
> > existing power source. This allows boards to specify their actual power
> > tree topology in devicetree.
> >
> > Signed-off-by: Guodong Xu <guodong@riscstar.com>
>
> These are good changes but I have a suggestion on the way
> you define the DLDO descriptors.  I might be mistaken but
> I think you should make this change.
>
> Aside from that:
>
> Reviewed-by: Alex Elder <elder@riscstar.com>
>
> > ---
> > v2: No change.
> > ---
> >   drivers/regulator/spacemit-p1.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
> > index 2b585ba01a93..57e6e00a73fa 100644
> > --- a/drivers/regulator/spacemit-p1.c
> > +++ b/drivers/regulator/spacemit-p1.c
> > @@ -87,13 +87,16 @@ static const struct linear_range p1_ldo_ranges[] = {
> >       }
> >
> >   #define P1_BUCK_DESC(_n) \
> > -     P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
> > +     P1_REG_DESC(BUCK, buck, _n, "vin" #_n, 0x47, BUCK_MASK, 255, p1_buck_ranges)
>
> That was a simple change...
>
> >   #define P1_ALDO_DESC(_n) \
> > -     P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
> > +     P1_REG_DESC(ALDO, aldo, _n, "aldoin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
>
> As stated before, I believe the 128 should be 117 here.  (If

I will explain this in another email.

> you change the earlier patch, make sure the change to 128
> doesn't persist here.)  Same comment for the DLDO regulators.
>
> > -#define P1_DLDO_DESC(_n) \
> > -     P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
> > +#define P1_DLDO1_DESC(_n) \
> > +     P1_REG_DESC(DLDO, dldo, _n, "dldoin1", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>
> Why can't you use _n here like you did for P1_BUCK_DESC() above?

The naming follows the P1 pinout definitions in the datasheet [1].

Unlike the BUCK regulators, which have individual input pins (e.g.,
VIN3 for BUCK3), the DLDOs share power inputs. For example, DLDOIN1 (pin 17)
powers DLDO1 through DLDO4. DLDOIN2 provides power to DLDO5, 6 and 7.

Since there are no physical pins named dldoin3, etc., I can't use the _n index
for the supply name argument like I did for the BUCKs.

Datasheet pin examples:
8 VIN3 PWR Buck3 power input (1:1 mapping)
17 DLDOIN1 PWR DLDO1~4 power input (1:Many mapping)

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

Best regards,
Guodong Xu

>
> > +
> > +#define P1_DLDO2_DESC(_n) \
> > +     P1_REG_DESC(DLDO, dldo, _n, "dldoin2", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>
> So this is generalizing the input, which is good.  The use
> of "buck5" here was a Banana Pi BPI-F3 design and but it
> doesn't have to be that way.
>
> >   static const struct regulator_desc p1_regulator_desc[] = {
> >       P1_BUCK_DESC(1),
> > @@ -108,13 +111,13 @@ static const struct regulator_desc p1_regulator_desc[] = {
> >       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),
> > +     P1_DLDO1_DESC(1),
> > +     P1_DLDO1_DESC(2),
> > +     P1_DLDO1_DESC(3),
> > +     P1_DLDO1_DESC(4),
> > +     P1_DLDO2_DESC(5),
> > +     P1_DLDO2_DESC(6),
> > +     P1_DLDO2_DESC(7),
> >   };
> >
> >   static int p1_regulator_probe(struct platform_device *pdev)
> >
>

Re: [PATCH v2 3/4] regulator: spacemit-p1: Update supply names

[Alex Elder]

On 1/28/26 8:47 AM, Guodong Xu wrote:
> Hi, Alex
> 
> On Wed, Jan 28, 2026 at 9:29 PM Alex Elder <elder@riscstar.com> wrote:
>>
>> On 1/23/26 6:20 PM, Guodong Xu wrote:
>>> Update supply names to match the P1 PMIC's actual hardware pinout where
>>> each buck has an individual VIN pin (vin1-vin6) and LDO groups have
>>> dedicated input pins (aldoin, dldoin1, dldoin2).
>>>
>>> The supply is a board design decision and should not be hardcoded to any
>>> existing power source. This allows boards to specify their actual power
>>> tree topology in devicetree.
>>>
>>> Signed-off-by: Guodong Xu <guodong@riscstar.com>
>>
>> These are good changes but I have a suggestion on the way
>> you define the DLDO descriptors.  I might be mistaken but
>> I think you should make this change.
>>
>> Aside from that:
>>
>> Reviewed-by: Alex Elder <elder@riscstar.com>
>>
>>> ---
>>> v2: No change.
>>> ---
>>>    drivers/regulator/spacemit-p1.c | 25 ++++++++++++++-----------
>>>    1 file changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
>>> index 2b585ba01a93..57e6e00a73fa 100644
>>> --- a/drivers/regulator/spacemit-p1.c
>>> +++ b/drivers/regulator/spacemit-p1.c
>>> @@ -87,13 +87,16 @@ static const struct linear_range p1_ldo_ranges[] = {
>>>        }
>>>
>>>    #define P1_BUCK_DESC(_n) \
>>> -     P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
>>> +     P1_REG_DESC(BUCK, buck, _n, "vin" #_n, 0x47, BUCK_MASK, 255, p1_buck_ranges)
>>
>> That was a simple change...
>>
>>>    #define P1_ALDO_DESC(_n) \
>>> -     P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
>>> +     P1_REG_DESC(ALDO, aldo, _n, "aldoin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
>>
>> As stated before, I believe the 128 should be 117 here.  (If
> 
> I will explain this in another email.
> 
>> you change the earlier patch, make sure the change to 128
>> doesn't persist here.)  Same comment for the DLDO regulators.
>>
>>> -#define P1_DLDO_DESC(_n) \
>>> -     P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>>> +#define P1_DLDO1_DESC(_n) \
>>> +     P1_REG_DESC(DLDO, dldo, _n, "dldoin1", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>>
>> Why can't you use _n here like you did for P1_BUCK_DESC() above?
> 
> The naming follows the P1 pinout definitions in the datasheet [1].
> 
> Unlike the BUCK regulators, which have individual input pins (e.g.,
> VIN3 for BUCK3), the DLDOs share power inputs. For example, DLDOIN1 (pin 17)
> powers DLDO1 through DLDO4. DLDOIN2 provides power to DLDO5, 6 and 7.

Ahh.  I didn't notice that.  There are two *groups* of DLDO
regulators, and each group is fed by one or the other poer
input.

Now I get it.

Thanks for the explanation.

					-Alex

> Since there are no physical pins named dldoin3, etc., I can't use the _n index
> for the supply name argument like I did for the BUCKs.
> 
> Datasheet pin examples:
> 8 VIN3 PWR Buck3 power input (1:1 mapping)
> 17 DLDOIN1 PWR DLDO1~4 power input (1:Many mapping)
> 
> Link: https://developer.spacemit.com/documentation?token=T1Btw2BdiiSlSXkAdibcoMetnag
> [1]
> 
> Best regards,
> Guodong Xu
> 
>>
>>> +
>>> +#define P1_DLDO2_DESC(_n) \
>>> +     P1_REG_DESC(DLDO, dldo, _n, "dldoin2", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>>
>> So this is generalizing the input, which is good.  The use
>> of "buck5" here was a Banana Pi BPI-F3 design and but it
>> doesn't have to be that way.
>>
>>>    static const struct regulator_desc p1_regulator_desc[] = {
>>>        P1_BUCK_DESC(1),
>>> @@ -108,13 +111,13 @@ static const struct regulator_desc p1_regulator_desc[] = {
>>>        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),
>>> +     P1_DLDO1_DESC(1),
>>> +     P1_DLDO1_DESC(2),
>>> +     P1_DLDO1_DESC(3),
>>> +     P1_DLDO1_DESC(4),
>>> +     P1_DLDO2_DESC(5),
>>> +     P1_DLDO2_DESC(6),
>>> +     P1_DLDO2_DESC(7),
>>>    };
>>>
>>>    static int p1_regulator_probe(struct platform_device *pdev)
>>>
>>

Re: [PATCH v2 1/4] regulator: spacemit-p1: Fix n_voltages for BUCK and LDO regulators

[Guodong Xu]

Hi, Alex

On Wed, Jan 28, 2026 at 9:28 PM Alex Elder <elder@riscstar.com> wrote:
>
> On 1/23/26 6:20 PM, Guodong Xu wrote:
> > Higher voltage settings were unusable due to incorrect n_voltages values
> > causing registration failures. For example, setting aldo4 to 3.3V failed
> > with -EINVAL because the required selector (123) exceeded the allowed
> > range (n_voltages=117).
> >
> > Fix by aligning n_voltages with the hardware register widths per the P1
> > datasheet [1]:
> > - BUCK: 255 (was 254), allows selectors 0-254, selector 255 is reserved
> > - LDO: 128 (was 117), allows selectors 0-127, selectors 0-10 are for
> >    suspend mode, valid operational range is 11-127
> >
> > This enables the full voltage range supported by the hardware.
> >
> > Fixes: 8b84d712ad84 ("regulator: spacemit: support SpacemiT P1 regulators")
> > Link: https://developer.spacemit.com/documentation [1]
> > Signed-off-by: Guodong Xu <guodong@riscstar.com>
> > ---
> > v2: No change.
> > ---
> >   drivers/regulator/spacemit-p1.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
> > index 2bf9137e12b1..2b585ba01a93 100644
> > --- a/drivers/regulator/spacemit-p1.c
> > +++ b/drivers/regulator/spacemit-p1.c
> > @@ -87,13 +87,13 @@ static const struct linear_range p1_ldo_ranges[] = {
> >       }
> >
> >   #define P1_BUCK_DESC(_n) \
> > -     P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 254, p1_buck_ranges)
> > +     P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
>
> This is correct.  There are 255 possible ranges, 0..254, and
> 255 is an illegal value.
>
> I think this bug is an artifact of a change I made while
> chasing an issue during development, and I neglected to
> change it back.
>
> Technically this is a bug fix but it doesn't matter because
> this voltage value (255 represents 3.450 volts) was not
> required.
>
> >   #define P1_ALDO_DESC(_n) \
> > -     P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 117, p1_ldo_ranges)
> > +     P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
>
> I would say this is not correct.
>
> The valid range of values in this register is 0xd-0x1f (11-127),
> which is 117 values; 0xd represents 0.500V and 0x1f represents
> 3.400V.
>
> Technically, all other values represent 0.5v (and could therefore
> be considered valid), but I believe those should never be used
> and intentionally considered them invalid.  If 0.5V is desired,
> 0xd should be used.
>
> Do you disagree with this?

I understand your concern about selectors 0-10. However, maybe you missed
this part:

Code snippet from the c file, Line 53:
(The p1_buck_ranges and p1_ldo_ranges are defined correctly.)

/* 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),
};

.linear_range, the number of valid voltage steps (selectors 11-127)
.n_voltages field, which defines the selector namespace (0 to 170, then to 254)

With n_voltages = 117, the maximum accessible selector is 116. This makes
selectors 117-127 unreachable, even though they're defined in the linear_range.

n_voltages = 128 doesn't enable those for operational use, it just allows
the full valid range (11-127) to be accessible.

This is why in my test for the K3 pico board, setting ALDO to 3.3V
(selector 123) or 3.4V (selector 127) fails with the current code.
I mean that leads me to this bug fix.

Best regards,
Guodong Xu


> >   #define P1_DLDO_DESC(_n) \
> > -     P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 117, p1_ldo_ranges)
> > +     P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
> >
> >   static const struct regulator_desc p1_regulator_desc[] = {
> >       P1_BUCK_DESC(1),
> >
>
> I have exactly the same comment about this change to the
> number of supported values.
>
>                                         -Alex

Re: [PATCH v2 1/4] regulator: spacemit-p1: Fix n_voltages for BUCK and LDO regulators

[Alex Elder]

On 1/28/26 9:26 AM, Guodong Xu wrote:
> Hi, Alex

I'm going to keep all this context, but I have a few comments below.

> On Wed, Jan 28, 2026 at 9:28 PM Alex Elder <elder@riscstar.com> wrote:
>>
>> On 1/23/26 6:20 PM, Guodong Xu wrote:
>>> Higher voltage settings were unusable due to incorrect n_voltages values
>>> causing registration failures. For example, setting aldo4 to 3.3V failed
>>> with -EINVAL because the required selector (123) exceeded the allowed
>>> range (n_voltages=117).
>>>
>>> Fix by aligning n_voltages with the hardware register widths per the P1
>>> datasheet [1]:
>>> - BUCK: 255 (was 254), allows selectors 0-254, selector 255 is reserved
>>> - LDO: 128 (was 117), allows selectors 0-127, selectors 0-10 are for
>>>     suspend mode, valid operational range is 11-127
>>>
>>> This enables the full voltage range supported by the hardware.
>>>
>>> Fixes: 8b84d712ad84 ("regulator: spacemit: support SpacemiT P1 regulators")
>>> Link: https://developer.spacemit.com/documentation [1]
>>> Signed-off-by: Guodong Xu <guodong@riscstar.com>
>>> ---
>>> v2: No change.
>>> ---
>>>    drivers/regulator/spacemit-p1.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/regulator/spacemit-p1.c b/drivers/regulator/spacemit-p1.c
>>> index 2bf9137e12b1..2b585ba01a93 100644
>>> --- a/drivers/regulator/spacemit-p1.c
>>> +++ b/drivers/regulator/spacemit-p1.c
>>> @@ -87,13 +87,13 @@ static const struct linear_range p1_ldo_ranges[] = {
>>>        }
>>>
>>>    #define P1_BUCK_DESC(_n) \
>>> -     P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 254, p1_buck_ranges)
>>> +     P1_REG_DESC(BUCK, buck, _n, "vin", 0x47, BUCK_MASK, 255, p1_buck_ranges)
>>
>> This is correct.  There are 255 possible ranges, 0..254, and
>> 255 is an illegal value.
>>
>> I think this bug is an artifact of a change I made while
>> chasing an issue during development, and I neglected to
>> change it back.
>>
>> Technically this is a bug fix but it doesn't matter because
>> this voltage value (255 represents 3.450 volts) was not
>> required.
>>
>>>    #define P1_ALDO_DESC(_n) \
>>> -     P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 117, p1_ldo_ranges)
>>> +     P1_REG_DESC(ALDO, aldo, _n, "vin", 0x5b, LDO_MASK, 128, p1_ldo_ranges)
>>
>> I would say this is not correct.
>>
>> The valid range of values in this register is 0xd-0x1f (11-127),
>> which is 117 values; 0xd represents 0.500V and 0x1f represents
>> 3.400V.
>>
>> Technically, all other values represent 0.5v (and could therefore
>> be considered valid), but I believe those should never be used
>> and intentionally considered them invalid.  If 0.5V is desired,
>> 0xd should be used.
>>
>> Do you disagree with this?
> 
> I understand your concern about selectors 0-10. However, maybe you missed
> this part:
> 
> Code snippet from the c file, Line 53:
> (The p1_buck_ranges and p1_ldo_ranges are defined correctly.)
> 
> /* 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),
> };
> 
> .linear_range, the number of valid voltage steps (selectors 11-127)
> .n_voltages field, which defines the selector namespace (0 to 170, then to 254)

I think you mean 117, not 170.  I don't understand what you mean
with "then to 254."

In any case...  you and I talked offline and I now accept that,
in order to allow selector values up through 127, the value of
regulator_desc->n_voltages needs to be 128 (or conceivably,
256).

This is shown in numerous places, with things like:

         if (selector >= desc->n_voltages)
                 return -EINVAL;

         if (selector < desc->linear_min_sel)
                 return 0;

In <linux/regulator/driver.h> it states "Selectors range from
zero to one less than regulator_desc.n_voltages" but it also
says n_voltages is the "Number of selectors available for
ops.list_voltage()."

The former meaning is really about all possible values that
can be stored in a selector register (field)--whether those
values are valid selectors or not.  While the latter description
sounds to me like the size of an array.  We have a case where
some values (0..10 and 128..255) don't really fit (they all
represent value 0.5 volts, while 11..127 represent a linear
range).  And I was thinking n_voltages was 117 (the size of
the array), rather than 128 (one more than the maximum
valid selector value).

In any case, some of this seems less clear than it could be.

But I'm not going to get in the way of your patch being
accepted.

Reviewed-by: Alex Elder <elder@riscstar.com>




> With n_voltages = 117, the maximum accessible selector is 116. This makes
> selectors 117-127 unreachable, even though they're defined in the linear_range.
> 
> n_voltages = 128 doesn't enable those for operational use, it just allows
> the full valid range (11-127) to be accessible.
> 
> This is why in my test for the K3 pico board, setting ALDO to 3.3V
> (selector 123) or 3.4V (selector 127) fails with the current code.
> I mean that leads me to this bug fix.
> 
> Best regards,
> Guodong Xu
> 
> 
>>>    #define P1_DLDO_DESC(_n) \
>>> -     P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 117, p1_ldo_ranges)
>>> +     P1_REG_DESC(DLDO, dldo, _n, "buck5", 0x67, LDO_MASK, 128, p1_ldo_ranges)
>>>
>>>    static const struct regulator_desc p1_regulator_desc[] = {
>>>        P1_BUCK_DESC(1),
>>>
>>
>> I have exactly the same comment about this change to the
>> number of supported values.
>>
>>                                          -Alex

Re: [PATCH v2 2/4] dt-bindings: mfd: spacemit,p1: Add individual regulator supply properties

[Rob Herring]

On Sat, Jan 24, 2026 at 08:20:17AM +0800, Guodong Xu wrote:
> Add supply properties that match the P1 PMIC's actual hardware topology
> where each buck converter has its own VIN pin and LDO groups share
> common input pins. Supply names are defined according to the pinout
> names in the P1 datasheet.
> 
> This allows different boards to describe their actual power tree
> connections in devicetree rather than hardcoding supply relationships
> in the driver.

You are breaking both forward and backwards compatibility changing both 
the dts and the driver. If that is fine to do, then explicitly say so. 

A secondary issue is the binding and dts go via differ trees and we end 
up with intermittent warnings.

> 
> Signed-off-by: Guodong Xu <guodong@riscstar.com>
> ---
> v2: Remove providers from the dts example.
>     Pass the 'make dt_binding_check' test.
> ---
>  .../devicetree/bindings/mfd/spacemit,p1.yaml       | 49 +++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 2 deletions(-)

Re: [PATCH v2 2/4] dt-bindings: mfd: spacemit,p1: Add individual regulator supply properties

[Guodong Xu]

Hi, Rob

On Fri, Jan 30, 2026 at 2:16 AM Rob Herring <robh@kernel.org> wrote:
>
> On Sat, Jan 24, 2026 at 08:20:17AM +0800, Guodong Xu wrote:
> > Add supply properties that match the P1 PMIC's actual hardware topology
> > where each buck converter has its own VIN pin and LDO groups share
> > common input pins. Supply names are defined according to the pinout
> > names in the P1 datasheet.
> >
> > This allows different boards to describe their actual power tree
> > connections in devicetree rather than hardcoding supply relationships
> > in the driver.
>
> You are breaking both forward and backwards compatibility changing both
> the dts and the driver. If that is fine to do, then explicitly say so.

The breakage is acceptable, so far as discussed and agreed with Yixun Lan
and Vivian. I will update the commit message to state this explicitly in v3.

>
> A secondary issue is the binding and dts go via differ trees and we end
> up with intermittent warnings.

I will add "vin-supply" back to the binding with "deprecated: true" in v3.
This should solve the binding and dts warnings during the merge window.

Thank you for the review.

Best regards,
Guodong Xu



>
> >
> > Signed-off-by: Guodong Xu <guodong@riscstar.com>
> > ---
> > v2: Remove providers from the dts example.
> >     Pass the 'make dt_binding_check' test.
> > ---
> >  .../devicetree/bindings/mfd/spacemit,p1.yaml       | 49 +++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 2 deletions(-)