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

[David Lechner]

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@xxxxxxxxx> 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@xxxxxxxxxxxx>
>>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
>>> 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.