Re: [PATCH RFC v3 8/8] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 11/04/2025 16:34, David Lechner wrote:
> On 4/7/25 6:37 AM, Matti Vaittinen wrote:
>> According to Jonathan, variable reference voltages are very rare. It is
>> unlikely it is needed, and supporting it makes the code a bit more
>> complex.
>>
>> Simplify the driver and drop the variable vref support.
>>
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
> 
> ...
> 
>>   static int adc128_probe(struct spi_device *spi)
>>   {
>>   	const struct adc128_configuration *config;
>> @@ -183,17 +173,12 @@ static int adc128_probe(struct spi_device *spi)
>>   	indio_dev->channels = config->channels;
>>   	indio_dev->num_channels = config->num_channels;
>>   
>> -	adc->reg = devm_regulator_get(&spi->dev, config->refname);
>> -	if (IS_ERR(adc->reg))
>> -		return PTR_ERR(adc->reg);
>> +	adc->vref_mv = devm_regulator_get_enable_read_voltage(&spi->dev,
>> +							   config->refname);
>> +	if (adc->vref_mv < 0)
>> +		return adc->vref_mv;
>>   
>> -	ret = regulator_enable(adc->reg);
>> -	if (ret < 0)
>> -		return ret;
>> -	ret = devm_add_action_or_reset(&spi->dev, adc128_disable_regulator,
>> -				       adc->reg);
>> -	if (ret)
>> -		return ret;
>> +	adc->vref_mv /= 1000;
> 
> In other drivers, we've been doing:
> 
> ret = devm_regulator_get_enable_read_voltage(...);
> if (ret < 0)
> 	return dev_err_probe(dev, ret, "failed to read '%s' voltage, ...);
> 
> adc->vref_mv = ret / 1000;
> 
> It can be easy to make a typo or forget to specify the voltage when creating
> a .dts, so I think the error message is helpful to catch that.

Good idea. Thanks.

> And we use ret to avoid having adc->vref_mv temporarily holding a
> value with the wrong units (and can make it have an unsigned type).

I'm not convinced about the benefits. The "temporary holding" is not 
really an issue as it is only held unmodified for the duration of the 
error check. Furthermore, converting the voltage unsigned does not add 
much as the regulator framework does any way return the voltage as integer.

Still, even if I am not convinced about the benefits, I don't really see 
any downsides in your suggestions either :)

>>   
>>   	if (config->num_other_regulators) {
>>   		ret = devm_regulator_bulk_get_enable(&spi->dev,
> 

Yours,
	-- Matti