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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251224010440.46ad717a@kemnade.info>
Date: Wed, 24 Dec 2025 09:12:35 +0100
From: Andreas Kemnade <andreas@...nade.info>
To: Mark Brown <broonie@...nel.org>
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, 22 Dec 2025 15:07:28 +0000
Mark Brown <broonie@...nel.org> wrote:

> 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.
> 
If the GPIO is wired... There are a half a dozen different implementations
of this driver in the wild, and I remember one not using a GPIO
probably for a device without the enable gpio wired up to the SoC.
So I think the i2c way of enabling things is required at least
as a fallback option. So we need some if (enable_gpio) somewhere.

> > > > +{    
> 
> > > 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.
> 
hmm, we have REGULATOR_MODE_FAST to maybe disable some pm. I have used
the autosuspend mechanism, so we do not do the time-consuming register cache
mechanism on every enable. But putting such into regulator core should be
more sane than having it in drivers.

Using standard helpers is in many places at least not so straight forward.
See the 6- in vposneg vsel.

vcom vsel has 9bits across two registers. So that is also odd.
vposneg has 2 bits used in a kind of RS flipflop mode for enable
if the enable pin is not available.

I would agree to remove the pm runtime stuff for now, so we can get the
basics in, and care about the optimazations later.

Regards,
Andreas