Re: [PATCH v6 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[David Lechner <dlechner@baylibre.com>]

On 4/25/25 7:54 AM, Kim Seer Paller wrote:
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.
> 
> Support for monitoring internal die temperature, output voltages, and
> current of a selected channel via the MUXOUT pin using an external ADC
> is currently not implemented.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---

Reviewed-by: David Lechner <dlechner@baylibre.com>

Just a few small things in the latest changes that could be improved...


> +#define AD3530R_SLD_TRIG_A			BIT(7)
> +#define AD3530R_OUTPUT_CONTROL_RANGE		BIT(2)
> +#define AD3530R_REFERENCE_CONTROL_SEL		BIT(0)
> +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> +#define AD3530R_OP_MODE_CHAN_MSK(chan)		(GENMASK(1, 0) << 2 * chan)

For safety:

#define AD3530R_OP_MODE_CHAN_MSK(chan)		(GENMASK(1, 0) << 2 * (chan))

...

> +static ssize_t ad3530r_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);
> +	int ret;
> +	unsigned int mask, val, reg, pdmode;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	mask = AD3530R_OP_MODE_CHAN_MSK(chan->channel);
> +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> +	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
> +	val = pdmode << 2 * chan->channel;

#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))

	mask = AD3530R_OP_MODE_CHAN_MSK(chan->channel);
	val = field_prep(mask, pdmode);


(Maybe someday this will be standardized: https://lore.kernel.org/all/cover.1739540679.git.geert+renesas@glider.be/)

> +
> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	st->chan[chan->channel].powerdown = powerdown;
> +
> +	return len;
> +}

...

> +static int ad3530r_setup(struct ad3530r_state *st, int vref)
> +{
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct gpio_desc *reset_gpio;
> +	int i, ret;
> +	u8 range_multiplier, val;
> +
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> +				     "Failed to get reset GPIO\n");
> +
> +	if (reset_gpio) {
> +		/* Perform hardware reset */
> +		fsleep(1 * USEC_PER_MSEC);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		/* Perform software reset */
> +		ret = regmap_update_bits(st->regmap, AD3530R_INTERFACE_CONFIG_A,
> +					 AD3530R_SW_RESET, AD3530R_SW_RESET);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fsleep(10 * USEC_PER_MSEC);
> +
> +	range_multiplier = 1;
> +	if (device_property_read_bool(dev, "adi,range-double")) {
> +		ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> +				      AD3530R_OUTPUT_CONTROL_RANGE);
> +		if (ret)
> +			return ret;
> +
> +		range_multiplier = 2;
> +	}
> +
> +	if (st->chip_info->internal_ref_support && vref == 0) {

At this point, we've already validated that if vref == 0 then
st->chip_info->internal_ref_support is true, so the check here is a bit
redundant.

> +		ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> +				      AD3530R_REFERENCE_CONTROL_SEL);
> +		if (ret)
> +			return ret;
> +
> +		st->vref_mV = range_multiplier * AD3530R_INTERNAL_VREF_mV;
> +	}
> +
> +	if (vref > 0)
> +		st->vref_mV = range_multiplier * vref / MILLI;

And we've already validated that vref >= 0. So slightly simpler would be:


	if (external_vref_uV) 
		st->vref_mV = range_multiplier * external_vref_uV / MILLI;
	} else {
		ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
				      AD3530R_REFERENCE_CONTROL_SEL);
		if (ret)
			return ret;

		st->vref_mV = range_multiplier * AD3530R_INTERNAL_VREF_mV;
	}


(I also renamed vref to external_vref_uV to make it more obvious where the value
is coming from and what the units are)