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

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 03:48:52PM +0200, Joshua Crofts wrote:
> On Tue, 28 Apr 2026 at 18:33, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On Mon, 27 Apr 2026 22:09:55 +0200
> > Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 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...

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

-- 
With Best Regards,
Andy Shevchenko