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

Andy Shevchenko
2026-02-12T14:42:50Z

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