* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-23 16:07 ` [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163 Lukas Metz
@ 2026-06-23 18:56 ` Siratul Islam
2026-06-24 8:30 ` Lukas
2026-06-23 19:39 ` Andy Shevchenko
2026-06-23 19:50 ` David Lechner
2 siblings, 1 reply; 17+ messages in thread
From: Siratul Islam @ 2026-06-23 18:56 UTC (permalink / raw)
To: lukas.metz
Cc: andy, conor+dt, devicetree, dlechner, jic23, krzk+dt, linux-iio,
linux-kernel, nuno.sa, robh
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-23 18:56 ` Siratul Islam
@ 2026-06-24 8:30 ` Lukas
2026-06-24 14:18 ` David Lechner
0 siblings, 1 reply; 17+ messages in thread
From: Lukas @ 2026-06-24 8:30 UTC (permalink / raw)
To: Siratul Islam
Cc: andy, conor+dt, devicetree, dlechner, jic23, krzk+dt, linux-iio,
linux-kernel, nuno.sa, robh
Thanks for the review. As i said this is my first time submitting a
patch. I have looked at already existing spi dac drivers for reference
but i seemed to have missed quite a lot. But the comments are greatly
appreciated.
On Wed, Jun 24, 2026 at 12:56:15AM +0600, Siratul Islam wrote:
> A link to the datasheet here would be nice.
I will try to add all the small suggestions i dont mention explicitly,
like style issues or using guard instead of manual lock/unlock to v2.
> > +
> > + 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.
I think i would add the suggestion from David Lechner to remove the
internal_ref property completly and add "the way of doing optional
voltage references". This includes using the macro
DAC8163_INTERNAL_REF_mV. Would this be acceptable?
> You have a CMD_SOFT_RST defined but not used. Should this be used to reset before doing any configuration?
Yes this is a command which isnt used at this point. But maybe it makes
sense to reset the DAC first when probing.
Best regards
Lukas
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-24 8:30 ` Lukas
@ 2026-06-24 14:18 ` David Lechner
0 siblings, 0 replies; 17+ messages in thread
From: David Lechner @ 2026-06-24 14:18 UTC (permalink / raw)
To: Lukas, Siratul Islam
Cc: andy, conor+dt, devicetree, jic23, krzk+dt, linux-iio,
linux-kernel, nuno.sa, robh
On 6/24/26 3:30 AM, Lukas wrote:
> Thanks for the review. As i said this is my first time submitting a
> patch. I have looked at already existing spi dac drivers for reference
> but i seemed to have missed quite a lot. But the comments are greatly
> appreciated.
>
> On Wed, Jun 24, 2026 at 12:56:15AM +0600, Siratul Islam wrote:
>> A link to the datasheet here would be nice.
>
> I will try to add all the small suggestions i dont mention explicitly,
> like style issues or using guard instead of manual lock/unlock to v2.
>
>>> +
>>> + 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.
>
> I think i would add the suggestion from David Lechner to remove the
> internal_ref property completly and add "the way of doing optional
> voltage references". This includes using the macro
> DAC8163_INTERNAL_REF_mV. Would this be acceptable?
>
>> You have a CMD_SOFT_RST defined but not used. Should this be used to reset before doing any configuration?
>
> Yes this is a command which isnt used at this point. But maybe it makes
> sense to reset the DAC first when probing.
In general we tend to reset IIO devices during probe. DACs can be an exception
though since they are output devices and resetting it could change the output.
This device is quite simple anyway, so reset probably isn't needed.
>
> Best regards
> Lukas
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-23 16:07 ` [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163 Lukas Metz
2026-06-23 18:56 ` Siratul Islam
@ 2026-06-23 19:39 ` Andy Shevchenko
2026-06-24 14:47 ` Lukas
2026-06-23 19:50 ` David Lechner
2 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2026-06-23 19:39 UTC (permalink / raw)
To: Lukas Metz
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel,
linux-iio, devicetree
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
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-23 19:39 ` Andy Shevchenko
@ 2026-06-24 14:47 ` Lukas
2026-06-24 15:27 ` David Lechner
0 siblings, 1 reply; 17+ messages in thread
From: Lukas @ 2026-06-24 14:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel,
linux-iio, devicetree
Thanks a lot for all the comments.
On Tue, Jun 23, 2026 at 10:39:23PM +0300, Andy Shevchenko wrote:
> > + 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.
>
I will remove the debug print. I tried my best to make this driver production-ready.
I saw that other drivers also have similar debug messages so i didnt
remove it after my first tests and thought it is ok to leave it in
there. My intention was to try to apply the suggestions and comments i
get and send a second revision. Do you think thats the right way?
Best regards
Lukas
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-24 14:47 ` Lukas
@ 2026-06-24 15:27 ` David Lechner
0 siblings, 0 replies; 17+ messages in thread
From: David Lechner @ 2026-06-24 15:27 UTC (permalink / raw)
To: Lukas, Andy Shevchenko
Cc: Jonathan Cameron, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-kernel, linux-iio,
devicetree
On 6/24/26 9:47 AM, Lukas wrote:
> Thanks a lot for all the comments.
>
> On Tue, Jun 23, 2026 at 10:39:23PM +0300, Andy Shevchenko wrote:
>
>>> + dev_dbg(dev, "%s: val=%d val2=%d\n", __func__, val, val2);
Trimming down the reply is good, but in this case, it would have been
helpful to leave a bit more context. I usually keep the full function
in a reply to have enough context if it isn't too long.
>>
>> No. Is it RFC? PoC? Or production-ready? If not the latter, come when it will
>> be production-ready.
>>
>
> I will remove the debug print. I tried my best to make this driver production-ready.
> I saw that other drivers also have similar debug messages so i didnt
This one jumped out to both of us as odd because of the __func__ and the
fact that it is in a very common operation (raw write). Usually debug is
for catching odd things that happen, not normal things.
> remove it after my first tests and thought it is ok to leave it in
> there. My intention was to try to apply the suggestions and comments i
> get and send a second revision. Do you think thats the right way?
Yes, you are doing it right. For a new driver like this, it is best
to wait about a week before submitting the next revision to give ample
time for review. Your replies so far where you think a suggestion was
wrong or have asked for clarification are spot on as to what we expect
for the review process.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163
2026-06-23 16:07 ` [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163 Lukas Metz
2026-06-23 18:56 ` Siratul Islam
2026-06-23 19:39 ` Andy Shevchenko
@ 2026-06-23 19:50 ` David Lechner
2 siblings, 0 replies; 17+ messages in thread
From: David Lechner @ 2026-06-23 19:50 UTC (permalink / raw)
To: Lukas Metz, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-kernel, linux-iio, devicetree
On 6/23/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.
Nice and simple driver. Mostly just needs better alignment with usual
IIO conventions.
>
> 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(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d238590a31f2..e82cc28e1bc3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -26394,6 +26394,12 @@ S: Odd Fixes
> F: drivers/clk/ti/
> F: include/linux/clk/ti.h
>
> +TI DAC8163 DAC DRIVER
> +M: Lukas Metz <lukas.metz@gmx.net>
> +L: linux-iio@vger.kernel.org
> +S: Maintained
> +F: drivers/iio/dac/ti-dac8163.c
> +
> TI DATA TRANSFORM AND HASHING ENGINE (DTHE) V2 CRYPTO DRIVER
> M: T Pratham <t-pratham@ti.com>
> L: linux-crypto@vger.kernel.org
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index db9f5c711b3d..6b6e5ee0732a 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -632,6 +632,16 @@ 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
> + help
> + Driver for the Texas Instruments digital-to-analog converter
> + family dacxx6x compatible with the variants DAC7562,
> + DAC7563, DAC8162, DAC8163, DAC8562 and DAC8563.
> +
> + 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..84a9dfb5347d
> --- /dev/null
> +++ b/drivers/iio/dac/ti-dac8163.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
Prefer GPL-2.0-only or GPL-2.0-or-later.
> +/*
> + * DACxx6x IIO driver (SPI)
> + */
> +
> +#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>
> +
> +#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)))
Usually, we try to avoid putting FIELD_PREP() in macros. This
is only used once in the code anyway, so dropping the macro
will actually save a line or two.
> +
> +#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)
> +
> +enum dacxx6x_ldac_modes {
In IIO, we always avoid putting xx in identifier names. Just use dac8163
everwhere instead since that is the name of the driver.
> + LDAC_MODE_ACTIVE = 0,
> + LDAC_MODE_INACTIVE = 1
> +};
> +
> +enum dacxx6x_voltage_reference {
> + VOLTAGE_REFERENCE_EXTERNAL = 0,
> + VOLTAGE_REFERENCE_INTERNAL = 1
> +};
> +
> +enum dacxx6x_supported_device_ids {
> + ID_DAC7562,
> + ID_DAC7563,
> + ID_DAC8162,
> + ID_DAC8163,
> + ID_DAC8562,
> + ID_DAC8563
> +};
> +
> +struct dacxx6x_state {
> + struct spi_device *spi;
> +
> + struct regulator *vref;
This is
> + struct gpio_desc *loaddacs;
LDAC is very common in DACs, so I would call this ldac_gpio. Also, it isn't
currently used outside of probe, so we don't really need it here.
> +
> + bool internal_ref;
> + int vref_uv;
As in a later comment, we should be able to drop these as well if we can
get rid of the remove() callback.
> +
> + unsigned int cached[2];
Can we get a more descriptive name for this? Is it raw value?
> +
> + /*
> + * 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, \
Please put each field on a new line.
> + .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),
> + }
> + },
> +};
We are trying to get rid of arrays like this in drivers. We can just make
individual structs instead.
> +
> +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);
This can be moved out of the case statement so we don't have to
repeat it each time.
> + 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 */
We've been writing this like:
*val = st->vref_uV / (MICRO/ MILLI);
Then we don't really need a comment.
> + *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;
> + 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);
Do we really need to keep this debug print?
> + 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,
Usually, we would declare `int ret;` at the top of the function.
> + (unsigned int)val
Using u32 type instead of unsigned int would make this probably fit
on one line too.
> + << chan->scan_type.shift);
> +
> + 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
> +};
> +
> +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;
> +
> + 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);
As in the dt-bindings, we would expect the bindings to be active low
so this would be GPIOD_OUT_HIGH to assert the LDAC signal.
Could also use a comment to say that for now we are just holding this
asserted so that individual outputs are updated when we each raw attribute.
> + 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);
> +
> + ret = regulator_enable(st->vref);
> + if (ret < 0)
> + return ret;
> + }
> +
> + mutex_init(&st->lock);
devm_mutex_init(). Also should not be in the middle of regulator code.
> +
> + 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;
> + }
> + }
The way we've been doing optional reference voltage lately is like this:
if (device_property_present(dev, "refin-supply")) {
ret = devm_regulator_get_enable_read_voltage(dev, "refin");
if (ret < 0)
return ret;
st->vref_mV = ret / (MICRO / MILLI);
} else {
st->vref_mV = DAC8163_INTERNAL_REF_mV;
}
This avoids the need for the ti,internal-ref DT property, avoid the need to convert
uV to mV later and the macro for the internal reference makes it self-documenting
so we don't need a comment. And it automacially cleans up after itself.
> +
> + gpiod_set_value(st->loaddacs, 0);
devm_gpiod_get_optional() already set this, so this is redundant.
> +
> + 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));
> +
Some of these lines are getting a bit long. We try to stick to close
to 80 columns in IIO when we can.
> + if (ret < 0)
> + goto err;
> +
> + info = spi_get_device_match_data(spi);
if (!info)
return -EINVAL;
> +
> + 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);
return 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);
> + mutex_destroy(&st->lock);
> + if (!st->internal_ref)
> + regulator_disable(st->vref);
We can use devm_* functions and avoid the need for the remove callback.
> +}
> +
> +#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),
> + {}
IIO style is `{ }` (with space inbetween braces)
> +};
> +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] },
> + {}
> +};
> +
> +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,
> +};
> +
> +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");
>
^ permalink raw reply [flat|nested] 17+ messages in thread