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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 16 Apr 2025 14:23:11 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/12/25 12:57 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>
> > ---  
> 
> Looks very good now. :-)
> 
> A made a few comments but maybe nothing serious enough to require a v5.

We have plenty of time in the cycle, so I think I would prefer a v5
just tidying up these last few bits.

Thanks,

Jonathan

> 
> > diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..ffa04f678b86d8da6f5e47c35c265b6648121843
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3530r.c
> > @@ -0,0 +1,506 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * AD3530R/AD3530 8-channel, 16-bit Voltage Output DAC Driver
> > + * AD3531R/AD3531 4-channel, 16-bit Voltage Output DAC Driver
> > + *
> > + * Copyright 2025 Analog Devices Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/kernel.h>  
> 
> Usually, we try to avoid including kernel.h - it includes too much.
> 
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +  
> ...
> 
> > +
> > +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> > +			 bool has_external_vref)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct gpio_desc *reset_gpio;
> > +	int i, ret;
> > +	bool has_range_multiplier;
> > +
> > +	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);
> > +
> > +	has_range_multiplier = false;
> > +	if (device_property_present(dev, "adi,range-double")) {  
> 
> Since this is a flag, I think device_property_read_bool() is preferred.
> 
> > +		ret = regmap_set_bits(st->regmap, AD3530R_OUTPUT_CONTROL_0,
> > +				      AD3530R_OUTPUT_CONTROL_RANGE);
> > +		if (ret)
> > +			return ret;
> > +
> > +		has_range_multiplier = true;
> > +	}
> > +
> > +	if (!has_external_vref && st->chip_info->internal_ref_support) {
> > +		ret = regmap_set_bits(st->regmap, AD3530R_REFERENCE_CONTROL_0,
> > +				      AD3530R_REFERENCE_CONTROL_SEL);
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->vref_mv = has_range_multiplier ?
> > +			      2 * AD3530R_INTERNAL_VREF_MV :
> > +			      AD3530R_INTERNAL_VREF_MV;
> > +	}
> > +
> > +	if (has_external_vref)
> > +		st->vref_mv = has_range_multiplier ? 2 * vref / 1000 : vref / 1000;
> > +  
> 
> I think this would be simpler as:
> 
> 	st->vref_mv = range_multiplier * vref / 1000;
> 
> where range_multiplier is 1 or 2.
> 
> > +	/* Set operating mode to normal operation. */
> > +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
> > +			   AD3530R_NORMAL_OPERATION);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> > +		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_HIGH);  
> 
> I guess it doesn't matter which state this starts in but GPIOD_OUT_LOW seems
> more natural since we toggle it high the low later.
> 
> > +	if (IS_ERR(st->ldac_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> > +				     "Failed to get ldac GPIO\n");
> > +
> > +	return 0;
> > +}
> > +