Re: [PATCH v2 1/6] iio: adc: ti-ads1015: use DEFINE_RUNTIME_DEV_PM_OPS()

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Jul 2026 00:50:57 +0530
Archit Anant <architanant5@gmail.com> wrote:

> Replace the deprecated SET_RUNTIME_PM_OPS() with the modern
> DEFINE_RUNTIME_DEV_PM_OPS() macro. This allows for the removal of the
> macro automatically handles dropping unused functions when PM is
> disabled.
> 
> Update the driver struct to use pm_ptr() to avoid unused variable
> warnings.
> 
> Signed-off-by: Archit Anant <architanant5@gmail.com>
Hi Archit,

This looks fine but did make me look at the code that was being protected
and in particular ads1015_set_conv_mode()

A few things jump out about that which might make sense for further improvement
if you want to take them on.


> static int ads1015_set_conv_mode(struct ads1015_data *data, int mode)
> {
> 	return regmap_update_bits(data->regmap, ADS1015_CFG_REG,
> 				  ADS1015_CFG_MOD_MASK,
> 				  mode << ADS1015_CFG_MOD_SHIFT);#

Use FIELD_PREP() here and drop the ADS1015_CFG_MOD_SHIFT macro.
That made me wonder how extensive _SHIFT macros are in this driver and the
answer is very!  Get rid of all of them in favour of FIELD_PREP()
and FIELD_GREP() and using the field masks.


> }
The other thing is the question of why this function exists at all
given with the value of mode inline it would be obvious what it is doing
without the wrapper and this is the only similar little helper function.

I'd squash it so we do the regmap_update_bits() calls directly instead
of via this helper function.

I did debate whether brining setting conv_invalid into the function made
sense but on balance I think not.

If you do make these changes, 1 patch for dropping all the _SHIFT
macros and replacing with FIELD_PREP() / FIELD_GET() and a second
patch to remove the helper function.

Thanks,

Jonathan


> ---
>  drivers/iio/adc/ti-ads1015.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
> index 8a272af69f7d..9cd620b71429 100644
> --- a/drivers/iio/adc/ti-ads1015.c
> +++ b/drivers/iio/adc/ti-ads1015.c
> @@ -1066,7 +1066,6 @@ static void ads1015_remove(struct i2c_client *client)
>  			 ERR_PTR(ret));
>  }
>  
> -#ifdef CONFIG_PM
>  static int ads1015_runtime_suspend(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> @@ -1087,12 +1086,11 @@ static int ads1015_runtime_resume(struct device *dev)
>  
>  	return ret;
>  }
> -#endif
>  
> -static const struct dev_pm_ops ads1015_pm_ops = {
> -	SET_RUNTIME_PM_OPS(ads1015_runtime_suspend,
> -			   ads1015_runtime_resume, NULL)
> -};
> +static DEFINE_RUNTIME_DEV_PM_OPS(ads1015_pm_ops,
> +				 ads1015_runtime_suspend,
> +				 ads1015_runtime_resume,
> +				 NULL);
>  
>  static const struct ads1015_chip_data ads1015_data = {
>  	.channels	= ads1015_channels,
> @@ -1147,7 +1145,7 @@ static struct i2c_driver ads1015_driver = {
>  	.driver = {
>  		.name = ADS1015_DRV_NAME,
>  		.of_match_table = ads1015_of_match,
> -		.pm = &ads1015_pm_ops,
> +		.pm = pm_ptr(&ads1015_pm_ops),
>  	},
>  	.probe		= ads1015_probe,
>  	.remove		= ads1015_remove,