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: <84fdaf7c-4d4b-491f-975c-ebb14350fafd@sirena.org.uk>
Date: Mon, 22 Dec 2025 12:36:02 +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 01:18:31PM +0100, Andreas Kemnade wrote:

> Add a driver for the TPS65185 regulator. Implement handling of the various
> gpio pins. Because the PWRUP (=enable) pin functionality can be achieved
> by just using two bits instead, just ensure that it is set to a stable
> value.

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.

> +static bool tps65185_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TPS65185_REG_TMST_VALUE:
> +	case TPS65185_REG_ENABLE:

Why is the enable register volatile?  I can't see anything in the
datasheet that suggests that it should be.

> +static int tps65185_runtime_suspend(struct device *dev)
> +{

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.

+       if (data->wakeup_gpio) {
+               ret = gpiod_set_value_cansleep(data->wakeup_gpio, 0);
+               if (ret)
+                       return ret;
+       }

This would usually be used for system suspend.

+       if (data->vin_reg) {
+               ret = regulator_disable(data->vin_reg);
+               if (ret)
+                       goto reenable_wkup;
+       }

Can the device really operate without power?

> +static irqreturn_t tps65185_irq_thread(int irq, void *dev_id)
> +{
> +	struct tps65185_data *data = dev_id;
> +	unsigned int int_status_1, int_status_2;
> +	int ret;
> +
> +	/* read both status to have irq cleared */
> +	regmap_read(data->regmap, TPS65185_REG_INT1, &int_status_1);
> +
> +	ret = regmap_read(data->regmap, TPS65185_REG_INT2, &int_status_2);
> +	if (!ret) {
> +		if (int_status_2 & BIT(0))
> +			complete(&data->tmst_completion);
> +	}
> +
> +	dev_dbg(data->dev, "irq status %02x %02x\n", int_status_1, int_status_2);
> +
> +	return IRQ_HANDLED;
> +}

This unconditionally reports an interrupt even if none was detected.

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