Re: [PATCH 2/2] iio: dac: add support for Microchip MCP48FEB02

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Thu, Feb 12, 2026 at 02:48:35PM +0200, Ariana Lazar wrote:
> This is the iio driver for Microchip MCP48FxBy1/2/4/8 series of buffered
> voltage output Digital-to-Analog Converters with nonvolatile or volatile
> memory and an SPI Interface.
> 
> The families support up to 8 output channels.
> 
> The devices can be 8-bit, 10-bit and 12-bit.

...

> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

I would split this group...

> +#include <linux/kstrtox.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.h>
> +#include <linux/time64.h>
> +#include <linux/types.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/units.h>
> +

...to be here as other IIO drivers do.

...

> +/* Gain Control and I2C Slave Address Reguster fields */
> +#define DAC_GAIN_MASK(ch)				(BIT(0) << (8 + (ch)))

Just BIT(8 + (ch)) should suffice.

> +#define DAC_GAIN_VAL(ch, val)				((val) << (8 + (ch)))

For the sake of consistency this may be also rewritten to

#define DAC_GAIN_VAL(ch, val)				((val) * BIT(8 + (ch)))

...

> +/**
> + * struct mcp48feb02_channel_data - channel configuration
> + * @ref_mode: chosen voltage for reference
> + * @use_2x_gain: output driver gain control
> + * @powerdown: is false if the channel is in normal operation mode
> + * @powerdown_mode: selected power-down mode
> + * @dac_data: dac value

DAC

> + */
> +struct mcp48feb02_channel_data {
> +	u8 ref_mode;
> +	bool use_2x_gain;
> +	bool powerdown;
> +	u8 powerdown_mode;
> +	u16 dac_data;

Wondering if the following arrangement is slightly better:

	u16 dac_data;
	u8 ref_mode;
	u8 powerdown_mode;
	bool powerdown;
	bool use_2x_gain;

> +};

...

> +/**
> + * struct mcp48feb02_data - chip configuration
> + * @chdata: options configured for each channel on the device
> + * @lock: prevents concurrent reads/writes to driver's state members
> + * @chip_features: pointer to features struct
> + * @scale_1: scales set on channels that are based on Vref1
> + * @scale: scales set on channels that are based on Vref/Vref0
> + * @active_channels_mask: enabled channels
> + * @regmap: regmap for directly accessing device register
> + * @labels: table with channels labels
> + * @phys_channels: physical channels on the device
> + * @vref1_buffered: Vref1 buffer is enabled
> + * @vref_buffered: Vref/Vref0 buffer is enabled
> + * @use_vref1: vref1-supply is defined
> + * @use_vref: vref-supply is defined
> + */
> +struct mcp48feb02_data {
> +	struct mcp48feb02_channel_data chdata[MCP48FEB02_MAX_CH];
> +	struct mutex lock; /* prevents concurrent reads/writes to driver's state members */
> +	const struct mcp48feb02_features *chip_features;
> +	int scale_1[2 * MCP48FEB02_MAX_SCALES_CH];
> +	int scale[2 * MCP48FEB02_MAX_SCALES_CH];

I would name it scale1 and scale0. This will increase readability to see that
the sizes are equal and that the first digit is the part of the name.

> +	unsigned long active_channels_mask;
> +	struct regmap *regmap;
> +	const char *labels[MCP48FEB02_MAX_CH];
> +	u16 phys_channels;
> +	bool vref1_buffered;
> +	bool vref_buffered;
> +	bool use_vref1;
> +	bool use_vref;
> +};

...

> +static int mcp48feb02_write_to_eeprom(struct mcp48feb02_data *data, unsigned int reg,
> +				      unsigned int val)
> +{
> +	int eewa_val, ret;

Is it okay that the eewa_val is signed?

> +	ret = regmap_read_poll_timeout(data->regmap, MCP48FEB02_GAIN_CTRL_STATUS_REG_ADDR,
> +				       eewa_val,
> +				       !(eewa_val & MCP48FEB02_GAIN_BIT_STATUS_EEWA_MASK),
> +				       USEC_PER_MSEC, USEC_PER_MSEC * 5);

I would rather put it as

				       1 * USEC_PER_MSEC, 5 * USEC_PER_MSEC);

