[PATCH 0/5] iio: adc: use devm_regulator_get_enable_read_voltage round 1

[PATCH 0/5] iio: adc: use devm_regulator_get_enable_read_voltage round 1

[David Lechner]

Following up from [1], this is the first round of patches to convert the
ADC drivers to use devm_regulator_get_enable_read_voltage().

Some of these are trivial but some aren't so I'm breaking them up into
smaller series to spread out the review load (and my work load).

[1]: https://lore.kernel.org/linux-iio/20240429-regulator-get-enable-get-votlage-v2-0-b1f11ab766c1@baylibre.com

---
David Lechner (5):
      iio: adc: ad7192: use devm_regulator_get_enable_read_voltage
      iio: adc: ad7266: use devm_regulator_get_enable_read_voltage
      iio: adc: ad7292: use devm_regulator_get_enable_read_voltage
      iio: adc: ad7793: use devm_regulator_get_enable_read_voltage
      iio: adc: ad7944: use devm_regulator_get_enable_read_voltage

 drivers/iio/adc/ad7192.c | 98 +++++++++++++++++-------------------------------
 drivers/iio/adc/ad7266.c | 37 +++++-------------
 drivers/iio/adc/ad7292.c | 40 +++++---------------
 drivers/iio/adc/ad7793.c | 24 ++----------
 drivers/iio/adc/ad7944.c | 62 ++++++++++--------------------
 5 files changed, 77 insertions(+), 184 deletions(-)
---
base-commit: 15895709c7dc5f1a8b53b3564fc2bed724209611
change-id: 20240531-iio-adc-ref-supply-refactor-93f962d40c23

[PATCH 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[David Lechner]

This makes use of the new devm_regulator_get_enable_read_voltage()
function to reduce boilerplate code.

Error messages have changed slightly since there are now fewer places
where we print an error. The rest of the logic of selecting which
supply to use as the reference voltage remains the same.

Also 1000 is replaced by MILLI in a few places for consistency.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7192.c | 98 +++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 63 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 0789121236d6..e08bf066b3f6 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -200,8 +200,6 @@ 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				aincom_mv;
@@ -1189,17 +1187,11 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
 	},
 };
 
