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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 2 Aug 2026 12:19:07 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/2/26 11:07 AM, 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                  |   1 +
> >  drivers/iio/dac/Kconfig      |  16 ++
> >  drivers/iio/dac/Makefile     |   1 +
> >  drivers/iio/dac/ti-dac8163.c | 449 +++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 467 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 314f235332f5..5512f5eaab44 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26399,6 +26399,7 @@ M:	Lukas Metz <lukas.metz@gmx.net>
> >  L:	linux-iio@vger.kernel.org
> >  S:	Maintained
> >  F:	Documentation/devicetree/bindings/iio/dac/ti,dac8163.yaml
> > +F:	drivers/iio/dac/ti-dac8163.c
> >  
> >  TI DATA TRANSFORM AND HASHING ENGINE (DTHE) V2 CRYPTO DRIVER
> >  M:	T Pratham <t-pratham@ti.com>
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index db9f5c711b3d..aa80b871995b 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -632,6 +632,22 @@ config TI_DAC7612
> >  
> >  	  If compiled as a module, it will be called ti-dac7612.
> >  
> > +config TI_DAC8163
> > +	tristate "Texas Instruments 12/14/16-bit 2-channel DAC driver"
> > +	depends on SPI_MASTER
> > +	select REGMAP_SPI
> > +	help
> > +	  Driver for the Texas Instruments digital-to-analog converter
> > +	  family dacxx6x compatible with the following variants
> > +	  - DAC7562 (2 channels, 12 bits, resets to zero)
> > +	  - DAC7563 (2 channels, 12 bits, resets to mid-scale)
> > +	  - DAC8162 (2 channels, 14 bits, resets to zero)
> > +	  - DAC8163 (2 channels, 14 bits, resets to mid-scale)
> > +	  - DAC8562 (2 channels, 16 bits, resets to zero)
> > +	  - DAC8563 (2 channels, 16 bits, resets to mid-scale)
> > +
> > +	  If compiled as a module, it will be called ti-dac8163.
> > +
> >  config VF610_DAC
> >  	tristate "Vybrid vf610 DAC driver"
> >  	depends on HAS_IOMEM
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > index 2a80bbf4e80a..359cde446623 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -62,4 +62,5 @@ obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
> >  obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o
> >  obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o
> >  obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o
> > +obj-$(CONFIG_TI_DAC8163) += ti-dac8163.o
> >  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> > diff --git a/drivers/iio/dac/ti-dac8163.c b/drivers/iio/dac/ti-dac8163.c
> > new file mode 100644
> > index 000000000000..35af7828dc22
> > --- /dev/null
> > +++ b/drivers/iio/dac/ti-dac8163.c
> > @@ -0,0 +1,449 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * DAC8163 IIO driver (SPI)
> > + * https://www.ti.com/de/lit/gpn/dac8163
> > + */
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/stddef.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include <linux/iio/iio.h>
> > +
> > +#define COMMAND_MASK GENMASK(6, 3)
> > +#define ADDRESS_MASK GENMASK(2, 0)
> > +
> > +#define CMD_WRITE_INPUT_REG	0x0
> > +#define CMD_UPDATE_DAC		0x1
> > +#define CMD_WRITE_UPDATE_ALL	0x2
> > +#define CMD_WRITE_UPDATE	0x3
> > +#define CMD_POWER_MODE		0x4
> > +#define CMD_SOFT_RST		0x5
> > +#define CMD_LDAC_MODE		0x6
> > +#define CMD_REF			0x7
These are very generic names and that's where we risk shadowing
a define somewhere else.  I'd prefix them all with
DAC8163 even though it leads to some longer lines.

> > +
> > +#define LDAC_CHANNEL_A_MASK BIT(0)
> > +#define LDAC_CHANNEL_B_MASK BIT(1)
> > +#define VREF_MASK	    BIT(0)

> > +
> > +	if (device_property_present(&spi->dev, "vrefin-supply")) {
> > +		ret = devm_regulator_get_enable_read_voltage(&spi->dev,
> > +							     "vrefin");
> > +		if (ret < 0)
> > +			return dev_err_probe(&spi->dev, ret,
> > +					     "failed to get vrefin voltage\n");
> > +
> > +		st->vref_mV = ret / (MICRO / MILLI);
> > +		internal_reference = false;
> > +		st->gain = 1;
> > +	} else {
> > +		st->vref_mV = DAC8163_INTERNAL_REF_mV;
> > +		internal_reference = true;
> > +		st->gain = 2;
> > +	}
> > +
> > +	ret = regmap_write(st->regmap, FIELD_PREP(COMMAND_MASK, CMD_SOFT_RST),
> > +			   FULL_RESET);
> > +	if (ret < 0)
> > +		return dev_err_probe(&spi->dev, ret,
> > +				     "failed to reset device\n");
> > +
> > +	ret = regmap_write(st->regmap, FIELD_PREP(COMMAND_MASK, CMD_LDAC_MODE),
> > +			   FIELD_PREP(LDAC_CHANNEL_A_MASK, LDAC_INACTIVE) |
> > +			   FIELD_PREP(LDAC_CHANNEL_B_MASK, LDAC_INACTIVE));  
> 
> 
> I know I have said many times before not to hide FIELD_PREP() in a macro.
> However, that was for register _values_, not register _addresseses_.
> 
> In this case, I think we can make an exception for the address like:
> 
> #define DAC8163_REG(cmd, addr) \
> 	(FIELD_PREP(COMMAND_MASK, (cmd)) | FIELD_PREP(ADDRESS_MASK, (addr)))

This makes sense but given it will then be the only place COMMAND_MASK
and ADDRESS_MASK are used, roll the GENMASK in there.

The generic nature of those macro names suggests to me we might
well get a naming clash with a header define in the long run so
i was going to suggest prefixing them.  Just not having them does
the job as well.
> 
> So we can write it like:
> 
> 	ret = regmap_write(st->regmap, DAC8163_REG(CMD_LDAC_MODE, 0),
> 			   FIELD_PREP(LDAC_CHANNEL_A_MASK, LDAC_INACTIVE) |
> 			   FIELD_PREP(LDAC_CHANNEL_B_MASK, LDAC_INACTIVE));
> 
> Having FIELD_PREP() on all of the register addresses throught this patch
> was really throwing me off and gets quite verbose and repetitive.
> 
> > +	if (ret < 0)