This follows the natural (from physics) reading — 1ms, 5ms.

> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(data->regmap, reg, val);
> +}

...

> +static ssize_t store_eeprom_store(struct device *dev, struct device_attribute *attr,
> +				  const char *buf, size_t len)
> +{
> +	struct mcp48feb02_data *data = iio_priv(dev_to_iio_dev(dev));
> +	unsigned int i, val, val1, eewa_val;
> +	bool state;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &state);
> +	if (ret)
> +		return ret;
> +
> +	if (!state)
> +		return 0;
> +
> +	/*
> +	 * Wait until the currently occurring EEPROM Write Cycle is completed.
> +	 * Only serial commands to the volatile memory are allowed.
> +	 */
> +	guard(mutex)(&data->lock);
> +
> +	/*
> +	 * Verify DAC Wiper and DAC Configuration are unlocked. If both are disabled,
> +	 * writing to EEPROM is available.
> +	 */
> +	ret = regmap_read(data->regmap, MCP48FEB02_WIPERLOCK_STATUS_REG_ADDR, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val) {
> +		dev_err(dev, "DAC Wiper and DAC Configuration not are unlocked.\n");

"are not"

> +		return -EINVAL;
> +	}
> +
> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		ret = mcp48feb02_write_to_eeprom(data, NV_REG_ADDR(i),
> +						 data->chdata[i].dac_data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_VREF_REG_ADDR, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp48feb02_write_to_eeprom(data, MCP48FEB02_NV_VREF_REG_ADDR, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_POWER_DOWN_REG_ADDR, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp48feb02_write_to_eeprom(data, MCP48FEB02_NV_POWER_DOWN_REG_ADDR, val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read_poll_timeout(data->regmap, MCP48FEB02_GAIN_CTRL_STATUS_REG_ADDR, eewa_val,
> +				       !(eewa_val & MCP48FEB02_GAIN_BIT_STATUS_EEWA_MASK),
> +				       USEC_PER_MSEC, USEC_PER_MSEC * 5);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_NV_GAIN_CTRL_I2C_SLAVE_REG_ADDR, &val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_GAIN_CTRL_STATUS_REG_ADDR, &val1);
> +	if (ret)
> +		return ret;
> +
> +	ret = mcp48feb02_write_to_eeprom(data, MCP48FEB02_NV_GAIN_CTRL_I2C_SLAVE_REG_ADDR,
> +					 (val1 & MCP48FEB02_GAIN_BITS_MASK) |
> +					 (val & MCP48FEB02_NV_I2C_SLAVE_ADDR_MASK));
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}

> +

Unneeded blank line.

> +static IIO_DEVICE_ATTR_WO(store_eeprom, 0);

...

> +static void mcp48feb02_init_scale(struct mcp48feb02_data *data, enum mcp48feb02_scale scale,
> +				  int vref_uV, int scale_avail[])
> +{
> +	u32 value_micro, value_int;
> +	u64 tmp;
> +
> +	/* vref_uV should not be negative */
> +	tmp = (u64)vref_uV * MILLI >> data->chip_features->resolution;

If vref_uV is guaranteed to be less than ~33V, this code can be transformed to
avoid 64-bit division. Hints: resolution is always great than 3; MILLI equals
to 2³*5³.

> +	value_int = div_u64_rem(tmp, MICRO, &value_micro);
> +	scale_avail[scale * 2] = value_int;
> +	scale_avail[scale * 2 + 1] = value_micro;
> +}

Since it's kinda a common stuff, perhaps one wants to add a helper
to include/linux/math.h.

...

> +static int mcp48feb02_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *ch,
> +				 const int **vals, int *type, int *length, long info)
> +{
> +	struct mcp48feb02_data *data = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (ch->type) {
> +		case IIO_VOLTAGE:
> +			if (data->phys_channels >= 4 && (ch->address % 2))
> +				*vals = data->scale_1;
> +			else
> +				*vals = data->scale;

Actually, if you put the scales as

	int scales[2][2 * MCP48FEB02_MAX_SCALES_CH];

this will become as simple as

			if (data->phys_channels >= 4)
				*vals = data->scales[ch->address];
			else
				*vals = data->scales[0];

OTOH, I am not sure if it can be always as

			*vals = data->scales[ch->address];

which would be the best approach.

> +			*length = 2 * MCP48FEB02_MAX_SCALES_CH;
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static void mcp48feb02_get_scale(int ch, struct mcp48feb02_data *data, int *val, int *val2)
> +{
> +	enum mcp48feb02_scale current_scale;
> +
> +	if (data->chdata[ch].ref_mode == MCP48FEB02_VREF_VDD)
> +		current_scale = MCP48FEB02_SCALE_VDD;
> +	else if (data->chdata[ch].use_2x_gain)
> +		current_scale = MCP48FEB02_SCALE_GAIN_X2;
> +	else
> +		current_scale = MCP48FEB02_SCALE_GAIN_X1;
> +
> +	if (data->phys_channels >= 4 && (ch % 2)) {
> +		*val = data->scale_1[current_scale * 2];
> +		*val2 = data->scale_1[current_scale * 2 + 1];
> +	} else {
> +		*val = data->scale[current_scale * 2];
> +		*val2 = data->scale[current_scale * 2 + 1];
> +	}

Ditto. I.o.w. you can avoid (ch % 2) for good.

> +}

...

> +static int mcp48feb02_set_scale(struct mcp48feb02_data *data, int ch, int scale)
> +{
> +	int tmp_val, ret;

Why is 'tmp_val' signed?

> +	ret = mcp48feb02_ch_scale(data, ch, scale);
> +	if (ret)
> +		return ret;
> +
> +	if (scale == MCP48FEB02_SCALE_GAIN_X2)
> +		tmp_val = MCP48FEB02_GAIN_BIT_X2;
> +	else
> +		tmp_val = MCP48FEB02_GAIN_BIT_X1;
> +
> +	ret = regmap_update_bits(data->regmap, MCP48FEB02_GAIN_CTRL_STATUS_REG_ADDR,
> +				 DAC_GAIN_MASK(ch), DAC_GAIN_VAL(ch, tmp_val));
> +	if (ret)
> +		return ret;
> +
> +	data->chdata[ch].use_2x_gain = tmp_val;
> +
> +	return 0;
> +}

...

> +static int mcp48feb02_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *ch,
> +				int val, int val2, long mask)
> +{
> +	struct mcp48feb02_data *data = iio_priv(indio_dev);
> +	int *tmp_scale, ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_write(data->regmap, REG_ADDR(ch->address), val);
> +		if (ret)
> +			return ret;
> +
> +		data->chdata[ch->address].dac_data = val;
> +		return 0;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (data->phys_channels >= 4 && (ch->address % 2))
> +			tmp_scale = data->scale_1;
> +		else
> +			tmp_scale = data->scale;

Same, (ch->address % 2) can be avoided.

> +		ret = mcp48feb02_check_scale(data, val, val2, tmp_scale);
> +		if (ret < 0)
> +			return ret;
> +
> +		return mcp48feb02_set_scale(data, ch->address, ret);
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int mcp48feb02_parse_fw(struct iio_dev *indio_dev,
> +			       const struct mcp48feb02_features *chip_features)
> +{
> +	struct iio_chan_spec chanspec = mcp48febxx_ch_template;
> +	struct mcp48feb02_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	struct iio_chan_spec *channels;
> +	u32 num_channels;

> +	u8 chan_idx = 0;

Assignments like this are harder to maintain and prone to subtle mistakes in
case the variable gets reused. Please, split it...

> +	guard(mutex)(&data->lock);
> +
> +	num_channels = device_get_child_node_count(dev);
> +	if (num_channels > chip_features->phys_channels)
> +		return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
> +
> +	if (!num_channels)

While this is standard pattern, I find == 0 is more explicit when we compare
counters, but it's up to you and maintainers.

> +		return dev_err_probe(dev, -EINVAL, "No channel specified in the devicetree.\n");
> +
> +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;

...to be here as

	chan_idx = 0;

> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 reg = 0;

Redundant assignment. "reg" is a mandatory property AFAICS from the below code.

> +		int ret;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Invalid channel number\n");
> +
> +		if (reg >= chip_features->phys_channels)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "The index of the channels does not match the chip\n");

> +		set_bit(reg, &data->active_channels_mask);

Is atomic bit operation required here?

> +		ret = fwnode_property_read_string(child, "label", &data->labels[reg]);
> +		if (ret)

> +			return dev_err_probe(dev, ret, "%pfw: invalid label\n",
> +					     fwnode_get_name(child));

Something is really wrong here. Please, fix accordingly.

> +		chanspec.address = reg;
> +		chanspec.channel = reg;
> +		channels[chan_idx] = chanspec;
> +		chan_idx++;
> +	}
> +
> +	indio_dev->num_channels = num_channels;
> +	indio_dev->channels = channels;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	data->phys_channels = chip_features->phys_channels;
> +
> +	data->vref_buffered = device_property_read_bool(dev, "microchip,vref-buffered");

> +	if (chip_features->have_ext_vref1)
> +		data->vref1_buffered = device_property_read_bool(dev, "microchip,vref1-buffered");

Alternatively can be

	if (device_property_read_bool(dev, "microchip,vref1-buffered"))
		data->vref1_buffered = chip_features->have_ext_vref1;

the difference is that vref1_buffered can be filled with "false", but I don't see
if it can be true before that. You may stick with your variant to avoid this side
effect.

> +	return 0;
> +}

