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

[Alex Elder <elder@riscstar.com>]

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)
>>>
>>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv