* [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely
@ 2019-11-29 16:53 Marco Felsch
2019-12-01 21:11 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Marco Felsch @ 2019-11-29 16:53 UTC (permalink / raw)
To: alexandru.Ardelean, StefanSerban.Popa, Michael.Hennerich, lars,
jic23, pmeerw, knaack.h
Cc: linux-iio, kernel
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 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,
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely
2019-11-29 16:53 [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely Marco Felsch
@ 2019-12-01 21:11 ` Jonathan Cameron
2019-12-02 6:26 ` Ardelean, Alexandru
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2019-12-01 21:11 UTC (permalink / raw)
To: Marco Felsch
Cc: alexandru.Ardelean, StefanSerban.Popa, Michael.Hennerich, lars,
pmeerw, knaack.h, linux-iio, kernel
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,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely
2019-12-01 21:11 ` Jonathan Cameron
@ 2019-12-02 6:26 ` Ardelean, Alexandru
2019-12-06 17:36 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Ardelean, Alexandru @ 2019-12-02 6:26 UTC (permalink / raw)
To: jic23, m.felsch
Cc: linux-iio, Hennerich, Michael, kernel, lars, pmeerw, Popa,
Stefan Serban, knaack.h
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,
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely
2019-12-02 6:26 ` Ardelean, Alexandru
@ 2019-12-06 17:36 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2019-12-06 17:36 UTC (permalink / raw)
To: Ardelean, Alexandru
Cc: m.felsch, linux-iio, Hennerich, Michael, kernel, lars, pmeerw,
Popa, Stefan Serban, knaack.h
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,
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-06 17:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 16:53 [PATCH v3] iio: adc: ad799x: add pm_ops to disable the device completely Marco Felsch
2019-12-01 21:11 ` Jonathan Cameron
2019-12-02 6:26 ` Ardelean, Alexandru
2019-12-06 17:36 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).