Re: [PATCH v2 2/2] iio: dac: adding support for Microchip MCP47FEB02

Andy Shevchenko
2025-11-04T10:25:28Z

On Mon, Nov 03, 2025 at 05:50:30PM +0200, Ariana Lazar wrote:
> This is the iio driver for Microchip MCP47F(E/V)B(0/1/2)1,
> MCP47F(E/V)B(0/1/2)2, MCP47F(E/V)B(0/1/2)4 and MCP47F(E/V)B(0/1/2)8 series
> of buffered voltage output Digital-to-Analog Converters with nonvolatile or
> volatile memory and an I2C Interface.
>
> The families support up to 8 output channels.
>
> The devices can be 8-bit, 10-bit and 12-bit.

...

> +config MCP47FEB02
> + tristate "MCP47F(E/V)B|(0/1/2)(1/2/4/8)DAC driver"

This is unreadable cryptic title. Make it more human-readable, like:

"Microchip MCP47F family of DAC driver"

> + depends on I2C
> + help
> + Say yes here if you want to build a driver for the Microchip

Too many spaces, see how it's done elsewhere.

> + MCP47FEB01, MCP47FEB11, MCP47FEB21, MCP47FEB02, MCP47FEB12,
> + MCP47FEB22, MCP47FVB01, MCP47FVB11, MCP47FVB21, MCP47FVB02,
> + MCP47FVB12, MCP47FVB02, MCP47FVB12, MCP47FVB22, MCP47FVB04,
> + MCP47FVB14, MCP47FVB24, MCP47FVB04, MCP47FVB08, MCP47FVB18,
> + MCP47FVB28, MCP47FEB04, MCP47FEB14 and MCP47FEB24 having up to 8

This is also unreadable, please split to groups (by family species and/or bits)
and sort each group accordingly, like

- E-group (8-bit): MCP47FEB01, MCP47FEB11, MCP47FEB21
- E-group (10-bit): MCP47FEB02, MCP47FEB12, MCP47FEB22
...

Note, I put a hypothetical text there, I haven't check this for the correctness!

> + channels, 8-bit, 10-bit or 12-bit digital-to-analog converter (DAC)
> + with I2C interface.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called mcp47feb02.

...

> + * Datasheet for MCP47FEBXX can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005375A.pdf
> + *
> + * Datasheet for MCP47FVBXX can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005405A.pdf
> + *
> + * Datasheet for MCP47FXBX4/8 can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP47FXBX48-Data-Sheet-DS200006368A.pdf

Avoid duplicating information, so far it can be just listed as

* Datasheet links:
*
* [MCP47FEBxx] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005375A.pdf
* [MCP47FVBxx] https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/20005405A.pdf
* [MCP47FxBx4/8] https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP47FXBX48-Data-Sheet-DS200006368A.pdf

(also note xx instead of XX).

Or propose a better style.

...

> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

Missing includes. E.g., for 'bool', for 'ARRAY_SIZE()', for 'guard()()'.
Follow IWYU (Include What You Use) principle.

...

> +#define MCP47FEB02_DAC0_REG_ADDR (0x00 << 3)

I assume it's the similar case as for below 0x10-0x17 range. Perhaps do both as
a macro with a parameter?

> +#define MCP47FEB02_VREF_REG_ADDR (0x08 << 3)
> +#define MCP47FEB02_POWER_DOWN_REG_ADDR (0x09 << 3)
> +#define MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR (0x0A << 3)
> +#define MCP47FEB02_WIPERLOCK_STATUS_REG_ADDR (0x0B << 3)
> +
> +#define MCP47FEB02_NV_DAC0_REG_ADDR (0x10 << 3)
> +#define MCP47FEB02_NV_DAC1_REG_ADDR (0x11 << 3)
> +#define MCP47FEB02_NV_DAC2_REG_ADDR (0x12 << 3)
> +#define MCP47FEB02_NV_DAC3_REG_ADDR (0x13 << 3)
> +#define MCP47FEB02_NV_DAC4_REG_ADDR (0x14 << 3)
> +#define MCP47FEB02_NV_DAC5_REG_ADDR (0x15 << 3)
> +#define MCP47FEB02_NV_DAC6_REG_ADDR (0x16 << 3)
> +#define MCP47FEB02_NV_DAC7_REG_ADDR (0x17 << 3)
> +#define MCP47FEB02_NV_VREF_REG_ADDR (0x18 << 3)
> +#define MCP47FEB02_NV_POWER_DOWN_REG_ADDR (0x19 << 3)
> +#define MCP47FEB02_NV_GAIN_BIT_I2C_SLAVE_REG_ADDR (0x1A << 3)

Drop this << 3 part, just do it at run-time. Or embed them, by providing
shifted values. Also, there is special formats for regmap, perhaps that's what
you wanted to begin with? *Yes, it might need some code to be added into
drivers/base/regmap.c.

...

> +#define MCP47FEB02_INTERNAL_BAND_GAP_MV 2440

_MV --> _mV

...

> +#define MCP47FEB02_DELAY_1_MS 1000

Drop '_1' and use (1 * USEC_PER_MSEC) as value.

But looking at the code this makes the definition useless, just use values
directly there.

...

> +struct mcp47feb02_features {
> + const char *name;
> + unsigned int phys_channels;
> + unsigned int resolution;
> + bool have_ext_vref1;
> + bool have_eeprom;

Inconsistent style. Be consistent.

> +};

