Re: [PATCH v5 3/3] iio: dac: Add AD5529R DAC driver support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Wed, Jul 01, 2026 at 08:40:41AM +0200, Janani Sunil wrote:
> Add support for AD5529R 16-channel, 12/16 bit Digital to Analog Converter
> from Analog Devices.
> 
> The device communicates over SPI and supports per-channel output range
> configuration. An optional external 4.096V reference can be used in
> place of the internal reference.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
A couple of nit-picks below.

...

> +static int ad5529r_find_output_range(const s32 *vals)
> +{
> +	for (unsigned int i = 0; i < ARRAY_SIZE(ad5529r_output_ranges_mV); i++) {
> +		if (vals[0] == ad5529r_output_ranges_mV[i][0] * 1000 &&
> +		    vals[1] == ad5529r_output_ranges_mV[i][1] * 1000)

So, the correct way is to have either defined constant in units.h or while now
(MICRO / MILLI). So, with a temporary pointer this can be achieved without
uglifying the code

		... *range = &ad5529r_output_ranges_mV[i];

		if (vals[0] == range[0] * (MICRO / MILLI) &&
		    vals[1] == range[1] * (MICRO / MILLI))

> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}

...

> +	/*
> +	 * The datasheet mentions a 4.096V external reference for correct
> +	 * operation.
> +	 */
> +	ret = devm_regulator_get_enable_optional(dev, "vref");
> +	if (ret == -ENODEV) {
> +		external_vref = false;
> +	} else if (ret) {
> +		return dev_err_probe(dev, ret,
> +				     "Failed to get and enable vref regulator\n");
> +	} else {
> +		external_vref = true;
> +	}

All branches are single statement. No {} are needed.

-- 
With Best Regards,
Andy Shevchenko