Re: [PATCH v2 4/4] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

[Jonathan Cameron]

On Mon, 24 Mar 2025 19:22:58 +0800
Kim Seer Paller <kimseer.paller@xxxxxxxxxx> wrote:

> The AD3530R/AD3530 is an 8-Channel, 16-Bit Voltage Output DAC, while the
> AD3531R/AD3531 is a 4-Channel, 16-Bit Voltage Output DAC. These devices
> include software-programmable gain controls that provide full-scale
> output spans of 2.5V or 5V for reference voltages of 2.5V. They operate
> from a single supply voltage range of 2.7V to 5.5V and are guaranteed to
> be monotonic by design. Additionally, these devices features a 2.5V,
> 5ppm/°C internal reference, which is disabled by default.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx>

This is a bit of an 'experiment' in reviewing after a red eye flight
so it may be even less coherent than I normally am!

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c

> +
> +struct ad3530r_state {
> +	struct spi_device *spi;

As below. This seems unnecessary.

> +	struct regmap *regmap;
> +	struct mutex lock; /* protect the state of the device */

Be more specific on that comment.  What state needs protecting
beyond the bus locking that also goes on?  Typically look
for sequences that need to be atomic and say what data causes
that to be the problem here.

> +	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;
> +};

> +
> +static int ad3530r_trigger_hw_ldac(struct gpio_desc *ldac_gpio)
> +{
> +	gpiod_set_value_cansleep(ldac_gpio, 0);
> +	usleep_range(AD3530R_LDAC_PULSE_US, AD3530R_LDAC_PULSE_US + 10);

fsleep()  unless this window is there for some reason I'm not seeing.

Maybe just usleep() is more appropriate.

> +	gpiod_set_value_cansleep(ldac_gpio, 1);
> +
> +	return 0;
> +}

> +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, 2);

sizeof(regval) instead of that 2.

> +		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;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = 0;

Don't report an offset at all if it is always zero.  That is the default
when it is not reported.

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

> +
> +#define AD3530R_CHAN_EXT_INFO(_name, _what, _read, _write) {		\
> +	.name = (_name),						\
> +	.read = (_read),						\
> +	.write = (_write),						\
> +	.private = (_what),						\
> +	.shared = IIO_SEPARATE,						\
> +}

Don't define a macro for filling these in unless you need it lots of times.
Just put the full initialisation inline.

> +
> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	AD3530R_CHAN_EXT_INFO("powerdown", 0, 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_ENUM("muxout_select", IIO_SHARED_BY_ALL, &ad3530r_muxout_select_enum),

For the muxout stuff see comments on the ABI docs patch.

> +	IIO_ENUM_AVAILABLE("muxout_select", IIO_SHARED_BY_ALL,
> +			   &ad3530r_muxout_select_enum),
> +	{ },
> +};
> +
> +#define AD3530R_CHAN(_chan) {						\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.output = 1,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE) |		\
> +			      BIT(IIO_CHAN_INFO_OFFSET),		\
> +	.ext_info = ad3530r_ext_info,					\
> +}

...

> +
> +static int ad3530r_setup(struct ad3530r_state *st)
> +{
> +	const struct ad3530r_chip_info *chip_info = st->chip_info;
> +	struct device *dev = &st->spi->dev;

st->spi is only ever used to get the dev.  You can get that 
from the regmap instead.  If that looks too messy for some reason then
you could also just store dev in st.

> +	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 */
> +		usleep_range(20, 25);
> +		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;
> +	}
> +
> +	usleep_range(10000, 15000);

fsleep() preferred.

> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, 0);
> +	if (ret)
> +		return ret;

...

> +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->spi = spi;
> +
> +	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");
> +
> +	mutex_init(&st->lock);
For new drivers, I'd prefer we use 
	ret = devm_mutex_init(&st->lock);
	if (ret)
		return ret;

It only brings a small benefit in debug cases but still nice to have
given we can now do it without explicit mutex_destroy() calls.

> +
> +	chip_info = spi_get_device_match_data(spi);
> +	if (!chip_info)
> +		return -ENODEV;
> +
> +	st->chip_info = chip_info;
> +
> +	ret = ad3530r_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	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 (ret > 0) {

This is going to do something odd if the voltage returned is 0. 
I'd just have ret >= 0 for this case.  We'll never see it in a real system, but
nice to have anyway.

> +		st->vref_mv = st->range_multiplier ? 2 * ret / 1000 : ret / 1000;
> +	} else {
> +		/* 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;
> +	}

Jonathan