Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
From: Andy Shevchenko
Date: Tue Jun 23 2026 - 15:39:45 EST
On Tue, Jun 23, 2026 at 06:07:27PM +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.
> +config TI_DAC8163
> + tristate "Texas Instruments 12/14/16-bit 2-channel DAC driver"
> + depends on SPI_MASTER
> + help
> + Driver for the Texas Instruments digital-to-analog converter
> + family dacxx6x compatible with the variants DAC7562,
> + DAC7563, DAC8162, DAC8163, DAC8562 and DAC8563.
This list doesn't scale. Put one part number per line as
- DAC7... (channel bits)
- DAC8... (...)
- ...
> + If compiled as a module, it will be called ti-dac8163.
...
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
No, for this header in new drivers. Can't we use device and/or fwnode property
APIs instead?
> +#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>
? Usually one wants dev_printk.h.
> +#include <linux/bitfield.h>
Keep the list sorted, and also follow the IWYU principle.
...
> +#define COMMAND_SET(x, y) (FIELD_PREP(COMMAND_MASK, (x)) | \
> + FIELD_PREP(ADDRESS_MASK, (y)))
This is ugly indentation. Compare to
#define COMMAND_SET(x, y) \
(FIELD_PREP(COMMAND_MASK, (x)) | FIELD_PREP(ADDRESS_MASK, (y)))
...
> +enum dacxx6x_ldac_modes {
> + LDAC_MODE_ACTIVE = 0,
> + LDAC_MODE_INACTIVE = 1
Leave trailing commas in the non-terminator entries here and there.
> +};
...
> +enum dacxx6x_supported_device_ids {
> + ID_DAC7562,
> + ID_DAC7563,
> + ID_DAC8162,
> + ID_DAC8163,
> + ID_DAC8562,
> + ID_DAC8563
> +};
This is solely used to make indexed array with chip_info. Instead kill this
enum and use separate structures.
...
> +struct dacxx6x_state {
> + struct spi_device *spi;
Why not regmap?
> + struct regulator *vref;
> + struct gpio_desc *loaddacs;
> +
> + bool internal_ref;
> + int vref_uv;
_uV
> + unsigned int cached[2];
> +
> + /*
> + * Lock to protect the state of the device from potential concurrent
> + * write accesses from userspace.
> + */
> + struct mutex lock;
> +};
...
> +#define DACXX6X_CHAN(id, resolution) \
> + { \
> + .type = IIO_VOLTAGE, .channel = (id), .output = 1, \
> + .indexed = 1, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
Do not put two or more things on the same line, it's unreadable.
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> + .scan_type = { .realbits = (resolution), \
> + .shift = 16 - (resolution) }, \
Make these to be 4 lines.
> + }
...
> +static const struct dacxx6x_chip_info dacxx6x_chip_info_table[6] = {
Drop the number in the square brackets, let compiler do that job. But see
above, this has to be just 6 different data structures.
> +};
...
> +static int dacxx6x_write_reg(struct dacxx6x_state *st, int reg, int addr,
> + unsigned int val)
> +{
> + u8 tx[3];
Are you sure about this? How would it work with DMA?
> + tx[0] = COMMAND_SET(reg, addr);
> + tx[1] = (val >> 8) & 0xff;
> + tx[2] = val & 0xff;
Use proper unaligned setters: put_unaligned_be16() from linux/unaligned.h.
> + 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)
Split logically:
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);
No. Is it RFC? PoC? Or production-ready? If not the latter, come when it will
be production-ready.
> + if (val2 != 0)
> + return -EINVAL;
> + if (val < 0 || val >= BIT(chan->scan_type.realbits))
> + return -EINVAL;
Why not ERANGE or something like this?
> +
> + mutex_lock(&st->lock);
Why not guard()()?
> + int ret = dacxx6x_write_reg(st, CMD_WRITE_UPDATE, chan->channel,
> + (unsigned int)val
> + << chan->scan_type.shift);
Awful indentation. Please, check
> + if (!ret)
No, use regular pattern: Check for error first.
> + st->cached[chan->channel] = val;
> + 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;
Reversed xmas tree order, please.
> + 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);
Is it being used?
> + st->loaddacs = devm_gpiod_get_optional(&spi->dev, "ti,loaddacs",
> + GPIOD_OUT_LOW);
In the above you used temporary variable for struct device, why not here?
> + if (IS_ERR(st->loaddacs))
> + return PTR_ERR(st->loaddacs);
> +
> + st->internal_ref =
> + device_property_read_bool(&spi->dev, "ti,internal-ref");
And here (and it becomes a single line as well).
> + if (!st->internal_ref) {
> + st->vref = devm_regulator_get(&spi->dev, "vref");
> + if (IS_ERR(st->vref))
> + return PTR_ERR(st->vref);
> +
> + ret = regulator_enable(st->vref);
> + if (ret < 0)
> + return ret;
> + }
Can't you get just voltage and be done with it for now?
> + mutex_init(&st->lock);
devm_mutex_init.
> + if (st->internal_ref) {
> + st->vref_uv = 2500000; /* 2.5V internal reference */
> + } else {
> + st->vref_uv = regulator_get_voltage(st->vref);
> + if (st->vref_uv < 0) {
> + ret = st->vref_uv;
> + goto err;
> + }
> + }
> +
> + 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));
> +
Stray blank line.
> + if (ret < 0)
> + goto err;
> +
> + ret = dacxx6x_write_reg(st, CMD_SEL_REFERENCE, 0,
> + FIELD_PREP(VOLTAGE_REFERENCE_MASK, st->internal_ref));
> +
Ditto.
> + if (ret < 0)
> + goto err;
> +
> + info = spi_get_device_match_data(spi);
We need to check for NULL here due to driver_override.
> + 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;
> +
> + ret = iio_device_register(indio_dev);
> + 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);
> + mutex_destroy(&st->lock);
> + if (!st->internal_ref)
> + regulator_disable(st->vref);
> +}
Finish devm conversion and drop this function.
...
> +#define DACXX6X_COMPATIBLE(of_compatible, id) \
> + { \
> + .compatible = of_compatible, \
> + .data = &dacxx6x_chip_info_table[id] \
> + }
No, just use directly, so drop this macro.
> +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),
> + {}
> +};
> +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] },
> + {}
> +};
> +
Unneeded blank line.
> +MODULE_DEVICE_TABLE(spi, dacxx6x_id_table);
> +
> +static struct spi_driver dacxx6x_driver = {
> + .driver = {
> + .name = "ti-dacxx6x",
> + .of_match_table = dacxx6x_of_match,
> + },
> + .probe = dacxx6x_probe,
> + .remove = dacxx6x_remove,
> + .id_table = dacxx6x_id_table,
> +};
> +
Ditto.
> +module_spi_driver(dacxx6x_driver);
--
With Best Regards,
Andy Shevchenko