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@xxxxxxxxxxxxxx> > > --- > > 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, -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
