* [PATCH 0/3] ADC AD799x improvements @ 2019-09-17 16:09 Marco Felsch 2019-09-17 16:09 ` [PATCH 1/3] iio: adc: ad799x: fix probe error handling Marco Felsch ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Marco Felsch @ 2019-09-17 16:09 UTC (permalink / raw) To: michael.hennerich, lars, stefan.popa, jic23, knaack.h, pmeerw Cc: linux-iio, kernel Hi, the main purpose of this serie is to add the pm_ops support. This is important to free the regulators we are using within the driver. Regards, Marco Marco Felsch (3): iio: adc: ad799x: fix probe error handling iio: adc: ad799x: factor out config register update iio: adc: ad799x: add pm_ops to disable the device completely drivers/iio/adc/ad799x.c | 70 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 7 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] iio: adc: ad799x: fix probe error handling 2019-09-17 16:09 [PATCH 0/3] ADC AD799x improvements Marco Felsch @ 2019-09-17 16:09 ` Marco Felsch 2019-09-18 6:29 ` Ardelean, Alexandru 2019-09-17 16:09 ` [PATCH 2/3] iio: adc: ad799x: factor out config register update Marco Felsch ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Marco Felsch @ 2019-09-17 16:09 UTC (permalink / raw) To: michael.hennerich, lars, stefan.popa, jic23, knaack.h, pmeerw Cc: linux-iio, kernel Since commit 0f7ddcc1bff1 ("iio:adc:ad799x: Write default config on probe and reset alert status on probe") the error path is wrong since it leaves the vref regulator on. Fix this by disabling both regulators. Fixes: 0f7ddcc1bff1 ("iio:adc:ad799x: Write default config on probe and reset alert status on probe") Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/iio/adc/ad799x.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c index 5a3ca5904ded..f658012baad8 100644 --- a/drivers/iio/adc/ad799x.c +++ b/drivers/iio/adc/ad799x.c @@ -810,10 +810,10 @@ static int ad799x_probe(struct i2c_client *client, ret = ad799x_write_config(st, st->chip_config->default_config); if (ret < 0) - goto error_disable_reg; + goto error_disable_vref; ret = ad799x_read_config(st); if (ret < 0) - goto error_disable_reg; + goto error_disable_vref; st->config = ret; ret = iio_triggered_buffer_setup(indio_dev, NULL, -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] iio: adc: ad799x: fix probe error handling 2019-09-17 16:09 ` [PATCH 1/3] iio: adc: ad799x: fix probe error handling Marco Felsch @ 2019-09-18 6:29 ` Ardelean, Alexandru 2019-10-05 12:38 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Ardelean, Alexandru @ 2019-09-18 6:29 UTC (permalink / raw) To: Popa, Stefan Serban, m.felsch, Hennerich, Michael, lars, jic23, pmeerw, knaack.h Cc: kernel, linux-iio On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > [External] > > Since commit 0f7ddcc1bff1 ("iio:adc:ad799x: Write default config on probe > and reset alert status on probe") the error path is wrong since it > leaves the vref regulator on. Fix this by disabling both regulators. > Good catch. Many thanks :) Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Fixes: 0f7ddcc1bff1 ("iio:adc:ad799x: Write default config on probe and > reset alert status on probe") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/iio/adc/ad799x.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > index 5a3ca5904ded..f658012baad8 100644 > --- a/drivers/iio/adc/ad799x.c > +++ b/drivers/iio/adc/ad799x.c > @@ -810,10 +810,10 @@ static int ad799x_probe(struct i2c_client *client, > > ret = ad799x_write_config(st, st->chip_config->default_config); > if (ret < 0) > - goto error_disable_reg; > + goto error_disable_vref; > ret = ad799x_read_config(st); > if (ret < 0) > - goto error_disable_reg; > + goto error_disable_vref; > st->config = ret; > > ret = iio_triggered_buffer_setup(indio_dev, NULL, ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] iio: adc: ad799x: fix probe error handling 2019-09-18 6:29 ` Ardelean, Alexandru @ 2019-10-05 12:38 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2019-10-05 12:38 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Popa, Stefan Serban, m.felsch, Hennerich, Michael, lars, pmeerw, knaack.h, kernel, linux-iio On Wed, 18 Sep 2019 06:29:19 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > > [External] > > > > Since commit 0f7ddcc1bff1 ("iio:adc:ad799x: Write default config on probe > > and reset alert status on probe") the error path is wrong since it > > leaves the vref regulator on. Fix this by disabling both regulators. > > > > Good catch. > Many thanks :) > > Reviewed-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Applied to the fixes-togreg branch of iio.git and marked for stable. Obviously this may delay the other two patches which will get queued up for the next merge window, but as we have plenty of time that shouldn't be an issue. Thanks, Jonathan > > > Fixes: 0f7ddcc1bff1 ("iio:adc:ad799x: Write default config on probe and > > reset alert status on probe") > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/iio/adc/ad799x.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > > index 5a3ca5904ded..f658012baad8 100644 > > --- a/drivers/iio/adc/ad799x.c > > +++ b/drivers/iio/adc/ad799x.c > > @@ -810,10 +810,10 @@ static int ad799x_probe(struct i2c_client *client, > > > > ret = ad799x_write_config(st, st->chip_config->default_config); > > if (ret < 0) > > - goto error_disable_reg; > > + goto error_disable_vref; > > ret = ad799x_read_config(st); > > if (ret < 0) > > - goto error_disable_reg; > > + goto error_disable_vref; > > st->config = ret; > > > > ret = iio_triggered_buffer_setup(indio_dev, NULL, ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] iio: adc: ad799x: factor out config register update 2019-09-17 16:09 [PATCH 0/3] ADC AD799x improvements Marco Felsch 2019-09-17 16:09 ` [PATCH 1/3] iio: adc: ad799x: fix probe error handling Marco Felsch @ 2019-09-17 16:09 ` Marco Felsch 2019-09-18 6:51 ` Ardelean, Alexandru 2019-09-17 16:09 ` [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely Marco Felsch 2019-09-18 7:00 ` [PATCH 0/3] ADC AD799x improvements Ardelean, Alexandru 3 siblings, 1 reply; 13+ messages in thread From: Marco Felsch @ 2019-09-17 16:09 UTC (permalink / raw) To: michael.hennerich, lars, stefan.popa, jic23, knaack.h, pmeerw Cc: linux-iio, kernel Factor out the configuration register update to reuse it during pm resume operation. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c index f658012baad8..af5a2de9c22f 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); -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iio: adc: ad799x: factor out config register update 2019-09-17 16:09 ` [PATCH 2/3] iio: adc: ad799x: factor out config register update Marco Felsch @ 2019-09-18 6:51 ` Ardelean, Alexandru 2019-09-18 8:52 ` Marco Felsch 0 siblings, 1 reply; 13+ messages in thread From: Ardelean, Alexandru @ 2019-09-18 6:51 UTC (permalink / raw) To: Popa, Stefan Serban, m.felsch, Hennerich, Michael, lars, jic23, pmeerw, knaack.h Cc: kernel, linux-iio On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > [External] > Comments inline. > Factor out the configuration register update to reuse it during pm > resume operation. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > index f658012baad8..af5a2de9c22f 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; I'm feeling this could go a bit further maybe. I'm noticing that patch 3 adds ad799x_suspend() & ad799x_resume(). It looks to me (I could be wrong), that this bit of code (with some minor re-ordering) is actually a ad799x_resume() call. Similarly, ad799x_suspend() could be added in ad799x_remove(). If that's the case, patch 2 & 3 could be squashed into a single patch that adds ad799x_suspend() & ad799x_resume() & also replaces them here and in the ad799x_remove() code. > > ret = iio_triggered_buffer_setup(indio_dev, NULL, > &ad799x_trigger_handler, NULL); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iio: adc: ad799x: factor out config register update 2019-09-18 6:51 ` Ardelean, Alexandru @ 2019-09-18 8:52 ` Marco Felsch 2019-09-18 10:17 ` Ardelean, Alexandru 0 siblings, 1 reply; 13+ messages in thread From: Marco Felsch @ 2019-09-18 8:52 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Popa, Stefan Serban, Hennerich, Michael, lars, jic23, pmeerw, knaack.h, kernel, linux-iio Hi, On 19-09-18 06:51, Ardelean, Alexandru wrote: > On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > > [External] > > > > Comments inline. > > > Factor out the configuration register update to reuse it during pm > > resume operation. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > > index f658012baad8..af5a2de9c22f 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; > > I'm feeling this could go a bit further maybe. > I'm noticing that patch 3 adds ad799x_suspend() & ad799x_resume(). Hm.. I don't know. You're right that this is needed for the resume(). I wanted to keep the changes minimal to speed up the review process. As you mentioned below I can squash patch 2 & 3. > It looks to me (I could be wrong), that this bit of code (with some minor > re-ordering) is actually a ad799x_resume() call. I would keep them seperate at least resume() and the probe() path. Regards, Marco > Similarly, ad799x_suspend() could be added in ad799x_remove(). > > If that's the case, patch 2 & 3 could be squashed into a single patch that > adds ad799x_suspend() & ad799x_resume() & also replaces them here and in > the ad799x_remove() code. > > > > > ret = iio_triggered_buffer_setup(indio_dev, NULL, > > &ad799x_trigger_handler, NULL); -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iio: adc: ad799x: factor out config register update 2019-09-18 8:52 ` Marco Felsch @ 2019-09-18 10:17 ` Ardelean, Alexandru 0 siblings, 0 replies; 13+ messages in thread From: Ardelean, Alexandru @ 2019-09-18 10:17 UTC (permalink / raw) To: m.felsch Cc: kernel, linux-iio, lars, knaack.h, Hennerich, Michael, jic23, Popa, Stefan Serban, pmeerw On Wed, 2019-09-18 at 10:52 +0200, Marco Felsch wrote: > [External] > > Hi, > > On 19-09-18 06:51, Ardelean, Alexandru wrote: > > On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > > > [External] > > > > > > > Comments inline. > > > > > Factor out the configuration register update to reuse it during pm > > > resume operation. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > drivers/iio/adc/ad799x.c | 23 +++++++++++++++++------ > > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c > > > index f658012baad8..af5a2de9c22f 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; > > > > I'm feeling this could go a bit further maybe. > > I'm noticing that patch 3 adds ad799x_suspend() & ad799x_resume(). > > Hm.. I don't know. You're right that this is needed for the resume(). I > wanted to keep the changes minimal to speed up the review process. As > you mentioned below I can squash patch 2 & 3. > > > It looks to me (I could be wrong), that this bit of code (with some > > minor > > re-ordering) is actually a ad799x_resume() call. > > I would keep them seperate at least resume() and the probe() path. I would be inclined to reuse resume() & suspend() in probe() & remove(), but it's not a big deal. We can leave it separately. > > Regards, > Marco > > > Similarly, ad799x_suspend() could be added in ad799x_remove(). > > > > If that's the case, patch 2 & 3 could be squashed into a single patch > > that > > adds ad799x_suspend() & ad799x_resume() & also replaces them here and > > in > > the ad799x_remove() code. > > > > > > > > ret = iio_triggered_buffer_setup(indio_dev, NULL, > > > &ad799x_trigger_handler, NULL); ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely 2019-09-17 16:09 [PATCH 0/3] ADC AD799x improvements Marco Felsch 2019-09-17 16:09 ` [PATCH 1/3] iio: adc: ad799x: fix probe error handling Marco Felsch 2019-09-17 16:09 ` [PATCH 2/3] iio: adc: ad799x: factor out config register update Marco Felsch @ 2019-09-17 16:09 ` Marco Felsch 2019-09-18 6:59 ` Ardelean, Alexandru 2019-09-18 7:00 ` [PATCH 0/3] ADC AD799x improvements Ardelean, Alexandru 3 siblings, 1 reply; 13+ messages in thread From: Marco Felsch @ 2019-09-17 16:09 UTC (permalink / raw) To: michael.hennerich, lars, stefan.popa, jic23, knaack.h, pmeerw 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> --- 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"); + 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) { + 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, -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely 2019-09-17 16:09 ` [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely Marco Felsch @ 2019-09-18 6:59 ` Ardelean, Alexandru 2019-09-18 8:37 ` Marco Felsch 0 siblings, 1 reply; 13+ messages in thread From: Ardelean, Alexandru @ 2019-09-18 6:59 UTC (permalink / raw) To: Popa, Stefan Serban, m.felsch, Hennerich, Michael, lars, jic23, pmeerw, knaack.h Cc: kernel, linux-iio 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@pengutronix.de> > --- > 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, ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely 2019-09-18 6:59 ` Ardelean, Alexandru @ 2019-09-18 8:37 ` Marco Felsch 0 siblings, 0 replies; 13+ messages in thread From: Marco Felsch @ 2019-09-18 8:37 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Popa, Stefan Serban, Hennerich, Michael, lars, jic23, pmeerw, knaack.h, kernel, linux-iio Hi Alex, On 19-09-18 06:59, Ardelean, Alexandru wrote: > 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@pengutronix.de> > > --- > > 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). Yes, you're right. I will change that in such a case to disable the regulators without checking the return val and I will change dev_err to dev_dbg. IMHO a developer should be informed about such a case at least during a debug session. This will help them to limit the possible failures why a voltage rail can't be disabled. Regards, Marco > > > + 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, -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] ADC AD799x improvements 2019-09-17 16:09 [PATCH 0/3] ADC AD799x improvements Marco Felsch ` (2 preceding siblings ...) 2019-09-17 16:09 ` [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely Marco Felsch @ 2019-09-18 7:00 ` Ardelean, Alexandru 2019-09-18 8:22 ` Marco Felsch 3 siblings, 1 reply; 13+ messages in thread From: Ardelean, Alexandru @ 2019-09-18 7:00 UTC (permalink / raw) To: Popa, Stefan Serban, m.felsch, Hennerich, Michael, lars, jic23, pmeerw, knaack.h Cc: kernel, linux-iio On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > [External] > > Hi, > > the main purpose of this serie is to add the pm_ops support. This is > important to free the regulators we are using within the driver. > Hey, Thanks for the patches. There are some minor issues with patch 3 with regard to error paths. Overall they look good. I do have a general comment that maybe ad799x_resume() & ad799x_suspend() could be re-used in ad799x_probe() & ad799x_remove(). Thanks Alex > Regards, > Marco > > Marco Felsch (3): > iio: adc: ad799x: fix probe error handling > iio: adc: ad799x: factor out config register update > iio: adc: ad799x: add pm_ops to disable the device completely > > drivers/iio/adc/ad799x.c | 70 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 63 insertions(+), 7 deletions(-) > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] ADC AD799x improvements 2019-09-18 7:00 ` [PATCH 0/3] ADC AD799x improvements Ardelean, Alexandru @ 2019-09-18 8:22 ` Marco Felsch 0 siblings, 0 replies; 13+ messages in thread From: Marco Felsch @ 2019-09-18 8:22 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Popa, Stefan Serban, Hennerich, Michael, lars, jic23, pmeerw, knaack.h, kernel, linux-iio Hi Alex, On 19-09-18 07:00, Ardelean, Alexandru wrote: > On Tue, 2019-09-17 at 18:09 +0200, Marco Felsch wrote: > > [External] > > > > Hi, > > > > the main purpose of this serie is to add the pm_ops support. This is > > important to free the regulators we are using within the driver. > > > > Hey, > > Thanks for the patches. > There are some minor issues with patch 3 with regard to error paths. > > Overall they look good. > I do have a general comment that maybe ad799x_resume() & ad799x_suspend() > could be re-used in ad799x_probe() & ad799x_remove(). Thanks for the quick response and comments you made. Please check my comments I made in line. Regards, Marco > > Thanks > Alex > > > > Regards, > > Marco > > > > Marco Felsch (3): > > iio: adc: ad799x: fix probe error handling > > iio: adc: ad799x: factor out config register update > > iio: adc: ad799x: add pm_ops to disable the device completely > > > > drivers/iio/adc/ad799x.c | 70 ++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 63 insertions(+), 7 deletions(-) > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-10-05 12:38 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-17 16:09 [PATCH 0/3] ADC AD799x improvements Marco Felsch 2019-09-17 16:09 ` [PATCH 1/3] iio: adc: ad799x: fix probe error handling Marco Felsch 2019-09-18 6:29 ` Ardelean, Alexandru 2019-10-05 12:38 ` Jonathan Cameron 2019-09-17 16:09 ` [PATCH 2/3] iio: adc: ad799x: factor out config register update Marco Felsch 2019-09-18 6:51 ` Ardelean, Alexandru 2019-09-18 8:52 ` Marco Felsch 2019-09-18 10:17 ` Ardelean, Alexandru 2019-09-17 16:09 ` [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely Marco Felsch 2019-09-18 6:59 ` Ardelean, Alexandru 2019-09-18 8:37 ` Marco Felsch 2019-09-18 7:00 ` [PATCH 0/3] ADC AD799x improvements Ardelean, Alexandru 2019-09-18 8:22 ` Marco Felsch
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).