iio: adc: ti-adc128s052: add support for TI's ADC121s021

[PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

Posted by Lothar Rubusch 3 months, 2 weeks ago

Replace the literal number 1000 by MILLI from linux/units.h

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/adc/ti-adc128s052.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index cf271c39e663..67bc7fbd52bc 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -18,6 +18,7 @@
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/units.h>
 
 struct adc128_configuration {
 	const struct iio_chan_spec	*channels;
@@ -189,7 +190,7 @@ static int adc128_probe(struct spi_device *spi)
 				     "failed to read '%s' voltage",
 				     config->refname);
 
-	adc->vref_mv = ret / 1000;
+	adc->vref_mv = ret / MILLI;
 
 	if (config->num_other_regulators) {
 		ret = devm_regulator_bulk_get_enable(&spi->dev,
-- 
2.39.5

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

Posted by Matti Vaittinen 3 months, 2 weeks ago

On 25/06/2025 20:02, Lothar Rubusch wrote:
> Replace the literal number 1000 by MILLI from linux/units.h
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>   drivers/iio/adc/ti-adc128s052.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index cf271c39e663..67bc7fbd52bc 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -18,6 +18,7 @@
>   #include <linux/property.h>
>   #include <linux/regulator/consumer.h>
>   #include <linux/spi/spi.h>
> +#include <linux/units.h>
>   
>   struct adc128_configuration {
>   	const struct iio_chan_spec	*channels;
> @@ -189,7 +190,7 @@ static int adc128_probe(struct spi_device *spi)
>   				     "failed to read '%s' voltage",
>   				     config->refname);
>   
> -	adc->vref_mv = ret / 1000;
> +	adc->vref_mv = ret / MILLI;

This makes no sense to me. What does it mean we divide the micro volts 
by 'milli'? It is clear when we divide micro volts by 1000 that we get 
milli volts. Also, the mv suffix in variable makes units clear already, 
division by MILLI just obfuscates things. I'd keep it as 1000.

I would just drop this unless Jonathan disagrees.

Yours,
	-- Matti

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

Posted by Jonathan Cameron 3 months, 1 week ago

On Thu, 26 Jun 2025 08:07:19 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 25/06/2025 20:02, Lothar Rubusch wrote:
> > Replace the literal number 1000 by MILLI from linux/units.h
> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >   drivers/iio/adc/ti-adc128s052.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > index cf271c39e663..67bc7fbd52bc 100644
> > --- a/drivers/iio/adc/ti-adc128s052.c
> > +++ b/drivers/iio/adc/ti-adc128s052.c
> > @@ -18,6 +18,7 @@
> >   #include <linux/property.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/spi/spi.h>
> > +#include <linux/units.h>
> >   
> >   struct adc128_configuration {
> >   	const struct iio_chan_spec	*channels;
> > @@ -189,7 +190,7 @@ static int adc128_probe(struct spi_device *spi)
> >   				     "failed to read '%s' voltage",
> >   				     config->refname);
> >   
> > -	adc->vref_mv = ret / 1000;
> > +	adc->vref_mv = ret / MILLI;  
> 
> This makes no sense to me. What does it mean we divide the micro volts 
> by 'milli'? It is clear when we divide micro volts by 1000 that we get 
> milli volts. Also, the mv suffix in variable makes units clear already, 
> division by MILLI just obfuscates things. I'd keep it as 1000.
> 

ret / (MICRO / MILLI) would be more descriptive as indicates
we are converting form microvolts to milivolts.
 
> I would just drop this unless Jonathan disagrees.
> 
> Yours,
> 	-- Matti

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

Posted by Andy Shevchenko 3 months, 2 weeks ago

On Wed, Jun 25, 2025 at 05:02:18PM +0000, Lothar Rubusch wrote:
> Replace the literal number 1000 by MILLI from linux/units.h

Yeah, but with this units (mV, uV, mA, uA) it would probably better to have
dedicated definitions, like we have for Hz, s and maybe something else
(bytes?).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: replace literal by unit expression

Posted by Jonathan Cameron 3 months, 1 week ago

On Wed, 25 Jun 2025 20:57:37 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Jun 25, 2025 at 05:02:18PM +0000, Lothar Rubusch wrote:
> > Replace the literal number 1000 by MILLI from linux/units.h  
> 
> Yeah, but with this units (mV, uV, mA, uA) it would probably better to have
> dedicated definitions, like we have for Hz, s and maybe something else
> (bytes?).

I'm not that keen on us getting loads unit specific conversion factors.

From a code readability point of view (given my wish to avoid lots
and lots of unit specific defines) I'd favour using
MICRO / MILLI here and let the compiler resolve that to 1000

>