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

[PATCH v2 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

---
Changes in v2:
- avoid else in return value checks
- fix unintentional changes in behavior
- use macros instead of comments for internal reference voltage
- Link to v1: https://lore.kernel.org/r/20240531-iio-adc-ref-supply-refactor-v1-0-4b313c0615ad@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 | 100 +++++++++++++++++------------------------------
 drivers/iio/adc/ad7266.c |  33 +++-------------
 drivers/iio/adc/ad7292.c |  36 +++--------------
 drivers/iio/adc/ad7793.c |  24 ++----------
 drivers/iio/adc/ad7944.c |  54 ++++++-------------------
 5 files changed, 62 insertions(+), 185 deletions(-)
---
base-commit: cc1ce839526a65620778617da0b022bd88e8a139
change-id: 20240531-iio-adc-ref-supply-refactor-93f962d40c23

[PATCH v2 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>
---

v2 changes:
- avoid using else on return value check of "aincom" regulator
- also fall back to dummy regulator on "avdd" in case of ENODEV
---
 drivers/iio/adc/ad7192.c | 100 +++++++++++++++++------------------------------
 1 file changed, 35 insertions(+), 65 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index 0789121236d6..409c73d16460 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,52 @@ 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");
-
-		st->aincom_mv = 0;
-	} else {
-		ret = regulator_enable(aincom);
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "aincom");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(&spi->dev, ret, "Failed to get AINCOM voltage\n");
+
+	st->aincom_mv = ret == -ENODEV ? 0 : ret / MILLI;
+
+	/* AVDD can optionally be used as reference voltage */
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "avdd");
+	if (ret == -ENODEV || 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 dev_err_probe(&spi->dev, ret,
-					     "Failed to enable specified AINCOM supply\n");
+					     "Failed to enable AVDD supply\n");
 
-		ret = devm_add_action_or_reset(&spi->dev, ad7192_reg_disable, aincom);
-		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;
+		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;
 	}
 
-	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;
-	}
-
-	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.2

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

[Jonathan Cameron]

On Wed, 12 Jun 2024 16:03:05 -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>

Complicated bit of code, but seems correct.
However, it crossed with Alisa-Dariana switching adding a
struct device *dev = &spi->dev to probe() that I picked up earlier
today.

I could unwind that but given Alisa-Dariana has a number of
other patches on this driver in flight, I'd like the two of you
to work out the best resolution between you.  Maybe easiest option
is that Alisa-Dariana sends this a first patch of the next
series I should pick up.

Thanks,

Jonathan

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

[Alisa-Dariana Roman]

On 15.06.2024 15:08, Jonathan Cameron wrote:
> On Wed, 12 Jun 2024 16:03:05 -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>
> 
> Complicated bit of code, but seems correct.
> However, it crossed with Alisa-Dariana switching adding a
> struct device *dev = &spi->dev to probe() that I picked up earlier
> today.
> 
> I could unwind that but given Alisa-Dariana has a number of
> other patches on this driver in flight, I'd like the two of you
> to work out the best resolution between you.  Maybe easiest option
> is that Alisa-Dariana sends this a first patch of the next
> series I should pick up.
> 
> Thanks,
> 
> Jonathan
I will add this patch to my series and send it shortly.

Kind regards,
Alisa-Dariana Roman.

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

[David Lechner]

On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> On 15.06.2024 15:08, Jonathan Cameron wrote:
> > On Wed, 12 Jun 2024 16:03:05 -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>
> >
> > Complicated bit of code, but seems correct.
> > However, it crossed with Alisa-Dariana switching adding a
> > struct device *dev = &spi->dev to probe() that I picked up earlier
> > today.
> >
> > I could unwind that but given Alisa-Dariana has a number of
> > other patches on this driver in flight, I'd like the two of you
> > to work out the best resolution between you.  Maybe easiest option
> > is that Alisa-Dariana sends this a first patch of the next
> > series I should pick up.
> >
> > Thanks,
> >
> > Jonathan
> I will add this patch to my series and send it shortly.
>
> Kind regards,
> Alisa-Dariana Roman.

Great, thanks!

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

[Alisa-Dariana Roman]

On 17.06.2024 16:22, David Lechner wrote:
> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
>>
>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>> On Wed, 12 Jun 2024 16:03:05 -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>
>>>
>>> Complicated bit of code, but seems correct.
>>> However, it crossed with Alisa-Dariana switching adding a
>>> struct device *dev = &spi->dev to probe() that I picked up earlier
>>> today.
>>>
>>> I could unwind that but given Alisa-Dariana has a number of
>>> other patches on this driver in flight, I'd like the two of you
>>> to work out the best resolution between you.  Maybe easiest option
>>> is that Alisa-Dariana sends this a first patch of the next
>>> series I should pick up.
>>>
>>> Thanks,
>>>
>>> Jonathan
>> I will add this patch to my series and send it shortly.
>>
>> Kind regards,
>> Alisa-Dariana Roman.
> 
> Great, thanks!

Just one quick question:

I am getting two such warnings when running the checkpatch script:

