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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 21 Apr 2025 12:24:54 +0800
Kim Seer Paller <kimseer.paller@analog.com> 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.
> 
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Hi.

Just one thing from a final pre merge look through.

The initialization of powerdown mode works but only because the NORMAL
mode == 0.  That should be setting it explicitly for each set of 4 channels
as needed.

I don't really mind how you solve that.  There are lots of options
to build up the 4 fields in each of those registers.

Jonathan

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..05bd191e5225bd267f42ba36bbd42a18e6f22291
> --- /dev/null
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -0,0 +1,503 @@

> +
> +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;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	mask = GENMASK(chan->address + 1, chan->address);

I think maybe we need a macro to get the mask from the channel number?
Using address for this seems overkill given how simple that maths is.
Ideally that macro could perhaps be used in the code below to avoid
all the defines I suggested.


> +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> +	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +	       << chan->address;
> +


> +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> +			 bool has_external_vref)
> +{

> +
> +	if (has_external_vref)
> +		st->vref_mv = range_multiplier * vref / 1000;
> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
> +			   AD3530R_NORMAL_OPERATION);
Is this actually doing that?  I think the register is lots of 2 bit
fields and this is only setting it for the first channel?

This works because that value is 0.  Logically however we should set it
to
	(AD3530R_NORMAL_OPERATION << 6) |
	(AD3530R_NORMAL_OPERATION << 4) |
	(AD3530R_NORMAL_OPERATION << 2) |
	(AD3530R_NORMAL_OPERATION << 0)

Or possibly better as

	FIELD_PREP(AD3530R_OP_MODE_0_CHAN0_MSK, AD3530R_NORMAL_OPERATION) |
	FIELD_PREP(AD3530R_OP_MODE_0_CHAN1_MSK, AD3530R_NORMAL_OPERATION) |

etc

Names are a bit long, so maybe consider shortening some of the defines.

> +	if (ret)
> +		return ret;
> +
> +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {

If we have it explicit that we have multiple fields this will become more obvious.
However I'd use the number 4 here rather than the number of channels the AD3531R happens
to have.


> +		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
> +				   AD3530R_NORMAL_OPERATION);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < st->chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad3530r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = AD3530R_MAX_REG_ADDR,
> +};