Re: [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI

Re: [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]

On Thu, Jul 23, 2026 at 04:43:11PM +0300, Ariana Lazar wrote:
> Add the SPI MCP48FxBy1/2/4/8 part numbers, the spi-max-frequency property
> and a devicetree example for SPI usage to the existing binding.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>

This is actually a v2, right? This was submitted before using a bit of a
different approach IIRC?
>  allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            pattern: "^microchip,mcp48f[ev]b[0-2][1248]$"
> +    then:
> +      $ref: /schemas/spi/spi-peripheral-props.yaml#
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - microchip,mcp47feb01
> +              - microchip,mcp47feb02
> +              - microchip,mcp47feb04
> +              - microchip,mcp47feb08
> +              - microchip,mcp47feb11
> +              - microchip,mcp47feb12
> +              - microchip,mcp47feb14
> +              - microchip,mcp47feb18
> +              - microchip,mcp47feb21
> +              - microchip,mcp47feb22
> +              - microchip,mcp47feb24
> +              - microchip,mcp47feb28
> +              - microchip,mcp47fvb01
> +              - microchip,mcp47fvb02
> +              - microchip,mcp47fvb04
> +              - microchip,mcp47fvb08
> +              - microchip,mcp47fvb11
> +              - microchip,mcp47fvb12
> +              - microchip,mcp47fvb14
> +              - microchip,mcp47fvb18
> +              - microchip,mcp47fvb21
> +              - microchip,mcp47fvb22
> +              - microchip,mcp47fvb24
> +              - microchip,mcp47fvb28
> +    then:
> +      properties:
> +        spi-max-frequency: false

I think these should be squashed into one conditional, since you can
just do "else: spi-max-frequency: false".

pw-bot: changes-requested

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Joshua Crofts]

On Thu, 23 Jul 2026 16:43:12 +0300
Ariana Lazar <ariana.lazar@microchip.com> 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.
> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---

...

> +#include <linux/device.h>

No need to include device.h, as struct device * is an opaque
pointer.

Instead, add dev_printk.h as you're using dev_err_probe().

> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>

Don't include mod_devicetable.h, this header has recently been
added to spi.h, thanks to the effort of Uwe Kleine-Konig.

> +#include <linux/pm.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "mcp47feb02.h"
> +

...

> +
> +static int mcp47feb02_spi_probe(struct spi_device *spi)
> +{
> +	const struct mcp47feb02_features *chip_features;
> +	struct device *dev = &spi->dev;
> +	struct regmap *regmap;
> +
> +	chip_features = spi_get_device_match_data(spi);
> +	if (!chip_features)
> +		return -EINVAL;

What about returning dev_err_probe() and -ENODEV instead of -EINVAL?
It's about 50-50 in the kernel but -ENODEV seems more suitable as an
error code if spi_get_device_match_data() fails.

-- 
Kind regards,
Joshua Crofts

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

[Joshua Crofts]

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