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

[Andy Shevchenko <andy@kernel.org>]

On Mon, Apr 21, 2025 at 12:24:54PM +0800, 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.

...

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>

> +#include <linux/device.h>

I don't see how you use this. But

+ dev_printk.h

> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#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>

...

> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))

> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4

Sounds to me that these two shouldn't be grouped with the rest here. Perhaps
move them out to after the LDAC_PULSE?

> +#define AD3530R_INTERNAL_VREF_MV		2500

_mV (yes, with Volts and Amperes we use proper spelling).

> +#define AD3530R_LDAC_PULSE_US			100

...

> +	int (*input_ch_reg)(unsigned int c);

c? channel?

...

> +	int vref_mv;

_mV

...

> +static int ad3530r_input_ch_reg(unsigned int c)
> +{
> +	return 2 * c + AD3530R_INPUT_CH;
> +}
> +
> +static int ad3531r_input_ch_reg(unsigned int c)
> +{
> +	return 2 * c + AD3531R_INPUT_CH;
> +}

c --> channel

...

> +static const char * const ad3530r_powerdown_modes[] = {
> +	"1kohm_to_gnd",

kOhm

> +	"7.7kohm_to_gnd",

Ditto.

> +	"32kohm_to_gnd",

Ditto.

> +};

...

> +static const struct iio_enum ad3530r_powerdown_mode_enum = {
> +	.items = ad3530r_powerdown_modes,
> +	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),

+ array_size.h

> +	.get = ad3530r_get_powerdown_mode,
> +	.set = ad3530r_set_powerdown_mode,
> +};

...

> +static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 char *buf)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	guard(mutex)(&st->lock);
> +	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);

+ sysfs.h

> +}

...

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

+ kstrtox.h

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

Please, move the operator to the previous line, Or even

	... pdmode;

	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode : 0;
	val = pdmode << ...;

> +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> +	if (ret)
> +		return ret;
> +
> +	st->chan[chan->channel].powerdown = powerdown;
> +
> +	return len;
> +}

...

> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val < 0 || val > U16_MAX)

U16_MAX is an abstract type with this limit, do you have any predefined HW
limit instead? Probably better to use that one as defined via BIT() / GENMASK().

> +			return -EINVAL;
> +
> +		return ad3530r_dac_write(st, chan->channel, val);
> +	default:
> +		return -EINVAL;
> +	}

...

> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.shared = IIO_SEPARATE,
> +		.read = ad3530r_get_dac_powerdown,
> +		.write = 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),
> +	{ }
> +};
> +
> +#define AD3530R_CHAN(_chan, _pos) {					\

Slightly better to have the curly braces on the same column as it's easier to
read.

#define AD3530R_CHAN(_chan, _pos)				\
{								\

(and make it one TAB less for the backslash).

> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.address = _pos,						\
> +	.output = 1,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE),			\
> +	.ext_info = ad3530r_ext_info,					\
> +}

...

> +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;
> +	u8 range_multiplier;

+ types.h (and especially for boolean type along with true/false definitions.

> +
> +	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");

+ err.h

> +	if (reset_gpio) {
> +		/* Perform hardware reset */
> +		fsleep(1000);

(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(10000);

10 * USEC_PER_MSEC

With these constants it's less error prone (when 3 or more zeroes) and easier
to get the units without looking into fsleep() implementation / documentation.

> +	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 (!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 = range_multiplier * AD3530R_INTERNAL_VREF_MV;
> +	}
> +
> +	if (has_external_vref)
> +		st->vref_mv = range_multiplier * vref / 1000;

MILLI?


> +	/* 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_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;
> +}

...

> +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (vref < 0 && vref != -ENODEV)
> +		return vref;
> +
> +	has_external_vref = vref != -ENODEV;

Wouldn't be better just make this 0 when it's == -ENODEV and check just the
value without having this additional boolean variable (note, I haven't checked
the meaning of Vref == 0 in case it's possible in real life and hardware
behaves adequately)?

> +	if (!st->chip_info->internal_ref_support && !has_external_vref)
> +		return -ENODEV;

> +	ret = ad3530r_setup(st, vref, has_external_vref);
> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko