linux-kernel - Re: [PATCH v3 0/2] regulator: pf530x: NXP PF530x regulator driver

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 for Android: free password hash cracker in your pocket

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

Message-ID: <cd9e526d-9f17-4c2d-99f7-a495e2d58d73@sirena.org.uk>
Date: Wed, 3 Sep 2025 17:08:54 +0100
From: Mark Brown <broonie@...nel.org>
To: Woodrow Douglass <wdouglass@...negierobotics.com>
Cc: Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v3 0/2] regulator: pf530x: NXP PF530x regulator driver

On Wed, Sep 03, 2025 at 11:31:36AM -0400, Woodrow Douglass wrote:
> On 9/2/25 11:08, Mark Brown wrote:
> > On Tue, Sep 02, 2025 at 10:21:33AM -0400, Woodrow Douglass wrote:

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

If the device can happily run at those levels then report as a warning.

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

That's fine, someone can always extend the functionality later.

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

IIRC something in there mentioned it was a metal revision which would
make sense for the name.  Given the name you may as well include them,
worst case they just get ignored best case it helps someone debug some
issue sometime.

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