-static void ad7192_reg_disable(void *reg)
-{
-	regulator_disable(reg);
-}
-
 static int ad7192_probe(struct spi_device *spi)
 {
 	struct ad7192_state *st;
 	struct iio_dev *indio_dev;
-	struct regulator *aincom;
-	int ret;
+	int ret, avdd_mv;
 
 	if (!spi->irq) {
 		dev_err(&spi->dev, "no IRQ?\n");
@@ -1219,74 +1211,54 @@ static int ad7192_probe(struct spi_device *spi)
 	 * Newer firmware should provide a zero volt fixed supply if wired to
 	 * ground.
 	 */
-	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
-	if (IS_ERR(aincom)) {
-		if (PTR_ERR(aincom) != -ENODEV)
-			return dev_err_probe(&spi->dev, PTR_ERR(aincom),
-					     "Failed to get AINCOM supply\n");
-
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
+	if (ret == -ENODEV)
 		st->aincom_mv = 0;
-	} else {
-		ret = regulator_enable(aincom);
-		if (ret)
-			return dev_err_probe(&spi->dev, ret,
-					     "Failed to enable specified AINCOM supply\n");
+	else if (ret < 0)
+		return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
+	else
+		st->aincom_mv = ret / MILLI;
 
-		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
+	/* AVDD can optionally be used as reference voltage */
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
+	if (ret == -EINVAL) {
+		/*
+		 * We get -EINVAL if avdd is a supply with unknown voltage. We
+		 * still need to enable it since it is also a power supply.
+		 */
+		ret = devm_regulator_get_enable(&spi->dev, "avdd");
 		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(aincom);
-		if (ret < 0)
 			return dev_err_probe(&spi->dev, ret,
-					     "Device tree error, AINCOM voltage undefined\n");
-		st->aincom_mv = ret / MILLI;
-	}
+					     "Failed to enable AVDD supply\n");
 
-	st->avdd = devm_regulator_get(&spi->dev, "avdd");
-	if (IS_ERR(st->avdd))
-		return PTR_ERR(st->avdd);
-
-	ret = regulator_enable(st->avdd);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
-		return ret;
+		avdd_mv = 0;
+	} else if (ret < 0) {
+		return dev_err_probe(&spi->dev, ret, "Failed to get AVDD voltage\n");
+	} else {
+		avdd_mv = ret / MILLI;
 	}
 
-	ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
-	if (ret)
-		return ret;
 
 	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
 	if (ret)
 		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
 
-	st->vref = devm_regulator_get_optional(&spi->dev, "vref");
-	if (IS_ERR(st->vref)) {
-		if (PTR_ERR(st->vref) != -ENODEV)
-			return PTR_ERR(st->vref);
-
-		ret = regulator_get_voltage(st->avdd);
-		if (ret < 0)
-			return dev_err_probe(&spi->dev, ret,
-					     "Device tree error, AVdd voltage undefined\n");
+	/*
+	 * This is either REFIN1 or REFIN2 depending on adi,refin2-pins-enable.
+	 * If this supply is not present, fall back to AVDD as reference.
+	 */
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
+	if (ret == -ENODEV) {
+		if (avdd_mv == 0)
+			return dev_err_probe(&spi->dev, -ENODEV,
+					     "No reference voltage available\n");
+
+		st->int_vref_mv = avdd_mv;
+	} else if (ret < 0) {
+		return ret;
 	} else {
-		ret = regulator_enable(st->vref);
-		if (ret) {
-			dev_err(&spi->dev, "Failed to enable specified Vref supply\n");
-			return ret;
-		}
-
-		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(st->vref);
-		if (ret < 0)
-			return dev_err_probe(&spi->dev, ret,
-					     "Device tree error, Vref voltage undefined\n");
+		st->int_vref_mv = ret / MILLI;
 	}
-	st->int_vref_mv = ret / 1000;
 
 	st->chip_info = spi_get_device_match_data(spi);
 	indio_dev->name = st->chip_info->name;

-- 
2.45.1

Re: [PATCH 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[Jonathan Cameron]

On Fri, 31 May 2024 16:19:32 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
> 
> Error messages have changed slightly since there are now fewer places
> where we print an error. The rest of the logic of selecting which
> supply to use as the reference voltage remains the same.
> 
> Also 1000 is replaced by MILLI in a few places for consistency.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Ouch diff didn't like this one much and it is a bit hard to read.

One case where I think this has an unintended side effect.
See below.

> ---
>  drivers/iio/adc/ad7192.c | 98 +++++++++++++++++-------------------------------
>  1 file changed, 35 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 0789121236d6..e08bf066b3f6 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -200,8 +200,6 @@ 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				aincom_mv;
> @@ -1189,17 +1187,11 @@ static const struct ad7192_chip_info ad7192_chip_info_tbl[] = {
>  	},
>  };
>  
> -static void ad7192_reg_disable(void *reg)
> -{
> -	regulator_disable(reg);
> -}
> -
>  static int ad7192_probe(struct spi_device *spi)
>  {
>  	struct ad7192_state *st;
>  	struct iio_dev *indio_dev;
> -	struct regulator *aincom;
> -	int ret;
> +	int ret, avdd_mv;
>  
>  	if (!spi->irq) {
>  		dev_err(&spi->dev, "no IRQ?\n");
> @@ -1219,74 +1211,54 @@ static int ad7192_probe(struct spi_device *spi)
>  	 * Newer firmware should provide a zero volt fixed supply if wired to
>  	 * ground.
>  	 */
> -	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
> -	if (IS_ERR(aincom)) {
> -		if (PTR_ERR(aincom) != -ENODEV)
> -			return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> -					     "Failed to get AINCOM supply\n");
> -
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
> +	if (ret == -ENODEV)
>  		st->aincom_mv = 0;
> -	} else {
> -		ret = regulator_enable(aincom);
> -		if (ret)
> -			return dev_err_probe(&spi->dev, ret,
> -					     "Failed to enable specified AINCOM supply\n");
> +	else if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
> +	else
> +		st->aincom_mv = ret / MILLI;
>  
> -		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> +	/* AVDD can optionally be used as reference voltage */
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
> +	if (ret == -EINVAL) {
> +		/*
> +		 * We get -EINVAL if avdd is a supply with unknown voltage. We
> +		 * still need to enable it since it is also a power supply.
> +		 */
> +		ret = devm_regulator_get_enable(&spi->dev, "avdd");

What happens if the entry simply isn't there in the DT.
Previously I think the regulator framework would supply a stub whereas
the devm_regulator_get_enable_read_voltage() returns -ENODEV so isn't
caught by the handling and I think it should be.

>  		if (ret)
> -			return ret;
> -
> -		ret = regulator_get_voltage(aincom);
> -		if (ret < 0)
>  			return dev_err_probe(&spi->dev, ret,
> -					     "Device tree error, AINCOM voltage undefined\n");
> -		st->aincom_mv = ret / MILLI;
> -	}
> +					     "Failed to enable AVDD supply\n");
>  
> -	st->avdd = devm_regulator_get(&spi->dev, "avdd");
> -	if (IS_ERR(st->avdd))
> -		return PTR_ERR(st->avdd);
> -
> -	ret = regulator_enable(st->avdd);
> -	if (ret) {
> -		dev_err(&spi->dev, "Failed to enable specified AVdd supply\n");
> -		return ret;
> +		avdd_mv = 0;
> +	} else if (ret < 0) {
> +		return dev_err_probe(&spi->dev, ret, "Failed to get AVDD voltage\n");
> +	} else {
> +		avdd_mv = ret / MILLI;
>  	}
>  
> -	ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->avdd);
> -	if (ret)
> -		return ret;
>  
>  	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
>  	if (ret)
>  		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVdd supply\n");
>  
> -	st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> -	if (IS_ERR(st->vref)) {
> -		if (PTR_ERR(st->vref) != -ENODEV)
> -			return PTR_ERR(st->vref);
> -
> -		ret = regulator_get_voltage(st->avdd);
> -		if (ret < 0)
> -			return dev_err_probe(&spi->dev, ret,
> -					     "Device tree error, AVdd voltage undefined\n");
> +	/*
> +	 * This is either REFIN1 or REFIN2 depending on adi,refin2-pins-enable.
> +	 * If this supply is not present, fall back to AVDD as reference.
> +	 */
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> +	if (ret == -ENODEV) {
> +		if (avdd_mv == 0)
> +			return dev_err_probe(&spi->dev, -ENODEV,
> +					     "No reference voltage available\n");
> +
> +		st->int_vref_mv = avdd_mv;
> +	} else if (ret < 0) {
> +		return ret;
>  	} else {
> -		ret = regulator_enable(st->vref);
> -		if (ret) {
> -			dev_err(&spi->dev, "Failed to enable specified Vref supply\n");
> -			return ret;
> -		}
> -
> -		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, st->vref);
> -		if (ret)
> -			return ret;
> -
> -		ret = regulator_get_voltage(st->vref);
> -		if (ret < 0)
> -			return dev_err_probe(&spi->dev, ret,
> -					     "Device tree error, Vref voltage undefined\n");
> +		st->int_vref_mv = ret / MILLI;
>  	}
> -	st->int_vref_mv = ret / 1000;
>  
>  	st->chip_info = spi_get_device_match_data(spi);
>  	indio_dev->name = st->chip_info->name;
> 

Re: [PATCH 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[David Lechner]

On 6/1/24 7:48 AM, Jonathan Cameron wrote:
> On Fri, 31 May 2024 16:19:32 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> This makes use of the new devm_regulator_get_enable_read_voltage()
>> function to reduce boilerplate code.
>>
>> Error messages have changed slightly since there are now fewer places
>> where we print an error. The rest of the logic of selecting which
>> supply to use as the reference voltage remains the same.
>>
>> Also 1000 is replaced by MILLI in a few places for consistency.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> Ouch diff didn't like this one much and it is a bit hard to read.
> 
> One case where I think this has an unintended side effect.
> See below.
> 

...

>> @@ -1219,74 +1211,54 @@ static int ad7192_probe(struct spi_device *spi)
>>  	 * Newer firmware should provide a zero volt fixed supply if wired to
>>  	 * ground.
>>  	 */
>> -	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
>> -	if (IS_ERR(aincom)) {
>> -		if (PTR_ERR(aincom) != -ENODEV)
>> -			return dev_err_probe(&spi->dev, PTR_ERR(aincom),
>> -					     "Failed to get AINCOM supply\n");
>> -
>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
>> +	if (ret == -ENODEV)
>>  		st->aincom_mv = 0;
>> -	} else {
>> -		ret = regulator_enable(aincom);
>> -		if (ret)
>> -			return dev_err_probe(&spi->dev, ret,
>> -					     "Failed to enable specified AINCOM supply\n");
>> +	else if (ret < 0)
>> +		return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
>> +	else
>> +		st->aincom_mv = ret / MILLI;
>>  
>> -		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
>> +	/* AVDD can optionally be used as reference voltage */
>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
>> +	if (ret == -EINVAL) {
>> +		/*
>> +		 * We get -EINVAL if avdd is a supply with unknown voltage. We
>> +		 * still need to enable it since it is also a power supply.
>> +		 */
>> +		ret = devm_regulator_get_enable(&spi->dev, "avdd");
> 
> What happens if the entry simply isn't there in the DT.
> Previously I think the regulator framework would supply a stub whereas
> the devm_regulator_get_enable_read_voltage() returns -ENODEV so isn't
> caught by the handling and I think it should be.

Ah, yes so:

if (ret == -EINVAL || ret == -ENODEV) {

Re: [PATCH 1/5] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage

[Jonathan Cameron]

On Mon, 3 Jun 2024 08:36:51 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/1/24 7:48 AM, Jonathan Cameron wrote:
> > On Fri, 31 May 2024 16:19:32 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> This makes use of the new devm_regulator_get_enable_read_voltage()
> >> function to reduce boilerplate code.
> >>
> >> Error messages have changed slightly since there are now fewer places
> >> where we print an error. The rest of the logic of selecting which
> >> supply to use as the reference voltage remains the same.
> >>
> >> Also 1000 is replaced by MILLI in a few places for consistency.
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>  
> > Ouch diff didn't like this one much and it is a bit hard to read.
> > 
> > One case where I think this has an unintended side effect.
> > See below.
> >   
> 
> ...
> 
> >> @@ -1219,74 +1211,54 @@ static int ad7192_probe(struct spi_device *spi)
> >>  	 * Newer firmware should provide a zero volt fixed supply if wired to
> >>  	 * ground.
> >>  	 */
> >> -	aincom = devm_regulator_get_optional(&spi->dev, "aincom");
> >> -	if (IS_ERR(aincom)) {
> >> -		if (PTR_ERR(aincom) != -ENODEV)
> >> -			return dev_err_probe(&spi->dev, PTR_ERR(aincom),
> >> -					     "Failed to get AINCOM supply\n");
> >> -
> >> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
> >> +	if (ret == -ENODEV)
> >>  		st->aincom_mv = 0;
> >> -	} else {
> >> -		ret = regulator_enable(aincom);
> >> -		if (ret)
> >> -			return dev_err_probe(&spi->dev, ret,
> >> -					     "Failed to enable specified AINCOM supply\n");
> >> +	else if (ret < 0)
> >> +		return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
> >> +	else
> >> +		st->aincom_mv = ret / MILLI;
> >>  
> >> -		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
> >> +	/* AVDD can optionally be used as reference voltage */
> >> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
> >> +	if (ret == -EINVAL) {
> >> +		/*
> >> +		 * We get -EINVAL if avdd is a supply with unknown voltage. We
> >> +		 * still need to enable it since it is also a power supply.
> >> +		 */
> >> +		ret = devm_regulator_get_enable(&spi->dev, "avdd");  
> > 
> > What happens if the entry simply isn't there in the DT.
> > Previously I think the regulator framework would supply a stub whereas
> > the devm_regulator_get_enable_read_voltage() returns -ENODEV so isn't
> > caught by the handling and I think it should be.  
> 
> Ah, yes so:
> 
> if (ret == -EINVAL || ret == -ENODEV) {
> 
Yes I think.

[PATCH 2/5] iio: adc: ad7266: use devm_regulator_get_enable_read_voltage

[David Lechner]

This makes use of the new devm_regulator_get_enable_read_voltage()
function to reduce boilerplate code.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7266.c | 37 ++++++++++---------------------------
 1 file changed, 10 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 353a97f9c086..026db1bedc0a 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -25,7 +25,6 @@
 
 struct ad7266_state {
 	struct spi_device	*spi;
-	struct regulator	*reg;
 	unsigned long		vref_mv;
 
 	struct spi_transfer	single_xfer[3];
@@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = {
 	"ad0", "ad1", "ad2",
 };
 
-static void ad7266_reg_disable(void *reg)
-{
-	regulator_disable(reg);
-}
-
 static int ad7266_probe(struct spi_device *spi)
 {
 	struct ad7266_platform_data *pdata = spi->dev.platform_data;
@@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi)
 
 	st = iio_priv(indio_dev);
 
-	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
-	if (!IS_ERR(st->reg)) {
-		ret = regulator_enable(st->reg);
-		if (ret)
-			return ret;
-
-		ret = devm_add_action_or_reset(&spi->dev, ad7266_reg_disable, st->reg);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(st->reg);
-		if (ret < 0)
-			return ret;
-
-		st->vref_mv = ret / 1000;
-	} else {
-		/* Any other error indicates that the regulator does exist */
-		if (PTR_ERR(st->reg) != -ENODEV)
-			return PTR_ERR(st->reg);
-		/* Use internal reference */
+	/*
+	 * Use external reference from vref if present, otherwise use 2.5V
+	 * internal reference.
+	 */
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
+	if (ret == -ENODEV)
 		st->vref_mv = 2500;
-	}
+	else if (ret < 0)
+		return ret;
+	else
+		st->vref_mv = ret / 1000;
 
 	if (pdata) {
 		st->fixed_addr = pdata->fixed_addr;

-- 
2.45.1

Re: [PATCH 2/5] iio: adc: ad7266: use devm_regulator_get_enable_read_voltage

[Nuno Sá]

On Fri, 2024-05-31 at 16:19 -0500, David Lechner wrote:
> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7266.c | 37 ++++++++++---------------------------
>  1 file changed, 10 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> index 353a97f9c086..026db1bedc0a 100644
> --- a/drivers/iio/adc/ad7266.c
> +++ b/drivers/iio/adc/ad7266.c
> @@ -25,7 +25,6 @@
>  
>  struct ad7266_state {
>  	struct spi_device	*spi;
> -	struct regulator	*reg;
>  	unsigned long		vref_mv;
>  
>  	struct spi_transfer	single_xfer[3];
> @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = {
>  	"ad0", "ad1", "ad2",
>  };
>  
> -static void ad7266_reg_disable(void *reg)
> -{
> -	regulator_disable(reg);
> -}
> -
>  static int ad7266_probe(struct spi_device *spi)
>  {
>  	struct ad7266_platform_data *pdata = spi->dev.platform_data;
> @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi)
>  
>  	st = iio_priv(indio_dev);
>  
> -	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
> -	if (!IS_ERR(st->reg)) {
> -		ret = regulator_enable(st->reg);
> -		if (ret)
> -			return ret;
> -
> -		ret = devm_add_action_or_reset(&spi->dev, ad7266_reg_disable, st-
> >reg);
> -		if (ret)
> -			return ret;
> -
> -		ret = regulator_get_voltage(st->reg);
> -		if (ret < 0)
> -			return ret;
> -
> -		st->vref_mv = ret / 1000;
> -	} else {
> -		/* Any other error indicates that the regulator does exist */
> -		if (PTR_ERR(st->reg) != -ENODEV)
> -			return PTR_ERR(st->reg);
> -		/* Use internal reference */
> +	/*
> +	 * Use external reference from vref if present, otherwise use 2.5V
> +	 * internal reference.
> +	 */

Not sure the comment brings any added value. The code is fairly self explanatory
IMO...

> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> +	if (ret == -ENODEV)
>  		st->vref_mv = 2500;
> -	}
> +	else if (ret < 0)
> +		return ret;
> +	else

I think it would be better (as that is the typical pattern) to first check for
errors. Also the 'return' in the middle of the else if () is a bit weird to me...
Maybe something like this?

if (ret < 0 && ret != -ENODEV)
	return ret;
if (ret == -ENODEV)
	st->vref_mv = 2500;
else
	st->vref_mv = ret / 1000;

or even replacing the if() else by
st->vref_mv = ret == -ENODEV ? 2500 : ret / 1000;

- Nuno Sá

Re: [PATCH 2/5] iio: adc: ad7266: use devm_regulator_get_enable_read_voltage

[David Lechner]

On 6/4/24 6:19 AM, Nuno Sá wrote:
> On Fri, 2024-05-31 at 16:19 -0500, David Lechner wrote:
>> This makes use of the new devm_regulator_get_enable_read_voltage()
>> function to reduce boilerplate code.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/iio/adc/ad7266.c | 37 ++++++++++---------------------------
>>  1 file changed, 10 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
>> index 353a97f9c086..026db1bedc0a 100644
>> --- a/drivers/iio/adc/ad7266.c
>> +++ b/drivers/iio/adc/ad7266.c
>> @@ -25,7 +25,6 @@
>>  
>>  struct ad7266_state {
>>  	struct spi_device	*spi;
>> -	struct regulator	*reg;
>>  	unsigned long		vref_mv;
>>  
>>  	struct spi_transfer	single_xfer[3];
>> @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = {
>>  	"ad0", "ad1", "ad2",
>>  };
>>  
>> -static void ad7266_reg_disable(void *reg)
>> -{
>> -	regulator_disable(reg);
>> -}
>> -
>>  static int ad7266_probe(struct spi_device *spi)
>>  {
>>  	struct ad7266_platform_data *pdata = spi->dev.platform_data;
>> @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi)
>>  
>>  	st = iio_priv(indio_dev);
>>  
>> -	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
>> -	if (!IS_ERR(st->reg)) {
>> -		ret = regulator_enable(st->reg);
>> -		if (ret)
>> -			return ret;
>> -
>> -		ret = devm_add_action_or_reset(&spi->dev, ad7266_reg_disable, st-
>>> reg);
>> -		if (ret)
>> -			return ret;
>> -
>> -		ret = regulator_get_voltage(st->reg);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		st->vref_mv = ret / 1000;
>> -	} else {
>> -		/* Any other error indicates that the regulator does exist */
>> -		if (PTR_ERR(st->reg) != -ENODEV)
>> -			return PTR_ERR(st->reg);
>> -		/* Use internal reference */
>> +	/*
>> +	 * Use external reference from vref if present, otherwise use 2.5V
>> +	 * internal reference.
>> +	 */
> 
> Not sure the comment brings any added value. The code is fairly self explanatory
> IMO...

Well, you do this every day. :-)

For someone who never wrote an IIO driver, it could be helpful.

> 
>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
>> +	if (ret == -ENODEV)
>>  		st->vref_mv = 2500;
>> -	}
>> +	else if (ret < 0)
>> +		return ret;
>> +	else
> 
> I think it would be better (as that is the typical pattern) to first check for
> errors. Also the 'return' in the middle of the else if () is a bit weird to me...
> Maybe something like this?
> 
> if (ret < 0 && ret != -ENODEV)
> 	return ret;
> if (ret == -ENODEV)
> 	st->vref_mv = 2500;
> else
> 	st->vref_mv = ret / 1000;
> 
> or even replacing the if() else by
> st->vref_mv = ret == -ENODEV ? 2500 : ret / 1000;
> 
> - Nuno Sá
> 

I think I like that better too.

Re: [PATCH 2/5] iio: adc: ad7266: use devm_regulator_get_enable_read_voltage

[Nuno Sá]

On Tue, 2024-06-04 at 09:10 -0500, David Lechner wrote:
> On 6/4/24 6:19 AM, Nuno Sá wrote:
> > On Fri, 2024-05-31 at 16:19 -0500, David Lechner wrote:
> > > This makes use of the new devm_regulator_get_enable_read_voltage()
> > > function to reduce boilerplate code.
> > > 
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >  drivers/iio/adc/ad7266.c | 37 ++++++++++---------------------------
> > >  1 file changed, 10 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
> > > index 353a97f9c086..026db1bedc0a 100644
> > > --- a/drivers/iio/adc/ad7266.c
> > > +++ b/drivers/iio/adc/ad7266.c
> > > @@ -25,7 +25,6 @@
> > >  
> > >  struct ad7266_state {
> > >  	struct spi_device	*spi;
> > > -	struct regulator	*reg;
> > >  	unsigned long		vref_mv;
> > >  
> > >  	struct spi_transfer	single_xfer[3];
> > > @@ -377,11 +376,6 @@ static const char * const ad7266_gpio_labels[] = {
> > >  	"ad0", "ad1", "ad2",
> > >  };
> > >  
> > > -static void ad7266_reg_disable(void *reg)
> > > -{
> > > -	regulator_disable(reg);
> > > -}
> > > -
> > >  static int ad7266_probe(struct spi_device *spi)
> > >  {
> > >  	struct ad7266_platform_data *pdata = spi->dev.platform_data;
> > > @@ -396,28 +390,17 @@ static int ad7266_probe(struct spi_device *spi)
> > >  
> > >  	st = iio_priv(indio_dev);
> > >  
> > > -	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
> > > -	if (!IS_ERR(st->reg)) {
> > > -		ret = regulator_enable(st->reg);
> > > -		if (ret)
> > > -			return ret;
> > > -
> > > -		ret = devm_add_action_or_reset(&spi->dev,
> > > ad7266_reg_disable, st-
> > > > reg);
> > > -		if (ret)
> > > -			return ret;
> > > -
> > > -		ret = regulator_get_voltage(st->reg);
> > > -		if (ret < 0)
> > > -			return ret;
> > > -
> > > -		st->vref_mv = ret / 1000;
> > > -	} else {
> > > -		/* Any other error indicates that the regulator does
> > > exist */
> > > -		if (PTR_ERR(st->reg) != -ENODEV)
> > > -			return PTR_ERR(st->reg);
> > > -		/* Use internal reference */
> > > +	/*
> > > +	 * Use external reference from vref if present, otherwise use
> > > 2.5V
> > > +	 * internal reference.
> > > +	 */
> > 
> > Not sure the comment brings any added value. The code is fairly self
> > explanatory
> > IMO...
> 
> Well, you do this every day. :-)

I guess... still not an excuse :)

> 
> For someone who never wrote an IIO driver, it could be helpful.

Still dunno but no strong feelings anyways...

- Nuno Sá

[PATCH 3/5] iio: adc: ad7292: use devm_regulator_get_enable_read_voltage

[David Lechner]

This makes use of the new devm_regulator_get_enable_read_voltage()
function to reduce boilerplate code.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7292.c | 40 ++++++++++------------------------------
 1 file changed, 10 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
index 6aadd14f459d..aad4b782a3d2 100644
--- a/drivers/iio/adc/ad7292.c
+++ b/drivers/iio/adc/ad7292.c
@@ -79,7 +79,6 @@ static const struct iio_chan_spec ad7292_channels_diff[] = {
 
 struct ad7292_state {
 	struct spi_device *spi;
-	struct regulator *reg;
 	unsigned short vref_mv;
 
 	__be16 d16 __aligned(IIO_DMA_MINALIGN);
@@ -250,13 +249,6 @@ static const struct iio_info ad7292_info = {
 	.read_raw = ad7292_read_raw,
 };
 
-static void ad7292_regulator_disable(void *data)
-{
-	struct ad7292_state *st = data;
-
-	regulator_disable(st->reg);
-}
-
 static int ad7292_probe(struct spi_device *spi)
 {
 	struct ad7292_state *st;
@@ -277,29 +269,17 @@ static int ad7292_probe(struct spi_device *spi)
 		return -EINVAL;
 	}
 
-	st->reg = devm_regulator_get_optional(&spi->dev, "vref");
-	if (!IS_ERR(st->reg)) {
-		ret = regulator_enable(st->reg);
-		if (ret) {
-			dev_err(&spi->dev,
-				"Failed to enable external vref supply\n");
-			return ret;
-		}
-
-		ret = devm_add_action_or_reset(&spi->dev,
-					       ad7292_regulator_disable, st);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(st->reg);
-		if (ret < 0)
-			return ret;
-
-		st->vref_mv = ret / 1000;
-	} else {
-		/* Use the internal voltage reference. */
+	/*
+	 * Use external reference from vref if present, otherwise use 1.25V
+	 * internal reference.
+	 */
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
+	if (ret == -ENODEV)
 		st->vref_mv = 1250;
-	}
+	else if (ret < 0)
+		return ret;
+	else
+		st->vref_mv = ret / 1000;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;

-- 
2.45.1

Re: [PATCH 3/5] iio: adc: ad7292: use devm_regulator_get_enable_read_voltage

[Marcelo Schmitt]

LGTM

Thanks,
Marcelo

On 05/31, David Lechner wrote:
> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
> 
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7292.c | 40 ++++++++++------------------------------
>  1 file changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
> index 6aadd14f459d..aad4b782a3d2 100644
> --- a/drivers/iio/adc/ad7292.c
> +++ b/drivers/iio/adc/ad7292.c

[PATCH 4/5] iio: adc: ad7793: use devm_regulator_get_enable_read_voltage

[David Lechner]

This makes use of the new devm_regulator_get_enable_read_voltage()
function to reduce boilerplate code.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7793.c | 24 +++---------------------
 1 file changed, 3 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index 5f8cb9aaac70..d4ad7e0b515a 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -152,7 +152,6 @@ struct ad7793_chip_info {
 
 struct ad7793_state {
 	const struct ad7793_chip_info	*chip_info;
-	struct regulator		*reg;
 	u16				int_vref_mv;
 	u16				mode;
 	u16				conf;
@@ -769,11 +768,6 @@ static const struct ad7793_chip_info ad7793_chip_info_tbl[] = {
 	},
 };
 
-static void ad7793_reg_disable(void *reg)
-{
-	regulator_disable(reg);
-}
-
 static int ad7793_probe(struct spi_device *spi)
 {
 	const struct ad7793_platform_data *pdata = spi->dev.platform_data;
@@ -800,23 +794,11 @@ static int ad7793_probe(struct spi_device *spi)
 	ad_sd_init(&st->sd, indio_dev, spi, &ad7793_sigma_delta_info);
 
 	if (pdata->refsel != AD7793_REFSEL_INTERNAL) {
-		st->reg = devm_regulator_get(&spi->dev, "refin");
-		if (IS_ERR(st->reg))
-			return PTR_ERR(st->reg);
-
-		ret = regulator_enable(st->reg);
-		if (ret)
-			return ret;
-
-		ret = devm_add_action_or_reset(&spi->dev, ad7793_reg_disable, st->reg);
-		if (ret)
+		ret = devm_regulator_get_enable_read_voltage(&spi->dev, "refin");
+		if (ret < 0)
 			return ret;
 
-		vref_mv = regulator_get_voltage(st->reg);
-		if (vref_mv < 0)
-			return vref_mv;
-
-		vref_mv /= 1000;
+		vref_mv = ret / 1000;
 	} else {
 		vref_mv = 1170; /* Build-in ref */
 	}

-- 
2.45.1

[PATCH 5/5] iio: adc: ad7944: use devm_regulator_get_enable_read_voltage

[David Lechner]

This makes use of the new devm_regulator_get_enable_read_voltage()
function to reduce boilerplate code.

Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7944.c | 62 +++++++++++++++---------------------------------
 1 file changed, 19 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
index e2cb64cef476..42bbcb904778 100644
--- a/drivers/iio/adc/ad7944.c
+++ b/drivers/iio/adc/ad7944.c
@@ -464,23 +464,16 @@ static const char * const ad7944_power_supplies[] = {
 	"avdd",	"dvdd",	"bvdd", "vio"
 };
 
-static void ad7944_ref_disable(void *ref)
-{
-	regulator_disable(ref);
-}
-
 static int ad7944_probe(struct spi_device *spi)
 {
 	const struct ad7944_chip_info *chip_info;
 	struct device *dev = &spi->dev;
 	struct iio_dev *indio_dev;
 	struct ad7944_adc *adc;
-	bool have_refin = false;
-	struct regulator *ref;
 	struct iio_chan_spec *chain_chan;
 	unsigned long *chain_scan_masks;
 	u32 n_chain_dev;
-	int ret;
+	int ret, ref_mv, refin_mv;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
 	if (!indio_dev)
@@ -531,47 +524,30 @@ static int ad7944_probe(struct spi_device *spi)
 	 * - external reference: REF is connected, REFIN is not connected
 	 */
 
-	ref = devm_regulator_get_optional(dev, "ref");
-	if (IS_ERR(ref)) {
-		if (PTR_ERR(ref) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(ref),
-					     "failed to get REF supply\n");
-
-		ref = NULL;
-	}
+	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (ret == -ENODEV)
+		ref_mv = 0;
+	else if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
+	else
+		ref_mv = ret / 1000;
 
-	ret = devm_regulator_get_enable_optional(dev, "refin");
-	if (ret == 0)
-		have_refin = true;
-	else if (ret != -ENODEV)
-		return dev_err_probe(dev, ret,
-				     "failed to get and enable REFIN supply\n");
+	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
+	if (ret == -ENODEV)
+		refin_mv = 0;
+	else if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
+	else
+		refin_mv = ret / 1000;
 
-	if (have_refin && ref)
+	if (ref_mv && refin_mv)
 		return dev_err_probe(dev, -EINVAL,
 				     "cannot have both refin and ref supplies\n");
 
-	if (ref) {
-		ret = regulator_enable(ref);
-		if (ret)
-			return dev_err_probe(dev, ret,
-					     "failed to enable REF supply\n");
-
-		ret = devm_add_action_or_reset(dev, ad7944_ref_disable, ref);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(ref);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "failed to get REF voltage\n");
-
-		/* external reference */
-		adc->ref_mv = ret / 1000;
-	} else {
-		/* internal reference */
+	if (ref_mv)
+		adc->ref_mv = ref_mv;
+	else
 		adc->ref_mv = AD7944_INTERNAL_REF_MV;
-	}
 
 	adc->cnv = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
 	if (IS_ERR(adc->cnv))

-- 
2.45.1

Re: [PATCH 5/5] iio: adc: ad7944: use devm_regulator_get_enable_read_voltage

[Jonathan Cameron]

On Fri, 31 May 2024 16:19:36 -0500
David Lechner <dlechner@baylibre.com> wrote:

> This makes use of the new devm_regulator_get_enable_read_voltage()
> function to reduce boilerplate code.
> 
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A possible corner case inline.

Patches 2-4 lgtm.
> ---
>  drivers/iio/adc/ad7944.c | 62 +++++++++++++++---------------------------------
>  1 file changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index e2cb64cef476..42bbcb904778 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -464,23 +464,16 @@ static const char * const ad7944_power_supplies[] = {
>  	"avdd",	"dvdd",	"bvdd", "vio"
>  };
>  
> -static void ad7944_ref_disable(void *ref)
> -{
> -	regulator_disable(ref);
> -}
> -
>  static int ad7944_probe(struct spi_device *spi)
>  {
>  	const struct ad7944_chip_info *chip_info;
>  	struct device *dev = &spi->dev;
>  	struct iio_dev *indio_dev;
>  	struct ad7944_adc *adc;
> -	bool have_refin = false;
> -	struct regulator *ref;
>  	struct iio_chan_spec *chain_chan;
>  	unsigned long *chain_scan_masks;
>  	u32 n_chain_dev;
> -	int ret;
> +	int ret, ref_mv, refin_mv;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>  	if (!indio_dev)
> @@ -531,47 +524,30 @@ static int ad7944_probe(struct spi_device *spi)
>  	 * - external reference: REF is connected, REFIN is not connected
>  	 */
>  
> -	ref = devm_regulator_get_optional(dev, "ref");
> -	if (IS_ERR(ref)) {
> -		if (PTR_ERR(ref) != -ENODEV)
> -			return dev_err_probe(dev, PTR_ERR(ref),
> -					     "failed to get REF supply\n");
> -
> -		ref = NULL;
> -	}
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (ret == -ENODEV)
> +		ref_mv = 0;
> +	else if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
> +	else
> +		ref_mv = ret / 1000;
>  
> -	ret = devm_regulator_get_enable_optional(dev, "refin");
> -	if (ret == 0)
> -		have_refin = true;
> -	else if (ret != -ENODEV)
> -		return dev_err_probe(dev, ret,
> -				     "failed to get and enable REFIN supply\n");
> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> +	if (ret == -ENODEV)
> +		refin_mv = 0;
> +	else if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
> +	else
> +		refin_mv = ret / 1000;
How does refin_mv get used?  Previously we never queried it's voltage (I assume
because it supplies an internal reference?

Are there any regulators that are real enough to enable but for which a voltage
can't be queried?  I think fixed regulators with gpio control are in this
category...

>  
> -	if (have_refin && ref)
> +	if (ref_mv && refin_mv)
>  		return dev_err_probe(dev, -EINVAL,
>  				     "cannot have both refin and ref supplies\n");
>  
> -	if (ref) {
> -		ret = regulator_enable(ref);
> -		if (ret)
> -			return dev_err_probe(dev, ret,
> -					     "failed to enable REF supply\n");
> -
> -		ret = devm_add_action_or_reset(dev, ad7944_ref_disable, ref);
> -		if (ret)
> -			return ret;
> -
> -		ret = regulator_get_voltage(ref);
> -		if (ret < 0)
> -			return dev_err_probe(dev, ret,
> -					     "failed to get REF voltage\n");
> -
> -		/* external reference */
> -		adc->ref_mv = ret / 1000;
> -	} else {
> -		/* internal reference */
> +	if (ref_mv)
> +		adc->ref_mv = ref_mv;
> +	else
>  		adc->ref_mv = AD7944_INTERNAL_REF_MV;
> -	}
>  
>  	adc->cnv = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
>  	if (IS_ERR(adc->cnv))
> 

Re: [PATCH 5/5] iio: adc: ad7944: use devm_regulator_get_enable_read_voltage

[David Lechner]

On 6/1/24 8:01 AM, Jonathan Cameron wrote:
> On Fri, 31 May 2024 16:19:36 -0500
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> This makes use of the new devm_regulator_get_enable_read_voltage()
>> function to reduce boilerplate code.
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
> A possible corner case inline.
> 
> Patches 2-4 lgtm.
>> ---
>>  drivers/iio/adc/ad7944.c | 62 +++++++++++++++---------------------------------
>>  1 file changed, 19 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
>> index e2cb64cef476..42bbcb904778 100644
>> --- a/drivers/iio/adc/ad7944.c
>> +++ b/drivers/iio/adc/ad7944.c
>> @@ -464,23 +464,16 @@ static const char * const ad7944_power_supplies[] = {
>>  	"avdd",	"dvdd",	"bvdd", "vio"
>>  };
>>  
>> -static void ad7944_ref_disable(void *ref)
>> -{
>> -	regulator_disable(ref);
>> -}
>> -
>>  static int ad7944_probe(struct spi_device *spi)
>>  {
>>  	const struct ad7944_chip_info *chip_info;
>>  	struct device *dev = &spi->dev;
>>  	struct iio_dev *indio_dev;
>>  	struct ad7944_adc *adc;
>> -	bool have_refin = false;
>> -	struct regulator *ref;
>>  	struct iio_chan_spec *chain_chan;
>>  	unsigned long *chain_scan_masks;
>>  	u32 n_chain_dev;
>> -	int ret;
>> +	int ret, ref_mv, refin_mv;
>>  
>>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>>  	if (!indio_dev)
>> @@ -531,47 +524,30 @@ static int ad7944_probe(struct spi_device *spi)
>>  	 * - external reference: REF is connected, REFIN is not connected
>>  	 */
>>  
>> -	ref = devm_regulator_get_optional(dev, "ref");
>> -	if (IS_ERR(ref)) {
>> -		if (PTR_ERR(ref) != -ENODEV)
>> -			return dev_err_probe(dev, PTR_ERR(ref),
>> -					     "failed to get REF supply\n");
>> -
>> -		ref = NULL;
>> -	}
>> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
>> +	if (ret == -ENODEV)
>> +		ref_mv = 0;
>> +	else if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
>> +	else
>> +		ref_mv = ret / 1000;
>>  
>> -	ret = devm_regulator_get_enable_optional(dev, "refin");
>> -	if (ret == 0)
>> -		have_refin = true;
>> -	else if (ret != -ENODEV)
>> -		return dev_err_probe(dev, ret,
>> -				     "failed to get and enable REFIN supply\n");
>> +	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
>> +	if (ret == -ENODEV)
>> +		refin_mv = 0;
>> +	else if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
>> +	else
>> +		refin_mv = ret / 1000;
> How does refin_mv get used?  Previously we never queried it's voltage (I assume
> because it supplies an internal reference?
> 
> Are there any regulators that are real enough to enable but for which a voltage
> can't be queried?  I think fixed regulators with gpio control are in this
> category...
> 

Hmm... don't remember why I did it that way (was a while ago). I will bring
back the have_refin flag instead.