[v3] Support ROHM BD79104 ADC

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

Posted by Matti Vaittinen 8 months, 2 weeks ago

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:
 v2 => v3:
  - Rename vref => vref_mv to make units visible
  - Divide vref once in the probe to avoid division every time the scale
    is requested
 v2:
  - New patch
---
 drivers/iio/adc/ti-adc128s052.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index d4721ad90f2c..c38468f299ce 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,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;
 
 	if (config->num_other_regulators) {
 		ret = devm_regulator_bulk_get_enable(&spi->dev,
-- 
2.49.0

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

Posted by David Lechner 8 months, 1 week ago

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.

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).

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

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

Posted by Matti Vaittinen 8 months, 1 week ago

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

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

Posted by Jonathan Cameron 8 months, 1 week ago

On Fri, 11 Apr 2025 08:34:42 -0500
David Lechner <dlechner@baylibre.com> 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.
> 
> 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).

Good idea. 
Applied patches 1-7. I did tweak this one as well but then couldn't make
up my mind on whether to change the type of vref_mv so I'll avoid making
a decision and will leave patch 8 for a v4 from Matti :)

Jonathan



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

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

Posted by Matti Vaittinen 8 months, 1 week ago

On 12/04/2025 14:42, Jonathan Cameron wrote:
> On Fri, 11 Apr 2025 08:34:42 -0500
> David Lechner <dlechner@baylibre.com> 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.
>>
>> 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).
> 
> Good idea.
> Applied patches 1-7. I did tweak this one as well but then couldn't make
> up my mind on whether to change the type of vref_mv so I'll avoid making
> a decision and will leave patch 8 for a v4 from Matti :)

Thanks for handling the 1-7 :) I'll re-spin this, but it's likely to 
take some time. I'm having 2 weeks off from work, and my motorbike and 
boat is requiring some attention ;)

I hope I have regained a lot of energy after 2 weeks ;)

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


Yours,
	-- Matti