| Message ID | 20190917160925.9791-4-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ADC AD799x improvements | expand |
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,
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,
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,
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(+)