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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 2 Dec 2019 06:26:21 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

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

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

thanks,

Jonathan

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