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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ab0fa023-91b6-48de-a71d-95eb9aa70c01@gmail.com>
Date: Mon, 14 Apr 2025 09:00:34 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: David Lechner <dlechner@...libre.com>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Nuno Sa <nuno.sa@...log.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC v3 8/8] iio: ti-adc128s052: Drop variable vref

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@...nel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.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