Re: [PATCH 3/3] iio: dac: Add MAX22007 DAC driver support
From: Janani Sunil
Date: Wed Jan 07 2026 - 10:28:37 EST
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@xxxxxxxxxx> wrote:
Add support for the MAX22007 4 channel DACwrap to 75 chars rather than 50-60ish
that drives a voltage or current output on each channel.
Noted. Will correct this.
Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>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.cmaybe use x or ch rather than channel to shorten the defines a little.
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))
Right, shall take up the change.
+#define MAX22007_CH_PWR_CH_MASK(channel) BIT(8 + (channel))Align after (
+#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)
Right. Shall take up the change.
+#define MAX22007_DAC_DATA_MASK GENMASK(15, 4)Add trailing comma. Not obvious there will never be more if other devices are supported
+#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
by the driver.
I'd also give these explicit values given they are written to HW.
= 0,
= 1,
Agreed. Shall take up the change.
+};This isn't bringing value over renaming the field mask define
+
+enum max22007_channel_power {
+ MAX22007_CH_POWER_OFF,
+ MAX22007_CH_POWER_ON,
to MAX22007_CH_PWRON_CH_MASK()
and using 0 / 1 as appropriate.
Got your point. Shall take this up.
+};I'd go with iio_chans just to make it clear there can be multiple.
+
+struct max22007_state {
+ struct spi_device *spi;
+ struct regmap *regmap;
+ struct iio_chan_spec *iio_chan;
I shall take this up.
+ u8 tx_buf[4] __aligned(IIO_DMA_MINALIGN);The use of only the first byte for tx and later for rx suggest this a
+ 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]
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.
+Where it doesn't hurt readability my preference is still to stay nearer to 80 chars.
+ 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,
Noted your point. Shall apply the readability preferences as said.
+ int *data)Given resolution is the same either way, drop that out of the if / else
+{
+ 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) */
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[] = {No trailing comma for a 'terminating' entry like this. We don't want
+ {
+ .name = "powerdown",
+ .read = max22007_read_dac_powerdown,
+ .write = max22007_write_dac_powerdown,
+ .shared = IIO_SEPARATE,
+ },
+ { },
to make it easy to add stuff after.
Sure, will remove them.
+};Please don't mix declarations that assign with those that don't. It makes
+
+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;
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;The template is only used here so I'd be tempted to just do
+
+ 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 = (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;If you do this to set a local type variable before the *iio_chan =
+
+ 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);
+ }
suggestion above, can assign it when filling in the rest of the chan_spec
I will take this up.
+With other changes suggested above, i will only be used in one place I think
+ 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++;
so you can do the ++ inline at that point. See above for details.
Got your point here.
+ }Use a local
+
+ *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));
struct device *dev = &spi->dev;
to shorten all the places you have &spi->dev currently in this function.
Will implement this.
+ if (!indio_dev)What sets the gpio high? I'd expect a transition from what is requested here
+ 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);
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))regmap_set_bits() saves repeating the mask.
+ 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);
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);
+}