Message-ID: <098d6f0e-95f0-4a25-938b-eee3bb35c77a@gmail.com>
Date: Fri, 9 Jan 2026 16:36:08 +0100
From: Janani Sunil <jan.sun97@...il.com>
To: Marcelo Schmitt <marcelo.schmitt1@...il.com>,
 Janani Sunil <janani.sunil@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Alexandru Ardelean <alexandru.ardelean@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: dac: Add MAX22007 DAC driver support

Hi Marcelo,

Thank you for reviewing the patch.

On 1/9/26 15:11, Marcelo Schmitt wrote:
> Overall, this is looking much better than previous versions.
> Here comes another round of review for this.
>
> On 01/08, Janani Sunil wrote:
>> Add support for the MAX22007, a 4-channel 12-bit DAC that drives
>> voltage or current output on each channel.
>>
>> Signed-off-by: Janani Sunil <janani.sunil@...log.com>
>> ---
>>   MAINTAINERS                |   1 +
>>   drivers/iio/dac/Kconfig    |  13 ++
>>   drivers/iio/dac/Makefile   |   1 +
>>   drivers/iio/dac/max22007.c | 507 +++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 522 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e1addbd21562..99dd3c947629 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1599,6 +1599,7 @@ L:	linux-iio@...r.kernel.org
>>   S:	Supported
>>   W:	https://ez.analog.com/linux-software-drivers
>>   F:	Documentation/devicetree/bindings/iio/dac/adi,max22007.yaml
>> +F:	drivers/iio/dac/max22007.c
>>   
>>   ANALOG DEVICES INC ADA4250 DRIVER
>>   M:	Antoniu Miclaus <antoniu.miclaus@...log.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..19557c008554
>> --- /dev/null
>> +++ b/drivers/iio/dac/max22007.c
>> @@ -0,0 +1,507 @@
> ...
>> +static u8 max22007_crc8_table[256];
> Can use CRC8_TABLE_SIZE to define the table size.

Will take this up.

>
> ...
>> +
>> +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 reg_byte = *(u8 *)reg;
> Odd casting. Not sure the const qualifier is needed for the reg parameter.
> See how other IIO drivers implement regmap_bus. ad7091r8 is one I recall from
> the top of my mind.

Noted. Will update this.

>> +	u8 calculated_crc, received_crc;
>> +	u8 crc_data[3];
>> +	u8 rx_buf[4];
>> +	int ret;
>> +
>> +	if (reg_size != 1)
>> +		return -EINVAL;
>> +
>> +	ret = spi_write_then_read(st->spi, &reg_byte, 1, rx_buf,
>> +				  val_size + MAX22007_CRC_OVERHEAD);
>> +	if (ret) {
>> +		dev_err(&st->spi->dev, "SPI transfer failed: %d\n", ret);
>> +		return ret;
>> +	}
> ...
>
>> +static int max22007_read_channel_data(struct max22007_state *state,
>> +				      unsigned int channel, int *data)
>> +{
>> +	int ret;
>> +	unsigned int reg_val;
> nitpicking: why not following reverse xmas tree convection here too?
>
> unsigned int reg_val;
> int ret;

Shall follow this format.

>
>> +
>> +	ret = regmap_read(state->regmap, MAX22007_DAC_CHANNEL_REG(channel), &reg_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 */
> Interesting that the external reference (if provided) is also expected to be 2.5 V.
> I'd set a define for that, e.g.
> #define MAX22007_REF_mV		2500
> Btw, what is that 5 in '5 * 2500'

Will add a macro for the reference voltage.
5 is the gain coefficient in the output stage (driver).

>
>> +		else
>> +			*val = 25;  /* Vref / (2 * Rsense) = 2500mV / 100 */
>> +		*val2 = 12;  /* 12-bit DAC resolution */
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
> ...
>> +
>> +static int max22007_parse_channel_cfg(struct max22007_state *st, u8 *num_channels)
>> +{
>> +	struct device *dev = &st->spi->dev;
>> +	int ret, num_chan;
>> +	int 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_chans = devm_kcalloc(dev, num_chan, sizeof(*st->iio_chans), GFP_KERNEL);
>> +	if (!st->iio_chans)
>> +		return -ENOMEM;
>> +
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch_func;
>> +		enum max22007_channel_mode mode;
>> +		enum iio_chan_type chan_type;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &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);
>> +
>> +		ret = fwnode_property_read_u32(child, "adi,ch-func", &ch_func);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "missing adi,ch-func property for %pfwP\n", child);
>> +
>> +		if (ch_func == 1) {
>> +			mode = MAX22007_VOLTAGE_MODE;
>> +			chan_type = IIO_VOLTAGE;
>> +		} else if (ch_func == 2) {
>> +			mode = MAX22007_CURRENT_MODE;
>> +			chan_type = IIO_CURRENT;
>> +		} else {
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "invalid adi,ch-func %u for %pfwP\n",
>> +					     ch_func, child);
>> +		}
>> +
>> +		st->iio_chans[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,
>> +		};
> IMHO, this would look cleaner with a channel template. See ad7124, ad4130,
> ad4170 for examples.

A channel template approach was followed initially and this approach was taken up based on the latest suggestions from the reviewers, since the template is not being reused elsewhere.

>
>> +
>> +		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;
>> +	}
>> +
>> +	*num_channels = num_chan;
>> +
>> +	return 0;
>> +}
>> +
> ...
>> +static int max22007_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	struct iio_dev *indio_dev;
>> +	struct max22007_state *state;
>> +	struct gpio_desc *reset_gpio;
>> +	u8 num_channels;
>> +	int ret, i;
>> +
>> +	indio_dev = devm_iio_device_alloc(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(dev, &max22007_regmap_bus, state,
>> +					 &max22007_regmap_config);
>> +	if (IS_ERR(state->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(state->regmap),
>> +				     "Failed to initialize regmap\n");
>> +
>> +	for (i = 0; i < MAX22007_NUM_SUPPLIES; i++)
>> +		state->supplies[i].supply = max22007_supply_names[i];
>> +
>> +	ret = devm_regulator_bulk_get(dev, MAX22007_NUM_SUPPLIES, state->supplies);
> devm_regulator_bulk_get_enable(), so max22007_regulator_disable() and
> state->supplies won't be needed anymore.

Will update the same.

>
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get regulators\n");
>> +
>> +	ret = regulator_bulk_enable(MAX22007_NUM_SUPPLIES, state->supplies);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, max22007_regulator_disable, state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> +	if (IS_ERR(reset_gpio))
>> +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
>> +				     "Failed to get reset GPIO\n");
> I'm not against the conventional way we've been getting and using reset
> GPIOs but, if other reviewers request to use the reset framework, you may do so
> with devm_reset_control_get_optional_exclusive_deasserted(dev, NULL).
> See [1] for an example (if needed).
>
> [1]: https://lore.kernel.org/linux-iio/6ae8e203f6fb6e9718271132bd35daef790ab574.1767795849.git.marcelo.schmitt@analog.com/

Sure.

>
>> +
>> +	if (reset_gpio) {
>> +		usleep_range(1000, 5000);
>> +		gpiod_set_value_cansleep(reset_gpio, 1);
>> +		usleep_range(1000, 5000);
>> +	} else {
>> +		ret = regmap_write(state->regmap, MAX22007_SOFT_RESET_REG,
>> +				   MAX22007_SOFT_RESET_BITS_MASK);
>> +		if (ret)
>> +			return ret;
>> +	}


Best Regards,
Janani Sunil

Powered by blists - more mailing lists