Re: [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 29 Nov 2019 17:53:14 +0100
Marco Felsch <m.felsch@pengutronix.de> wrote:

> The device is always in a low-power state due to the hardware design. It
> wakes up upon a conversion request and goes back into the low-power
> state. The pm ops are added to disable the device completely and to free
> the regulator. Disbaling the device completely should be not that
> notable but freeing the regulator is important. Because if it is a shared
> power-rail the regulator won't be disabled during suspend-to-ram/disk
> and so all devices connected to that rail keeps on.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
Looks fine to me, but I'd like to leave it a little longer for Alex
and anyone else who is interested to have time to take a look.

Thanks,

Jonathan

> ---
> Hi,
> 
> this v3 contains the comments made on [1].
> 
> [1] https://www.spinics.net/lists/linux-iio/msg48135.html
> 
> Changelog:
> v3:
> - add resync error check during resume
> 
> v2:
> - squash patch 2 & 3
> - call regulator_disable() unconditional during suspend()
> - drop dev_err() messages during suspend
> - fix error path within resume()
> ---
>  drivers/iio/adc/ad799x.c | 66 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index f658012baad8..ef013af1aec0 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -167,6 +167,21 @@ static int ad799x_read_config(struct ad799x_state *st)
>  	}
>  }
>  
> +static int ad799x_update_config(struct ad799x_state *st, u16 config)
> +{
> +	int ret;
> +
> +	ret = ad799x_write_config(st, config);
> +	if (ret < 0)
> +		return ret;
> +	ret = ad799x_read_config(st);
> +	if (ret < 0)
> +		return ret;
> +	st->config = ret;
> +
> +	return 0;
> +}
> +
>  /**
>   * ad799x_trigger_handler() bh of trigger launched polling to ring buffer
>   *
> @@ -808,13 +823,9 @@ static int ad799x_probe(struct i2c_client *client,
>  	indio_dev->channels = st->chip_config->channel;
>  	indio_dev->num_channels = chip_info->num_channels;
>  
> -	ret = ad799x_write_config(st, st->chip_config->default_config);
> -	if (ret < 0)
> -		goto error_disable_vref;
> -	ret = ad799x_read_config(st);
> -	if (ret < 0)
> +	ret = ad799x_update_config(st, st->chip_config->default_config);
> +	if (ret)
>  		goto error_disable_vref;
> -	st->config = ret;
>  
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  		&ad799x_trigger_handler, NULL);
> @@ -864,6 +875,48 @@ static int ad799x_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +static int __maybe_unused ad799x_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct ad799x_state *st = iio_priv(indio_dev);
> +
> +	regulator_disable(st->vref);
> +	regulator_disable(st->reg);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused ad799x_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct ad799x_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regulator_enable(st->reg);
> +	if (ret) {
> +		dev_err(dev, "Unable to enable vcc regulator\n");
> +		return ret;
> +	}
> +	ret = regulator_enable(st->vref);
> +	if (ret) {
> +		regulator_disable(st->reg);
> +		dev_err(dev, "Unable to enable vref regulator\n");
> +		return ret;
> +	}
> +
> +	/* resync config */
> +	ret = ad799x_update_config(st, st->config);
> +	if (ret) {
> +		regulator_disable(st->vref);
> +		regulator_disable(st->reg);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ad799x_pm_ops, ad799x_suspend, ad799x_resume);
> +
>  static const struct i2c_device_id ad799x_id[] = {
>  	{ "ad7991", ad7991 },
>  	{ "ad7995", ad7995 },
> @@ -881,6 +934,7 @@ MODULE_DEVICE_TABLE(i2c, ad799x_id);
>  static struct i2c_driver ad799x_driver = {
>  	.driver = {
>  		.name = "ad799x",
> +		.pm = &ad799x_pm_ops,
>  	},
>  	.probe = ad799x_probe,
>  	.remove = ad799x_remove,