[v2] iio: adc: ad799x: add pm_ops to disable the device completely
diff mbox series

Message ID 20191127114857.11862-1-m.felsch@pengutronix.de
State New
Headers show
Series
  • [v2] iio: adc: ad799x: add pm_ops to disable the device completely
Related show

Commit Message

Marco Felsch Nov. 27, 2019, 11:48 a.m. UTC
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>
---
Hi,

this v2 contains the comments made on [1].

[1] https://patchwork.kernel.org/cover/11149119/

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 | 60 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 54 insertions(+), 6 deletions(-)

Comments

Ardelean, Alexandru Nov. 28, 2019, 7:46 a.m. UTC | #1
On Wed, 2019-11-27 at 12:48 +0100, Marco Felsch 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.
> 

Hey,

Just one comment.
Sorry for not catching this earlier.
I sometimes miss things on review.

Thanks
Alex

> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> Hi,
> 
> this v2 contains the comments made on [1].
> 
> [1] https://patchwork.kernel.org/cover/11149119/
> 
> 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 | 60 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 54 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> index f658012baad8..89e673423e47 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,42 @@ 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 */
> +	ad799x_update_config(st, st->config);
> +

[Again] Sorry for not catching this earlier.

Is it worth doing an error check here?
Maybe

ret = ad799x_update_config(st, st->config);
if (ret < 0) {
   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 +928,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,
Marco Felsch Nov. 28, 2019, 9:47 a.m. UTC | #2
Hey Ardelean,

On 19-11-28 07:46, Ardelean, Alexandru wrote:
> On Wed, 2019-11-27 at 12:48 +0100, Marco Felsch 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.
> > 
> 
> Hey,
> 
> Just one comment.
> Sorry for not catching this earlier.
> I sometimes miss things on review.

No worries.

> Thanks
> Alex
> 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > Hi,
> > 
> > this v2 contains the comments made on [1].
> > 
> > [1] https://patchwork.kernel.org/cover/11149119/
> > 
> > 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 | 60 ++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 54 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> > index f658012baad8..89e673423e47 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,42 @@ 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 */
> > +	ad799x_update_config(st, st->config);
> > +
> 
> [Again] Sorry for not catching this earlier.
> 
> Is it worth doing an error check here?
> Maybe
> 
> ret = ad799x_update_config(st, st->config);
> if (ret < 0) {
>    regulator_disable(st->vref);
>    regulator_disable(st->reg);
>    return ret;
> }

Good point, I will change that.

Regards,
  Marco 

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

Patch
diff mbox series

diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
index f658012baad8..89e673423e47 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,42 @@  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 */
+	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 },
@@ -881,6 +928,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,