[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <501849d1-2158-471a-af25-da3dbb070422@riscstar.com>
Date: Wed, 28 Jan 2026 07:28:48 -0600
From: Alex Elder <elder@...cstar.com>
To: Guodong Xu <guodong@...cstar.com>, Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Yixun Lan <dlan@...too.org>,
Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
Paul Walmsley <pjw@...nel.org>, Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>
Cc: linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org,
spacemit@...ts.linux.dev, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/4] regulator: spacemit-p1: Fix n_voltages for BUCK
and LDO regulators
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@...cstar.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
Powered by blists - more mailing lists