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

[David Lechner]

On 4/3/25 12:33 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.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>
> ---

...

> +#define AD3530R_INTERFACE_CONFIG_A		0x00
> +#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
> +#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> +#define AD3530R_OUTPUT_CONTROL_0		0x2A
> +#define AD3530R_REFERENCE_CONTROL_0		0x3C
> +#define AD3530R_SW_LDAC_TRIG_A			0xE5
> +#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
> +
> +#define AD3531R_SW_LDAC_TRIG_A			0xDD
> +#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)
> +
> +#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
> +#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
> +#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
> +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> +
> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4
> +#define AD3530R_CH(c)				(c)
> +#define AD3530R_32KOHM_POWERDOWN_MODE		3
> +#define AD3530R_INTERNAL_VREF_MV		2500
> +#define AD3530R_LDAC_PULSE_US			100
> +
> +struct ad3530r_chan {
> +	unsigned int powerdown_mode;

IMHO, code would be easier to understand if this was an enum.

> +	bool powerdown;
> +};
> +
> +struct ad3530r_chip_info {
> +	const char *name;
> +	const struct iio_chan_spec *channels;
> +	int (*input_ch_reg)(unsigned int c);
> +	const int iio_chan;
> +	unsigned int num_channels;
> +	unsigned int sw_ldac_trig_reg;
> +	bool internal_ref_support;
> +};
> +
> +struct ad3530r_state {
> +	struct regmap *regmap;
> +	/* lock to protect against multiple access to the device and shared data */
> +	struct mutex lock;
> +	struct ad3530r_chan chan[AD3530R_MAX_CHANNELS];
> +	const struct ad3530r_chip_info *chip_info;
> +	struct gpio_desc *ldac_gpio;
> +	int vref_mv;
> +	u8 ldac;
> +	bool range_multiplier;

nit: call this has_range_multiplier or change to u8 and save the actual multipler value here.

> +};
> +

...

> +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;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	switch (chan->channel) {
> +	case AD3530R_CH(0) ... AD3530R_CH(AD3531R_MAX_CHANNELS - 1):

Switch seems hard to read and not needed since there aren't any gaps.

	if (chan->channel < AD3531R_MAX_CHANNELS)

shoud work, no?


> +		mask = GENMASK(chan->channel * 2 + 1, chan->channel * 2);

Could use the chan->address field to hide the calucation a bit.

> +		val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +		      << (chan->channel * 2);

I saw a series that proposed a non-const field_prep function recently. Not sure
that made it through the bikeshedding though. Could be nice to hide some of this
in a macro for readability if that isn't available.

> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD3530R_OUTPUT_OPERATING_MODE_0,
> +					 mask, val);
> +		if (ret)
> +			return ret;
> +
> +		st->chan[chan->channel].powerdown = powerdown;
> +		return len;
> +	case AD3530R_CH(AD3531R_MAX_CHANNELS) ...
> +	     AD3530R_CH(AD3530R_MAX_CHANNELS - 1):
> +		mask = GENMASK((chan->channel - 4) * 2 + 1,
> +			       (chan->channel - 4) * 2);
> +		val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +		      << ((chan->channel - 4) * 2);
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD3530R_OUTPUT_OPERATING_MODE_1,
> +					 mask, val);
> +		if (ret)
> +			return ret;
> +
> +		st->chan[chan->channel].powerdown = powerdown;
> +		return len;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad3530r_trigger_hw_ldac(struct gpio_desc *ldac_gpio)
> +{
> +	gpiod_set_value_cansleep(ldac_gpio, 1);
> +	fsleep(AD3530R_LDAC_PULSE_US);
> +	gpiod_set_value_cansleep(ldac_gpio, 0);
> +
> +	return 0;
> +}
> +
> +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> +			     unsigned int val)
> +{
> +	int ret;
> +	__be16 reg_val;
> +
> +	guard(mutex)(&st->lock);
> +	reg_val = cpu_to_be16(val);
> +
> +	ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
> +				&reg_val, sizeof(reg_val));
> +	if (ret)
> +		return ret;
> +
> +	if (st->ldac_gpio)
> +		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> +
> +	return regmap_update_bits(st->regmap, st->chip_info->sw_ldac_trig_reg,
> +				  AD3530R_SW_LDAC_TRIG_MASK,
> +				  FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1));

