linux-kernel - Re: [PATCH 1/2] regulator: pf530x: Add a driver for the NXP PF5300 Regulator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <91086d3b-01e0-4334-894d-3627fa132709@sirena.org.uk>
Date: Tue, 2 Sep 2025 16:08:26 +0100
From: Mark Brown <broonie@...nel.org>
To: Woodrow Douglass <wdouglass@...negierobotics.com>
Cc: linux-kernel@...r.kernel.org, Liam Girdwood <lgirdwood@...il.com>
Subject: Re: [PATCH 1/2] regulator: pf530x: Add a driver for the NXP PF5300
 Regulator

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 //.

> +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.

> +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.

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)