WARNING: else is not generally useful after a break or return
#1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
+		return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
+	} else {

Should I switch the last two branches to get rid of the warnings or just 
ignore them?

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

[David Lechner]

On 6/17/24 8:38 AM, Alisa-Dariana Roman wrote:
> On 17.06.2024 16:22, David Lechner wrote:
>> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
>> <alisadariana@gmail.com> wrote:
>>>
>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>>> On Wed, 12 Jun 2024 16:03:05 -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>
>>>>
>>>> Complicated bit of code, but seems correct.
>>>> However, it crossed with Alisa-Dariana switching adding a
>>>> struct device *dev = &spi->dev to probe() that I picked up earlier
>>>> today.
>>>>
>>>> I could unwind that but given Alisa-Dariana has a number of
>>>> other patches on this driver in flight, I'd like the two of you
>>>> to work out the best resolution between you.  Maybe easiest option
>>>> is that Alisa-Dariana sends this a first patch of the next
>>>> series I should pick up.
>>>>
>>>> Thanks,
>>>>
>>>> Jonathan
>>> I will add this patch to my series and send it shortly.
>>>
>>> Kind regards,
>>> Alisa-Dariana Roman.
>>
>> Great, thanks!
> 
> Just one quick question:
> 
> I am getting two such warnings when running the checkpatch script:
> 
> WARNING: else is not generally useful after a break or return
> #1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
> +    } else {
> 
> Should I switch the last two branches to get rid of the warnings or just ignore them?
> 

In the other patches, I was able to reorder things to avoid this
warning, but since this one was more complicated, I just ignored
this warning.

We can't just remove the else in this case because the return
is inside of an `else if`.

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

[Alisa-Dariana Roman]

On 17.06.2024 16:48, David Lechner wrote:
> On 6/17/24 8:38 AM, Alisa-Dariana Roman wrote:
>> On 17.06.2024 16:22, David Lechner wrote:
>>> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
>>> <alisadariana@gmail.com> wrote:
>>>>
>>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>>>> On Wed, 12 Jun 2024 16:03:05 -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>
>>>>>
>>>>> Complicated bit of code, but seems correct.
>>>>> However, it crossed with Alisa-Dariana switching adding a
>>>>> struct device *dev = &spi->dev to probe() that I picked up earlier
>>>>> today.
>>>>>
>>>>> I could unwind that but given Alisa-Dariana has a number of
>>>>> other patches on this driver in flight, I'd like the two of you
>>>>> to work out the best resolution between you.  Maybe easiest option
>>>>> is that Alisa-Dariana sends this a first patch of the next
>>>>> series I should pick up.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Jonathan
>>>> I will add this patch to my series and send it shortly.
>>>>
>>>> Kind regards,
>>>> Alisa-Dariana Roman.
>>>
>>> Great, thanks!
>>
>> Just one quick question:
>>
>> I am getting two such warnings when running the checkpatch script:
>>
>> WARNING: else is not generally useful after a break or return
>> #1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
>> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>> +    } else {
>>
>> Should I switch the last two branches to get rid of the warnings or just ignore them?
>>
> 
> In the other patches, I was able to reorder things to avoid this
> warning, but since this one was more complicated, I just ignored
> this warning.
> 
> We can't just remove the else in this case because the return
> is inside of an `else if`.

	/* AVDD can optionally be used as reference voltage */
	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
	if (ret == -ENODEV || 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(dev, "avdd");
		if (ret)
			return dev_err_probe(dev, ret,
					     "Failed to enable AVDD supply\n");

		avdd_mv = 0;
	} else if (ret >= 0) {
		avdd_mv = ret / MILLI;
	} else {
		return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
	}

Would switching the last two branches, in order to get rid of the 
warnings, make the code harder to understand?

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

[David Lechner]

On Mon, Jun 17, 2024 at 9:10 AM Alisa-Dariana Roman
<alisadariana@gmail.com> wrote:
>
> On 17.06.2024 16:48, David Lechner wrote:
> > On 6/17/24 8:38 AM, Alisa-Dariana Roman wrote:
> >> On 17.06.2024 16:22, David Lechner wrote:
> >>> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
> >>> <alisadariana@gmail.com> wrote:
> >>>>
> >>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
> >>>>> On Wed, 12 Jun 2024 16:03:05 -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>
> >>>>>
> >>>>> Complicated bit of code, but seems correct.
> >>>>> However, it crossed with Alisa-Dariana switching adding a
> >>>>> struct device *dev = &spi->dev to probe() that I picked up earlier
> >>>>> today.
> >>>>>
> >>>>> I could unwind that but given Alisa-Dariana has a number of
> >>>>> other patches on this driver in flight, I'd like the two of you
> >>>>> to work out the best resolution between you.  Maybe easiest option
> >>>>> is that Alisa-Dariana sends this a first patch of the next
> >>>>> series I should pick up.
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Jonathan
> >>>> I will add this patch to my series and send it shortly.
> >>>>
> >>>> Kind regards,
> >>>> Alisa-Dariana Roman.
> >>>
> >>> Great, thanks!
> >>
> >> Just one quick question:
> >>
> >> I am getting two such warnings when running the checkpatch script:
> >>
> >> WARNING: else is not generally useful after a break or return
> >> #1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
> >> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
> >> +    } else {
> >>
> >> Should I switch the last two branches to get rid of the warnings or just ignore them?
> >>
> >
> > In the other patches, I was able to reorder things to avoid this
> > warning, but since this one was more complicated, I just ignored
> > this warning.
> >
> > We can't just remove the else in this case because the return
> > is inside of an `else if`.
>
>         /* AVDD can optionally be used as reference voltage */
>         ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>         if (ret == -ENODEV || 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(dev, "avdd");
>                 if (ret)
>                         return dev_err_probe(dev, ret,
>                                              "Failed to enable AVDD supply\n");
>
>                 avdd_mv = 0;
>         } else if (ret >= 0) {
>                 avdd_mv = ret / MILLI;
>         } else {
>                 return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>         }
>
> Would switching the last two branches, in order to get rid of the
> warnings, make the code harder to understand?
>

I did it in the other order because usually we like to handle the
error case first.

To make it more like the other patches, we could do something like
this. The only thing i don't like about it is that `ret` on the very
last line could come from two different places. But it is logically
sound in the current form.

    /* AVDD can optionally be used as reference voltage */
    ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
    if (ret == -ENODEV || 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(dev, "avdd");
        if (ret)
            return dev_err_probe(dev, ret,
                         "Failed to enable AVDD supply\n");
    } else if (ret < 0) {
        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
    }

    avdd_mv = ret <= 0 ? 0 : ret / MILLI;

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

[Alisa-Dariana Roman]

On 17.06.2024 18:28, David Lechner wrote:
> On Mon, Jun 17, 2024 at 9:10 AM Alisa-Dariana Roman
> <alisadariana@gmail.com> wrote:
>>
>> On 17.06.2024 16:48, David Lechner wrote:
>>> On 6/17/24 8:38 AM, Alisa-Dariana Roman wrote:
>>>> On 17.06.2024 16:22, David Lechner wrote:
>>>>> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
>>>>> <alisadariana@gmail.com> wrote:
>>>>>>
>>>>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>>>>>> On Wed, 12 Jun 2024 16:03:05 -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>
>>>>>>>
>>>>>>> Complicated bit of code, but seems correct.
>>>>>>> However, it crossed with Alisa-Dariana switching adding a
>>>>>>> struct device *dev = &spi->dev to probe() that I picked up earlier
>>>>>>> today.
>>>>>>>
>>>>>>> I could unwind that but given Alisa-Dariana has a number of
>>>>>>> other patches on this driver in flight, I'd like the two of you
>>>>>>> to work out the best resolution between you.  Maybe easiest option
>>>>>>> is that Alisa-Dariana sends this a first patch of the next
>>>>>>> series I should pick up.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> Jonathan
>>>>>> I will add this patch to my series and send it shortly.
>>>>>>
>>>>>> Kind regards,
>>>>>> Alisa-Dariana Roman.
>>>>>
>>>>> Great, thanks!
>>>>
>>>> Just one quick question:
>>>>
>>>> I am getting two such warnings when running the checkpatch script:
>>>>
>>>> WARNING: else is not generally useful after a break or return
>>>> #1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
>>>> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>>> +    } else {
>>>>
>>>> Should I switch the last two branches to get rid of the warnings or just ignore them?
>>>>
>>>
>>> In the other patches, I was able to reorder things to avoid this
>>> warning, but since this one was more complicated, I just ignored
>>> this warning.
>>>
>>> We can't just remove the else in this case because the return
>>> is inside of an `else if`.
>>
>>          /* AVDD can optionally be used as reference voltage */
>>          ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>>          if (ret == -ENODEV || 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(dev, "avdd");
>>                  if (ret)
>>                          return dev_err_probe(dev, ret,
>>                                               "Failed to enable AVDD supply\n");
>>
>>                  avdd_mv = 0;
>>          } else if (ret >= 0) {
>>                  avdd_mv = ret / MILLI;
>>          } else {
>>                  return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>          }
>>
>> Would switching the last two branches, in order to get rid of the
>> warnings, make the code harder to understand?
>>
> 
> I did it in the other order because usually we like to handle the
> error case first.
> 
> To make it more like the other patches, we could do something like
> this. The only thing i don't like about it is that `ret` on the very
> last line could come from two different places. But it is logically
> sound in the current form.
> 
>      /* AVDD can optionally be used as reference voltage */
>      ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>      if (ret == -ENODEV || 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(dev, "avdd");
>          if (ret)
>              return dev_err_probe(dev, ret,
>                           "Failed to enable AVDD supply\n");
>      } else if (ret < 0) {
>          return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>      }
> 
>      avdd_mv = ret <= 0 ? 0 : ret / MILLI;

Maybe this would make it a bit clearer, but yes, the ret == 0 could 
still come from two different places :(.

avdd_mv = ret == 0 ? 0 : ret / MILLI;

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

[David Lechner]

On 6/17/24 11:01 AM, Alisa-Dariana Roman wrote:
> On 17.06.2024 18:28, David Lechner wrote:
>> On Mon, Jun 17, 2024 at 9:10 AM Alisa-Dariana Roman
>> <alisadariana@gmail.com> wrote:
>>>
>>> On 17.06.2024 16:48, David Lechner wrote:
>>>> On 6/17/24 8:38 AM, Alisa-Dariana Roman wrote:
>>>>> On 17.06.2024 16:22, David Lechner wrote:
>>>>>> On Mon, Jun 17, 2024 at 4:35 AM Alisa-Dariana Roman
>>>>>> <alisadariana@gmail.com> wrote:
>>>>>>>
>>>>>>> On 15.06.2024 15:08, Jonathan Cameron wrote:
>>>>>>>> On Wed, 12 Jun 2024 16:03:05 -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>
>>>>>>>>
>>>>>>>> Complicated bit of code, but seems correct.
>>>>>>>> However, it crossed with Alisa-Dariana switching adding a
>>>>>>>> struct device *dev = &spi->dev to probe() that I picked up earlier
>>>>>>>> today.
>>>>>>>>
>>>>>>>> I could unwind that but given Alisa-Dariana has a number of
>>>>>>>> other patches on this driver in flight, I'd like the two of you
>>>>>>>> to work out the best resolution between you.  Maybe easiest option
>>>>>>>> is that Alisa-Dariana sends this a first patch of the next
>>>>>>>> series I should pick up.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Jonathan
>>>>>>> I will add this patch to my series and send it shortly.
>>>>>>>
>>>>>>> Kind regards,
>>>>>>> Alisa-Dariana Roman.
>>>>>>
>>>>>> Great, thanks!
>>>>>
>>>>> Just one quick question:
>>>>>
>>>>> I am getting two such warnings when running the checkpatch script:
>>>>>
>>>>> WARNING: else is not generally useful after a break or return
>>>>> #1335: FILE: ./drivers/iio/adc/ad7192.c:1335:
>>>>> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>>>> +    } else {
>>>>>
>>>>> Should I switch the last two branches to get rid of the warnings or just ignore them?
>>>>>
>>>>
>>>> In the other patches, I was able to reorder things to avoid this
>>>> warning, but since this one was more complicated, I just ignored
>>>> this warning.
>>>>
>>>> We can't just remove the else in this case because the return
>>>> is inside of an `else if`.
>>>
>>>          /* AVDD can optionally be used as reference voltage */
>>>          ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>>>          if (ret == -ENODEV || 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(dev, "avdd");
>>>                  if (ret)
>>>                          return dev_err_probe(dev, ret,
>>>                                               "Failed to enable AVDD supply\n");
>>>
>>>                  avdd_mv = 0;
>>>          } else if (ret >= 0) {
>>>                  avdd_mv = ret / MILLI;
>>>          } else {
>>>                  return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>>          }
>>>
>>> Would switching the last two branches, in order to get rid of the
>>> warnings, make the code harder to understand?
>>>
>>
>> I did it in the other order because usually we like to handle the
>> error case first.
>>
>> To make it more like the other patches, we could do something like
>> this. The only thing i don't like about it is that `ret` on the very
>> last line could come from two different places. But it is logically
>> sound in the current form.
>>
>>      /* AVDD can optionally be used as reference voltage */
>>      ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
>>      if (ret == -ENODEV || 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(dev, "avdd");
>>          if (ret)
>>              return dev_err_probe(dev, ret,
>>                           "Failed to enable AVDD supply\n");
>>      } else if (ret < 0) {
>>          return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>>      }
>>
>>      avdd_mv = ret <= 0 ? 0 : ret / MILLI;
> 
> Maybe this would make it a bit clearer, but yes, the ret == 0 could still come from two different places :(.
> 
> avdd_mv = ret == 0 ? 0 : ret / MILLI;
> 

We could make a ret2 local variable inside of the if block to avoid writing over ret.

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

[Alisa-Dariana Roman]

On 17.06.2024 23:00, David Lechner wrote:
...

> 
> We could make a ret2 local variable inside of the if block to avoid writing over ret.
> 

Nice! If this looks alright, I will send it along my updated series.

 From f84206b85b04f89d57b9293dd93e017efe8b350c Mon Sep 17 00:00:00 2001
From: David Lechner <dlechner@baylibre.com>
Date: Wed, 12 Jun 2024 16:03:05 -0500
Subject: [PATCH] iio: adc: ad7192: use 
devm_regulator_get_enable_read_voltage

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 | 101 +++++++++++++--------------------------
  1 file changed, 34 insertions(+), 67 deletions(-)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index c7fb51a90e87..2448b01e0d59 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,18 +1187,12 @@ 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 device *dev = &spi->dev;
  	struct ad7192_state *st;
  	struct iio_dev *indio_dev;
-	struct regulator *aincom;
-	int ret;
+	int ret, ret2, avdd_mv;

  	if (!spi->irq)
  		return dev_err_probe(dev, -ENODEV, "Failed to get IRQ\n");
@@ -1218,72 +1210,47 @@ 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(dev, "aincom");
-	if (IS_ERR(aincom)) {
-		if (PTR_ERR(aincom) != -ENODEV)
-			return dev_err_probe(dev, PTR_ERR(aincom),
-					     "Failed to get AINCOM supply\n");
-
-		st->aincom_mv = 0;
-	} else {
-		ret = regulator_enable(aincom);
-		if (ret)
-			return dev_err_probe(dev, ret,
-					     "Failed to enable specified AINCOM supply\n");
-
-		ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(aincom);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Device tree error, AINCOM voltage undefined\n");
-		st->aincom_mv = ret / MILLI;
+	ret = devm_regulator_get_enable_read_voltage(dev, "aincom");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get AINCOM voltage\n");
+
+	st->aincom_mv = ret == -ENODEV ? 0 : ret / MILLI;
+
+	/* AVDD can optionally be used as reference voltage */
+	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
+	if (ret == -ENODEV || 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.
+		 */
+		ret2 = devm_regulator_get_enable(dev, "avdd");
+		if (ret2)
+			return dev_err_probe(dev, ret2,
+					     "Failed to enable AVDD supply\n");
+	} else if (ret < 0) {
+		return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
  	}

-	st->avdd = devm_regulator_get(dev, "avdd");
-	if (IS_ERR(st->avdd))
-		return PTR_ERR(st->avdd);
-
-	ret = regulator_enable(st->avdd);
-	if (ret)
-		return dev_err_probe(dev, ret,
-				     "Failed to enable specified AVdd supply\n");
-
-	ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
-	if (ret)
-		return ret;
+	avdd_mv = (ret == -ENODEV) | (ret == -EINVAL) ? 0 : ret / MILLI;

  	ret = devm_regulator_get_enable(dev, "dvdd");
  	if (ret)
  		return dev_err_probe(dev, ret, "Failed to enable specified DVdd 
supply\n");

-	st->vref = devm_regulator_get_optional(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(dev, ret,
-					     "Device tree error, AVdd voltage undefined\n");
-	} else {
-		ret = regulator_enable(st->vref);
-		if (ret)
-			return dev_err_probe(dev, ret,
-					     "Failed to enable specified Vref supply\n");
-
-		ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->vref);
-		if (ret)
-			return ret;
-
-		ret = regulator_get_voltage(st->vref);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Device tree error, Vref 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(dev, "vref");
+	if (ret == -ENODEV) {
+		if (avdd_mv == 0)
+			return dev_err_probe(dev, -ENODEV,
+					     "No reference voltage available\n");
+	} else if (ret < 0) {
+		return ret;
  	}
-	st->int_vref_mv = ret / 1000;
+
+	st->int_vref_mv = ret == -ENODEV ? avdd_mv : ret / MILLI;

  	st->chip_info = spi_get_device_match_data(spi);
  	indio_dev->name = st->chip_info->name;
-- 
2.34.1

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

[David Lechner]

On 6/18/24 4:45 AM, Alisa-Dariana Roman wrote:
> On 17.06.2024 23:00, David Lechner wrote:
> ...
> 
>>
>> We could make a ret2 local variable inside of the if block to avoid writing over ret.
>>
> 
> Nice! If this looks alright, I will send it along my updated series.
> 
> From f84206b85b04f89d57b9293dd93e017efe8b350c Mon Sep 17 00:00:00 2001
> From: David Lechner <dlechner@baylibre.com>
> Date: Wed, 12 Jun 2024 16:03:05 -0500
> Subject: [PATCH] iio: adc: ad7192: use devm_regulator_get_enable_read_voltage
> 
> 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 | 101 +++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index c7fb51a90e87..2448b01e0d59 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,18 +1187,12 @@ 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 device *dev = &spi->dev;
>      struct ad7192_state *st;
>      struct iio_dev *indio_dev;
> -    struct regulator *aincom;
> -    int ret;
> +    int ret, ret2, avdd_mv;
> 
>      if (!spi->irq)
>          return dev_err_probe(dev, -ENODEV, "Failed to get IRQ\n");
> @@ -1218,72 +1210,47 @@ 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(dev, "aincom");
> -    if (IS_ERR(aincom)) {
> -        if (PTR_ERR(aincom) != -ENODEV)
> -            return dev_err_probe(dev, PTR_ERR(aincom),
> -                         "Failed to get AINCOM supply\n");
> -
> -        st->aincom_mv = 0;
> -    } else {
> -        ret = regulator_enable(aincom);
> -        if (ret)
> -            return dev_err_probe(dev, ret,
> -                         "Failed to enable specified AINCOM supply\n");
> -
> -        ret = devm_add_action_or_reset(dev, ad7192_reg_disable, aincom);
> -        if (ret)
> -            return ret;
> -
> -        ret = regulator_get_voltage(aincom);
> -        if (ret < 0)
> -            return dev_err_probe(dev, ret,
> -                         "Device tree error, AINCOM voltage undefined\n");
> -        st->aincom_mv = ret / MILLI;
> +    ret = devm_regulator_get_enable_read_voltage(dev, "aincom");
> +    if (ret < 0 && ret != -ENODEV)
> +        return dev_err_probe(dev, ret, "Failed to get AINCOM voltage\n");
> +
> +    st->aincom_mv = ret == -ENODEV ? 0 : ret / MILLI;
> +
> +    /* AVDD can optionally be used as reference voltage */
> +    ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> +    if (ret == -ENODEV || ret == -EINVAL) {

               int ret2;

I would declare ret2 here since it is the only place it is used.

> +        /*
> +         * We get -EINVAL if avdd is a supply with unknown voltage. We
> +         * still need to enable it since it is also a power supply.
> +         */
> +        ret2 = devm_regulator_get_enable(dev, "avdd");
> +        if (ret2)
> +            return dev_err_probe(dev, ret2,
> +                         "Failed to enable AVDD supply\n");
> +    } else if (ret < 0) {
> +        return dev_err_probe(dev, ret, "Failed to get AVDD voltage\n");
>      }
> 
> -    st->avdd = devm_regulator_get(dev, "avdd");
> -    if (IS_ERR(st->avdd))
> -        return PTR_ERR(st->avdd);
> -
> -    ret = regulator_enable(st->avdd);
> -    if (ret)
> -        return dev_err_probe(dev, ret,
> -                     "Failed to enable specified AVdd supply\n");
> -
> -    ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->avdd);
> -    if (ret)
> -        return ret;
> +    avdd_mv = (ret == -ENODEV) | (ret == -EINVAL) ? 0 : ret / MILLI;

This could be simplified to ret < 0.

Or if you want to leave it explicit, use || instead of |.

And () aren't really needed either.

> 
>      ret = devm_regulator_get_enable(dev, "dvdd");
>      if (ret)
>          return dev_err_probe(dev, ret, "Failed to enable specified DVdd supply\n");
> 
> -    st->vref = devm_regulator_get_optional(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(dev, ret,
> -                         "Device tree error, AVdd voltage undefined\n");
> -    } else {
> -        ret = regulator_enable(st->vref);
> -        if (ret)
> -            return dev_err_probe(dev, ret,
> -                         "Failed to enable specified Vref supply\n");
> -
> -        ret = devm_add_action_or_reset(dev, ad7192_reg_disable, st->vref);
> -        if (ret)
> -            return ret;
> -
> -        ret = regulator_get_voltage(st->vref);
> -        if (ret < 0)
> -            return dev_err_probe(dev, ret,
> -                         "Device tree error, Vref 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(dev, "vref");
> +    if (ret == -ENODEV) {
> +        if (avdd_mv == 0)
> +            return dev_err_probe(dev, -ENODEV,
> +                         "No reference voltage available\n");
> +    } else if (ret < 0) {
> +        return ret;
>      }
> -    st->int_vref_mv = ret / 1000;
> +
> +    st->int_vref_mv = ret == -ENODEV ? avdd_mv : ret / MILLI;
> 
>      st->chip_info = spi_get_device_match_data(spi);
>      indio_dev->name = st->chip_info->name;

[PATCH v2 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>
---

v2 changes:
- avoid else in return value check
- use macro instead of comment to explain internal reference voltage
---
 drivers/iio/adc/ad7266.c | 33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)

diff --git a/drivers/iio/adc/ad7266.c b/drivers/iio/adc/ad7266.c
index 353a97f9c086..874d2dc34f92 100644
--- a/drivers/iio/adc/ad7266.c
+++ b/drivers/iio/adc/ad7266.c
@@ -23,9 +23,10 @@
 
 #include <linux/platform_data/ad7266.h>
 
+#define AD7266_INTERNAL_REF_MV	2500
+
 struct ad7266_state {
 	struct spi_device	*spi;
-	struct regulator	*reg;
 	unsigned long		vref_mv;
 
 	struct spi_transfer	single_xfer[3];
@@ -377,11 +378,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 +392,11 @@ 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;
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
+	if (ret < 0 && ret != -ENODEV)
+		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 */
-		st->vref_mv = 2500;
-	}
+	st->vref_mv = ret == -ENODEV ? AD7266_INTERNAL_REF_MV : ret / 1000;
 
 	if (pdata) {
 		st->fixed_addr = pdata->fixed_addr;

-- 
2.45.2

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

[Jonathan Cameron]

On Wed, 12 Jun 2024 16:03:06 -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>
Applied.

Thanks,

[PATCH v2 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>
---
v2 changes:
* avoid else in return value check
* use macro instead of comment to document internal reference voltage
---
 drivers/iio/adc/ad7292.c | 36 ++++++------------------------------
 1 file changed, 6 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
index 6aadd14f459d..87ffe66058a1 100644
--- a/drivers/iio/adc/ad7292.c
+++ b/drivers/iio/adc/ad7292.c
@@ -17,6 +17,8 @@
 
 #define ADI_VENDOR_ID 0x0018
 
+#define AD7292_INTERNAL_REF_MV 1250
+
 /* AD7292 registers definition */
 #define AD7292_REG_VENDOR_ID		0x00
 #define AD7292_REG_CONF_BANK		0x05
@@ -79,7 +81,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 +251,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 +271,11 @@ 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;
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
+	if (ret < 0 && ret == -ENODEV)
+		return ret;
 
-		st->vref_mv = ret / 1000;
-	} else {
-		/* Use the internal voltage reference. */
-		st->vref_mv = 1250;
-	}
+	st->vref_mv = ret == -ENODEV ? AD7292_INTERNAL_REF_MV : ret / 1000;
 
 	indio_dev->name = spi_get_device_id(spi)->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;

-- 
2.45.2

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

[Nuno Sá]

On Wed, 2024-06-12 at 16:03 -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>
> ---
> v2 changes:
> * avoid else in return value check
> * use macro instead of comment to document internal reference voltage
> ---
>  drivers/iio/adc/ad7292.c | 36 ++++++------------------------------
>  1 file changed, 6 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
> index 6aadd14f459d..87ffe66058a1 100644
> --- a/drivers/iio/adc/ad7292.c
> +++ b/drivers/iio/adc/ad7292.c
> @@ -17,6 +17,8 @@
>  
>  #define ADI_VENDOR_ID 0x0018
>  
> +#define AD7292_INTERNAL_REF_MV 1250
> +
>  /* AD7292 registers definition */
>  #define AD7292_REG_VENDOR_ID		0x00
>  #define AD7292_REG_CONF_BANK		0x05
> @@ -79,7 +81,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 +251,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 +271,11 @@ 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;
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> +	if (ret < 0 && ret == -ENODEV)

ret != -ENODEV?

- Nuno Sá

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

[David Lechner]

On 6/14/24 10:11 AM, Nuno Sá wrote:
> On Wed, 2024-06-12 at 16:03 -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>
>> ---
>> v2 changes:
>> * avoid else in return value check
>> * use macro instead of comment to document internal reference voltage
>> ---
>>  drivers/iio/adc/ad7292.c | 36 ++++++------------------------------
>>  1 file changed, 6 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
>> index 6aadd14f459d..87ffe66058a1 100644
>> --- a/drivers/iio/adc/ad7292.c
>> +++ b/drivers/iio/adc/ad7292.c
>> @@ -17,6 +17,8 @@
>>  
>>  #define ADI_VENDOR_ID 0x0018
>>  
>> +#define AD7292_INTERNAL_REF_MV 1250
>> +
>>  /* AD7292 registers definition */
>>  #define AD7292_REG_VENDOR_ID		0x00
>>  #define AD7292_REG_CONF_BANK		0x05
>> @@ -79,7 +81,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 +251,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 +271,11 @@ 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;
>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
>> +	if (ret < 0 && ret == -ENODEV)
> 
> ret != -ENODEV?

yup, I messed this one up

> 
> - Nuno Sá
> 

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

[Jonathan Cameron]

On Fri, 14 Jun 2024 10:16:26 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/14/24 10:11 AM, Nuno Sá wrote:
> > On Wed, 2024-06-12 at 16:03 -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>
> >> ---
> >> v2 changes:
> >> * avoid else in return value check
> >> * use macro instead of comment to document internal reference voltage
> >> ---
> >>  drivers/iio/adc/ad7292.c | 36 ++++++------------------------------
> >>  1 file changed, 6 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad7292.c b/drivers/iio/adc/ad7292.c
> >> index 6aadd14f459d..87ffe66058a1 100644
> >> --- a/drivers/iio/adc/ad7292.c
> >> +++ b/drivers/iio/adc/ad7292.c
> >> @@ -17,6 +17,8 @@
> >>  
> >>  #define ADI_VENDOR_ID 0x0018
> >>  
> >> +#define AD7292_INTERNAL_REF_MV 1250
> >> +
> >>  /* AD7292 registers definition */
> >>  #define AD7292_REG_VENDOR_ID		0x00
> >>  #define AD7292_REG_CONF_BANK		0x05
> >> @@ -79,7 +81,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 +251,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 +271,11 @@ 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;
> >> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> >> +	if (ret < 0 && ret == -ENODEV)  
> > 
> > ret != -ENODEV?  
> 
> yup, I messed this one up
Fixed up whilst applying.  Applied
> 
> > 
> > - Nuno Sá
> >   
> 

[PATCH v2 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>
---
v2 changes: none
---
 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.2

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

[Jonathan Cameron]

On Wed, 12 Jun 2024 16:03:08 -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>
Applied

[PATCH v2 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>
---

v2 changes:
- don't read voltage from refin regulator
- avoid else in return value checks
---
 drivers/iio/adc/ad7944.c | 54 +++++++++++-------------------------------------
 1 file changed, 12 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
index e2cb64cef476..f8bf03feba07 100644
--- a/drivers/iio/adc/ad7944.c
+++ b/drivers/iio/adc/ad7944.c
@@ -464,23 +464,17 @@ 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;
+	bool have_refin;
 	struct iio_chan_spec *chain_chan;
 	unsigned long *chain_scan_masks;
 	u32 n_chain_dev;
-	int ret;
+	int ret, ref_mv;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
 	if (!indio_dev)
@@ -531,47 +525,23 @@ 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");
+	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
 
-		ref = NULL;
-	}
+	ref_mv = ret == -ENODEV ? 0 : 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");
+	if (ret < 0 && ret == -ENODEV)
+		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
+
+	have_refin = ret != -ENODEV;
 
-	if (have_refin && ref)
+	if (have_refin && ref_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 */
-		adc->ref_mv = AD7944_INTERNAL_REF_MV;
-	}
+	adc->ref_mv = ref_mv ?: AD7944_INTERNAL_REF_MV;
 
 	adc->cnv = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
 	if (IS_ERR(adc->cnv))

-- 
2.45.2

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

[Nuno Sá]

On Wed, 2024-06-12 at 16:03 -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>
> ---
> 
> v2 changes:
> - don't read voltage from refin regulator
> - avoid else in return value checks
> ---
>  drivers/iio/adc/ad7944.c | 54 +++++++++++-------------------------------------
>  1 file changed, 12 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index e2cb64cef476..f8bf03feba07 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -464,23 +464,17 @@ 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;
> +	bool have_refin;
>  	struct iio_chan_spec *chain_chan;
>  	unsigned long *chain_scan_masks;
>  	u32 n_chain_dev;
> -	int ret;
> +	int ret, ref_mv;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>  	if (!indio_dev)
> @@ -531,47 +525,23 @@ 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");
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (ret < 0 && ret != -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
>  
> -		ref = NULL;
> -	}
> +	ref_mv = ret == -ENODEV ? 0 : 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");
> +	if (ret < 0 && ret == -ENODEV)
> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
> +

ret != -ENODEV right?

- Nuno Sá

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

[David Lechner]

On 6/14/24 10:16 AM, Nuno Sá wrote:
> On Wed, 2024-06-12 at 16:03 -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>
>> ---
>>
>> v2 changes:
>> - don't read voltage from refin regulator
>> - avoid else in return value checks
>> ---
>>  drivers/iio/adc/ad7944.c | 54 +++++++++++-------------------------------------
>>  1 file changed, 12 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
>> index e2cb64cef476..f8bf03feba07 100644
>> --- a/drivers/iio/adc/ad7944.c
>> +++ b/drivers/iio/adc/ad7944.c
>> @@ -464,23 +464,17 @@ 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;
>> +	bool have_refin;
>>  	struct iio_chan_spec *chain_chan;
>>  	unsigned long *chain_scan_masks;
>>  	u32 n_chain_dev;
>> -	int ret;
>> +	int ret, ref_mv;
>>  
>>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
>>  	if (!indio_dev)
>> @@ -531,47 +525,23 @@ 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");
>> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
>> +	if (ret < 0 && ret != -ENODEV)
>> +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
>>  
>> -		ref = NULL;
>> -	}
>> +	ref_mv = ret == -ENODEV ? 0 : 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");
>> +	if (ret < 0 && ret == -ENODEV)
>> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
>> +
> 
> ret != -ENODEV right?

oof, yeah messed that one too

> 
> - Nuno Sá
> 
> 

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

[Jonathan Cameron]

On Fri, 14 Jun 2024 10:19:43 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 6/14/24 10:16 AM, Nuno Sá wrote:
> > On Wed, 2024-06-12 at 16:03 -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>
> >> ---
> >>
> >> v2 changes:
> >> - don't read voltage from refin regulator
> >> - avoid else in return value checks
> >> ---
> >>  drivers/iio/adc/ad7944.c | 54 +++++++++++-------------------------------------
> >>  1 file changed, 12 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> >> index e2cb64cef476..f8bf03feba07 100644
> >> --- a/drivers/iio/adc/ad7944.c
> >> +++ b/drivers/iio/adc/ad7944.c
> >> @@ -464,23 +464,17 @@ 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;
> >> +	bool have_refin;
> >>  	struct iio_chan_spec *chain_chan;
> >>  	unsigned long *chain_scan_masks;
> >>  	u32 n_chain_dev;
> >> -	int ret;
> >> +	int ret, ref_mv;
> >>  
> >>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> >>  	if (!indio_dev)
> >> @@ -531,47 +525,23 @@ 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");
> >> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> >> +	if (ret < 0 && ret != -ENODEV)
> >> +		return dev_err_probe(dev, ret, "failed to get REF voltage\n");
> >>  
> >> -		ref = NULL;
> >> -	}
> >> +	ref_mv = ret == -ENODEV ? 0 : 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");
> >> +	if (ret < 0 && ret == -ENODEV)
> >> +		return dev_err_probe(dev, ret, "failed to get REFIN voltage\n");
> >> +  
> > 
> > ret != -ENODEV right?  
> 
> oof, yeah messed that one too
> 
Fixed up as well and applied.

Enough patches bouncing around that I'd rather clear these little things by
hand than see the patch again :)

Jonathan

> > 
> > - Nuno Sá
> > 
> >   
> 

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

[Nuno Sá]

On Wed, 2024-06-12 at 16:03 -0500, David Lechner wrote:
> 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
> 
> ---

Just two things that look like typos... With that:

Reviewed-by: Nuno Sa <nuno.sa@analog.com>