[PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()

Dmitry Torokhov posted 6 patches 1 month ago
There is a newer version of this series
[PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Dmitry Torokhov 1 month ago
The regulator is enabled for the entire time the driver is bound to the
device, and we only need to access it to fetch voltage, which can be
done at probe time.

Switch to using devm_regulator_get_enable_read_voltage() which
simplifies probing and unbinding code.

Suggested-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/iio/adc/ti-ads7950.c | 45 +++++++++++---------------------------------
 1 file changed, 11 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 273c35e03185..847e83baa876 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -341,19 +341,9 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 	return st->single_rx;
 }
 
-static int ti_ads7950_get_range(struct ti_ads7950_state *st)
+static unsigned int ti_ads7950_get_range(struct ti_ads7950_state *st)
 {
-	int vref;
-
-	if (st->vref_mv) {
-		vref = st->vref_mv;
-	} else {
-		vref = regulator_get_voltage(st->reg);
-		if (vref < 0)
-			return vref;
-
-		vref /= 1000;
-	}
+	unsigned int vref = st->vref_mv;
 
 	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
 		vref *= 2;
@@ -382,11 +372,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		ret = ti_ads7950_get_range(st);
-		if (ret < 0)
-			return ret;
-
-		*val = ret;
+		*val = ti_ads7950_get_range(st);
 		*val2 = (1 << chan->scan_type.realbits) - 1;
 
 		return IIO_VAL_FRACTIONAL;
@@ -580,30 +566,24 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	spi_message_init_with_transfers(&st->scan_single_msg,
 					st->scan_single_xfer, 3);
 
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
+	if (ret < 0)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to get regulator \"vref\"\n");
+
 	/* Use hard coded value for reference voltage in ACPI case */
 	if (ACPI_COMPANION(&spi->dev))
 		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
+	else
+		st->vref_mv = ret / 1000;
 
 	mutex_init(&st->slock);
 
-	st->reg = devm_regulator_get(&spi->dev, "vref");
-	if (IS_ERR(st->reg)) {
-		ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
-				     "Failed to get regulator \"vref\"\n");
-		goto error_destroy_mutex;
-	}
-
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
-		goto error_destroy_mutex;
-	}
-
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 					 &ti_ads7950_trigger_handler, NULL);
 	if (ret) {
 		dev_err(&spi->dev, "Failed to setup triggered buffer\n");
-		goto error_disable_reg;
+		goto error_destroy_mutex;
 	}
 
 	ret = ti_ads7950_init_hw(st);
@@ -643,8 +623,6 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	iio_device_unregister(indio_dev);
 error_cleanup_ring:
 	iio_triggered_buffer_cleanup(indio_dev);
-error_disable_reg:
-	regulator_disable(st->reg);
 error_destroy_mutex:
 	mutex_destroy(&st->slock);
 
@@ -659,7 +637,6 @@ static void ti_ads7950_remove(struct spi_device *spi)
 	gpiochip_remove(&st->chip);
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
-	regulator_disable(st->reg);
 	mutex_destroy(&st->slock);
 }
 

-- 
2.53.0.473.g4a7958ca14-goog
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Jonathan Cameron 1 month ago
On Thu, 05 Mar 2026 11:21:56 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> The regulator is enabled for the entire time the driver is bound to the
> device, and we only need to access it to fetch voltage, which can be
> done at probe time.
> 
> Switch to using devm_regulator_get_enable_read_voltage() which
> simplifies probing and unbinding code.
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Hi.

I think this broke the ACPI case (where regulator isn't available).

Jonathan

> ---
>  drivers/iio/adc/ti-ads7950.c | 45 +++++++++++---------------------------------
>  1 file changed, 11 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 273c35e03185..847e83baa876 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -341,19 +341,9 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  	return st->single_rx;
>  }
>  
> -static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> +static unsigned int ti_ads7950_get_range(struct ti_ads7950_state *st)
>  {
> -	int vref;
> -
> -	if (st->vref_mv) {
> -		vref = st->vref_mv;
> -	} else {
> -		vref = regulator_get_voltage(st->reg);
> -		if (vref < 0)
> -			return vref;
> -
> -		vref /= 1000;
> -	}
> +	unsigned int vref = st->vref_mv;
>  
>  	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
>  		vref *= 2;
> @@ -382,11 +372,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		ret = ti_ads7950_get_range(st);
> -		if (ret < 0)
> -			return ret;
> -
> -		*val = ret;
> +		*val = ti_ads7950_get_range(st);
>  		*val2 = (1 << chan->scan_type.realbits) - 1;
>  
>  		return IIO_VAL_FRACTIONAL;
> @@ -580,30 +566,24 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	spi_message_init_with_transfers(&st->scan_single_msg,
>  					st->scan_single_xfer, 3);
>  
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> +	if (ret < 0)

I think you need to check for -ENODEV and if you see than then
see if the acpi route below applies.  Otherwise on ACPI this will fail
and we'll fail the probe.


> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to get regulator \"vref\"\n");
> +
>  	/* Use hard coded value for reference voltage in ACPI case */
>  	if (ACPI_COMPANION(&spi->dev))
>  		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
> +	else
> +		st->vref_mv = ret / 1000;
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Andy Shevchenko 1 month ago
On Sat, Mar 07, 2026 at 11:49:47AM +0000, Jonathan Cameron wrote:
> On Thu, 05 Mar 2026 11:21:56 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

