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

David Lechner
2025-04-25T16:48:53Z

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)