...

> +static int mcp48feb02_init_ctrl_regs(struct mcp48feb02_data *data)
> +{
> +	unsigned int i, vref_ch, gain_ch, pd_ch;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_VREF_REG_ADDR, &vref_ch);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_GAIN_CTRL_STATUS_REG_ADDR, &gain_ch);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, MCP48FEB02_POWER_DOWN_REG_ADDR, &pd_ch);
> +	if (ret)
> +		return ret;
> +
> +	gain_ch = gain_ch & MCP48FEB02_GAIN_BITS_MASK;
> +	for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> +		struct device *dev = regmap_get_device(data->regmap);
> +		unsigned int pd_tmp;
> +
> +		data->chdata[i].ref_mode = (vref_ch >> (2 * i)) & MCP48FEB02_DAC_CTRL_MASK;
> +		data->chdata[i].use_2x_gain = (gain_ch >> i)  & MCP48FEB02_GAIN_BIT_MASK;
> +
> +		/*
> +		 * Inform the user that the current voltage reference read from the volatile
> +		 * register of the chip is different from the one specified in the device tree.
> +		 * Considering that the user cannot have an external voltage reference connected
> +		 * to the pin and select the internal Band Gap at the same time, in order to avoid
> +		 * miscofiguring the reference voltage, the volatile register will not be written.
> +		 * In order to overwrite the setting from volatile register with the one from the
> +		 * device tree, the user needs to write the chosen scale.
> +		 */
> +		switch (data->chdata[i].ref_mode) {
> +		case MCP48FEB02_INTERNAL_BAND_GAP:
> +			if (data->phys_channels >= 4 && (i % 2) && data->use_vref1) {
> +				dev_dbg(dev, "ch[%u]: was configured to use internal band gap", i);
> +				dev_dbg(dev, "ch[%u]: reference voltage set to VREF1", i);
> +				break;

> +			}
> +			if ((data->phys_channels < 4 || (data->phys_channels >= 4 && !(i % 2))) &&
> +			    data->use_vref) {

I don't see how these two conditionals can be run both.

> +				dev_dbg(dev, "ch[%u]: was configured to use internal band gap", i);
> +				dev_dbg(dev, "ch[%u]: reference voltage set to VREF", i);
> +				break;
> +			}

With that in mind, the above can be simplified a bit.

			if (data->use_vref && ((data->phys_channels >= 4 && !(i % 2)) ||
					       (data->phys_channels < 4))) {
				dev_dbg(dev, "ch[%u]: was configured to use internal band gap\n", i);
				dev_dbg(dev, "ch[%u]: reference voltage set to Vref\n", i);
			} else if (data->use_vref1 && data->phys_channels >= 4 && (i % 2)) {
				dev_dbg(dev, "ch[%u]: was configured to use internal band gap\n", i);
				dev_dbg(dev, "ch[%u]: reference voltage set to Vref1\n", i);
			}

The conditionals were reshuffled to make it shorter and easier to compare
(yes, there is a pair of unneeded parentheses for the sake of good looking
 code, a.k.a. readability).

Also note, the messages were missing trailing '\n'; I lowered REF --> ref
in them.

> +			break;
> +		case MCP48FEB02_EXTERNAL_VREF_UNBUFFERED:
> +		case MCP48FEB02_EXTERNAL_VREF_BUFFERED:
> +			if (data->phys_channels >= 4 && (i % 2) && !data->use_vref1) {
> +				dev_dbg(dev, "ch[%u]: was configured to use VREF1", i);
> +				dev_dbg(dev,
> +					"ch[%u]: reference voltage set to internal band gap", i);
> +				break;
> +			}
> +			if ((data->phys_channels < 4 || (data->phys_channels >= 4 && !(i % 2))) &&
> +			    !data->use_vref) {
> +				dev_dbg(dev, "ch[%u]: was configured to use VREF", i);
> +				dev_dbg(dev,
> +					"ch[%u]: reference voltage set to internal band gap", i);
> +				break;
> +			}
> +			break;

Ditto.

> +		}
> +
> +		pd_tmp = (pd_ch >> (2 * i)) & MCP48FEB02_DAC_CTRL_MASK;
> +		data->chdata[i].powerdown_mode = pd_tmp ? (pd_tmp - 1) : pd_tmp;
> +		data->chdata[i].powerdown = !!(data->chdata[i].powerdown_mode);
> +	}
> +
> +	return 0;
> +}

