Add support for the MAX22007 4 channel DAC
that drives a voltage or current output on each channel.
Signed-off-by: Janani Sunil <janani.sunil@analog.com>
---
MAINTAINERS | 1 +
drivers/iio/dac/Kconfig | 13 ++
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/max22007.c | 487 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 502 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 6561455732c9..7efd5cf98023 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1600,6 +1600,7 @@ S: Supported
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/dac/adi,max22007.yaml
F: Documentation/iio/max22007.rst
+F: drivers/iio/dac/max22007.c
ANALOG DEVICES INC ADA4250 DRIVER
M: Antoniu Miclaus <antoniu.miclaus@analog.com>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 7cd3caec1262..4a31993f5b14 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -482,6 +482,19 @@ config MAX517
This driver can also be built as a module. If so, the module
will be called max517.
+config MAX22007
+ tristate "Analog Devices MAX22007 DAC Driver"
+ depends on SPI
+ select REGMAP_SPI
+ select CRC8
+ help
+ Say Y here if you want to build a driver for Analog Devices MAX22007.
+
+ MAX22007 is a quad-channel, 12-bit, voltage-output digital to
+ analog converter (DAC) with SPI interface.
+
+ If compiled as a module, it will be called max22007.
+
config MAX5522
tristate "Maxim MAX5522 DAC driver"
depends on SPI_MASTER
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index e6ac4c67e337..0bbc6d09d22c 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_LTC2664) += ltc2664.o
obj-$(CONFIG_LTC2688) += ltc2688.o
obj-$(CONFIG_M62332) += m62332.o
obj-$(CONFIG_MAX517) += max517.o
+obj-$(CONFIG_MAX22007) += max22007.o
obj-$(CONFIG_MAX5522) += max5522.o
obj-$(CONFIG_MAX5821) += max5821.o
obj-$(CONFIG_MCP4725) += mcp4725.o
diff --git a/drivers/iio/dac/max22007.c b/drivers/iio/dac/max22007.c
new file mode 100644
index 000000000000..0d57fee27367
--- /dev/null
+++ b/drivers/iio/dac/max22007.c
@@ -0,0 +1,487 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * max22007.c - MAX22007 DAC driver
+ *
+ * Driver for Analog Devices MAX22007 Digital to Analog Converter.
+ *
+ * Copyright (c) 2025 Analog Devices Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/crc8.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/unaligned.h>
+
+#define MAX22007_NUM_CHANNELS 4
+#define MAX22007_REV_ID_REG 0x00
+#define MAX22007_STAT_INTR_REG 0x01
+#define MAX22007_INTERRUPT_EN_REG 0x02
+#define MAX22007_CONFIG_REG 0x03
+#define MAX22007_CONTROL_REG 0x04
+#define MAX22007_CHANNEL_MODE_REG 0x05
+#define MAX22007_SOFT_RESET_REG 0x06
+#define MAX22007_DAC_CHANNEL_REG(ch) (0x07 + (ch))
+#define MAX22007_GPIO_CTRL_REG 0x0B
+#define MAX22007_GPIO_DATA_REG 0x0C
+#define MAX22007_GPI_EDGE_INT_CTRL_REG 0x0D
+#define MAX22007_GPI_INT_STATUS_REG 0x0E
+
+/* Channel mask definitions */
+#define MAX22007_CH_MODE_CH_MASK(channel) BIT(12 + (channel))
+#define MAX22007_CH_PWR_CH_MASK(channel) BIT(8 + (channel))
+#define MAX22007_DAC_LATCH_MODE_MASK(channel) BIT(12 + (channel))
+#define MAX22007_LDAC_UPDATE_MASK(channel) BIT(12 + (channel))
+#define MAX22007_SW_RST_MASK BIT(8)
+#define MAX22007_SW_CLR_MASK BIT(12)
+#define MAX22007_SOFT_RESET_BITS_MASK (MAX22007_SW_RST_MASK | \
+ MAX22007_SW_CLR_MASK)
+#define MAX22007_DAC_DATA_MASK GENMASK(15, 4)
+#define MAX22007_DAC_MAX_RAW GENMASK(11, 0)
+#define MAX22007_CRC8_POLYNOMIAL 0x8C
+#define MAX22007_CRC_EN_MASK BIT(0)
+#define MAX22007_RW_MASK BIT(0)
+#define MAX22007_CRC_OVERHEAD 1
+
+/* Field value preparation macros with masking */
+#define MAX22007_CH_PWR_VAL(channel, val) (((val) & 0x1) << (8 + (channel)))
+#define MAX22007_CH_MODE_VAL(channel, val) (((val) & 0x1) << (12 + (channel)))
+#define MAX22007_DAC_LATCH_MODE_VAL(channel, val) (((val) & 0x1) << (12 + (channel)))
+
+static u8 max22007_crc8_table[256];
+
+enum max22007_channel_mode {
+ MAX22007_VOLTAGE_MODE,
+ MAX22007_CURRENT_MODE
+};
+
+enum max22007_channel_power {
+ MAX22007_CH_POWER_OFF,
+ MAX22007_CH_POWER_ON,
+};
+
+struct max22007_state {
+ struct spi_device *spi;
+ struct regmap *regmap;
+ struct iio_chan_spec *iio_chan;
+ u8 tx_buf[4] __aligned(IIO_DMA_MINALIGN);
+ u8 rx_buf[4];
+};
+
+static int max22007_spi_read(void *context, const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct max22007_state *st = context;
+ u8 calculated_crc, received_crc;
+ u8 crc_data[3];
+ int ret;
+ struct spi_transfer xfer = {
+ .tx_buf = st->tx_buf,
+ .rx_buf = st->rx_buf,
+ };
+
+ xfer.len = reg_size + val_size + MAX22007_CRC_OVERHEAD;
+
+ memcpy(st->tx_buf, reg, reg_size);
+
+ ret = spi_sync_transfer(st->spi, &xfer, 1);
+ if (ret) {
+ dev_err(&st->spi->dev, "SPI transfer failed: %d\n", ret);
+ return ret;
+ }
+
+ crc_data[0] = st->tx_buf[0];
+ crc_data[1] = st->rx_buf[1];
+ crc_data[2] = st->rx_buf[2];
+
+ calculated_crc = crc8(max22007_crc8_table, crc_data, 3, 0x00);
+ received_crc = st->rx_buf[3];
+
+ if (calculated_crc != received_crc) {
+ dev_err(&st->spi->dev, "CRC mismatch on read register %02x:\n", *(u8 *)reg);
+ return -EIO;
+ }
+
+ /* Ignore the dummy byte 0 */
+ memcpy(val, &st->rx_buf[1], val_size);
+
+ return 0;
+}
+
+static int max22007_spi_write(void *context, const void *data, size_t count)
+{
+ struct max22007_state *st = context;
+ struct spi_transfer xfer = {
+ .tx_buf = st->tx_buf,
+ .rx_buf = st->rx_buf,
+ };
+
+ memset(st->tx_buf, 0, sizeof(st->tx_buf));
+
+ xfer.len = count + MAX22007_CRC_OVERHEAD;
+
+ memcpy(st->tx_buf, data, count);
+ st->tx_buf[count] = crc8(max22007_crc8_table, st->tx_buf,
+ sizeof(st->tx_buf) - 1, 0x00);
+
+ return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+static bool max22007_reg_readable(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MAX22007_REV_ID_REG:
+ case MAX22007_STAT_INTR_REG:
+ case MAX22007_CONFIG_REG:
+ case MAX22007_CONTROL_REG:
+ case MAX22007_CHANNEL_MODE_REG:
+ case MAX22007_SOFT_RESET_REG:
+ case MAX22007_GPIO_CTRL_REG:
+ case MAX22007_GPIO_DATA_REG:
+ case MAX22007_GPI_EDGE_INT_CTRL_REG:
+ case MAX22007_GPI_INT_STATUS_REG:
+ return true;
+ case MAX22007_DAC_CHANNEL_REG(0) ... MAX22007_DAC_CHANNEL_REG(MAX22007_NUM_CHANNELS - 1):
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool max22007_reg_writable(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case MAX22007_CONFIG_REG:
+ case MAX22007_CONTROL_REG:
+ case MAX22007_CHANNEL_MODE_REG:
+ case MAX22007_SOFT_RESET_REG:
+ case MAX22007_GPIO_CTRL_REG:
+ case MAX22007_GPIO_DATA_REG:
+ case MAX22007_GPI_EDGE_INT_CTRL_REG:
+ return true;
+ case MAX22007_DAC_CHANNEL_REG(0) ... MAX22007_DAC_CHANNEL_REG(MAX22007_NUM_CHANNELS - 1):
+ return true;
+ default:
+ return false;
+ }
+}
+
+static const struct regmap_bus max22007_regmap_bus = {
+ .read = max22007_spi_read,
+ .write = max22007_spi_write,
+ .read_flag_mask = MAX22007_RW_MASK,
+ .reg_format_endian_default = REGMAP_ENDIAN_BIG,
+ .val_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static const struct regmap_config max22007_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .reg_shift = -1,
+ .readable_reg = max22007_reg_readable,
+ .writeable_reg = max22007_reg_writable,
+ .max_register = 0x0E,
+};
+
+static int max22007_write_channel_data(struct max22007_state *state, unsigned int channel,
+ unsigned int data)
+{
+ unsigned int reg_val;
+
+ if (data > MAX22007_DAC_MAX_RAW)
+ return -EINVAL;
+
+ reg_val = FIELD_PREP(MAX22007_DAC_DATA_MASK, data);
+
+ return regmap_write(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), reg_val);
+}
+
+static int max22007_read_channel_data(struct max22007_state *state, unsigned int channel,
+ int *data)
+{
+ int ret;
+ unsigned int reg_val;
+
+ ret = regmap_read(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), ®_val);
+ if (ret)
+ return ret;
+
+ *data = FIELD_GET(MAX22007_DAC_DATA_MASK, reg_val);
+
+ return 0;
+}
+
+static int max22007_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct max22007_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = max22007_read_channel_data(st, chan->channel, val);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ if (chan->type == IIO_VOLTAGE) {
+ *val = 5 * 2500; /* 5 * Vref(2.5V) in mV */
+ *val2 = 12; /* 12-bit DAC resolution (2^12) */
+ } else {
+ *val = 25; /* Vref / (2 * Rsense) = 2500mV / 100 */
+ *val2 = 12; /* 12-bit DAC resolution (2^12) */
+ }
+ return IIO_VAL_FRACTIONAL_LOG2;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int max22007_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct max22007_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ return max22007_write_channel_data(st, chan->channel, val);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info max22007_info = {
+ .read_raw = max22007_read_raw,
+ .write_raw = max22007_write_raw,
+};
+
+static ssize_t max22007_read_dac_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct max22007_state *st = iio_priv(indio_dev);
+ unsigned int reg_val;
+ bool powerdown;
+ int ret;
+
+ ret = regmap_read(st->regmap, MAX22007_CHANNEL_MODE_REG, ®_val);
+ if (ret)
+ return ret;
+
+ powerdown = !(reg_val & MAX22007_CH_PWR_CH_MASK(chan->channel));
+
+ return sysfs_emit(buf, "%d\n", powerdown);
+}
+
+static ssize_t max22007_write_dac_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf, size_t len)
+{
+ struct max22007_state *st = iio_priv(indio_dev);
+ bool powerdown;
+ int ret;
+
+ ret = kstrtobool(buf, &powerdown);
+ if (ret)
+ return ret;
+
+ if (powerdown)
+ ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
+ MAX22007_CH_PWR_CH_MASK(chan->channel),
+ MAX22007_CH_PWR_VAL(chan->channel, MAX22007_CH_POWER_OFF));
+ else
+ ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
+ MAX22007_CH_PWR_CH_MASK(chan->channel),
+ MAX22007_CH_PWR_VAL(chan->channel, MAX22007_CH_POWER_ON));
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static const struct iio_chan_spec_ext_info max22007_ext_info[] = {
+ {
+ .name = "powerdown",
+ .read = max22007_read_dac_powerdown,
+ .write = max22007_write_dac_powerdown,
+ .shared = IIO_SEPARATE,
+ },
+ { },
+};
+
+static const struct iio_chan_spec max22007_channel_template = {
+ .output = 1,
+ .indexed = 1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
+ .ext_info = max22007_ext_info,
+};
+
+static int max22007_parse_channel_cfg(struct max22007_state *st, u8 *num_channels)
+{
+ struct device *dev = &st->spi->dev;
+ struct iio_chan_spec *iio_chan;
+ int ret, num_chan = 0, i = 0;
+ u32 reg;
+
+ num_chan = device_get_child_node_count(dev);
+ if (!num_chan)
+ return dev_err_probe(dev, -ENODEV, "no channels configured\n");
+
+ st->iio_chan = devm_kcalloc(dev, num_chan, sizeof(*st->iio_chan), GFP_KERNEL);
+ if (!st->iio_chan)
+ return -ENOMEM;
+
+ device_for_each_child_node_scoped(dev, child) {
+ const char *channel_type_str;
+ enum max22007_channel_mode mode;
+
+ ret = fwnode_property_read_u32(child, "reg", ®);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to read reg property of %pfwP\n", child);
+
+ if (reg >= MAX22007_NUM_CHANNELS)
+ return dev_err_probe(dev, -EINVAL,
+ "reg out of range in %pfwP\n", child);
+
+ iio_chan = &st->iio_chan[i];
+
+ *iio_chan = max22007_channel_template;
+ iio_chan->channel = reg;
+
+ ret = fwnode_property_read_string(child, "adi,type", &channel_type_str);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "missing adi,type property for %pfwP\n", child);
+
+ if (strcmp(channel_type_str, "current") == 0) {
+ mode = MAX22007_CURRENT_MODE;
+ iio_chan->type = IIO_CURRENT;
+ } else if (strcmp(channel_type_str, "voltage") == 0) {
+ mode = MAX22007_VOLTAGE_MODE;
+ iio_chan->type = IIO_VOLTAGE;
+ } else {
+ return dev_err_probe(dev, -EINVAL,
+ "invalid adi,type '%s' for %pfwP\n",
+ channel_type_str, child);
+ }
+
+ ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
+ MAX22007_CH_MODE_CH_MASK(reg),
+ MAX22007_CH_MODE_VAL(reg, mode));
+ if (ret)
+ return ret;
+
+ /* Set DAC to transparent mode (immediate update) */
+ ret = regmap_update_bits(st->regmap, MAX22007_CONFIG_REG,
+ MAX22007_DAC_LATCH_MODE_MASK(reg),
+ MAX22007_DAC_LATCH_MODE_VAL(reg, 1));
+ if (ret)
+ return ret;
+
+ i++;
+ }
+
+ *num_channels = num_chan;
+
+ return 0;
+}
+
+static int max22007_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct max22007_state *state;
+ struct gpio_desc *reset_gpio;
+ u8 num_channels;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ state = iio_priv(indio_dev);
+ state->spi = spi;
+
+ crc8_populate_lsb(max22007_crc8_table, MAX22007_CRC8_POLYNOMIAL);
+
+ state->regmap = devm_regmap_init(&spi->dev, &max22007_regmap_bus, state,
+ &max22007_regmap_config);
+ if (IS_ERR(state->regmap))
+ return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
+ "Failed to initialize regmap\n");
+
+ reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(reset_gpio))
+ return dev_err_probe(&spi->dev, PTR_ERR(reset_gpio),
+ "Failed to get reset GPIO\n");
+
+ if (reset_gpio) {
+ gpiod_set_value_cansleep(reset_gpio, 0);
+ } else {
+ ret = regmap_write(state->regmap, MAX22007_SOFT_RESET_REG,
+ MAX22007_SOFT_RESET_BITS_MASK);
+ if (ret)
+ return ret;
+ }
+
+ ret = regmap_update_bits(state->regmap, MAX22007_CONFIG_REG,
+ MAX22007_CRC_EN_MASK,
+ MAX22007_CRC_EN_MASK);
+ if (ret)
+ return ret;
+
+ ret = max22007_parse_channel_cfg(state, &num_channels);
+ if (ret)
+ return ret;
+
+ indio_dev->info = &max22007_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = state->iio_chan;
+ indio_dev->num_channels = num_channels;
+ indio_dev->name = "max22007";
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id max22007_id[] = {
+ { "max22007" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, max22007_id);
+
+static const struct of_device_id max22007_of_match[] = {
+ { .compatible = "adi,max22007" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max22007_of_match);
+
+static struct spi_driver max22007_driver = {
+ .driver = {
+ .name = "max22007",
+ .of_match_table = max22007_of_match,
+ },
+ .probe = max22007_probe,
+ .id_table = max22007_id,
+};
+module_spi_driver(max22007_driver);
+
+MODULE_AUTHOR("Janani Sunil <janani.sunil@analog.com>");
+MODULE_DESCRIPTION("Analog Devices MAX22007 DAC");
+MODULE_LICENSE("GPL");
--
2.43.0On Fri, 19 Dec 2025 16:31:17 +0100
Janani Sunil <janani.sunil@analog.com> wrote:
> Add support for the MAX22007 4 channel DAC
> that drives a voltage or current output on each channel.
wrap to 75 chars rather than 50-60ish
>
> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Hi Janani
A few minor things inline. Also add turning on any required
power supplies. See how other drivers do it with a single call
in in probe. If your board is using always on supplies it will just
work as a stub regulator will be provided by the regulator core.
Thanks,
Jonathan
> diff --git a/drivers/iio/dac/max22007.c b/drivers/iio/dac/max22007.c
> new file mode 100644
> index 000000000000..0d57fee27367
> --- /dev/null
> +++ b/drivers/iio/dac/max22007.c
> @@ -0,0 +1,487 @@
> +
> +#define MAX22007_NUM_CHANNELS 4
> +#define MAX22007_REV_ID_REG 0x00
> +#define MAX22007_STAT_INTR_REG 0x01
> +#define MAX22007_INTERRUPT_EN_REG 0x02
> +#define MAX22007_CONFIG_REG 0x03
> +#define MAX22007_CONTROL_REG 0x04
> +#define MAX22007_CHANNEL_MODE_REG 0x05
> +#define MAX22007_SOFT_RESET_REG 0x06
> +#define MAX22007_DAC_CHANNEL_REG(ch) (0x07 + (ch))
> +#define MAX22007_GPIO_CTRL_REG 0x0B
> +#define MAX22007_GPIO_DATA_REG 0x0C
> +#define MAX22007_GPI_EDGE_INT_CTRL_REG 0x0D
> +#define MAX22007_GPI_INT_STATUS_REG 0x0E
> +
> +/* Channel mask definitions */
> +#define MAX22007_CH_MODE_CH_MASK(channel) BIT(12 + (channel))
maybe use x or ch rather than channel to shorten the defines a little.
> +#define MAX22007_CH_PWR_CH_MASK(channel) BIT(8 + (channel))
> +#define MAX22007_DAC_LATCH_MODE_MASK(channel) BIT(12 + (channel))
> +#define MAX22007_LDAC_UPDATE_MASK(channel) BIT(12 + (channel))
> +#define MAX22007_SW_RST_MASK BIT(8)
> +#define MAX22007_SW_CLR_MASK BIT(12)
> +#define MAX22007_SOFT_RESET_BITS_MASK (MAX22007_SW_RST_MASK | \
> + MAX22007_SW_CLR_MASK)
Align after (
> +#define MAX22007_DAC_DATA_MASK GENMASK(15, 4)
> +#define MAX22007_DAC_MAX_RAW GENMASK(11, 0)
> +#define MAX22007_CRC8_POLYNOMIAL 0x8C
> +#define MAX22007_CRC_EN_MASK BIT(0)
> +#define MAX22007_RW_MASK BIT(0)
> +#define MAX22007_CRC_OVERHEAD 1
> +
> +/* Field value preparation macros with masking */
> +#define MAX22007_CH_PWR_VAL(channel, val) (((val) & 0x1) << (8 + (channel)))
> +#define MAX22007_CH_MODE_VAL(channel, val) (((val) & 0x1) << (12 + (channel)))
> +#define MAX22007_DAC_LATCH_MODE_VAL(channel, val) (((val) & 0x1) << (12 + (channel)))
> +
> +static u8 max22007_crc8_table[256];
> +
> +enum max22007_channel_mode {
> + MAX22007_VOLTAGE_MODE,
> + MAX22007_CURRENT_MODE
Add trailing comma. Not obvious there will never be more if other devices are supported
by the driver.
I'd also give these explicit values given they are written to HW.
= 0,
= 1,
> +};
> +
> +enum max22007_channel_power {
> + MAX22007_CH_POWER_OFF,
> + MAX22007_CH_POWER_ON,
This isn't bringing value over renaming the field mask define
to MAX22007_CH_PWRON_CH_MASK()
and using 0 / 1 as appropriate.
> +};
> +
> +struct max22007_state {
> + struct spi_device *spi;
> + struct regmap *regmap;
> + struct iio_chan_spec *iio_chan;
I'd go with iio_chans just to make it clear there can be multiple.
> + u8 tx_buf[4] __aligned(IIO_DMA_MINALIGN);
> + u8 rx_buf[4];
> +};
> +
> +static int max22007_spi_read(void *context, const void *reg, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + struct max22007_state *st = context;
> + u8 calculated_crc, received_crc;
> + u8 crc_data[3];
> + int ret;
> + struct spi_transfer xfer = {
> + .tx_buf = st->tx_buf,
> + .rx_buf = st->rx_buf,
> + };
> +
> + xfer.len = reg_size + val_size + MAX22007_CRC_OVERHEAD;
> +
> + memcpy(st->tx_buf, reg, reg_size);
> +
> + ret = spi_sync_transfer(st->spi, &xfer, 1);
> + if (ret) {
> + dev_err(&st->spi->dev, "SPI transfer failed: %d\n", ret);
> + return ret;
> + }
> +
> + crc_data[0] = st->tx_buf[0];
> + crc_data[1] = st->rx_buf[1];
> + crc_data[2] = st->rx_buf[2]
The use of only the first byte for tx and later for rx suggest this a
spi_write_then_read()
Using that should simplify your code a little particularly as it doesn't need
dma safe buffers (bounces anyway).
I'd be tempted to check reg_size == 1 then hard code that through this function.
> +
> + calculated_crc = crc8(max22007_crc8_table, crc_data, 3, 0x00);
> + received_crc = st->rx_buf[3];
> +
> + if (calculated_crc != received_crc) {
> + dev_err(&st->spi->dev, "CRC mismatch on read register %02x:\n", *(u8 *)reg);
> + return -EIO;
> + }
> +
> + /* Ignore the dummy byte 0 */
> + memcpy(val, &st->rx_buf[1], val_size);
> +
> + return 0;
> +}
> +
> +static int max22007_spi_write(void *context, const void *data, size_t count)
> +{
> + struct max22007_state *st = context;
> + struct spi_transfer xfer = {
> + .tx_buf = st->tx_buf,
> + .rx_buf = st->rx_buf,
> + };
> +
> + memset(st->tx_buf, 0, sizeof(st->tx_buf));
> +
> + xfer.len = count + MAX22007_CRC_OVERHEAD;
> +
> + memcpy(st->tx_buf, data, count);
> + st->tx_buf[count] = crc8(max22007_crc8_table, st->tx_buf,
> + sizeof(st->tx_buf) - 1, 0x00);
> +
> + return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +static int max22007_write_channel_data(struct max22007_state *state, unsigned int channel,
> + unsigned int data)
> +{
> + unsigned int reg_val;
> +
> + if (data > MAX22007_DAC_MAX_RAW)
> + return -EINVAL;
> +
> + reg_val = FIELD_PREP(MAX22007_DAC_DATA_MASK, data);
> +
> + return regmap_write(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), reg_val);
> +}
> +
> +static int max22007_read_channel_data(struct max22007_state *state, unsigned int channel,
Where it doesn't hurt readability my preference is still to stay nearer to 80 chars.
> + int *data)
> +{
> + int ret;
> + unsigned int reg_val;
> +
> + ret = regmap_read(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), ®_val);
> + if (ret)
> + return ret;
> +
> + *data = FIELD_GET(MAX22007_DAC_DATA_MASK, reg_val);
> +
> + return 0;
> +}
> +
> +static int max22007_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct max22007_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = max22007_read_channel_data(st, chan->channel, val);
> + if (ret)
> + return ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->type == IIO_VOLTAGE) {
> + *val = 5 * 2500; /* 5 * Vref(2.5V) in mV */
> + *val2 = 12; /* 12-bit DAC resolution (2^12) */
Given resolution is the same either way, drop that out of the if / else
if (chan->type == IIO_VOLTAGE)
*val = ...
else
*val = ...
val2 = 12; /* 12-bit DAC resolution */
The 2^12 is a bit confusing so I'd drop that bit.
> + } else {
> + *val = 25; /* Vref / (2 * Rsense) = 2500mV / 100 */
> + *val2 = 12; /* 12-bit DAC resolution (2^12) */
> + }
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +static const struct iio_chan_spec_ext_info max22007_ext_info[] = {
> + {
> + .name = "powerdown",
> + .read = max22007_read_dac_powerdown,
> + .write = max22007_write_dac_powerdown,
> + .shared = IIO_SEPARATE,
> + },
> + { },
No trailing comma for a 'terminating' entry like this. We don't want
to make it easy to add stuff after.
> +};
> +
> +static const struct iio_chan_spec max22007_channel_template = {
> + .output = 1,
> + .indexed = 1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .ext_info = max22007_ext_info,
> +};
> +
> +static int max22007_parse_channel_cfg(struct max22007_state *st, u8 *num_channels)
> +{
> + struct device *dev = &st->spi->dev;
> + struct iio_chan_spec *iio_chan;
> + int ret, num_chan = 0, i = 0;
Please don't mix declarations that assign with those that don't. It makes
it to easy to miss which ones are in which category.
int num_chan = 0, i = 0;
int ret;
> + u32 reg;
> +
> + num_chan = device_get_child_node_count(dev);
> + if (!num_chan)
> + return dev_err_probe(dev, -ENODEV, "no channels configured\n");
> +
> + st->iio_chan = devm_kcalloc(dev, num_chan, sizeof(*st->iio_chan), GFP_KERNEL);
> + if (!st->iio_chan)
> + return -ENOMEM;
> +
> + device_for_each_child_node_scoped(dev, child) {
> + const char *channel_type_str;
> + enum max22007_channel_mode mode;
> +
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to read reg property of %pfwP\n", child);
> +
> + if (reg >= MAX22007_NUM_CHANNELS)
> + return dev_err_probe(dev, -EINVAL,
> + "reg out of range in %pfwP\n", child);
> +
> + iio_chan = &st->iio_chan[i];
> +
> + *iio_chan = max22007_channel_template;
The template is only used here so I'd be tempted to just do
*iio_chan = (struct iio_chan_spec) {
.output = 1,
.indexed = 1,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
.ext_info = max22007_ext_info,
.channel = reg,
};
inline here.
Or after other changes suggested below you can do
st->iio_chan[i++] = (struct iio_chan_spec) {
.output = 1,
.indexed = 1,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
.ext_info = max22007_ext_info,
.channel = reg,
.type = chan_type,
}
> + iio_chan->channel = reg;
> +
> + ret = fwnode_property_read_string(child, "adi,type", &channel_type_str);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "missing adi,type property for %pfwP\n", child);
> +
> + if (strcmp(channel_type_str, "current") == 0) {
> + mode = MAX22007_CURRENT_MODE;
> + iio_chan->type = IIO_CURRENT;
> + } else if (strcmp(channel_type_str, "voltage") == 0) {
> + mode = MAX22007_VOLTAGE_MODE;
> + iio_chan->type = IIO_VOLTAGE;
> + } else {
> + return dev_err_probe(dev, -EINVAL,
> + "invalid adi,type '%s' for %pfwP\n",
> + channel_type_str, child);
> + }
If you do this to set a local type variable before the *iio_chan =
suggestion above, can assign it when filling in the rest of the chan_spec
> +
> + ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
> + MAX22007_CH_MODE_CH_MASK(reg),
> + MAX22007_CH_MODE_VAL(reg, mode));
> + if (ret)
> + return ret;
> +
> + /* Set DAC to transparent mode (immediate update) */
> + ret = regmap_update_bits(st->regmap, MAX22007_CONFIG_REG,
> + MAX22007_DAC_LATCH_MODE_MASK(reg),
> + MAX22007_DAC_LATCH_MODE_VAL(reg, 1));
> + if (ret)
> + return ret;
> +
> + i++;
With other changes suggested above, i will only be used in one place I think
so you can do the ++ inline at that point. See above for details.
> + }
> +
> + *num_channels = num_chan;
> +
> + return 0;
> +}
> +
> +static int max22007_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct max22007_state *state;
> + struct gpio_desc *reset_gpio;
> + u8 num_channels;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
Use a local
struct device *dev = &spi->dev;
to shorten all the places you have &spi->dev currently in this function.
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + state = iio_priv(indio_dev);
> + state->spi = spi;
> +
> + crc8_populate_lsb(max22007_crc8_table, MAX22007_CRC8_POLYNOMIAL);
> +
> + state->regmap = devm_regmap_init(&spi->dev, &max22007_regmap_bus, state,
> + &max22007_regmap_config);
> + if (IS_ERR(state->regmap))
> + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> + "Failed to initialize regmap\n");
> +
> + reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
What sets the gpio high? I'd expect a transition from what is requested here
to what is set in the set_value() below.
> + if (IS_ERR(reset_gpio))
> + return dev_err_probe(&spi->dev, PTR_ERR(reset_gpio),
> + "Failed to get reset GPIO\n");
> +
> + if (reset_gpio) {
> + gpiod_set_value_cansleep(reset_gpio, 0);
> + } else {
> + ret = regmap_write(state->regmap, MAX22007_SOFT_RESET_REG,
> + MAX22007_SOFT_RESET_BITS_MASK);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_update_bits(state->regmap, MAX22007_CONFIG_REG,
> + MAX22007_CRC_EN_MASK,
> + MAX22007_CRC_EN_MASK);
regmap_set_bits() saves repeating the mask.
> + if (ret)
> + return ret;
> +
> + ret = max22007_parse_channel_cfg(state, &num_channels);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &max22007_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = state->iio_chan;
> + indio_dev->num_channels = num_channels;
> + indio_dev->name = "max22007";
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
Hi Jonathan,
Thank you for your review.
On 12/19/25 18:25, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 16:31:17 +0100
> Janani Sunil <janani.sunil@analog.com> wrote:
>
>> Add support for the MAX22007 4 channel DAC
>> that drives a voltage or current output on each channel.
> wrap to 75 chars rather than 50-60ish
Noted. Will correct this.
>> Signed-off-by: Janani Sunil <janani.sunil@analog.com>
> Hi Janani
>
> A few minor things inline. Also add turning on any required
> power supplies. See how other drivers do it with a single call
> in in probe. If your board is using always on supplies it will just
> work as a stub regulator will be provided by the regulator core.
>
>
> Thanks,
>
> Jonathan
Will take a reference from the other drivers and add the power supply configurations.
>> diff --git a/drivers/iio/dac/max22007.c b/drivers/iio/dac/max22007.c
>> new file mode 100644
>> index 000000000000..0d57fee27367
>> --- /dev/null
>> +++ b/drivers/iio/dac/max22007.c
>> @@ -0,0 +1,487 @@
>> +
>> +#define MAX22007_NUM_CHANNELS 4
>> +#define MAX22007_REV_ID_REG 0x00
>> +#define MAX22007_STAT_INTR_REG 0x01
>> +#define MAX22007_INTERRUPT_EN_REG 0x02
>> +#define MAX22007_CONFIG_REG 0x03
>> +#define MAX22007_CONTROL_REG 0x04
>> +#define MAX22007_CHANNEL_MODE_REG 0x05
>> +#define MAX22007_SOFT_RESET_REG 0x06
>> +#define MAX22007_DAC_CHANNEL_REG(ch) (0x07 + (ch))
>> +#define MAX22007_GPIO_CTRL_REG 0x0B
>> +#define MAX22007_GPIO_DATA_REG 0x0C
>> +#define MAX22007_GPI_EDGE_INT_CTRL_REG 0x0D
>> +#define MAX22007_GPI_INT_STATUS_REG 0x0E
>> +
>> +/* Channel mask definitions */
>> +#define MAX22007_CH_MODE_CH_MASK(channel) BIT(12 + (channel))
> maybe use x or ch rather than channel to shorten the defines a little.
Right, shall take up the change.
>
>> +#define MAX22007_CH_PWR_CH_MASK(channel) BIT(8 + (channel))
>> +#define MAX22007_DAC_LATCH_MODE_MASK(channel) BIT(12 + (channel))
>> +#define MAX22007_LDAC_UPDATE_MASK(channel) BIT(12 + (channel))
>> +#define MAX22007_SW_RST_MASK BIT(8)
>> +#define MAX22007_SW_CLR_MASK BIT(12)
>> +#define MAX22007_SOFT_RESET_BITS_MASK (MAX22007_SW_RST_MASK | \
>> + MAX22007_SW_CLR_MASK)
> Align after (
Right. Shall take up the change.
>
>> +#define MAX22007_DAC_DATA_MASK GENMASK(15, 4)
>> +#define MAX22007_DAC_MAX_RAW GENMASK(11, 0)
>> +#define MAX22007_CRC8_POLYNOMIAL 0x8C
>> +#define MAX22007_CRC_EN_MASK BIT(0)
>> +#define MAX22007_RW_MASK BIT(0)
>> +#define MAX22007_CRC_OVERHEAD 1
>> +
>> +/* Field value preparation macros with masking */
>> +#define MAX22007_CH_PWR_VAL(channel, val) (((val) & 0x1) << (8 + (channel)))
>> +#define MAX22007_CH_MODE_VAL(channel, val) (((val) & 0x1) << (12 + (channel)))
>> +#define MAX22007_DAC_LATCH_MODE_VAL(channel, val) (((val) & 0x1) << (12 + (channel)))
>> +
>> +static u8 max22007_crc8_table[256];
>> +
>> +enum max22007_channel_mode {
>> + MAX22007_VOLTAGE_MODE,
>> + MAX22007_CURRENT_MODE
> Add trailing comma. Not obvious there will never be more if other devices are supported
> by the driver.
>
> I'd also give these explicit values given they are written to HW.
> = 0,
> = 1,
Agreed. Shall take up the change.
>
>> +};
>> +
>> +enum max22007_channel_power {
>> + MAX22007_CH_POWER_OFF,
>> + MAX22007_CH_POWER_ON,
> This isn't bringing value over renaming the field mask define
> to MAX22007_CH_PWRON_CH_MASK()
> and using 0 / 1 as appropriate.
Got your point. Shall take this up.
>
>> +};
>> +
>> +struct max22007_state {
>> + struct spi_device *spi;
>> + struct regmap *regmap;
>> + struct iio_chan_spec *iio_chan;
> I'd go with iio_chans just to make it clear there can be multiple.
I shall take this up.
>
>> + u8 tx_buf[4] __aligned(IIO_DMA_MINALIGN);
>> + u8 rx_buf[4];
>> +};
>> +
>> +static int max22007_spi_read(void *context, const void *reg, size_t reg_size,
>> + void *val, size_t val_size)
>> +{
>> + struct max22007_state *st = context;
>> + u8 calculated_crc, received_crc;
>> + u8 crc_data[3];
>> + int ret;
>> + struct spi_transfer xfer = {
>> + .tx_buf = st->tx_buf,
>> + .rx_buf = st->rx_buf,
>> + };
>> +
>> + xfer.len = reg_size + val_size + MAX22007_CRC_OVERHEAD;
>> +
>> + memcpy(st->tx_buf, reg, reg_size);
>> +
>> + ret = spi_sync_transfer(st->spi, &xfer, 1);
>> + if (ret) {
>> + dev_err(&st->spi->dev, "SPI transfer failed: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + crc_data[0] = st->tx_buf[0];
>> + crc_data[1] = st->rx_buf[1];
>> + crc_data[2] = st->rx_buf[2]
> The use of only the first byte for tx and later for rx suggest this a
> spi_write_then_read()
>
> Using that should simplify your code a little particularly as it doesn't need
> dma safe buffers (bounces anyway).
>
> I'd be tempted to check reg_size == 1 then hard code that through this function.
You are right. Shall implement this.
>
>> +
>> + calculated_crc = crc8(max22007_crc8_table, crc_data, 3, 0x00);
>> + received_crc = st->rx_buf[3];
>> +
>> + if (calculated_crc != received_crc) {
>> + dev_err(&st->spi->dev, "CRC mismatch on read register %02x:\n", *(u8 *)reg);
>> + return -EIO;
>> + }
>> +
>> + /* Ignore the dummy byte 0 */
>> + memcpy(val, &st->rx_buf[1], val_size);
>> +
>> + return 0;
>> +}
>> +
>> +static int max22007_spi_write(void *context, const void *data, size_t count)
>> +{
>> + struct max22007_state *st = context;
>> + struct spi_transfer xfer = {
>> + .tx_buf = st->tx_buf,
>> + .rx_buf = st->rx_buf,
>> + };
>> +
>> + memset(st->tx_buf, 0, sizeof(st->tx_buf));
>> +
>> + xfer.len = count + MAX22007_CRC_OVERHEAD;
>> +
>> + memcpy(st->tx_buf, data, count);
>> + st->tx_buf[count] = crc8(max22007_crc8_table, st->tx_buf,
>> + sizeof(st->tx_buf) - 1, 0x00);
>> +
>> + return spi_sync_transfer(st->spi, &xfer, 1);
>> +}
>> +static int max22007_write_channel_data(struct max22007_state *state, unsigned int channel,
>> + unsigned int data)
>> +{
>> + unsigned int reg_val;
>> +
>> + if (data > MAX22007_DAC_MAX_RAW)
>> + return -EINVAL;
>> +
>> + reg_val = FIELD_PREP(MAX22007_DAC_DATA_MASK, data);
>> +
>> + return regmap_write(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), reg_val);
>> +}
>> +
>> +static int max22007_read_channel_data(struct max22007_state *state, unsigned int channel,
> Where it doesn't hurt readability my preference is still to stay nearer to 80 chars.
Noted your point. Shall apply the readability preferences as said.
>
>> + int *data)
>> +{
>> + int ret;
>> + unsigned int reg_val;
>> +
>> + ret = regmap_read(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), ®_val);
>> + if (ret)
>> + return ret;
>> +
>> + *data = FIELD_GET(MAX22007_DAC_DATA_MASK, reg_val);
>> +
>> + return 0;
>> +}
>> +
>> +static int max22007_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct max22007_state *st = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = max22007_read_channel_data(st, chan->channel, val);
>> + if (ret)
>> + return ret;
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_SCALE:
>> + if (chan->type == IIO_VOLTAGE) {
>> + *val = 5 * 2500; /* 5 * Vref(2.5V) in mV */
>> + *val2 = 12; /* 12-bit DAC resolution (2^12) */
> Given resolution is the same either way, drop that out of the if / else
> if (chan->type == IIO_VOLTAGE)
> *val = ...
> else
> *val = ...
> val2 = 12; /* 12-bit DAC resolution */
>
> The 2^12 is a bit confusing so I'd drop that bit.
Yes, it can be confusing. I shall drop the redundant explanation.
>
>> + } else {
>> + *val = 25; /* Vref / (2 * Rsense) = 2500mV / 100 */
>> + *val2 = 12; /* 12-bit DAC resolution (2^12) */
>> + }
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>
>> +static const struct iio_chan_spec_ext_info max22007_ext_info[] = {
>> + {
>> + .name = "powerdown",
>> + .read = max22007_read_dac_powerdown,
>> + .write = max22007_write_dac_powerdown,
>> + .shared = IIO_SEPARATE,
>> + },
>> + { },
> No trailing comma for a 'terminating' entry like this. We don't want
> to make it easy to add stuff after.
Sure, will remove them.
>> +};
>> +
>> +static const struct iio_chan_spec max22007_channel_template = {
>> + .output = 1,
>> + .indexed = 1,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>> + .ext_info = max22007_ext_info,
>> +};
>> +
>> +static int max22007_parse_channel_cfg(struct max22007_state *st, u8 *num_channels)
>> +{
>> + struct device *dev = &st->spi->dev;
>> + struct iio_chan_spec *iio_chan;
>> + int ret, num_chan = 0, i = 0;
> Please don't mix declarations that assign with those that don't. It makes
> it to easy to miss which ones are in which category.
> int num_chan = 0, i = 0;
> int ret;
> Noted on this. I will separate them accordingly.
>> + u32 reg;
>> +
>> + num_chan = device_get_child_node_count(dev);
>> + if (!num_chan)
>> + return dev_err_probe(dev, -ENODEV, "no channels configured\n");
>> +
>> + st->iio_chan = devm_kcalloc(dev, num_chan, sizeof(*st->iio_chan), GFP_KERNEL);
>> + if (!st->iio_chan)
>> + return -ENOMEM;
>> +
>> + device_for_each_child_node_scoped(dev, child) {
>> + const char *channel_type_str;
>> + enum max22007_channel_mode mode;
>> +
>> + ret = fwnode_property_read_u32(child, "reg", ®);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "failed to read reg property of %pfwP\n", child);
>> +
>> + if (reg >= MAX22007_NUM_CHANNELS)
>> + return dev_err_probe(dev, -EINVAL,
>> + "reg out of range in %pfwP\n", child);
>> +
>> + iio_chan = &st->iio_chan[i];
>> +
>> + *iio_chan = max22007_channel_template;
> The template is only used here so I'd be tempted to just do
> *iio_chan = (struct iio_chan_spec) {
> .output = 1,
> .indexed = 1,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> .ext_info = max22007_ext_info,
> .channel = reg,
> };
> inline here.
> Or after other changes suggested below you can do
>
> st->iio_chan[i++] = (struct iio_chan_spec) {
> .output = 1,
> .indexed = 1,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> .ext_info = max22007_ext_info,
> .channel = reg,
> .type = chan_type,
> }
This is a great idea. I will take this up.
>> + iio_chan->channel = reg;
>> +
>> + ret = fwnode_property_read_string(child, "adi,type", &channel_type_str);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "missing adi,type property for %pfwP\n", child);
>> +
>> + if (strcmp(channel_type_str, "current") == 0) {
>> + mode = MAX22007_CURRENT_MODE;
>> + iio_chan->type = IIO_CURRENT;
>> + } else if (strcmp(channel_type_str, "voltage") == 0) {
>> + mode = MAX22007_VOLTAGE_MODE;
>> + iio_chan->type = IIO_VOLTAGE;
>> + } else {
>> + return dev_err_probe(dev, -EINVAL,
>> + "invalid adi,type '%s' for %pfwP\n",
>> + channel_type_str, child);
>> + }
> If you do this to set a local type variable before the *iio_chan =
> suggestion above, can assign it when filling in the rest of the chan_spec
I will take this up.
>
>> +
>> + ret = regmap_update_bits(st->regmap, MAX22007_CHANNEL_MODE_REG,
>> + MAX22007_CH_MODE_CH_MASK(reg),
>> + MAX22007_CH_MODE_VAL(reg, mode));
>> + if (ret)
>> + return ret;
>> +
>> + /* Set DAC to transparent mode (immediate update) */
>> + ret = regmap_update_bits(st->regmap, MAX22007_CONFIG_REG,
>> + MAX22007_DAC_LATCH_MODE_MASK(reg),
>> + MAX22007_DAC_LATCH_MODE_VAL(reg, 1));
>> + if (ret)
>> + return ret;
>> +
>> + i++;
> With other changes suggested above, i will only be used in one place I think
> so you can do the ++ inline at that point. See above for details.
Got your point here.
>> + }
>> +
>> + *num_channels = num_chan;
>> +
>> + return 0;
>> +}
>> +
>> +static int max22007_probe(struct spi_device *spi)
>> +{
>> + struct iio_dev *indio_dev;
>> + struct max22007_state *state;
>> + struct gpio_desc *reset_gpio;
>> + u8 num_channels;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> Use a local
> struct device *dev = &spi->dev;
> to shorten all the places you have &spi->dev currently in this function.
Will implement this.
>
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + state = iio_priv(indio_dev);
>> + state->spi = spi;
>> +
>> + crc8_populate_lsb(max22007_crc8_table, MAX22007_CRC8_POLYNOMIAL);
>> +
>> + state->regmap = devm_regmap_init(&spi->dev, &max22007_regmap_bus, state,
>> + &max22007_regmap_config);
>> + if (IS_ERR(state->regmap))
>> + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
>> + "Failed to initialize regmap\n");
>> +
>> + reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW);
> What sets the gpio high? I'd expect a transition from what is requested here
> to what is set in the set_value() below.
It needs to be toggled within the probe. Will add proper implementation for this.
>
>> + if (IS_ERR(reset_gpio))
>> + return dev_err_probe(&spi->dev, PTR_ERR(reset_gpio),
>> + "Failed to get reset GPIO\n");
>> +
>> + if (reset_gpio) {
>> + gpiod_set_value_cansleep(reset_gpio, 0);
>> + } else {
>> + ret = regmap_write(state->regmap, MAX22007_SOFT_RESET_REG,
>> + MAX22007_SOFT_RESET_BITS_MASK);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + ret = regmap_update_bits(state->regmap, MAX22007_CONFIG_REG,
>> + MAX22007_CRC_EN_MASK,
>> + MAX22007_CRC_EN_MASK);
> regmap_set_bits() saves repeating the mask.
That's a good point. I will take this up.
>
>> + if (ret)
>> + return ret;
>> +
>> + ret = max22007_parse_channel_cfg(state, &num_channels);
>> + if (ret)
>> + return ret;
>> +
>> + indio_dev->info = &max22007_info;
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> + indio_dev->channels = state->iio_chan;
>> + indio_dev->num_channels = num_channels;
>> + indio_dev->name = "max22007";
>> +
>> + return devm_iio_device_register(&spi->dev, indio_dev);
>> +}
>
On Wed, 7 Jan 2026 16:28:31 +0100 Janani Sunil <jan.sun97@gmail.com> wrote: > Hi Jonathan, > > Thank you for your review. > > On 12/19/25 18:25, Jonathan Cameron wrote: > > On Fri, 19 Dec 2025 16:31:17 +0100 > > Janani Sunil <janani.sunil@analog.com> wrote: > > > >> Add support for the MAX22007 4 channel DAC > >> that drives a voltage or current output on each channel. > > wrap to 75 chars rather than 50-60ish > > Noted. Will correct this. > > >> Signed-off-by: Janani Sunil <janani.sunil@analog.com> > > Hi Janani > > > > A few minor things inline. Also add turning on any required > > power supplies. See how other drivers do it with a single call > > in in probe. If your board is using always on supplies it will just > > work as a stub regulator will be provided by the regulator core. > > > > > > Thanks, > > > > Jonathan > > Will take a reference from the other drivers and add the power supply configurations. Hi Janani, A small process thing you should take into account for future replies. Generally kernel mailing lists are very high volume with a lot of review feedback. As such we tend to go for a balance of efficiency over politeness. So when you agree to a particular bit of feedback, just crop out that bit of the message in any reply (if you agree to all of it no need to reply at all!). That lets us focus in quickly on the bits that need more discussion. The politeness bit is resolve by adding a thank alongside the change log in the next version. I end up sending this message to someone most weeks, so don't worry about it! It feels unnatural to all of us initially. Thanks Jonathan
© 2016 - 2026 Red Hat, Inc.