...

> I think this broke the ACPI case (where regulator isn't available).

Right, and I even have an HW to test (if required).

...

> > +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> > +	if (ret < 0)
> 
> I think you need to check for -ENODEV and if you see than then
> see if the acpi route below applies.  Otherwise on ACPI this will fail
> and we'll fail the probe.

Yep, just make it default without even checking the fwnode type.
No need to bring the whole acpi.h into the mix.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Dmitry Torokhov 1 month ago
On Sun, Mar 08, 2026 at 10:32:13PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 07, 2026 at 11:49:47AM +0000, Jonathan Cameron wrote:
> > On Thu, 05 Mar 2026 11:21:56 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> ...
> 
> > I think this broke the ACPI case (where regulator isn't available).
> 
> Right, and I even have an HW to test (if required).
> 
> ...
> 
> > > +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> > > +	if (ret < 0)
> > 
> > I think you need to check for -ENODEV and if you see than then
> > see if the acpi route below applies.  Otherwise on ACPI this will fail
> > and we'll fail the probe.
> 
> Yep, just make it default without even checking the fwnode type.
> No need to bring the whole acpi.h into the mix.

I do not think this would be correct behavior. On non-ACPI systems the
regulator is mandatory, and the driver should not blindly apply scale
from ACPI systems just because regulator is missing in the DTS.

Thanks.

-- 
Dmitry
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Andy Shevchenko 1 month ago
On Mon, Mar 09, 2026 at 05:35:58AM +0000, Dmitry Torokhov wrote:
> On Sun, Mar 08, 2026 at 10:32:13PM +0200, Andy Shevchenko wrote:
> > On Sat, Mar 07, 2026 at 11:49:47AM +0000, Jonathan Cameron wrote:
> > > On Thu, 05 Mar 2026 11:21:56 -0800
> > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

...

> > > > +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> > > > +	if (ret < 0)
> > > 
> > > I think you need to check for -ENODEV and if you see than then
> > > see if the acpi route below applies.  Otherwise on ACPI this will fail
> > > and we'll fail the probe.
> > 
> > Yep, just make it default without even checking the fwnode type.
> > No need to bring the whole acpi.h into the mix.
> 
> I do not think this would be correct behavior. On non-ACPI systems the
> regulator is mandatory, and the driver should not blindly apply scale
> from ACPI systems just because regulator is missing in the DTS.

I mistakenly thought that original code doesn't have that cneck. Yes, the
ACPI_COMPANION() should be preserved.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by David Lechner 1 month ago
On 3/7/26 5:49 AM, Jonathan Cameron wrote:
> On Thu, 05 Mar 2026 11:21:56 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
>> The regulator is enabled for the entire time the driver is bound to the
>> device, and we only need to access it to fetch voltage, which can be
>> done at probe time.
>>
>> Switch to using devm_regulator_get_enable_read_voltage() which
>> simplifies probing and unbinding code.
>>
>> Suggested-by: David Lechner <dlechner@baylibre.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Hi.
> 
> I think this broke the ACPI case (where regulator isn't available).
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/ti-ads7950.c | 45 +++++++++++---------------------------------
>>  1 file changed, 11 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>> index 273c35e03185..847e83baa876 100644
>> --- a/drivers/iio/adc/ti-ads7950.c
>> +++ b/drivers/iio/adc/ti-ads7950.c
>> @@ -341,19 +341,9 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>>  	return st->single_rx;
>>  }
>>  
>> -static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>> +static unsigned int ti_ads7950_get_range(struct ti_ads7950_state *st)
>>  {
>> -	int vref;
>> -
>> -	if (st->vref_mv) {
>> -		vref = st->vref_mv;
>> -	} else {
>> -		vref = regulator_get_voltage(st->reg);
>> -		if (vref < 0)
>> -			return vref;
>> -
>> -		vref /= 1000;
>> -	}
>> +	unsigned int vref = st->vref_mv;
>>  
>>  	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
>>  		vref *= 2;
>> @@ -382,11 +372,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>>  
>>  		return IIO_VAL_INT;
>>  	case IIO_CHAN_INFO_SCALE:
>> -		ret = ti_ads7950_get_range(st);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		*val = ret;
>> +		*val = ti_ads7950_get_range(st);
>>  		*val2 = (1 << chan->scan_type.realbits) - 1;
>>  
>>  		return IIO_VAL_FRACTIONAL;
>> @@ -580,30 +566,24 @@ static int ti_ads7950_probe(struct spi_device *spi)
>>  	spi_message_init_with_transfers(&st->scan_single_msg,
>>  					st->scan_single_xfer, 3);
>>  
>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
>> +	if (ret < 0)
> 
> I think you need to check for -ENODEV and if you see than then
> see if the acpi route below applies.  Otherwise on ACPI this will fail
> and we'll fail the probe.
> 

Or do something like:

	if (ACPI_COMPANION(&spi->dev)) {
		ret = devm_regulator_get_enable(&spi->dev, "vref");
		if (ret)
			return dev_err_probe(&spi->dev, ret,
					     "Failed to get regulator \"vref\"\n");

 		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
	} else {

		ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
		if (ret < 0)
			return dev_err_probe(&spi->dev, ret,
					     "Failed to get regulator \"vref\"\n");

		st->vref_mv = ret / 1000;
	}

> 
>> +		return dev_err_probe(&spi->dev, ret,
>> +				     "Failed to get regulator \"vref\"\n");
>> +
>>  	/* Use hard coded value for reference voltage in ACPI case */
>>  	if (ACPI_COMPANION(&spi->dev))
>>  		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
>> +	else
>> +		st->vref_mv = ret / 1000;
> 
>
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Dmitry Torokhov 1 month ago
On Sat, Mar 07, 2026 at 11:43:32AM -0600, David Lechner wrote:
> On 3/7/26 5:49 AM, Jonathan Cameron wrote:
> > On Thu, 05 Mar 2026 11:21:56 -0800
> > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > 
> >> The regulator is enabled for the entire time the driver is bound to the
> >> device, and we only need to access it to fetch voltage, which can be
> >> done at probe time.
> >>
> >> Switch to using devm_regulator_get_enable_read_voltage() which
> >> simplifies probing and unbinding code.
> >>
> >> Suggested-by: David Lechner <dlechner@baylibre.com>
> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Hi.
> > 
> > I think this broke the ACPI case (where regulator isn't available).

Oh, you're right.

> > 
> > Jonathan
> > 
> >> ---
> >>  drivers/iio/adc/ti-ads7950.c | 45 +++++++++++---------------------------------
> >>  1 file changed, 11 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> >> index 273c35e03185..847e83baa876 100644
> >> --- a/drivers/iio/adc/ti-ads7950.c
> >> +++ b/drivers/iio/adc/ti-ads7950.c
> >> @@ -341,19 +341,9 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
> >>  	return st->single_rx;
> >>  }
> >>  
> >> -static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> >> +static unsigned int ti_ads7950_get_range(struct ti_ads7950_state *st)
> >>  {
> >> -	int vref;
> >> -
> >> -	if (st->vref_mv) {
> >> -		vref = st->vref_mv;
> >> -	} else {
> >> -		vref = regulator_get_voltage(st->reg);
> >> -		if (vref < 0)
> >> -			return vref;
> >> -
> >> -		vref /= 1000;
> >> -	}
> >> +	unsigned int vref = st->vref_mv;
> >>  
> >>  	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
> >>  		vref *= 2;
> >> @@ -382,11 +372,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
> >>  
> >>  		return IIO_VAL_INT;
> >>  	case IIO_CHAN_INFO_SCALE:
> >> -		ret = ti_ads7950_get_range(st);
> >> -		if (ret < 0)
> >> -			return ret;
> >> -
> >> -		*val = ret;
> >> +		*val = ti_ads7950_get_range(st);
> >>  		*val2 = (1 << chan->scan_type.realbits) - 1;
> >>  
> >>  		return IIO_VAL_FRACTIONAL;
> >> @@ -580,30 +566,24 @@ static int ti_ads7950_probe(struct spi_device *spi)
> >>  	spi_message_init_with_transfers(&st->scan_single_msg,
> >>  					st->scan_single_xfer, 3);
> >>  
> >> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> >> +	if (ret < 0)
> > 
> > I think you need to check for -ENODEV and if you see than then
> > see if the acpi route below applies.  Otherwise on ACPI this will fail
> > and we'll fail the probe.
> > 
> 
> Or do something like:
> 
> 	if (ACPI_COMPANION(&spi->dev)) {
> 		ret = devm_regulator_get_enable(&spi->dev, "vref");
> 		if (ret)
> 			return dev_err_probe(&spi->dev, ret,
> 					     "Failed to get regulator \"vref\"\n");
> 
>  		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;

We know that concept of regulators is not exposed on ACPI systems, and
we'd get a dummy here, so maybe just store st->vref_mv and not bother
with acquiring and enabling the dummy regulator?

Thanks.

-- 
Dmitry
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by David Lechner 1 month ago
On 3/7/26 12:04 PM, Dmitry Torokhov wrote:
> On Sat, Mar 07, 2026 at 11:43:32AM -0600, David Lechner wrote:
>> On 3/7/26 5:49 AM, Jonathan Cameron wrote:
>>> On Thu, 05 Mar 2026 11:21:56 -0800
>>> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>
>>>> The regulator is enabled for the entire time the driver is bound to the
>>>> device, and we only need to access it to fetch voltage, which can be
>>>> done at probe time.
>>>>
>>>> Switch to using devm_regulator_get_enable_read_voltage() which
>>>> simplifies probing and unbinding code.
>>>>
>>>> Suggested-by: David Lechner <dlechner@baylibre.com>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> Hi.
>>>
>>> I think this broke the ACPI case (where regulator isn't available).
> 
> Oh, you're right.
> 
>>>
>>> Jonathan
>>>
>>>> ---
>>>>  drivers/iio/adc/ti-ads7950.c | 45 +++++++++++---------------------------------
>>>>  1 file changed, 11 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>>>> index 273c35e03185..847e83baa876 100644
>>>> --- a/drivers/iio/adc/ti-ads7950.c
>>>> +++ b/drivers/iio/adc/ti-ads7950.c
>>>> @@ -341,19 +341,9 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>>>>  	return st->single_rx;
>>>>  }
>>>>  
>>>> -static int ti_ads7950_get_range(struct ti_ads7950_state *st)
>>>> +static unsigned int ti_ads7950_get_range(struct ti_ads7950_state *st)
>>>>  {
>>>> -	int vref;
>>>> -
>>>> -	if (st->vref_mv) {
>>>> -		vref = st->vref_mv;
>>>> -	} else {
>>>> -		vref = regulator_get_voltage(st->reg);
>>>> -		if (vref < 0)
>>>> -			return vref;
>>>> -
>>>> -		vref /= 1000;
>>>> -	}
>>>> +	unsigned int vref = st->vref_mv;
>>>>  
>>>>  	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
>>>>  		vref *= 2;
>>>> @@ -382,11 +372,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>>>>  
>>>>  		return IIO_VAL_INT;
>>>>  	case IIO_CHAN_INFO_SCALE:
>>>> -		ret = ti_ads7950_get_range(st);
>>>> -		if (ret < 0)
>>>> -			return ret;
>>>> -
>>>> -		*val = ret;
>>>> +		*val = ti_ads7950_get_range(st);
>>>>  		*val2 = (1 << chan->scan_type.realbits) - 1;
>>>>  
>>>>  		return IIO_VAL_FRACTIONAL;
>>>> @@ -580,30 +566,24 @@ static int ti_ads7950_probe(struct spi_device *spi)
>>>>  	spi_message_init_with_transfers(&st->scan_single_msg,
>>>>  					st->scan_single_xfer, 3);
>>>>  
>>>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
>>>> +	if (ret < 0)
>>>
>>> I think you need to check for -ENODEV and if you see than then
>>> see if the acpi route below applies.  Otherwise on ACPI this will fail
>>> and we'll fail the probe.
>>>
>>
>> Or do something like:
>>
>> 	if (ACPI_COMPANION(&spi->dev)) {
>> 		ret = devm_regulator_get_enable(&spi->dev, "vref");
>> 		if (ret)
>> 			return dev_err_probe(&spi->dev, ret,
>> 					     "Failed to get regulator \"vref\"\n");
>>
>>  		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;
> 
> We know that concept of regulators is not exposed on ACPI systems, and
> we'd get a dummy here, so maybe just store st->vref_mv and not bother
> with acquiring and enabling the dummy regulator?
> 
> Thanks.
> 

Sounds OK to me.
Re: [PATCH v3 5/6] iio: adc: ti-ads7950: switch to using devm_regulator_get_enable_read_voltage()
Posted by Jonathan Cameron 1 month ago
On Sat, 7 Mar 2026 12:07:40 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 3/7/26 12:04 PM, Dmitry Torokhov wrote:
> > On Sat, Mar 07, 2026 at 11:43:32AM -0600, David Lechner wrote:  
> >> On 3/7/26 5:49 AM, Jonathan Cameron wrote:  
> >>> On Thu, 05 Mar 2026 11:21:56 -0800
> >>> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>>  
> >>>> The regulator is enabled for the entire time the driver is bound to the
> >>>> device, and we only need to access it to fetch voltage, which can be
> >>>> done at probe time.
> >>>>
> >>>> Switch to using devm_regulator_get_enable_read_voltage() which
> >>>> simplifies probing and unbinding code.
> >>>>
> >>>> Suggested-by: David Lechner <dlechner@baylibre.com>
> >>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>  
> >>> Hi.
> >>>
> >>> I think this broke the ACPI case (where regulator isn't available).  
> > 
> > Oh, you're right.
> >   
> >>>
> >>> Jonathan
> >>>  
> >>>> ---
> >>>>  drivers/iio/adc/ti-ads7950.c | 45 +++++++++++---------------------------------
> >>>>  1 file changed, 11 insertions(+), 34 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> >>>> index 273c35e03185..847e83baa876 100644
> >>>> --- a/drivers/iio/adc/ti-ads7950.c
> >>>> +++ b/drivers/iio/adc/ti-ads7950.c
> >>>> @@ -341,19 +341,9 @@ static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
> >>>>  	return st->single_rx;
> >>>>  }
> >>>>  
> >>>> -static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> >>>> +static unsigned int ti_ads7950_get_range(struct ti_ads7950_state *st)
> >>>>  {
> >>>> -	int vref;
> >>>> -
> >>>> -	if (st->vref_mv) {
> >>>> -		vref = st->vref_mv;
> >>>> -	} else {
> >>>> -		vref = regulator_get_voltage(st->reg);
> >>>> -		if (vref < 0)
> >>>> -			return vref;
> >>>> -
> >>>> -		vref /= 1000;
> >>>> -	}
> >>>> +	unsigned int vref = st->vref_mv;
> >>>>  
> >>>>  	if (st->cmd_settings_bitmask & TI_ADS7950_CR_RANGE_5V)
> >>>>  		vref *= 2;
> >>>> @@ -382,11 +372,7 @@ static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
> >>>>  
> >>>>  		return IIO_VAL_INT;
> >>>>  	case IIO_CHAN_INFO_SCALE:
> >>>> -		ret = ti_ads7950_get_range(st);
> >>>> -		if (ret < 0)
> >>>> -			return ret;
> >>>> -
> >>>> -		*val = ret;
> >>>> +		*val = ti_ads7950_get_range(st);
> >>>>  		*val2 = (1 << chan->scan_type.realbits) - 1;
> >>>>  
> >>>>  		return IIO_VAL_FRACTIONAL;
> >>>> @@ -580,30 +566,24 @@ static int ti_ads7950_probe(struct spi_device *spi)
> >>>>  	spi_message_init_with_transfers(&st->scan_single_msg,
> >>>>  					st->scan_single_xfer, 3);
> >>>>  
> >>>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vref");
> >>>> +	if (ret < 0)  
> >>>
> >>> I think you need to check for -ENODEV and if you see than then
> >>> see if the acpi route below applies.  Otherwise on ACPI this will fail
> >>> and we'll fail the probe.
> >>>  
> >>
> >> Or do something like:
> >>
> >> 	if (ACPI_COMPANION(&spi->dev)) {
> >> 		ret = devm_regulator_get_enable(&spi->dev, "vref");
> >> 		if (ret)
> >> 			return dev_err_probe(&spi->dev, ret,
> >> 					     "Failed to get regulator \"vref\"\n");
> >>
> >>  		st->vref_mv = TI_ADS7950_VA_MV_ACPI_DEFAULT;  
> > 
> > We know that concept of regulators is not exposed on ACPI systems, and
> > we'd get a dummy here, so maybe just store st->vref_mv and not bother
> > with acquiring and enabling the dummy regulator?
> > 
> > Thanks.
> >   
> 
> Sounds OK to me.
Likewise.

J