1. Patchew
  2. linux
  3. [PATCH v2 0/5] iio: dac: ad5504: fix voltage scaling and add missing bindings
  4. View patch

[PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt

Taha Ed-Dafili posted 5 patches 4 weeks ago
[PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt
Posted by Taha Ed-Dafili 4 weeks ago 
The AD5504 full-scale range is hardware-determined by the
R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
VCC regulator voltage to calculate the scale.

Update the probe function to read the standard "output-range-microvolt"
property as a two-element array to determine the correct full-scale range.
Use the MILLI macro for clearer millivolt assignments and simplify the
probe logic using a local device pointer.

Suggested-by: David Lechner <dlechner@baylibre.com>
Suggested-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Taha Ed-Dafili <0rayn.dev@gmail.com>
---
 drivers/iio/dac/ad5504.c | 43 ++++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/dac/ad5504.c b/drivers/iio/dac/ad5504.c
index e71218c44982..cd563460fc0a 100644
--- a/drivers/iio/dac/ad5504.c
+++ b/drivers/iio/dac/ad5504.c
@@ -12,10 +12,12 @@
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -271,28 +273,31 @@ static const struct iio_chan_spec ad5504_channels[] = {
 
 static int ad5504_probe(struct spi_device *spi)
 {
-	const struct ad5504_platform_data *pdata = dev_get_platdata(&spi->dev);
+	struct device *dev = &spi->dev;
+	const struct ad5504_platform_data *pdata = dev_get_platdata(dev);
 	struct iio_dev *indio_dev;
 	struct ad5504_state *st;
 	int ret;
+	u32 range[2];
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	st = iio_priv(indio_dev);
 
-	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
-	if (ret < 0 && ret != -ENODEV)
+	ret = devm_regulator_get_enable(dev, "vcc");
+	if (ret)
 		return ret;
-	if (ret == -ENODEV) {
-		if (pdata->vref_mv)
-			st->vref_mv = pdata->vref_mv;
-		else
-			dev_warn(&spi->dev, "reference voltage unspecified\n");
-	} else {
-		st->vref_mv = ret / 1000;
-	}
+
+	st->vref_mv = 60 * MILLI;
+	ret = device_property_read_u32_array(dev, "output-range-microvolt",
+					     range, 2);
+	if (!ret && range[1] == 30 * MICRO)
+		st->vref_mv = 30 * MILLI;
+
+	if (pdata && pdata->vref_mv)
+		st->vref_mv = pdata->vref_mv;
 
 	st->spi = spi;
 	indio_dev->name = spi_get_device_id(st->spi)->name;
@@ -305,17 +310,17 @@ static int ad5504_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	if (spi->irq) {
-		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
-					   NULL,
-					   &ad5504_event_handler,
-					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					   spi_get_device_id(st->spi)->name,
-					   indio_dev);
+		ret = devm_request_threaded_irq(dev, spi->irq,
+						NULL,
+						&ad5504_event_handler,
+						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+						spi_get_device_id(st->spi)->name,
+						indio_dev);
 		if (ret)
 			return ret;
 	}
 
-	return devm_iio_device_register(&spi->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static const struct spi_device_id ad5504_id[] = {
-- 
2.47.3
Re: [PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt
Posted by Krzysztof Kozlowski 4 weeks ago 
On 10/03/2026 18:48, Taha Ed-Dafili wrote:
> The AD5504 full-scale range is hardware-determined by the
> R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
> VCC regulator voltage to calculate the scale.
> 
> Update the probe function to read the standard "output-range-microvolt"
> property as a two-element array to determine the correct full-scale range.
> Use the MILLI macro for clearer millivolt assignments and simplify the
> probe logic using a local device pointer.
> 
> Suggested-by: David Lechner <dlechner@baylibre.com>
> Suggested-by: Andy Shevchenko <andy@kernel.org>

Where were these suggestions given?

Best regards,
Krzysztof
Re: [PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt
Posted by Andy Shevchenko 4 weeks ago 
On Tue, Mar 10, 2026 at 05:48:34PM +0000, Taha Ed-Dafili wrote:
> The AD5504 full-scale range is hardware-determined by the
> R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
> VCC regulator voltage to calculate the scale.
> 
> Update the probe function to read the standard "output-range-microvolt"
> property as a two-element array to determine the correct full-scale range.
> Use the MILLI macro for clearer millivolt assignments and simplify the
> probe logic using a local device pointer.

...

>  static int ad5504_probe(struct spi_device *spi)
>  {
> -	const struct ad5504_platform_data *pdata = dev_get_platdata(&spi->dev);
> +	struct device *dev = &spi->dev;
> +	const struct ad5504_platform_data *pdata = dev_get_platdata(dev);
>  	struct iio_dev *indio_dev;
>  	struct ad5504_state *st;
>  	int ret;
> +	u32 range[2];

Preserve the reversed xmas tree order.

> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));

Unrelated change. This should be split into another patch that makes use
of it here and there.

I have a déjà vu about these comments...

>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	st = iio_priv(indio_dev);
>  
> -	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
> -	if (ret < 0 && ret != -ENODEV)
> +	ret = devm_regulator_get_enable(dev, "vcc");
> +	if (ret)
>  		return ret;

> -	if (ret == -ENODEV) {

Why remove this condition?

This might break use of the driver on ACPI systems.

> -		if (pdata->vref_mv)
> -			st->vref_mv = pdata->vref_mv;
> -		else
> -			dev_warn(&spi->dev, "reference voltage unspecified\n");
> -	} else {
> -		st->vref_mv = ret / 1000;
> -	}
> +
> +	st->vref_mv = 60 * MILLI;
> +	ret = device_property_read_u32_array(dev, "output-range-microvolt",
> +					     range, 2);

ARRAY_SIZE()
(will require array_size.h)

> +	if (!ret && range[1] == 30 * MICRO)
> +		st->vref_mv = 30 * MILLI;

This looks unusual and hard to follow. It also misses the validation
of the min of the range.

> +	if (pdata && pdata->vref_mv)
> +		st->vref_mv = pdata->vref_mv;

No, pdata should go.

>  
>  	st->spi = spi;
>  	indio_dev->name = spi_get_device_id(st->spi)->name;

...

>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	if (spi->irq) {
> -		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> -					   NULL,
> -					   &ad5504_event_handler,
> -					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -					   spi_get_device_id(st->spi)->name,
> -					   indio_dev);
> +		ret = devm_request_threaded_irq(dev, spi->irq,
> +						NULL,
> +						&ad5504_event_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +						spi_get_device_id(st->spi)->name,
> +						indio_dev);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	return devm_iio_device_register(&spi->dev, indio_dev);
> +	return devm_iio_device_register(dev, indio_dev);
>  }

Unrelated changes.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 3/5] iio: dac: ad5504: Align headers with IWYU principle
Posted by Taha Ed-Dafili 3 weeks, 6 days ago 
Hi Andy,

Thank you for the detailed review across the series.

> >  #include <linux/device.h>
> Is this still being used directly.

Yes, it is still needed for `struct device` and `dev_get_platdata()`.

> >  #include <linux/kernel.h>
> And what is this for?

You are right, this appears to be a leftover. I will double-check its
usage and drop it in v3 if it's no longer needed.

> The rest of the changes looks correct, but still missing headers:
> linux/kstrtox.h
> 
> asm/byteorder.h
> 
> Might be more.

Good catch on these. I will add <asm/byteorder.h> (for cpu_to_be16()) and
<linux/kstrtox.h>. I will also do another general pass over the includes
to better align with IWYU principles before sending v3.

Regarding the probe function (Patch 4):

I will split the `struct device *dev = &spi->dev;` refactoring into its
own preparatory patch. For the scale calculation, I will restore the
`-ENODEV` check to maintain ACPI compatibility, use `ARRAY_SIZE()`
(adding <linux/array_size.h>), validate the range array indices properly,
and drop the legacy `pdata` fallback.

I will include all of these fixes in v3.

Best regards,
Taha
Re: [PATCH v2 4/5] iio: dac: ad5504: fix scale via output-range-microvolt
Posted by Nuno Sá 3 weeks, 6 days ago 
On Tue, 2026-03-10 at 21:16 +0200, Andy Shevchenko wrote:
> On Tue, Mar 10, 2026 at 05:48:34PM +0000, Taha Ed-Dafili wrote:
> > The AD5504 full-scale range is hardware-determined by the
> > R_SEL pin (0-30V or 0-60V). Previously, the driver incorrectly used the
> > VCC regulator voltage to calculate the scale.
> > 
> > Update the probe function to read the standard "output-range-microvolt"
> > property as a two-element array to determine the correct full-scale range.
> > Use the MILLI macro for clearer millivolt assignments and simplify the
> > probe logic using a local device pointer.
> 
> ...
> 
> >  static int ad5504_probe(struct spi_device *spi)
> >  {
> > -	const struct ad5504_platform_data *pdata = dev_get_platdata(&spi->dev);
> > +	struct device *dev = &spi->dev;
> > +	const struct ad5504_platform_data *pdata = dev_get_platdata(dev);
> >  	struct iio_dev *indio_dev;
> >  	struct ad5504_state *st;
> >  	int ret;
> > +	u32 range[2];
> 
> Preserve the reversed xmas tree order.
> 
> > -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> 
> Unrelated change. This should be split into another patch that makes use
> of it here and there.
> 
> I have a déjà vu about these comments...
> 
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> >  
> >  	st = iio_priv(indio_dev);
> >  
> > -	ret = devm_regulator_get_enable_read_voltage(&spi->dev, "vcc");
> > -	if (ret < 0 && ret != -ENODEV)
> > +	ret = devm_regulator_get_enable(dev, "vcc");
> > +	if (ret)
> >  		return ret;
> 
> > -	if (ret == -ENODEV) {
> 
> Why remove this condition?
> 
> This might break use of the driver on ACPI systems.
> 
> > -		if (pdata->vref_mv)
> > -			st->vref_mv = pdata->vref_mv;
> > -		else
> > -			dev_warn(&spi->dev, "reference voltage unspecified\n");
> > -	} else {
> > -		st->vref_mv = ret / 1000;
> > -	}
> > +
> > +	st->vref_mv = 60 * MILLI;
> > +	ret = device_property_read_u32_array(dev, "output-range-microvolt",
> > +					     range, 2);
> 
> ARRAY_SIZE()
> (will require array_size.h)
> 
> > +	if (!ret && range[1] == 30 * MICRO)
> > +		st->vref_mv = 30 * MILLI;
> 
> This looks unusual and hard to follow. It also misses the validation
> of the min of the range.

And the max FWIW. If the property is given we should be strict about it and make
sure only valid inputs are given. 

- Nuno Sá
> 
> > +	if (pdata && pdata->vref_mv)
> > +		st->vref_mv = pdata->vref_mv;
> 
> No, pdata should go.
> 
> >  
> >  	st->spi = spi;
> >  	indio_dev->name = spi_get_device_id(st->spi)->name;
> 
> ...
> 
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> >  	if (spi->irq) {
> > -		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
> > -					   NULL,
> > -					   &ad5504_event_handler,
> > -					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > -					   spi_get_device_id(st->spi)->name,
> > -					   indio_dev);
> > +		ret = devm_request_threaded_irq(dev, spi->irq,
> > +						NULL,
> > +						&ad5504_event_handler,
> > +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +						spi_get_device_id(st->spi)->name,
> > +						indio_dev);
> >  		if (ret)
> >  			return ret;
> >  	}
> >  
> > -	return devm_iio_device_register(&spi->dev, indio_dev);
> > +	return devm_iio_device_register(dev, indio_dev);
> >  }
> 
> Unrelated changes.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.