RE: [PATCH v2 4/4] iio: dac: ad3530r: Add support for AD3532R/AD3532

[Paller, Kim Seer]

> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Sent: Monday, June 15, 2026 6:06 PM
> To: Paller, Kim Seer <KimSeer.Paller@xxxxxxxxxx>
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; David Lechner
> <dlechner@xxxxxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Andy
> Shevchenko <andy@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Rob Herring <robh@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley <conor+dt@xxxxxxxxxx>; linux-
> iio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux <linux@xxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 4/4] iio: dac: ad3530r: Add support for
> AD3532R/AD3532
> 
> [External]
> 
> On Mon, Jun 15, 2026 at 02:20:18PM +0800, Kim Seer Paller wrote:
> > The AD3532R/AD3532 is a 16-channel, 16-bit voltage output DAC with a
> > dual-bank register architecture (bank 0 at 0x1000 for channels 0-7,
> > bank 1 at 0x3000 for channels 8-15). It shares similar functionality
> > with AD3530R (channel configuration, LDAC triggering, powerdown
> > control), the main difference being the register address map due to
> > the dual-bank architecture, handled by table-driven helpers.
> >
> > Add AD3532R-specific register definitions, channel specs, per-bank
> > register arrays, a dedicated ad3532r_set_dac_powerdown(), and per-chip
> > regmap_config to limit debugfs-exposed register space to each
> > variant's actual address range.
> 
> ...
> 
> 
> >  	help
> > -	  Say yes here to build support for Analog Devices AD3530R, AD3531R
> > -	  Digital to Analog Converter.
> > +	  Say yes here to build support for Analog Devices AD3530/AD3530R,
> > +	  AD3531/AD3531R, and AD3532/AD3532R Digital to Analog
> Converters.
> 
> This just shows how unscalable the above text is. That's why we usually
> recommend to make the list explicit and separated.
> 
> 	  Say yes here to build support for the following Analog Devices
> 	  Digital to Analog Converters:
> 	  - AD3530/AD3530R (8-channel)
> 	  - AD3531/AD3531R (4-channel)
> 	  - AD3532/AD3532R (16-channel)
> 
> (and looking into the C-file change, perhaps add here as well distinctive
> information, such as number of channels, in the parentheses).
> 
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called ad3530r.
> 
> ...
> 
> > +#define AD3532R_INTERFACE_CONFIG_A_0		0x1000
> > +#define AD3532R_INTERFACE_CONFIG_A_1		0x3000
> > +#define AD3532R_OUTPUT_OPERATING_MODE_0		0x1020
> > +#define AD3532R_OUTPUT_OPERATING_MODE_1		0x1021
> > +#define AD3532R_OUTPUT_OPERATING_MODE_2		0x3020
> > +#define AD3532R_OUTPUT_OPERATING_MODE_3		0x3021
> > +#define AD3532R_OUTPUT_CONTROL_0		0x102A
> > +#define AD3532R_OUTPUT_CONTROL_1		0x302A
> > +#define AD3532R_REFERENCE_CONTROL_0		0x103C
> > +#define AD3532R_REFERENCE_CONTROL_1		0x303C
> > +#define AD3532R_SW_LDAC_TRIG_0			0x10E5
> > +#define AD3532R_SW_LDAC_TRIG_1			0x30E5
> > +#define AD3532R_INPUT_CH_0			0x10EB
> > +#define AD3532R_INPUT_CH_1			0x30EB
> > +#define AD3532R_MAX_REG_ADDR			0x30F9
> 
> Hmm... I dunno if it's better to sort by values (so the "bank" 0 goes together
> followed by "bank" 1). Jonathan, what's your preference here? Nuno, David?
> 
> ...
> 
> > +static ssize_t ad3532r_set_dac_powerdown(struct iio_dev *indio_dev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 const char *buf, size_t len)
> > +{
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +	unsigned int reg, pdmode, mask, val, local_ch;
> > +	bool powerdown;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &powerdown);
> 
> Do you need to include kstrtox.h?

Yes, kstrtobool() is declared in <linux/kstrtox.h>. I also ran IWYU and
it confirms <linux/kstrtox.h> belongs in the include list.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&st->lock);
> 
> + blank line here.
> 
> > +	local_ch = chan->channel % AD3530R_CH_PER_BANK;
> 
> > +	reg = (chan->channel < AD3530R_CH_PER_BANK ?
> AD3532R_OUTPUT_OPERATING_MODE_0 :
> > +	       AD3532R_OUTPUT_OPERATING_MODE_2) +
> > +	       local_ch / AD3530R_CH_PER_REG;
> 
> This is unreadable. Can you refactor it?

Would this be clearer?

unsigned int bank_base;

local_ch = chan->channel % AD3530R_CH_PER_BANK;
bank_base = chan->channel < AD3530R_CH_PER_BANK ?
	    AD3532R_OUTPUT_OPERATING_MODE_0 : AD3532R_OUTPUT_OPERATING_MODE_2;
reg = bank_base + local_ch / AD3530R_CH_PER_REG;

> 
> > +	mask = AD3530R_OP_MODE_CHAN_MSK(local_ch %
> AD3530R_CH_PER_REG);
> > +
> > +	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode :
> 0;
> > +	val = field_prep(mask, pdmode);
> > +
> > +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->chan[chan->channel].powerdown = powerdown;
> > +
> > +	return len;
> > +}
> 
> ...
> 
> > +	.num_banks = ARRAY_SIZE(ad3532r_if_config),
> 
> Also check if array_size.h is included.
> 
> --
> With Best Regards,
> Andy Shevchenko
>