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

["Ardelean, Alexandru" <alexandru.Ardelean@analog.com>]

On Sun, 2019-12-01 at 21:11 +0000, Jonathan Cameron wrote:
> 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.
> 

Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

> 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,