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

From: Andy Shevchenko <andriy.shevchenko@intel.com>
To: Janani Sunil <janani.sunil@analog.com>
Cc: "Lars-Peter Clausen" <lars@metafoo.de>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"David Lechner" <dlechner@baylibre.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	"Andy Shevchenko" <andy@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Kozlowski" <krzk+dt@kernel.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Shuah Khan" <skhan@linuxfoundation.org>,
	"Mark Brown" <broonie@kernel.org>,
	"Marius Cristea" <marius.cristea@microchip.com>,
	"Marcus Folkesson" <marcus.folkesson@gmail.com>,
	"Kent Gustavsson" <kent@minoris.se>,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Daire McNamara" <daire.mcnamara@microchip.com>,
	"Paul Walmsley" <pjw@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Alexandre Ghiti" <alex@ghiti.fr>,
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	"Janani Sunil" <jan.sun97@gmail.com>,
	linux-spi@vger.kernel.org, "Kent Gustavsson" <nedo80@gmail.com>,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH v11 3/3] iio: dac: Add AD5529R DAC driver support
Date: Wed, 2 Sep 2026 15:56:49 +0300	[thread overview]
Message-ID: <apgdEf3pi_soGr3T@ashevche-desk.local> (raw)
In-Reply-To: <20260902-ad5529r-driver-v11-3-301c491a0719@analog.com>

On Wed, Sep 02, 2026 at 11:26:17AM +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.

Since there might be a v12 (depends on Jonathan), a few more nit-picks.

...

> +static const s32 ad5529r_output_ranges_mV[8][2] = {
> +	[AD5529R_RANGE_0V_5V] = { 0, 5000 },
> +	[AD5529R_RANGE_0V_10V] = { 0, 10000 },
> +	[AD5529R_RANGE_0V_20V] = { 0, 20000 },
> +	[AD5529R_RANGE_0V_40V] = { 0, 40000 },
> +	[AD5529R_RANGE_M5V_5V] = { -5000, 5000 },
> +	[AD5529R_RANGE_M10V_10V] = { -10000, 10000 },
> +	[AD5529R_RANGE_M15V_15V] = { -15000, 15000 },
> +	[AD5529R_RANGE_M20V_20V] = { -20000, 20000 },
> +};

Perhaps use MILLI multiplier?

...

> +static int ad5529r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ad5529r_state *st = iio_priv(indio_dev);
> +	unsigned int reg_addr, reg_val_h;
> +	int ret, range_idx, span_mV;

Why is 'range_idx' signed?

> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		/*
> +		 * Read from DAC_INPUT_A register rather than DAC_DATA_READBACK.
> +		 * The DAC operates in transparent mode and directly reflects
> +		 * whatever value is written to the INPUT_A register.
> +		 */
> +		reg_addr = AD5529R_REG_DAC_INPUT_A(chan->channel);
> +		ret = regmap_read(st->regmap_16bit, reg_addr, &reg_val_h);
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_val_h;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		range_idx = st->output_range_idx[chan->channel];
> +
> +		/*
> +		 * The datasheet specifies a 4.096 V external reference,
> +		 * matching the nominal output voltage of the internal
> +		 * reference.
> +		 */
> +		span_mV = ad5529r_output_ranges_mV[range_idx][1] -
> +			  ad5529r_output_ranges_mV[range_idx][0];
> +		*val = span_mV;
> +		*val2 = st->model_data->resolution;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		range_idx = st->output_range_idx[chan->channel];
> +
> +		if (ad5529r_output_ranges_mV[range_idx][0] < 0)
> +			*val = -BIT(st->model_data->resolution - 1);
> +		else
> +			*val = 0;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko