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

[Siratul Islam]

On Wed, 2026-07-08 at 11:52 +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.
>  
>  	  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
Missing "select REGMAP_SPI" here.
> +	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)
> 
> 
> +// 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/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
I asked you to add this "mod_devicetable.h" header previously. But I noticed some reviews by Uwe 
to use more specific headers. 
In your case these should be 


#include <linux/device-id/spi.h>
#include <linux/device-id/of.h>

unless I'm missing something.

> +#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_SET(x, y) \
> +	(FIELD_PREP(COMMAND_MASK, (x)) | FIELD_PREP(ADDRESS_MASK, (y)))
> +
> +#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
> +
> +#define LDAC_CHANNEL_A_MASK BIT(0)
> +#define LDAC_CHANNEL_B_MASK BIT(1)
> +#define VREF_MASK BIT(0)
Since the MASKs are defined together, align the values.

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

> +
> +#define DAC8163_INTERNAL_REF_mV 2500
> +
...
> +
> +#define DAC8163_MID_SCALE(resolution) \
> +	(BIT((resolution) - 1) << (16 - (resolution)))
> +
> +static const struct dac8163_chip_info dac7562_chip_info = {
> +	.name = "dac7562",
> +	.channels = {
> +		DAC8163_CHAN(0, 12),
> +		DAC8163_CHAN(1, 12),
> +	},
> +	.default_output_reg = 0,
> +};
> +
> +static const struct dac8163_chip_info dac7563_chip_info = {
> +	.name = "dac7563",
> +	.channels = {
> +		DAC8163_CHAN(0, 12),
> +		DAC8163_CHAN(1, 12),
> +	},
> +	.default_output_reg = DAC8163_MID_SCALE(12),
These resolution values are repeated multiple times. I think they could be defined as something like
#define DAC8163_RES_12_BIT	12 // 14, 16

> +};
> +
> +static const struct dac8163_chip_info dac8162_chip_info = {
> +	.name = "dac8162",
> +	.channels = {
> +		DAC8163_CHAN(0, 14),
> +		DAC8163_CHAN(1, 14),
> +	},
> +	.default_output_reg = 0,
> +};
> +
> +static const struct dac8163_chip_info dac8163_chip_info = {
> +	.name = "dac8163",
> +	.channels = {
> +		DAC8163_CHAN(0, 14),
> +		DAC8163_CHAN(1, 14),
> +	},
> +	.default_output_reg = DAC8163_MID_SCALE(14),
> +};
> +
> +static const struct dac8163_chip_info dac8562_chip_info = {
> +	.name = "dac8562",
> +	.channels = {
> +		DAC8163_CHAN(0, 16),
> +		DAC8163_CHAN(1, 16),
> +	},
> +	.default_output_reg = 0,
> +};
> +
> +static const struct dac8163_chip_info dac8563_chip_info = {
> +	.name = "dac8563",
> +	.channels = {
> +		DAC8163_CHAN(0, 16),
> +		DAC8163_CHAN(1, 16),
> +	},
> +	.default_output_reg = DAC8163_MID_SCALE(16),
> +};
> +
> +static int dac8163_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
Put val on the same line as val2. i.e.,
+static int dac8163_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)

> +{
> 
...
> +
> +static const struct iio_info dac8163_iio_info = {
> +	.write_raw = dac8163_write_raw,
> +	.read_raw = dac8163_read_raw
Missing trailing comma here.
> +};
> +
> +static bool dac8163_reg_false(struct device *dev, unsigned int ref)
> +{
> +	return false;
> +}
> +
> +static int dac8163_probe(struct spi_device *spi)
> +{
> 
...
> +
> +	const struct reg_default reg_defaults[] = {
> +		{
> +			.reg = CMD_SET(CMD_WRITE_UPDATE, 0),
> +			.def = info->default_output_reg,
> +		},
> +		{
> +			.reg = CMD_SET(CMD_WRITE_UPDATE, 1),
> +			.def = info->default_output_reg,
> +		},
> +	};
> +
> +	const struct regmap_config regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 16,
> +
> +		.max_register = CMD_REF << 3,
> +		.cache_type = REGCACHE_MAPLE,
> +
> +		.volatile_reg = dac8163_reg_false,
> +
> +		.reg_defaults = reg_defaults,
> +		.num_reg_defaults = ARRAY_SIZE(reg_defaults),
> +	};
Why are these defined inside probe? Should be defined outside the function.
> +
> +	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
Use "dev_err_probe"
return dev_err_probe(dev, PTR_ERR(st->regmap),
				     "regmap initialization failed\n");
> +
> +	// for now we keep the ldac pin asserted permanently so that the output
> +	// is updated immediately after a write to the channels raw attribute
> +	ldac_gpio = devm_gpiod_get_optional(&spi->dev, "ldac",
> +
...
> 
> +
> +static const struct spi_device_id dac8163_id_table[] = {
> +	{
> +		.name = "dac7562",
> +		.driver_data = (kernel_ulong_t)&dac7562_chip_info
Missing trailing commas here too, this and the ones below.
> +	},
> +	{
> +		.name = "dac7563",
> +		.driver_data = (kernel_ulong_t)&dac7563_chip_info
> +	},
> +	{
> +		.name = "dac8162",
> +		.driver_data = (kernel_ulong_t)&dac8162_chip_info
> +	},
> +	{
> +		.name = "dac8163",
> +		.driver_data = (kernel_ulong_t)&dac8163_chip_info
> +	},
> +	{
> +		.name = "dac8562",
> +		.driver_data = (kernel_ulong_t)&dac8562_chip_info
> +	},
> +	{
> +		.name = "dac8563",
> +		.driver_data = (kernel_ulong_t)&dac8563_chip_info
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, dac8163_id_table);
> +
> +static struct spi_driver dac8163_driver = {
> +	.driver = {
> +		.name = "dac8163",
> +		.of_match_table = dac8163_of_match,
> +	},
> +	.probe = dac8163_probe,
> +	.id_table = dac8163_id_table,
> +};
> +module_spi_driver(dac8163_driver);
> +
> +MODULE_AUTHOR("Lukas Metz <lukas.metz@xxxxxxx>");
> +MODULE_DESCRIPTION("Texas Instruments 12/14/16-bit 2-channel DAC driver");
> +MODULE_LICENSE("GPL");

--
Best regards,
Sirat