Can be simplifed with regmap_set_bits().


> +}
> +
> +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long info)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	__be16 reg_val;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(st->regmap,
> +				       st->chip_info->input_ch_reg(chan->channel),
> +				       &reg_val, sizeof(reg_val));
> +		if (ret)
> +			return ret;
> +
> +		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(reg_val));
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 16;

This needs to take into account the range multipler.

> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> +			      ad3530r_get_dac_powerdown,
> +			      ad3530r_set_dac_powerdown),
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> +			   &ad3530r_powerdown_mode_enum),
> +	{ },

iio style is no trailing comma on sentinil  

	{ }

> +};
> +

...

> +static int ad3530r_setup(struct ad3530r_state *st)
> +{
> +	const struct ad3530r_chip_info *chip_info = st->chip_info;
> +	struct device *dev = regmap_get_device(st->regmap);
> +	struct gpio_desc *reset_gpio;
> +	int i, ret;
> +
> +	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(1000);
> +		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(10000);
> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> +		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1, 0);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_32KOHM_POWERDOWN_MODE;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	if (device_property_present(dev, "adi,double-output-range")) {

This is not documented in the DT bindings.

This could instead be implemented by making the out_voltage_scale attribute
writeable.

If we really do need it in the devicetree because we want to protect against
someone accidentally setting it too high, then the property should be the
actual multipler value with an enum specifier of 1, 2 and a default of 1
rather than a flag (e.g. adi,output-multipler).

> +		st->range_multiplier = true;
> +
> +		return regmap_update_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> +					  AD3530R_OUTPUT_CONTROL_MASK,
> +					  FIELD_PREP(AD3530R_OUTPUT_CONTROL_MASK, 1));

nit: can be simplified with regmap_set_bits().

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad3530r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,

Should have at least a .max_register.

.reg_read and .reg_write tables can make debuging easier too.

> +};
> +
> +static const struct iio_info ad3530r_info = {
> +	.read_raw = ad3530r_read_raw,
> +	.write_raw = ad3530r_write_raw,
> +	.debugfs_reg_access = &ad3530r_reg_access,

nit: style is not consistent - can omit &.

> +};
> +
> +static int ad3530r_probe(struct spi_device *spi)
> +{
> +	static const char * const regulators[] = { "vdd", "iovdd" };
> +	const struct ad3530r_chip_info *chip_info;
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad3530r_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->regmap = devm_regmap_init_spi(spi, &ad3530r_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Failed to init regmap");
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	chip_info = spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return -ENODEV;
> +
> +	st->chip_info = chip_info;

nit: local variable isn't that useful since st->chip_info is short enough.

> +
> +	ret = ad3530r_setup(st);
> +	if (ret)
> +		return ret;

Setting up the chip before turning on power would not work well if the
regulators are actually controlled. :-)

> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
> +					     regulators);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (ret < 0 && ret != -ENODEV)
> +		return ret;
> +
> +	if (chip_info->internal_ref_support && ret == -ENODEV) {
> +		/* Internal reference. */
> +		ret = regmap_update_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> +					 AD3530R_REFERENCE_CONTROL_MASK,
> +					 FIELD_PREP(AD3530R_REFERENCE_CONTROL_MASK, 1));
> +		if (ret)
> +			return ret;
> +
> +		st->vref_mv = st->range_multiplier ?
> +			      2 * AD3530R_INTERNAL_VREF_MV :
> +			      AD3530R_INTERNAL_VREF_MV;
> +	} else {
> +		st->vref_mv = st->range_multiplier ? 2 * ret / 1000 : ret / 1000;

ret can be -ENODEV here if !chip_info->internal_ref_support

> +	}> +
> +	indio_dev->name = chip_info->name;
> +	indio_dev->info = &ad3530r_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = chip_info->channels;
> +	indio_dev->num_channels = chip_info->num_channels;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +