Re: [PATCH 1/4] iio: pressure: mpl3115: convert probe to fully devm managed

Hi Jonathan, thanks for the detailed review.

On Sat, 30 May 2026 20:39:35 +0900 SeungJu Cheon <[email protected]> wrote:
> > Register a devm action to return the device to standby,
> > replacing the cleanup previously performed in
> > mpl3115_remove().
>
> This should have noted that in error paths the standby wasn't there
> and now is. However see below; I think the patch need to be split in
> at least two parts.

You're right. I'll split it into two patches in v2:
  1. Add the standby cleanup on the probe error path (no devm yet).
  2. Convert to devm-managed allocation.
and note the error-path behaviour change explicitly.

> Very short wrap. Exactly whether to limit commit messages to 72 or
> 75 chars is a bit of a matter of opinion, but this is 50 something.

Understood, I'll rewrap the commit messages closer to 75 chars.

> > Move mpl3115_standby() and suspend/resume helpers above
> > probe to satisfy declaration ordering requirements.
>
> Not obvious why the suspend/resume moved. Just move the minimum to
> make the patch easier to read.

Agreed, that was unnecessary churn. I'll move only what's needed and
leave suspend/resume in place.

> > No functional change.
>
> Not true. That error path gaining suspend of device is a functional
> change.

You're right, I'll drop that line. The error-path change will live in
the split-out fix patch and be described there.

> Take opportunity to tidy indent up now we are less fuzzy about 80
> chars.

Will fix the indentation as you show.

> Andy covered this. I think we ripped out most instances of this a
> year or so back as it is a pointless bit of bouncing back and forth
> between container structure and the contained.

Yes, switching to dev_get_drvdata() in v2.

> Andy called out the question of why these moved. It makes the patch
> somewhat harder to review, so don't do that in this patch even if
> there is a reason to do it.

Understood, I'll leave suspend/resume untouched here.

> So in the error path for this function we will now call this standby
> action. If there isn't an equivalent in the original code, break
> this up.
> Patch 1. Add that cleanup without any devm stuff ...
> Patch 2. Devm stuff.

Confirmed there's no standby on the error path in the current code,
so I'll break it up exactly as you describe.

> Indent as something like:
>         ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>                                               mpl3115_trigger_handler, NULL);

Will do.

Thanks again.

