Re: [PATCH v0 10/14] iio: magnetometer: ak8975: switch to using managed resources

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 29 Apr 2026 17:34:04 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Apr 29, 2026 at 03:48:52PM +0200, Joshua Crofts wrote:
> > On Tue, 28 Apr 2026 at 18:33, Jonathan Cameron <jic23@kernel.org> wrote:  
> > > On Mon, 27 Apr 2026 22:09:55 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> 
> This is combined reply to both of you.
> 
> ...
> 
> > > One day I'll have time to write up some notes on patterns that work
> > > for runtime pm + devm. I don't think think this one does - note original
> > > code didn't work either as would underflow on regulators in some paths
> > > and hence print warnings.  
> 
> > > > +static void devm_ak8975_power_off(void *data)
> > > > +{
> > > > +     ak8975_set_mode(data, POWER_DOWN);  
> > >
> > > Add a comment somewhere appropriate on what we are undoing with this.
> > >
> > > It's a 'might not be runtime suspended' at time of remove I think
> > > rather than the mode being set at that point in probe.
> > >
> > > If it is suspended, we shouldn't call the regulator disables
> > > in power_off.  So maybe gate this whole thing on
> > > whether runtime suspended.  If you do that be careful with what
> > > state we are in until runtime PM is enabled.  Maybe you already have that
> > > covered elsewhere.  
> > 
> > Okay, I'll add a pm_runtime_status_suspended() check here.  
> 
> Also check for the DPM_FLAG_SMART_SUSPEND and others first.  They might
> be helpful.
> 
> > > > +     ak8975_power_off(data);
> > > > +}  
> 
> Hmm... I thought that we have the device is on (unless SMART_SUSPEND is
> provided) during removal stage. But I think you are referring to the case
> when we call this one on the already suspended device (which is probably
> happens before managed resource tear down) and hence regulators will go
> into unbalanced state. Yeah, it would be nice to read some howto:s on
> the topic...

Exactly that - it doesn't come out of runtime suspend on unbind hence
any regulators disabled for that don't come back on line and we underflow.
My mental model of this stuff was wrong until recently as I assumed
we exited runtime suspend before entering remove, but in general that's not
the case.

> 
> > > If following comments above and below you'd need to set the state
> > > to active here.  If you go that way, make sure to add a comment to
> > > say why it is up here (and hopefully prevent someone thinking they can
> > > move it down).  
> > 
> > I'll make sure to write a comment warning others about moving the function call.
> >   
> > > > +     ret = devm_add_action_or_reset(dev, devm_ak8975_power_off, data);
> > > > +     if (ret)
> > > > +             return ret;  
> 
> Oh, true. Before accessing the device we should know what is the state of PM.
> 
> 
> > > >       /* Enable runtime PM */
> > > > -     pm_runtime_get_noresume(&client->dev);
> > > > -     pm_runtime_set_active(&client->dev);
> > > > -     pm_runtime_enable(&client->dev);
> > > > +     ret = devm_pm_runtime_set_active_enabled(dev);  
> > >
> > > This hits the race with whether we can query if the device is disabled mentioned
> > > above. Dance where we need to be able to check that before we turn runtime pm on
> > > so that we can correctly do regulator disables if we error out before here.  
> > 
> > So just do a devm_pm_runtime_enable() instead?  
> 
> This needs to be thought through...
> 
> Jonathan, do we have already examples in IIO which do something similar
> in the correct way?
We've had a few 'fixes' around this recently rather than people generally getting
it right first time.

Sean (+CC) fixed a similar case deep in this patch:

https://lore.kernel.org/all/20250909-icm42pmreg-v4-1-2bf763662c5c@geanix.com/
>