linux-kernel - Re: [PATCH 2/2] regulator: Add TPS65185 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2e850c1c-67ed-44af-94b1-2ccc35e50bb8@sirena.org.uk>
Date: Mon, 22 Dec 2025 15:07:28 +0000
From: Mark Brown <broonie@...nel.org>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Liam Girdwood <lgirdwood@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Guenter Roeck <linux@...ck-us.net>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH 2/2] regulator: Add TPS65185 driver

On Mon, Dec 22, 2025 at 02:45:22PM +0100, Andreas Kemnade wrote:
> Mark Brown <broonie@...nel.org> wrote:

> > The reason for having GPIO controlled enables on devices with register
> > maps is that it's generally substantially faster to update a GPIO than
> > to do I2C I/O.

> well we are talking about 30ms turning on time here.

> [  130.816647] tps65185 1-0068: turning on...
> [  130.849970] tps65185 1-0068: turned on

> So if we have 100khz i2c, so, we have around 0.1ms per byte, so
> the read/modify/write sequence should be done in <1ms. So I guess that is
> neglectible and allows the flexibility to not have that pin.

Every little helps, and not every I2C controller is a model of
efficiency and programmability.  Note that we do have core support for
GPIO enables, it's not really any effort to support them.

> > > +{  

> > Implementing runtime suspend in a regulator is *very* non-idiomatic and
> > is leading to large amounts of open coding throughout the driver.
> > What's the story here?  I'm very surprised that this wasn't in the
> > changelog.

> OK, lets look around in the datasheet. We are apparently dealing
> with 130µA here which can be saved. But that should be acceptable to be
> only done on system suspend even if the regulator is off most times.
> So no really strong technical reason here. I am just too used to testing
> power management using runtime suspend.

It does feel like something where if we're going to do it we should
update the core to take runtime PM references rather than open coding it
in a driver that's otherwise able to use the standard helpers.  I do
worry about the impact on enable times (you'd have to power up the
supply and sync the register cache) but I guess people could disable
runtime PM for specific devices if it's an issue, and it'll never apply
to primary PMICs anyway.

> > > +static irqreturn_t tps65185_irq_thread(int irq, void *dev_id)
> > > +{

> > This unconditionally reports an interrupt even if none was detected.

> Hmm, this seems like some common pattern, if some irq occurs,
> check some registers and if something is set, do something about it,
> and then unconditionally return IRQ_HANDLED.

It is a common bug but it's a bug nontheless, it means that genirq can't
detect and mitigate stuck interrupts and that the interrupt can't be
shared.

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