Re: [PATCH 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules

[Joshua Crofts <joshua.crofts1@gmail.com>]

On Thu, 23 Jul 2026 16:43:10 +0300
Ariana Lazar <ariana.lazar@microchip.com> wrote:

> diff --git a/drivers/iio/dac/mcp47feb02-i2c.c b/drivers/iio/dac/mcp47feb02-i2c.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..808c51d0afdf564321abcd46a5a7d9595c5472da
> --- /dev/null
> +++ b/drivers/iio/dac/mcp47feb02-i2c.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for MCP47FEB02 Multi-Channel DAC with I2C interface
> + *
> + * Copyright (C) 2026 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Ariana Lazar <ariana.lazar@microchip.com>
> + *
> + * Datasheet links for devices with I2C interface:
> + * [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
> + */
> +#include <linux/device.h>

struct device *dev is an opaque pointer, no need to include device.h

On the other hand, please include dev_printk.h for dev_err_probe().
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>

Remove mod_devicetable.h, no need to include it as it's in spi.h
> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +
> +#include "mcp47feb02.h"
> +
> +/* Parts with EEPROM memory */
> +MCP47FEB02_CHIP_INFO(mcp47feb01, 1, 8,  false, true);
> +MCP47FEB02_CHIP_INFO(mcp47feb02, 2, 8,  false, true);
> +MCP47FEB02_CHIP_INFO(mcp47feb04, 4, 8,  true,  true);
> +MCP47FEB02_CHIP_INFO(mcp47feb08, 8, 8,  true,  true);
> +MCP47FEB02_CHIP_INFO(mcp47feb11, 1, 10, false, true);
> +MCP47FEB02_CHIP_INFO(mcp47feb12, 2, 10, false, true);
> +MCP47FEB02_CHIP_INFO(mcp47feb14, 4, 10, true,  true);
> +MCP47FEB02_CHIP_INFO(mcp47feb18, 8, 10, true,  true);
> +MCP47FEB02_CHIP_INFO(mcp47feb21, 1, 12, false, true);
> +MCP47FEB02_CHIP_INFO(mcp47feb22, 2, 12, false, true);
> +MCP47FEB02_CHIP_INFO(mcp47feb24, 4, 12, true,  true);
> +MCP47FEB02_CHIP_INFO(mcp47feb28, 8, 12, true,  true);
> +
> +/* Parts without EEPROM memory */
> +MCP47FEB02_CHIP_INFO(mcp47fvb01, 1, 8,  false, false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb02, 2, 8,  false, false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb04, 4, 8,  true,  false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb08, 8, 8,  true,  false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb11, 1, 10, false, false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb12, 2, 10, false, false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb14, 4, 10, true,  false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb18, 8, 10, true,  false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb21, 1, 12, false, false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb22, 2, 12, false, false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb24, 4, 12, true,  false);
> +MCP47FEB02_CHIP_INFO(mcp47fvb28, 8, 12, true,  false);
> +
> +static int mcp47feb02_i2c_probe(struct i2c_client *client)
> +{
> +	const struct mcp47feb02_features *chip_features;
> +	struct device *dev = &client->dev;
> +	struct regmap *regmap;
> +
> +	chip_features = i2c_get_match_data(client);
> +	if (!chip_features)
> +		return -EINVAL;

return dev_err_probe + -ENODEV.
> +
> +	if (chip_features->have_eeprom)
> +		regmap = devm_regmap_init_i2c(client, &mcp47feb02_regmap_config);
> +	else
> +		regmap = devm_regmap_init_i2c(client, &mcp47fvb02_regmap_config);
> +
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "Error initializing I2C regmap\n");
> +
> +	return mcp47feb02_common_probe(chip_features, regmap);
> +}
> +
> +static const struct i2c_device_id mcp47feb02_i2c_id[] = {
> +	{ "mcp47feb01", (kernel_ulong_t)&mcp47feb01_chip_features },

Forgot to mention this in patch 3, but please use named initializers.

> +	{ "mcp47feb02", (kernel_ulong_t)&mcp47feb02_chip_features },
> +	{ "mcp47feb04", (kernel_ulong_t)&mcp47feb04_chip_features },
> +	{ "mcp47feb08", (kernel_ulong_t)&mcp47feb08_chip_features },
> +	{ "mcp47feb11", (kernel_ulong_t)&mcp47feb11_chip_features },
> +	{ "mcp47feb12", (kernel_ulong_t)&mcp47feb12_chip_features },
> +	{ "mcp47feb14", (kernel_ulong_t)&mcp47feb14_chip_features },

...

> diff --git a/drivers/iio/dac/mcp47feb02.h b/drivers/iio/dac/mcp47feb02.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..7dbf157d7d6dcfeda7e47141ad447dfa0a79fd51
> --- /dev/null
> +++ b/drivers/iio/dac/mcp47feb02.h
> @@ -0,0 +1,153 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef __DRIVERS_IIO_DAC_MCP47FEB02_H__
> +#define __DRIVERS_IIO_DAC_MCP47FEB02_H__
> +
> +#include <linux/bitops.h>

bits.h should suffice.

> +#include <linux/device.h>

No need for device.h
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

You're missing mutex.h, types.h.

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

If we're going by IWYU, you also don't need this header.
> +
> +/* Register addresses must be left shifted with 3 positions in order to append command mask */
> +#define MCP47FEB02_DAC0_REG_ADDR			0x00
> +#define MCP47FEB02_VREF_REG_ADDR			0x40
> +#define MCP47FEB02_POWER_DOWN_REG_ADDR			0x48
> +#define MCP47FEB02_DAC_CTRL_MASK			GENMASK(1, 0)
> +
> +#define MCP47FEB02_GAIN_CTRL_STATUS_REG_ADDR		0x50
> +#define MCP47FEB02_GAIN_BIT_MASK			BIT(0)
> +#define MCP47FEB02_GAIN_BIT_STATUS_EEWA_MASK		BIT(6)
> +#define MCP47FEB02_GAIN_BITS_MASK			GENMASK(15, 8)
> +
> +#define MCP47FEB02_WIPERLOCK_STATUS_REG_ADDR		0x58
> +
> +#define MCP47FEB02_NV_DAC0_REG_ADDR			0x80
> +#define MCP47FEB02_NV_VREF_REG_ADDR			0xC0
> +#define MCP47FEB02_NV_POWER_DOWN_REG_ADDR		0xC8
> +#define MCP47FEB02_NV_GAIN_CTRL_I2C_SLAVE_REG_ADDR	0xD0
> +#define MCP47FEB02_NV_I2C_SLAVE_ADDR_MASK		GENMASK(7, 0)
> +
> +/* Voltage reference, Power-Down control register and DAC Wiperlock status register fields */
> +#define DAC_CTRL_MASK(ch)				(GENMASK(1, 0) << (2 * (ch)))
> +#define DAC_CTRL_VAL(ch, val)				((val) << (2 * (ch)))
> +
> +/* Gain Control and I2C Slave Address Reguster fields */

You probably meant Register?

> +#define DAC_GAIN_MASK(ch)				(BIT(0) << (8 + (ch)))
> +#define DAC_GAIN_VAL(ch, val)				((val) << (8 + (ch)))
> +
> +#define REG_ADDR(reg)					((reg) << 3)
> +#define NV_REG_ADDR(reg)				((NV_DAC_ADDR_OFFSET + (reg)) << 3)
> +#define READFLAG_MASK					GENMASK(2, 1)
> +
> +#define MCP47FEB02_MAX_CH				8
> +#define MCP47FEB02_MAX_SCALES_CH			3
> +#define MCP47FEB02_DAC_WIPER_UNLOCKED			0
> +#define MCP47FEB02_NORMAL_OPERATION			0
> +#define MCP47FEB02_INTERNAL_BAND_GAP_uV			2440000
> +#define NV_DAC_ADDR_OFFSET				0x10
> +


-- 
Kind regards,
Joshua Crofts