[PATCH 0/3] ADC AD799x improvements

[PATCH 0/3] ADC AD799x improvements

[Marco Felsch]

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

[PATCH 1/3] iio: adc: ad799x: fix probe error handling

[Marco Felsch]

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

[PATCH 2/3] iio: adc: ad799x: factor out config register update

[Marco Felsch]

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

[PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely

[Marco Felsch]

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

Re: [PATCH 1/3] iio: adc: ad799x: fix probe error handling

[Ardelean, Alexandru]

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,

Re: [PATCH 2/3] iio: adc: ad799x: factor out config register update

[Ardelean, Alexandru]

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);

Re: [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely

[Ardelean, Alexandru]

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,

Re: [PATCH 0/3] ADC AD799x improvements

[Ardelean, Alexandru]

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(-)
> 

Re: [PATCH 0/3] ADC AD799x improvements

[Marco Felsch]

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 |

Re: [PATCH 3/3] iio: adc: ad799x: add pm_ops to disable the device completely

[Marco Felsch]

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 |

Re: [PATCH 2/3] iio: adc: ad799x: factor out config register update

[Marco Felsch]

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 |

Re: [PATCH 2/3] iio: adc: ad799x: factor out config register update

[Ardelean, Alexandru]

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);

Re: [PATCH 1/3] iio: adc: ad799x: fix probe error handling

[Jonathan Cameron]

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,