[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 at 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 at 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 at 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