* Re: [PATCH 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules
[not found] ` <20260723-mcp47feb02_refactor-v1-1-ee59e63672bc@microchip.com>
@ 2026-07-23 21:18 ` Joshua Crofts
0 siblings, 0 replies; 3+ messages in thread
From: Joshua Crofts @ 2026-07-23 21:18 UTC (permalink / raw)
To: Ariana Lazar
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-kernel,
linux-iio, devicetree
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
^ permalink raw reply [flat|nested] 3+ messages in thread