On Sun, May 31, 2026 at 12:10 AM Jonathan Cameron <[email protected]> wrote:
>
> On Sat, 30 May 2026 20:39:35 +0900
> SeungJu Cheon <[email protected]> wrote:
>
> > Convert probe to use devm-managed resource allocation,
> > removing the need for an explicit remove callback.
>
> Hi SeungJu,
>
> >
> > Replace iio_triggered_buffer_setup() and
> > iio_device_register() with their devm equivalents.
> > Register a devm action to return the device to standby,
> > replacing the cleanup previously performed in
> > mpl3115_remove().
>
> This should have noted that in error paths the standby wasn't there and now is.
> However see below; I think the patch need to be split in at least two parts.
>
> Very short wrap.  Exactly whether to limit commit messages to 72 or 75 chars
> is a bit of a matter of opinion, but this is 50 something.
> >
> > Move mpl3115_standby() and suspend/resume helpers above
> > probe to satisfy declaration ordering requirements.
> Not obvious why the suspend/resume moved.  Just move the minimum to make
> the patch easier to read.
> >
> > No functional change.
>
> Not true. That error path gaining suspend of device is a functional change.
>
> >
> > Signed-off-by: SeungJu Cheon <[email protected]>
> > ---
> >  drivers/iio/pressure/mpl3115.c | 81 ++++++++++++++++------------------
> >  1 file changed, 39 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/mpl3115.c b/drivers/iio/pressure/mpl3115.c
> > index aeac1586f12e..befb6d48efa9 100644
> > --- a/drivers/iio/pressure/mpl3115.c
> > +++ b/drivers/iio/pressure/mpl3115.c
> > @@ -691,6 +691,33 @@ static int mpl3115_trigger_probe(struct mpl3115_data *data,
> >       return 0;
> >  }
> >
> > +static int mpl3115_standby(struct mpl3115_data *data)
> > +{
> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +             data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
>
> Take opportunity to tidy indent up now we are less fuzzy about 80 chars.
>
>         return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
>                                          data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> is the preferred style.
>
> > +}
> > +
> > +static void mpl3115_standby_action(void *d)
> > +{
> > +     mpl3115_standby(d);
> > +}
> > +
> > +static int mpl3115_suspend(struct device *dev)
> > +{
> > +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>
> Andy covered this. I think we ripped out most instances of this a year
> or so back as it is a pointless bit of bouncing back and forth between
> container structure and the contained.
>
> > +
> > +     return mpl3115_standby(iio_priv(indio_dev));
> > +}
> > +
> > +static int mpl3115_resume(struct device *dev)
> > +{
> > +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +     struct mpl3115_data *data = iio_priv(indio_dev);
> > +
> > +     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > +                                      data->ctrl_reg1);
> > +}
>
> Andy called out the question of why these moved. It makes the patch
> somewhat harder to review, so don't do that in this patch even if there
> is a reason to do it.
>
> > +
> >  static int mpl3115_probe(struct i2c_client *client)
> >  {
> >       const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > @@ -730,53 +757,24 @@ static int mpl3115_probe(struct i2c_client *client)
> >       if (ret < 0)
> >               return ret;
> >
> > -     ret = mpl3115_trigger_probe(data, indio_dev);
> > +     ret = devm_add_action_or_reset(&client->dev, mpl3115_standby_action,
> > +                                    data);
>
> So in the error path for this function we will now call this standby
> action. If there isn't an equivalent in the original code, break this up.
>
> Patch 1. Add that cleanup without any devm stuff (almost a fix but it's
>          only going to waste a bit of power in an unlikely error path so
>          we probably won't backport it).
> Patch 2. Devm stuff.
>
> >       if (ret)
> >               return ret;
> >
> > -     ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > -             mpl3115_trigger_handler, NULL);
> > -     if (ret < 0)
> > +     ret = mpl3115_trigger_probe(data, indio_dev);
> > +     if (ret)
> >               return ret;
> >
> > -     ret = iio_device_register(indio_dev);
> > -     if (ret < 0)
> > -             goto buffer_cleanup;
> > -     return 0;
> > -
> > -buffer_cleanup:
> > -     iio_triggered_buffer_cleanup(indio_dev);
> > -     return ret;
> > -}
> > -
> > -static int mpl3115_standby(struct mpl3115_data *data)
> > -{
> > -     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -             data->ctrl_reg1 & ~MPL3115_CTRL1_ACTIVE);
> > -}
> > -
> > -static void mpl3115_remove(struct i2c_client *client)
> > -{
> > -     struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > -
> > -     iio_device_unregister(indio_dev);
> > -     iio_triggered_buffer_cleanup(indio_dev);
> > -     mpl3115_standby(iio_priv(indio_dev));
> > -}
> > -
> > -static int mpl3115_suspend(struct device *dev)
> > -{
> > -     return mpl3115_standby(iio_priv(i2c_get_clientdata(
> > -             to_i2c_client(dev))));
> > -}
> > -
> > -static int mpl3115_resume(struct device *dev)
> > -{
> > -     struct mpl3115_data *data = iio_priv(i2c_get_clientdata(
> > -             to_i2c_client(dev)));
> > +     ret = devm_iio_triggered_buffer_setup(&client->dev,
> > +                                   indio_dev,
> > +                                   NULL,
> > +                                   mpl3115_trigger_handler,
> > +                                   NULL);
>
> Indent as something like:
>
>         ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
>                                               mpl3115_trigger_handler, NULL);
>
> > +     if (ret)
> > +             return ret;
> >
> > -     return i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > -             data->ctrl_reg1);
> > +     return devm_iio_device_register(&client->dev, indio_dev);
> >  }
> >
> >  static DEFINE_SIMPLE_DEV_PM_OPS(mpl3115_pm_ops, mpl3115_suspend,
> > @@ -801,7 +799,6 @@ static struct i2c_driver mpl3115_driver = {
> >               .pm     = pm_sleep_ptr(&mpl3115_pm_ops),
> >       },
> >       .probe = mpl3115_probe,
> > -     .remove = mpl3115_remove,
> >       .id_table = mpl3115_id,
> >  };
> >  module_i2c_driver(mpl3115_driver);
>