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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 30 May 2026 20:39:35 +0900
SeungJu Cheon <suunj1331@gmail.com> 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 <suunj1331@gmail.com>
> ---
>  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);