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

[Andy Shevchenko]

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