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

[Andreas Kemnade <andreas@kemnade.info>]

On Mon, 22 Dec 2025 12:36:02 +0000
Mark Brown <broonie@kernel.org> wrote:

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

> > +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.
> 
Bit 7/6 are volatile. They reset automatically after operation done
Quote: "NOTE: After transition bit is cleared automatically"

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

> +       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?
> 
No, it cannot. So yes, if we require the regulator we have
simplier code here.

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

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.

Regards,
Andreas