...

> +struct mcp47feb02_channel_data {
> + enum vref_mode ref_mode;
> + u8 powerdown_mode;
> + bool use_2x_gain;
> + bool powerdown;
> + u16 dac_data;

Have you ran `pahole`? Please do, and amend the data types accordingly.

> +};

...

> +struct mcp47feb02_data {
> + struct mcp47feb02_channel_data chdata[MCP47FEBXX_MAX_CH];
> + int scale_1[MCP47FEB02_MAX_VALS_SCALES_CH];
> + int scale[MCP47FEB02_MAX_VALS_SCALES_CH];
> + const struct mcp47feb02_features *info;
> + const char *labels[MCP47FEBXX_MAX_CH];
> + unsigned long active_channels_mask;

> + struct i2c_client *client;
> + struct regmap *regmap;

Why both are needed?

> + bool vref1_buffered;
> + bool vref_buffered;
> + u16 phys_channels;
> + struct mutex lock; /* synchronize access to driver's state members */
> + bool use_vref1;
> + bool use_vref;
> +};

...

> +static ssize_t mcp47feb02_store_eeprom(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct mcp47feb02_data *data = iio_priv(dev_to_iio_dev(dev));
> + int ret, i, val, val1, eewa_val;

Do you expect i to hold the signed value?

> + bool state;
> +
> + ret = kstrtobool(buf, &state);

> + if (ret < 0)

Why ' < 0'?

> + return ret;
> +
> + if (!state)
> + return 0;
> +
> + /*
> + * Verify DAC Wiper and DAC Configuratioin are unlocked. If both are disabled,
> + * writing to EEPROM is available.
> + */
> + ret = regmap_read(data->regmap, MCP47FEB02_WIPERLOCK_STATUS_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + if (val) {
> + dev_err(dev, "DAC Wiper and DAC Configuration not are unlocked.\n");
> + return -EINVAL;
> + }
> +
> + for_each_set_bit(i, &data->active_channels_mask, data->phys_channels) {
> + ret = mcp47feb02_write_to_eeprom(data, i << 3, data->chdata[i].dac_data);
> + if (ret)
> + return ret;
> + }
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_VREF_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + ret = mcp47feb02_write_to_eeprom(data, MCP47FEB02_NV_VREF_REG_ADDR, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + ret = mcp47feb02_write_to_eeprom(data, MCP47FEB02_NV_POWER_DOWN_REG_ADDR, val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read_poll_timeout(data->regmap, MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR, eewa_val,
> + !(eewa_val & MCP47FEB02_GAIN_BIT_STATUS_EEWA_MASK),
> + MCP47FEB02_DELAY_1_MS, MCP47FEB02_DELAY_1_MS * 5);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_NV_GAIN_BIT_I2C_SLAVE_REG_ADDR, &val);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, MCP47FEB02_GAIN_BIT_STATUS_REG_ADDR, &val1);
> + if (ret)
> + return ret;
> +
> + ret = mcp47feb02_write_to_eeprom(data, MCP47FEB02_NV_GAIN_BIT_I2C_SLAVE_REG_ADDR,
> + (val1 & MCP47FEB02_VOLATILE_GAIN_BIT_MASK) |
> + (val & MCP47FEB02_NV_I2C_SLAVE_ADDR_MASK));
> + if (ret)
> + return ret;
> +
> + return len;
> +}
> +

Blank line should go after IIO_DEVICE_ATTR()...

> +static IIO_DEVICE_ATTR(store_eeprom, 0200, NULL, mcp47feb02_store_eeprom, 0);

...here, also Why not IIO_DEVICE_ATTR_WO()?

> +static struct attribute *mcp47feb02_attributes[] = {
> + &iio_dev_attr_store_eeprom.dev_attr.attr,
> + NULL
> +};

...

> +static int mcp47feb02_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct mcp47feb02_data *data = iio_priv(indio_dev);
> + int ret, ch;

Why ch is signed?

> + u8 pd_mode;
> +
> + guard(mutex)(&data->lock);
> +
> + for_each_set_bit(ch, &data->active_channels_mask, data->phys_channels) {
> + data->chdata[ch].powerdown = true;
> + pd_mode = data->chdata[ch].powerdown_mode + 1;

> + regmap_update_bits(data->regmap, MCP47FEB02_POWER_DOWN_REG_ADDR,
> + DAC_CTRL_MASK(ch), DAC_CTRL_VAL(ch, pd_mode));

Missed error check.

> + if (ret)
> + return ret;
> +
> + ret = regmap_write(data->regmap, ch << 3, data->chdata[ch].dac_data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}

I stopped here, I think it's already warrants a new version.

...

> + tmp = (s64)vref_mv * 1000000LL >> data->info->resolution;

vref_mV
MICRO, MEGA, ... from units.h

> + value_int = div_s64_rem(tmp, 1000000LL, &value_micro);

...

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

Is the atomic op required here?

...

> + if (fwnode_property_present(child, "label"))

Useless check as you don't handle an error code from below anyway (means
optional property).

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

...

May you split this to add the main functionality with the subset of the
supported chips and add, for example, FxBx later in a separate patch?
This will help a lot with reviewing and pushing your patches forward.
Note, considering my comments above I don't think this will make v6.19-rc1,
so you have plenty of time to polish this and maybe even split more.

--
With Best Regards,
Andy Shevchenko