Re: [PATCH 2/2] regulator: Add TPS65185 driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]