Message-Id: <20250902-pf530x-v3-0-4242e7687761@carnegierobotics.com>
Date: Wed,  3 Sep 2025 11:31:36 -0400
From: Woodrow Douglass <wdouglass@...negierobotics.com>
To: Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Woodrow Douglass <wdouglass@...negierobotics.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>
Cc: linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: [PATCH v3 0/2] regulator: pf530x: NXP PF530x regulator driver

All,

Sorry for resubmitting the original patches, i thought that's what was
wanted. I'm trying very hard not to break ettiquite here. Below are
some responses to your earlier comments. Thank you.

On 9/2/25 11:08, Mark Brown wrote:
> On Tue, Sep 02, 2025 at 10:21:33AM -0400, Woodrow Douglass wrote:
> 
>>  obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
>>  obj-$(CONFIG_REGULATOR_PCA9450) += pca9450-regulator.o
>>  obj-$(CONFIG_REGULATOR_PF9453) += pf9453-regulator.o
>> +obj-$(CONFIG_REGULATOR_PF530X) += pf530x-regulator.o
>>  obj-$(CONFIG_REGULATOR_PF8X00) += pf8x00-regulator.o
>>  obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
>>  obj-$(CONFIG_REGULATOR_PV88060) += pv88060-regulator.o
> 
> I'd say please keep this sorted but there's some cleanup needed here
> already so whatever, let's deal with that separately.
> 
>> +static const struct regmap_config pf530x_regmap_config = {
>> +       .reg_bits = 8,
>> +       .val_bits = 8,
>> +       .max_register = PF530X_OTP_MODE,
>> +       .cache_type = REGCACHE_RBTREE,
>> +};
> 
> In general it's better to use _MAPLE register caches unless you've got a
> good reason not to, the data structure is more modern.
> 
>> +static int pf530x_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	//first get mode
> 
> Usual comment style would have a space after the //.
>

this exact comment got lost in the fallout of the regmap changes below, but
i've fixed this issue elsewhere in the file, thanks.