...

> +static int mcp48feb02_probe(struct spi_device *spi)
> +{
> +	const struct mcp48feb02_features *chip_features;
> +	struct device *dev = &spi->dev;
> +	struct mcp48feb02_data *data;
> +	struct iio_dev *indio_dev;

> +	int vref1_uV = 0;
> +	int vref_uV = 0;

Please, split the assignments (the rationale was given somewhere above).

> +	int vdd_uV;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +
> +	chip_features = spi_get_device_match_data(spi);
> +	if (!chip_features)
> +		return -EINVAL;
> +
> +	data->chip_features = chip_features;
> +
> +	if (chip_features->have_eeprom) {
> +		data->regmap = devm_regmap_init_spi(spi, &mcp48feb02_regmap_config);
> +		indio_dev->info = &mcp48feb02_info;
> +	} else {
> +		data->regmap = devm_regmap_init_spi(spi, &mcp48fvb02_regmap_config);
> +		indio_dev->info = &mcp48fvb02_info;
> +	}
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(dev, PTR_ERR(data->regmap), "Error initializing spi regmap\n");

SPI

> +	indio_dev->name = chip_features->name;
> +
> +	ret = mcp48feb02_parse_fw(indio_dev, chip_features);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error parsing firmware data\n");
> +
> +	ret = devm_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (ret < 0)
> +		return ret;
> +
> +	vdd_uV = ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vref");
> +	if (ret > 0) {
> +		vref_uV = ret;
> +		data->use_vref = true;
> +	} else {
> +		dev_dbg(dev, "using internal band gap as voltage reference.\n");
> +		dev_dbg(dev, "External Vref is unavailable.\n");
> +	}
> +
> +	if (chip_features->have_ext_vref1) {
> +		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> +		if (ret > 0) {
> +			vref1_uV = ret;
> +			data->use_vref1 = true;
> +		} else {
> +			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
> +			dev_dbg(dev, "External Vref1 is unavailable.\n");
> +		}
> +	}
> +
> +	ret = mcp48feb02_init_ctrl_regs(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error initialising vref register\n");
> +
> +	ret = mcp48feb02_init_ch_scales(data, vdd_uV, vref_uV, vref1_uV);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}


-- 
With Best Regards,
Andy Shevchenko