Re: [PATCH 3/8] iio: magnetometer: ak8975: switch to using managed resources

[Nuno Sá]

On Sat, May 09, 2026 at 03:32:59PM +0200, Joshua Crofts wrote:
> On Sat, 9 May 2026 at 11:02, Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> >
> > On Thu, 2026-05-07 at 16:35 +0200, Joshua Crofts via B4 Relay wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > >
> > > Switch the driver to use managed resources (devm_*) which simplifier
> > > error handling and allows removing ak8975_remove() method from
> > > the driver.
> > >
> > > Note, on error path we now also set mode to POWER_DOWN state which is
> > > fine. Even if the device is in that mode, there is no problem to set
> > > that mode again, it should be no-op.
> > >
> > > Additionally, remove any pm_runtime_get/put*() function calls that
> > > dummy cycled the counter to autosuspend the device.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > > Co-developed-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> > > Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> > > ---
> > >  drivers/iio/magnetometer/ak8975.c | 74 ++++++++++++++++++++++-----------------
> > >  1 file changed, 41 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> > > index
> > > 63b6e8465f5f3873841550a1cd03ce86b95d1d67..1d8f448d5179fe9b33af989cc2f456ac91bc2f17
> > > 100644
> > > --- a/drivers/iio/magnetometer/ak8975.c
> > > +++ b/drivers/iio/magnetometer/ak8975.c
> > > @@ -898,9 +898,24 @@ static irqreturn_t ak8975_handle_trigger(int irq, void *p)
> > >       return IRQ_HANDLED;
> > >  }
> > >
> > > +static void devm_ak8975_power_off(void *data)
> > > +{
> > > +     struct ak8975_data *ak = data;
> > > +     struct device *dev = &ak->client->dev;
> > > +
> > > +     /* Only power down if currently active */
> > > +     if (pm_runtime_status_suspended(dev))
> > > +             return;
> > > +
> > > +     /* Soft-stop the chip before hard-stopping the regulators */
> > > +     ak8975_set_mode(data, POWER_DOWN);
> > > +     ak8975_power_off(data);
> > > +}
> > > +
> > >  static int ak8975_probe(struct i2c_client *client)
> > >  {
> > >       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > > +     struct device *dev = &client->dev;
> > >       struct ak8975_data *data;
> > >       struct iio_dev *indio_dev;
> > >       struct gpio_desc *eoc_gpiod;
> > > @@ -968,10 +983,21 @@ static int ak8975_probe(struct i2c_client *client)
> > >       if (ret)
> > >               return ret;
> > >
> > > +     /*
> > > +      * Set device as active early so pm_runtime_status_suspended() works
> > > +      * correctly if probe fails before pm_runtime is enabled. Do not attempt
> > > +      * to move this line lower.
> > > +      */
> > > +     pm_runtime_set_active(dev);
> > > +
> > > +     ret = devm_add_action_or_reset(dev, devm_ak8975_power_off, data);
> > > +     if (ret)
> > > +             return ret;
> >
> > This looks a bit hackish to me. Why don't we just register this powerdown action
> > after enabling PM?
> 
> This is to prevent a resource leak. If we register the power_off
> function at the end,
> any potential probe() failures will leave the regulators on.

Okay it does make sense! I now see the action is registered after
ak8975_power_on() which what makes sense. Still would like to avoid that
weird pm_runtime_set_active() call. It would be nice if we could
guarantee through PM that the device get's suspended on unbind. Oh well,
I guess this is ok.

- Nuno Sá

> 
> > > +
> > >       ret = ak8975_who_i_am(client, data->def->type);
> > >       if (ret) {
> > >               dev_err(&client->dev, "Unexpected device\n");
> > > -             goto power_off;
> > > +             return ret;
> > >       }
> > >       dev_dbg(&client->dev, "Asahi compass chip %s\n", name);
> > >
> > > @@ -979,10 +1005,13 @@ static int ak8975_probe(struct i2c_client *client)
> > >       ret = ak8975_setup(client);
> > >       if (ret) {
> > >               dev_err(&client->dev, "%s initialization fails\n", name);
> > > -             goto power_off;
> > > +             return ret;
> > >       }
> > >
> > > -     mutex_init(&data->lock);
> > > +     ret = devm_mutex_init(dev, &data->lock);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > >       indio_dev->channels = ak8975_channels;
> > >       indio_dev->num_channels = ARRAY_SIZE(ak8975_channels);
> > >       indio_dev->info = &ak8975_info;
> > > @@ -990,52 +1019,32 @@ static int ak8975_probe(struct i2c_client *client)
> > >       indio_dev->modes = INDIO_DIRECT_MODE;
> > >       indio_dev->name = name;
> > >
> > > -     ret = iio_triggered_buffer_setup(indio_dev, NULL, ak8975_handle_trigger,
> > > -                                      NULL);
> > > +     ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > +                                           ak8975_handle_trigger, NULL);
> > >       if (ret) {
> > >               dev_err(&client->dev, "triggered buffer setup failed\n");
> > > -             goto power_off;
> > > +             return ret;
> > >       }
> > >
> > > -     ret = iio_device_register(indio_dev);
> > > +     ret = devm_iio_device_register(dev, indio_dev);
> > >       if (ret) {
> > >               dev_err(&client->dev, "device register failed\n");
> > > -             goto cleanup_buffer;
> > > +             return ret;
> > >       }
> > >
> > >       /* Enable runtime PM */
> > > -     pm_runtime_get_noresume(&client->dev);
> > > -     pm_runtime_set_active(&client->dev);
> > > -     pm_runtime_enable(&client->dev);
> > > +     ret = devm_pm_runtime_enable(dev);
> > > +     if (ret)
> > > +             return ret;
> >
> > Maybe it would make sense to move this before devm_iio_device_register(). At the
> > point we register the device, userspace can start to interact with the device where
> > we have pm calls? Not that it is a problem (I think) but makes sense to me to enable
> > PM before exposing the device.
> 
> Sure, I agree with this.
> 
> -- 
> Kind regards
> 
> CJD