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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 24 Jun 2026 00:56:15 +0600
Siratul Islam <siratul.islam@linux.dev> wrote:

> 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.

Good review. I added a few things on top (reduces duplication as you didn't crop
- fair enough given distribution of comments!)

> > diff --git a/MAINTAINERS b/MAINTAINERS  


> > +enum dacxx6x_supported_device_ids {
> > +	ID_DAC7562,
> > +	ID_DAC7563,
> > +	ID_DAC8162,
> > +	ID_DAC8163,
> > +	ID_DAC8562,
> > +	ID_DAC8563
> > +};

This enum provides too tempting a way to indicate devices.
hence just drop it ;)  Named structures as below will be neater.

> > +  
> 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];
Maybe rename to make it entirely obvious what is cached. 

> > +
> > +#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) },                \
> > +	}
Format this as one entry per line.
	{
		.type = IIO_VOLTAGE,					\
		.channel = (id),					\
		...
		.scan_type = {
			.real_bits = (...

etc

> > +
> > +static const struct dacxx6x_chip_info dacxx6x_chip_info_table[6] = {
> > +	[ID_DAC7562] = {
As below and in other reviews - separate structures is going to be neater.
> > +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?

Might as well do it at declaration.

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

There was a comment on DMA - look into dma safety requirements for SPI
and how we solve them.  Here you might just want to use spi_write_then_read()
but make sure you understand why that helps.

> > +
> > +	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:
Comment on scope below menas
	case IIO_CHAN_INFO_RAW: {
		int ret;

		if (val2 != 0) ...

> > +		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,

For normal variables we still use old style c in the kernel, so declaration at
top of scope (I suggest adding more local scope anyway)

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

Style of this can be made more readable using some newish toys.

Include cleanup.h

Add scope to the case with {} and then
		guard(mutex)(&st->lock);
		ret = dac8163_write_reg(st, CMD_WRITE_UPDATE, chan->channel,
					(u32)val << cabn->scan_type.shift);
		if (ret)
			return ret;
		
		st->cached[chan->channel] = val;
		return 0;
	}
		
> > +		mutex_unlock(&st->lock);
> > +		return ret;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}

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

David covered how to cleanly get the voltage and not keep a reference
to the regulator around + that auto disables on error return from probe
or on remove of driver.

> > +		if (IS_ERR(st->vref))
> > +			return PTR_ERR(st->vref);  
> Maybe use return dev_err_probe?

...

> > +
> > +#define DACXX6X_COMPATIBLE(of_compatible, id)        \
> > +	{                                            \
> > +		.compatible = of_compatible,         \
> > +		.data = &dacxx6x_chip_info_table[id] \

> > +	}
This macro saves a few lines but I think it would clearer
just done long hand instead.  The macro would get messier
anyway when you move away from an array + enum.

> > +

...

> > +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] },

Using an array and an enum is something we used to do but it scales badly.
So now days we use named structures instead (I think David raised this)

> > +	{}  
> Same here
> > +};
> > +

No blank line here either. Convention rather than a hard rule but
nice to be consistent.

> > +MODULE_DEVICE_TABLE(spi, dacxx6x_id_table);