On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote:
> [External]
>
Comments inline
> 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@xxxxxxxxxxxxxx>
> ---
> drivers/iio/adc/ad799x.c | 45 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index af5a2de9c22f..32d242ecb12c 100644
> --- a/drivers/iio/adc/ad799x.c
> +++ b/drivers/iio/adc/ad799x.c
> @@ -875,6 +875,50 @@ 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);
> + int ret;
> +
> + ret = regulator_disable(st->vref);
> + if (ret) {
> + dev_err(dev, "Unable to disable vref regulator\n");
Exiting here will leave st->reg undisabled.
I don't know if this should do more that just disable the regulators.
I am not sure if we should print anything here if regulator_disable()
errors. Usually (in this case) the system would go to sleep, and that's it.
Errors would be more important in the ad799x_resume() case (than here).
But, if these messages are important, you could maybe print with
"dev_warn()" instead of "dev_err()".
I am not sure if errors when disabling regulators (in this case here)
classify as errors (instead of warnings).
> + return ret;
> + }
> + ret = regulator_disable(st->reg);
> + if (ret) {
> > + dev_err(dev, "Unable to disable vcc regulator\n");
> + return ret;
> + }
> +
> + 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) {
Should there be a "regulator_disable(st->reg);" call here ?
> + dev_err(dev, "Unable to enable vref regulator\n");
> + return ret;
> + }
> + /* resync config */
> + ad799x_update_config(st, st->config);
> +
> + 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 },
> @@ -892,6 +936,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,
