Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163

[Siratul Islam <siratul.islam@linux.dev>]

On Tue, 2026-06-23 at 18:07 +0200, Lukas Metz wrote:
> The DAC756x, DAC816x, and DAC856x devices are low-power, voltage-output,
> dual-channel, 12-, 14-, and 16-bit digital-to-analog converters (DACs),
> respectively. These devices include a 2.5-V, 4-ppm/°C internal
> reference, giving a full-scale output voltage range of 2.5 V or 5 V.
> 
> Signed-off-by: Lukas Metz <lukas.metz@gmx.net>
> ---
>  MAINTAINERS                  |   6 +
>  drivers/iio/dac/Kconfig      |  10 ++
>  drivers/iio/dac/Makefile     |   1 +
>  drivers/iio/dac/ti-dac8163.c | 339 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 356 insertions(+)
Hi! I took a quick look, and probably missed a lot of stuff. But here are my thoughts.
> diff --git a/MAINTAINERS b/MAINTAINERS
...
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DACxx6x IIO driver (SPI)
> + */
A link to the datasheet here would be nice.
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/printk.h>
> +#include <linux/bitfield.h>
Sort the includes alphabetically. And include what you use. "mod_devicetable.h" is missing for example.
While at it, separate the core headers from "<linux/iio/iio.h>". add ir in a sepatate line.
> +
> +#define COMMAND_MASK GENMASK(6, 3)
> +#define ADDRESS_MASK GENMASK(2, 0)
> +
> +#define COMMAND_SET(x, y) (FIELD_PREP(COMMAND_MASK, (x)) | \
> +							FIELD_PREP(ADDRESS_MASK, (y)))
I'd align the FIELD_PREPs to make it look better. It may also fit in a single line.
> +
> +#define CMD_WRITE_INPUT_REG	0x0
> +#define CMD_UPDATE_DAC	0x1
> +#define CMD_WRITE_UPDATE_ALL	0x2
> +#define CMD_WRITE_UPDATE	0x3
> +#define CMD_SET_PWR_MODE		0x4
> +#define CMD_SOFT_RST			0x5
> +
> +#define CMD_LDAC_MODE		0x6
> +#define LDAC_MODE_CHANNEL_A_MASK BIT(0)
> +#define LDAC_MODE_CHANNEL_B_MASK BIT(1)
> +
> +#define CMD_SEL_REFERENCE	0x7
> +#define VOLTAGE_REFERENCE_MASK BIT(0)
> +
Group the CMD values together, also all these values would look better aligned.
> +enum dacxx6x_ldac_modes {
> +	LDAC_MODE_ACTIVE = 0,
> +	LDAC_MODE_INACTIVE = 1
> +};
A trailing comma would be nice.
> +
> +enum dacxx6x_voltage_reference {
> +	VOLTAGE_REFERENCE_EXTERNAL = 0,
> +	VOLTAGE_REFERENCE_INTERNAL = 1
> +};
Same here
> +
> +enum dacxx6x_supported_device_ids {
> +	ID_DAC7562,
> +	ID_DAC7563,
> +	ID_DAC8162,
> +	ID_DAC8163,
> +	ID_DAC8562,
> +	ID_DAC8563
> +};
> +
Here too.
> 
> +struct dacxx6x_state {
Since the filename is dac8163.c, how about naming the functions/structs/other symbols that as well instead of dacxx6x?
> +	struct spi_device *spi;
> +
How about use regmap?
> +	struct regulator *vref;
> +	struct gpio_desc *loaddacs;
> +
> +	bool internal_ref;
> +	int vref_uv;
> +
> +	unsigned int cached[2];
> +
> +	/*
> +	 * Lock to protect the state of the device from potential concurrent
> +	 * write accesses from userspace.
> +	 */
> +	struct mutex lock;
> +};
> +
> +struct dacxx6x_chip_info {
> +	const char *name;
> +	const struct iio_chan_spec channels[2];
> +};
> +
> +#define DACXX6X_CHAN(id, resolution)                                        \
> +	{                                                                   \
> +		.type = IIO_VOLTAGE, .channel = (id), .output = 1,          \
> +		.indexed = 1, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),       \
> +		.scan_type = { .realbits = (resolution),                    \
> +			       .shift = 16 - (resolution) },                \
> +	}
> +
> +static const struct dacxx6x_chip_info dacxx6x_chip_info_table[6] = {
> +	[ID_DAC7562] = {
> +		.name = "dac7562",
> +		.channels = {
> +			DACXX6X_CHAN(0, 12),
> +			DACXX6X_CHAN(1, 12),
> +		}
> +	},
> +	[ID_DAC7563] = {
> +		.name = "dac7563",
> +		.channels = {
> +			DACXX6X_CHAN(0, 12),
> +			DACXX6X_CHAN(1, 12),
> +		}
> +	},
> +	[ID_DAC8162] = {
> +		.name = "dac8162",
> +		.channels = {
> +			DACXX6X_CHAN(0, 14),
> +			DACXX6X_CHAN(1, 14),
> +		}
> +	},
> +	[ID_DAC8163] = {
> +		.name = "dac8163",
> +		.channels = {
> +			DACXX6X_CHAN(0, 14),
> +			DACXX6X_CHAN(1, 14),
> +		}
> +	},
> +	[ID_DAC8562] = {
> +		.name = "dac8562",
> +		.channels = {
> +			DACXX6X_CHAN(0, 16),
> +			DACXX6X_CHAN(1, 16),
> +		}
> +	},
> +	[ID_DAC8563] = {
> +		.name = "dac8563",
> +		.channels = {
> +			DACXX6X_CHAN(0, 16),
> +			DACXX6X_CHAN(1, 16),
> +		}
> +	},
> +};
> +
> +static int dacxx6x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct dacxx6x_state *st;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		st = iio_priv(indio_dev);
Could this be assigned  before the switch?
> +		mutex_lock(&st->lock);
> +		*val = st->cached[chan->channel];
> +		mutex_unlock(&st->lock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		st = iio_priv(indio_dev);
> +		*val = st->vref_uv / MILLI; /* vref in mV */
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int dacxx6x_write_reg(struct dacxx6x_state *st, int reg, int addr,
> +			     unsigned int val)
> +{
> +	u8 tx[3];
> +
> +	tx[0] = COMMAND_SET(reg, addr);
> +	tx[1] = (val >> 8) & 0xff;
How about put_unaligned_be16?
> 
> +	tx[2] = val & 0xff;
> +
> +	return spi_write(st->spi, tx, sizeof(tx));
> +}
> +
> +static int dacxx6x_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct dacxx6x_state *st = iio_priv(indio_dev);
> +	struct device *dev = &st->spi->dev;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		dev_dbg(dev, "%s: val=%d val2=%d\n", __func__, val, val2);
> +		if (val2 != 0)
> +			return -EINVAL;
> +
> +		if (val < 0 || val >= BIT(chan->scan_type.realbits))
> +			return -EINVAL;
> +
> +		mutex_lock(&st->lock);
> +		int ret = dacxx6x_write_reg(st, CMD_WRITE_UPDATE, chan->channel,
> +					    (unsigned int)val
> +						    << chan->scan_type.shift);
This case should be enclosed { }. Also, Use guard() from "cleanup.h" instead of manual mutex lock/unlock. Here and in
other places.
> +
> +		if (!ret)
> +			st->cached[chan->channel] = val;
> +		mutex_unlock(&st->lock);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info dacxx6x_iio_info = {
> +	.write_raw = dacxx6x_write_raw,
> +	.read_raw = dacxx6x_read_raw
Trailing comma here
> +};
> +
> +static int dacxx6x_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct dacxx6x_state *st;
> +	const struct dacxx6x_chip_info *info;
> +	int ret;
Sort these in a reverse christmas tree order.
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	st->loaddacs = devm_gpiod_get_optional(&spi->dev, "ti,loaddacs",
> +					       GPIOD_OUT_LOW);
Vendor prefixes are not needed here.
> +	if (IS_ERR(st->loaddacs))
> +		return PTR_ERR(st->loaddacs);
> +
> +	st->internal_ref =
> +		device_property_read_bool(&spi->dev, "ti,internal-ref");
> +
> +	if (!st->internal_ref) {
> +		st->vref = devm_regulator_get(&spi->dev, "vref");
> +		if (IS_ERR(st->vref))
> +			return PTR_ERR(st->vref);
Maybe use return dev_err_probe?
> +
> +		ret = regulator_enable(st->vref);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	mutex_init(&st->lock);
use devm_mutex_init.
> +
> +	if (st->internal_ref) {
> +		st->vref_uv = 2500000; /* 2.5V internal reference */
A note on where this value came from or why this was chosen, or a reference to datasheet would be better.
> +	} else {
> +		st->vref_uv = regulator_get_voltage(st->vref);
> +		if (st->vref_uv < 0) {
> +			ret = st->vref_uv;
> +			goto err;
> +		}
> +	}
> +
You have a CMD_SOFT_RST defined but not used. Should this be used to reset before doing any configuration?
> +	gpiod_set_value(st->loaddacs, 0);
> +
> +	ret = dacxx6x_write_reg(st, CMD_LDAC_MODE, 0,
> +				FIELD_PREP(LDAC_MODE_CHANNEL_A_MASK, LDAC_MODE_INACTIVE) |
> +				FIELD_PREP(LDAC_MODE_CHANNEL_B_MASK, LDAC_MODE_INACTIVE));
> +
> +	if (ret < 0)
> +		goto err;
> +
> +	ret = dacxx6x_write_reg(st, CMD_SEL_REFERENCE, 0,
> +				FIELD_PREP(VOLTAGE_REFERENCE_MASK, st->internal_ref));
> +
> +	if (ret < 0)
> +		goto err;
> +
> +	info = spi_get_device_match_data(spi);
> +
> +	indio_dev->name = info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &dacxx6x_iio_info;
> +	indio_dev->channels = info->channels;
> +	indio_dev->num_channels = 2;
use ARRAY_SIZE(info->channels) and include linux/array_size.h
> +
> +	ret = iio_device_register(indio_dev);
Use devm_iio_device_register
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	if (!st->internal_ref)
> +		regulator_disable(st->vref);
> +	mutex_destroy(&st->lock);
> +	return ret;
> +}
> +
> +static void dacxx6x_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct dacxx6x_state *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
Using devm would help here too. No need to unregister manually
> +	mutex_destroy(&st->lock);
> +	if (!st->internal_ref)
> +		regulator_disable(st->vref);
> +}
> +
> +#define DACXX6X_COMPATIBLE(of_compatible, id)        \
> +	{                                            \
> +		.compatible = of_compatible,         \
> +		.data = &dacxx6x_chip_info_table[id] \
> +	}
> +
> +static const struct of_device_id dacxx6x_of_match[] = {
> +	DACXX6X_COMPATIBLE("ti,dac7562", ID_DAC7562),
> +	DACXX6X_COMPATIBLE("ti,dac7563", ID_DAC7563),
> +	DACXX6X_COMPATIBLE("ti,dac8162", ID_DAC8162),
> +	DACXX6X_COMPATIBLE("ti,dac8163", ID_DAC8163),
> +	DACXX6X_COMPATIBLE("ti,dac8562", ID_DAC8562),
> +	DACXX6X_COMPATIBLE("ti,dac8563", ID_DAC8563),
> +	{}
{} should have a space "{ }"
> +};
> +MODULE_DEVICE_TABLE(of, dacxx6x_of_match);
> +
> +static const struct spi_device_id dacxx6x_id_table[] = {
> +	{ "dac7562", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC7562] },
> +	{ "dac7563", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC7563] },
> +	{ "dac8162", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8162] },
> +	{ "dac8163", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8163] },
> +	{ "dac8562", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8562] },
> +	{ "dac8563", (kernel_ulong_t)&dacxx6x_chip_info_table[ID_DAC8563] },
> +	{}
Same here
> +};
> +
> +MODULE_DEVICE_TABLE(spi, dacxx6x_id_table);
> +
> +static struct spi_driver dacxx6x_driver = {
> +	.driver = {
> +		.name = "ti-dacxx6x",
Name doesn't need vendor prefix.
> +		.of_match_table = dacxx6x_of_match,
> +	},
> +	.probe = dacxx6x_probe,
> +	.remove = dacxx6x_remove,
> +	.id_table = dacxx6x_id_table,
> +};
> +
No space here. 
> +module_spi_driver(dacxx6x_driver);
> +
> +MODULE_AUTHOR("Lukas Metz <lukas.metz@gmx.net>");
> +MODULE_DESCRIPTION("Texas Instruments 12/14/16-bit 2-channel DAC driver");
> +MODULE_LICENSE("GPL");

Thanks
Sirat