[PATCH] iio: ti-adc128s052: Drop variable vref

[PATCH] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

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>

---
Revision history:
RFCv3 to this patch:
 - Split out of the series (as other patches were applied)
 - Print an error if Vref can't be read
 - RFCv3: https://lore.kernel.org/all/db5cb2e1543e03d5a9953faa3934d66f4621cd12.1744022065.git.mazziesaccount@gmail.com/
---
 drivers/iio/adc/ti-adc128s052.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index d4721ad90f2c..383098206246 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -29,13 +29,12 @@ struct adc128_configuration {
 struct adc128 {
 	struct spi_device *spi;
 
-	struct regulator *reg;
 	/*
 	 * Serialize the SPI 'write-channel + read data' accesses and protect
 	 * the shared buffer.
 	 */
 	struct mutex lock;
-
+	int vref_mv;
 	union {
 		__be16 buffer16;
 		u8 buffer[2];
@@ -81,11 +80,7 @@ static int adc128_read_raw(struct iio_dev *indio_dev,
 
 	case IIO_CHAN_INFO_SCALE:
 
-		ret = regulator_get_voltage(adc->reg);
-		if (ret < 0)
-			return ret;
-
-		*val = ret / 1000;
+		*val = adc->vref_mv;
 		*val2 = 12;
 		return IIO_VAL_FRACTIONAL_LOG2;
 
@@ -155,11 +150,6 @@ static const struct iio_info adc128_info = {
 	.read_raw = adc128_read_raw,
 };
 
-static void adc128_disable_regulator(void *reg)
-{
-	regulator_disable(reg);
-}
-
 static int adc128_probe(struct spi_device *spi)
 {
 	const struct adc128_configuration *config;
@@ -183,17 +173,14 @@ 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);
-
-	ret = regulator_enable(adc->reg);
+	ret = devm_regulator_get_enable_read_voltage(&spi->dev,
+							   config->refname);
 	if (ret < 0)
-		return ret;
-	ret = devm_add_action_or_reset(&spi->dev, adc128_disable_regulator,
-				       adc->reg);
-	if (ret)
-		return ret;
+		return dev_err_probe(&spi->dev, ret,
+				     "failed to read '%s' voltage",
+				     config->refname);
+
+	adc->vref_mv = ret / 1000;
 
 	if (config->num_other_regulators) {
 		ret = devm_regulator_bulk_get_enable(&spi->dev,

base-commit: 350224bdb9725aab5b90d72fc3db7618ebd232ae
-- 
2.49.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 10:02 AM Matti Vaittinen
<mazziesaccount@gmail.com> 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.

...

> +       int vref_mv;

vref_mV please. And yes, I know historical and other reasons for them
all being small, but let's try to be more scientific in these crazy
days.

...

> +       adc->vref_mv = ret / 1000;

MILLI ?
Or actually a time to introduce MILLIVOLT_PER_VOLT in units.h ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen]

On 28/04/2025 10:08, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 10:02 AM Matti Vaittinen
> <mazziesaccount@gmail.com> 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.
> 
> ...
> 
>> +       int vref_mv;
> 
> vref_mV please. And yes, I know historical and other reasons for them
> all being small, but let's try to be more scientific in these crazy
> days.

Sorry Andy but I see zero reason to use capital letters here. In my 
opinion, this is perfectly clear as it is. Capital letters in variables 
are ugly (to me) and absolutely not needed to explain the meaning.

> ...
> 
>> +       adc->vref_mv = ret / 1000;
> 
> MILLI ?

I suppose using MILLI is Ok. (Although 1000 seems still clear enough to 
me. Seeing the amount of zeroes at a glance gets troublesome for me at 
10000).

> Or actually a time to introduce MILLIVOLT_PER_VOLT in units.h ?

I really fail to see the benefit. Do you think we should add 
MILLIx_PER_x for each unit we can imagine/use?

That doesn't really scale or make sense to me. We have MILLI. It does 
not really matter if it is volts, amps, ohms or horse heads - it's still 
1000. It just gets cumbersome to search the headers to see if we have 
some fancy define for unit we have at our hands.

And, to repeat myself - for me even the 1000 is still clear as it is.

Yours,
	-- Matti

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[Andy Shevchenko]

On Mon, Apr 28, 2025 at 12:45 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 28/04/2025 10:08, Andy Shevchenko wrote:
> > On Mon, Apr 28, 2025 at 10:02 AM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:

...

> >> +       int vref_mv;
> >
> > vref_mV please. And yes, I know historical and other reasons for them
> > all being small, but let's try to be more scientific in these crazy
> > days.
>
> Sorry Andy but I see zero reason to use capital letters here. In my
> opinion, this is perfectly clear as it is. Capital letters in variables
> are ugly (to me) and absolutely not needed to explain the meaning.

And I see zero reason to not use the correct scientific unitis there.
Note, that regulator framework and some other drivers are using that
and I consider this is the correct way to go.

...

> > Or actually a time to introduce MILLIVOLT_PER_VOLT in units.h ?
>
> I really fail to see the benefit. Do you think we should add
> MILLIx_PER_x for each unit we can imagine/use?
>
> That doesn't really scale or make sense to me. We have MILLI. It does
> not really matter if it is volts, amps, ohms or horse heads - it's still
> 1000. It just gets cumbersome to search the headers to see if we have
> some fancy define for unit we have at our hands.

In some contexts this gives the understanding of the units and how big
the value is. Even with some defined constants it's not possible to
describe in their names everything that this definition adds. I do see
a benefit of it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen]

On 28/04/2025 12:49, Andy Shevchenko wrote:
> On Mon, Apr 28, 2025 at 12:45 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 28/04/2025 10:08, Andy Shevchenko wrote:
>>> On Mon, Apr 28, 2025 at 10:02 AM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
> 
> ...
> 
>>>> +       int vref_mv;
>>>
>>> vref_mV please. And yes, I know historical and other reasons for them
>>> all being small, but let's try to be more scientific in these crazy
>>> days.
>>
>> Sorry Andy but I see zero reason to use capital letters here. In my
>> opinion, this is perfectly clear as it is. Capital letters in variables
>> are ugly (to me) and absolutely not needed to explain the meaning.
> 
> And I see zero reason to not use the correct scientific unitis there.

I find this ridiculous reason. There is zero confusion what the vref_mv 
stands for, and I prefer variables in lower case.

> Note, that regulator framework

Actually, the regulator framework uses _both_ lower and upper case in 
some places, and I believe the reason is that bikeshedding over the 
capitalization isn't Mark's cup of tea.

> and some other drivers are using that
> and I consider this is the correct way to go.
> 
> ...
> 
>>> Or actually a time to introduce MILLIVOLT_PER_VOLT in units.h ?
>>
>> I really fail to see the benefit. Do you think we should add
>> MILLIx_PER_x for each unit we can imagine/use?
>>
>> That doesn't really scale or make sense to me. We have MILLI. It does
>> not really matter if it is volts, amps, ohms or horse heads - it's still
>> 1000. It just gets cumbersome to search the headers to see if we have
>> some fancy define for unit we have at our hands.
> 
> In some contexts this gives the understanding of the units and how big
> the value is. Even with some defined constants it's not possible to
> describe in their names everything that this definition adds. I do see
> a benefit of it.

You can suggest adding it if you wish. I still find it silly as I don't 
see why one wouldn't then end up adding base SI units too.

MILLIVOLT_PER_VOLT
MILLIKILOGRAM_PER_KILOGRAM
MILLIMETER_PER_METER
MILLISECOND_PER_SECON
MILLIAMPERE_PER_AMPERE
MILLIKELVIN_PER_KELVIN
MILLIMOLE_PER_MOLE
MILLICANDELA_PER_CANDELA

Oh, and because Volt is added, then also:

Newton, Heryz, Coulomb, Henry, Farad, Ohm, Siemens, Weber, Tesla, Joule, 
Watt, Radian, Bequerel and Lumen.

And, sure adding Kelvin warrants adding Celsius and Fahrenheit. Oh, 
probably also imperial units, right?

All being the same MILLI.

No thanks. I am opposing this as it will only make life of a code writer 
(and maybe also reader) harder. I'm not suggesting a patch adding 
something I feel is rather silly.

Yours,
	-- Matti

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[Jonathan Cameron]

On Mon, 28 Apr 2025 12:45:13 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 28/04/2025 10:08, Andy Shevchenko wrote:
> > On Mon, Apr 28, 2025 at 10:02 AM Matti Vaittinen
> > <mazziesaccount@gmail.com> 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.  
> > 
> > ...
> >   
> >> +       int vref_mv;  
> > 
> > vref_mV please. And yes, I know historical and other reasons for them
> > all being small, but let's try to be more scientific in these crazy
> > days.  
> 
> Sorry Andy but I see zero reason to use capital letters here. In my 
> opinion, this is perfectly clear as it is. Capital letters in variables 
> are ugly (to me) and absolutely not needed to explain the meaning.
> 
> > ...
> >   
> >> +       adc->vref_mv = ret / 1000;  
> > 
> > MILLI ?  
> 
> I suppose using MILLI is Ok. (Although 1000 seems still clear enough to 
> me. Seeing the amount of zeroes at a glance gets troublesome for me at 
> 10000).
> 
> > Or actually a time to introduce MILLIVOLT_PER_VOLT in units.h ?  

It's logically neither. If we did go that way it's MICROVOLT_PER_MILLIVOLT.
So we move to constants it should be
	adc->vref_mv = ret / (MICRO / MILLI);

I'm not seeing that as worthwhile though when people have been particularly
keen on these units.h defines I have argued in favour of similar constructs.

Anyhow on basis of moving forwards and parking this discussion
for the future, I'll apply the patch with just the alignment tweak
David pointed out.  

Jonathan



> 
> I really fail to see the benefit. Do you think we should add 
> MILLIx_PER_x for each unit we can imagine/use?
> 
> That doesn't really scale or make sense to me. We have MILLI. It does 
> not really matter if it is volts, amps, ohms or horse heads - it's still 
> 1000. It just gets cumbersome to search the headers to see if we have 
> some fancy define for unit we have at our hands.
> 
> And, to repeat myself - for me even the 1000 is still clear as it is.
> 
> Yours,
> 	-- Matti

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[David Lechner]

On 4/28/25 2:02 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>
> 
> ---

Reviewed-by: David Lechner <dlechner@baylibre.com>

> @@ -183,17 +173,14 @@ 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);
> -
> -	ret = regulator_enable(adc->reg);
> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev,
> +							   config->refname);

Is this properly aligned to the opening "("?

Re: [PATCH] iio: ti-adc128s052: Drop variable vref

[Matti Vaittinen]

On 28/04/2025 18:16, David Lechner wrote:
> On 4/28/25 2:02 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>
>>
>> ---
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> 
>> @@ -183,17 +173,14 @@ 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);
>> -
>> -	ret = regulator_enable(adc->reg);
>> +	ret = devm_regulator_get_enable_read_voltage(&spi->dev,
>> +							   config->refname);
> 
> Is this properly aligned to the opening "("?

Thanks David. No, it's off by one tab. Nicely spotted :)

Yours,
	-- Matti