2023-05-30 08:03:32

by Fabrizio Lamarque

Subject: [PATCH v3 3/5] iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source

From: Fabrizio Lamarque <[email protected]>

Add missing vref-supply and fix avdd-supply used as if it were vref.

AD7192 requires three independent voltage sources, digital supply (on
pin DVdd), analog supply (on AVdd) and reference voltage (VRef on
alternate pin pair REFIN1 or REFIN2).

Emit a warning message when AVdd is used in place of VRef for backwards
compatibility.

Fixes: b581f748cce0 ("staging: iio: adc: ad7192: move out of staging")
Signed-off-by: Fabrizio Lamarque <[email protected]>
---
drivers/iio/adc/ad7192.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 5a9c8898f8af..bc41323ea810 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -177,6 +177,7 @@ struct ad7192_chip_info {
struct ad7192_state {
const struct ad7192_chip_info *chip_info;
struct regulator *avdd;
+ struct regulator *vref;
struct clk *mclk;
u16 int_vref_mv;
u32 fclk;
@@ -1014,11 +1015,31 @@ static int ad7192_probe(struct spi_device *spi)
if (ret)
return ret;

+ st->vref = devm_regulator_get_optional(&spi->dev, "vref");
+ if (!IS_ERR(st->vref)) {
+ ret = regulator_enable(st->vref);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to enable specified VRef supply\n");
+
+ ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
+ if (ret)
+ return ret;
+ } else if (PTR_ERR(st->vref) != -ENODEV) {
+ return PTR_ERR(st->vref);
+ }
+
ret = devm_regulator_get_enable(&spi->dev, "dvdd");
if (ret)
return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");

- ret = regulator_get_voltage(st->avdd);
+
+ if (!IS_ERR(st->vref)) {
+ ret = regulator_get_voltage(st->vref);
+ } else {
+ dev_warn(&spi->dev, "Using AVdd in place of VRef. Likely an old DTS\n");
+ ret = regulator_get_voltage(st->avdd);
+ }
if (ret < 0) {
dev_err(&spi->dev, "Device tree error, reference voltage undefined\n");
return ret;
--
2.34.1

2023-06-03 13:04:51

by Andy Shevchenko

Subject: Re: [PATCH v3 3/5] iio: adc: ad7192: Use VRef instead of AVdd as reference voltage source

Tue, May 30, 2023 at 09:53:09AM +0200, [email protected] kirjoitti:
> From: Fabrizio Lamarque <[email protected]>
>
> Add missing vref-supply and fix avdd-supply used as if it were vref.
>
> AD7192 requires three independent voltage sources, digital supply (on
> pin DVdd), analog supply (on AVdd) and reference voltage (VRef on
> alternate pin pair REFIN1 or REFIN2).
>
> Emit a warning message when AVdd is used in place of VRef for backwards
> compatibility.

...

> + st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> + if (!IS_ERR(st->vref)) {
> + ret = regulator_enable(st->vref);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable specified VRef supply\n");
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
> + if (ret)
> + return ret;
> + } else if (PTR_ERR(st->vref) != -ENODEV) {
> + return PTR_ERR(st->vref);
> + }

Wouldn't this be better?

if (IS_ERR(st->vref)) {
if (PTR_ERR(st->vref) != -ENODEV)
return PTR_ERR(st->vref);
} else {

...

> if (ret)
> return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
>
> - ret = regulator_get_voltage(st->avdd);
> +

One blank line is enough.

> + if (!IS_ERR(st->vref)) {
> + ret = regulator_get_voltage(st->vref);

Why negative conditional? Usual pattern is to check for errors first, so

if (IS_ERR(st->vref)) {
dev_warn(...);
...
} else {
ret = regulator_get_voltage(st->vref);
}

> + } else {
> + dev_warn(&spi->dev, "Using AVdd in place of VRef. Likely an old DTS\n");
> + ret = regulator_get_voltage(st->avdd);
> + }

--
With Best Regards,
Andy Shevchenko