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

[Nuno Sá]

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.