>> +static int pf530x_get_status(struct regulator_dev *rdev)
>> +{
> 
> I would have expected this function to check INT_SENSE1/2 for current
> error statuses and report those.

I have added the INT_SENSE1 bits to the REGULATOR_STATUS_ERROR return value. I'm
not sure how to properly represent the thermal bits in INT_SENSE2 here -- this
part can safely run at high temperatures, some of those bits are just
informational in some designs (including the board I'm working on now)

> 
>> +static int pf530x_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> 
> I see INT_STATUS2 has thermal warning/error interrupts in it as well.
> Not essential but it'd be nice to also check those.  These statuses are
> also clear on write so I'd expect a write to clear them, even though the
> device lacks an actual interrupt line so it's all somewhat ornamantal
> ATM :/  I suppse we ought to implement some core thing to do polling for
> non-interrupting regulators, but that's definitely out of scope for this
> driver.
> 
>> +static const struct regulator_ops pf530x_regulator_ops = {
>> +	.enable = regulator_enable_regmap,
>> +	.disable = regulator_disable_regmap,
>> +	.is_enabled = pf530x_is_enabled,
> 
> The custom is_enabled() operation doesn't seem to line up with the
> generic regmap enable/disable operations, and we don't seem to have
> enable_val or disable_val in the regulator_desc which the generic ops
> expect.  The whole connection with the modes seems a bit odd, the
> standby voltages look like they'd more naturally map to the regulator
> API's suspend mode but perhaps these devices are not usually integrated
> in that way and this would be controlled separately to system suspend.

I agree, I was misguided here. I've added enable_reg, enable_mask, enable_val,
and disable_val to the regulator_desc initializer, and moved to the regmap
function from helpers.c. I'm moving the "suspend mode" settings too here. The
board i'm working with has the suspend pin grounded, so I can't really test
suspend mode -- supporting that may have to wait for a future patchset.

> 
>> +static int pf530x_identify(struct pf530x_chip *chip)
>> +{
> 
>> +	dev_info(chip->dev, "%s Regulator found.\n", name);
> 
> It wouldn't hurt to read and log the data in REV, EMREV and PROG_ID too
> (it can be helpful when debugging).

I've added REV and PROG_ID, EMREV is listed as "Reserved for NXP Internal Use"
on page 95 of the datasheet, and  -- I can include those bits here but i'm not
sure they're very useful; if you'd like me to include those bits anyway i will

On 9/2/25 15:19, Krzysztof Kozlowski wrote:
> On 02/09/2025 16:21, Woodrow Douglass wrote:
>> Bindings for the pf530x series of voltage regulators
>>
>> Signed-off-by: Woodrow Douglass <wdouglass@...negierobotics.com>
>> ---
>>  .../regulator/nxp,pf530x-regulator.yaml       | 74 +++++++++++++++++++
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> 
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time.
> 
> Please kindly resend and include all necessary To/Cc entries.
> </form letter>
> 
> 
>>  1 file changed, 74 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/regulator/nxp,pf530x-regulator.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/nxp,pf530x-regulator.yaml b/Documentation/devicetree/bindings/regulator/nxp,pf530x-regulator.yaml
>> new file mode 100644
>> index 000000000000..f1065b167491
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/nxp,pf530x-regulator.yaml
> 
> 
> Filename should match compatible, so nxp,pf5300.yaml.
> 
>> @@ -0,0 +1,74 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/regulator/nxp,pf530x-regulator.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NXP PF5300/PF5301/PF5302 PMIC regulators
>> +
>> +maintainers:
>> +  - Woodrow Douglass <wdouglass@...negierobotics.com>
>> +
>> +description: |
>> +  The PF5300, PF5301, and PF5302 integrate high-performance buck converters, 12 A, 8 A,
>> +  and 15 A, respectively, to power high-end automotive and industrial processors. With adaptive
>> +  voltage positioning and a high-bandwidth loop, they offer transient regulation to minimize capacitor
>> +  requirements.
> 
> Wrap according to Linux coding style.
> 
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - nxp,pf5300
>> +      - nxp,pf5301
>> +      - nxp,pf5302
> 
> Your driver clearly suggests these are compatible, so express it (see
> example schema).
>

I'm not sure I understand this comment. The difference in these parts is
only the current limit, so the software interface is compatible -- should I
only have a single "compatible" string (nxp,pf5300) and ignore the other
two variants? Seems like it would limit searchability for future users of
the driver, but maybe i'm not understanding what you're asking for here?
I was following nxp,pf8x00-regulator.yaml as an example (I am also using
that regulator in this design) and i guess it must be a bit dated.

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  regulators:
> 
> No need for this node.
> 
>> +    type: object
>> +    description: |
>> +      list of regulators provided by this controller
>> +
>> +    properties:
>> +      SW1:
> 
> No need, drop the node.
> 
>> +        type: object
>> +        $ref: regulator.yaml#
>> +        description:
>> +          Properties for the regulator.
>> +
>> +        properties:
>> +          regulator-name:
>> +            pattern: "^SW1$"
> 
> No, drop entirely regulator-name. Just embed the properties in parent node.
> 
>> +            description:
>> +              Name of the single regulator
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - regulators
>> +

I have removed the regulators node (and refactored the driver to
not require it)

>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c1 {
> 
> i2c
> 
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        vddi_0_75@28 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).
> 
> See also DTS coding style.
> 
>

I have updated this example to be more generic.

> 
> Best regards,
> Krzysztof
> 

Thanks again,
Woodrow Douglass

--
2.39.5

---
Changes in v3:
- Replaced REGCACHE_RBTREE with REGCACHE_MAPLE
- Replaced pf530x_is_enabled function with regulator_is_enabled_regmap
- Added status bits from INT_SENSE1 to pf530x_get_status function
- Added extra context to info print upon chip identification
- Reworked devtree to not require nested "regulators" subnode
- Some minor reformatting of comment style and long lines
- Link to v2: https://lore.kernel.org/r/20250902-pf530x-v2-0-f105eb073cb1@carnegierobotics.com

---
Woodrow Douglass (2):
      regulator: pf530x: Add a driver for the NXP PF5300 Regulator
      regulator: pf530x: dt-bindings: nxp,pf530x-regulator

 .../devicetree/bindings/regulator/nxp,pf5300.yaml  |  52 +++
 MAINTAINERS                                        |   6 +
 drivers/regulator/Kconfig                          |  12 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/pf530x-regulator.c               | 359 +++++++++++++++++++++
 5 files changed, 430 insertions(+)
---
base-commit: b320789d6883cc00ac78ce83bccbfe7ed58afcf0
change-id: 20250902-pf530x-6db7b921120c

Best regards,
-- 
Woodrow Douglass <wdouglass@...negierobotics.com>

Powered by blists - more mailing lists