Linux IIO development
 help / color / mirror / Atom feed
* Re: [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI
       [not found] ` <20260723-mcp47feb02_refactor-v1-2-ee59e63672bc@microchip.com>
@ 2026-07-23 16:38   ` Conor Dooley
  2026-07-24 12:30     ` Ariana.Lazar
  0 siblings, 1 reply; 6+ messages in thread
From: Conor Dooley @ 2026-07-23 16:38 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

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] iio: dac: add support for Microchip MCP48FEB02
       [not found] ` <20260723-mcp47feb02_refactor-v1-3-ee59e63672bc@microchip.com>
@ 2026-07-23 21:02   ` Joshua Crofts
  0 siblings, 0 replies; 6+ messages in thread
From: Joshua Crofts @ 2026-07-23 21:02 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: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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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
  2026-07-26  0:14   ` Jonathan Cameron
  1 sibling, 0 replies; 6+ 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] 6+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI
  2026-07-23 16:38   ` [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI Conor Dooley
@ 2026-07-24 12:30     ` Ariana.Lazar
  2026-07-24 12:44       ` Conor Dooley
  0 siblings, 1 reply; 6+ messages in thread
From: Ariana.Lazar @ 2026-07-24 12:30 UTC (permalink / raw)
  To: conor
  Cc: dlechner, nuno.sa, linux-iio, devicetree, robh, jic23, andy,
	krzk+dt, linux-kernel, conor+dt

Hi Conor,

> This is actually a v2, right? This was submitted before using a bit
> of a
> different approach IIRC?

This is the first version combining both the refactoring and the SPI
support into a single series. Originally I have sent the refactoring
patches separately and in order to avoid confusion (cannot do a diff
with old patches) and to be tracked easily I have decided to make this
series as version 1.

If you think it is a good idea to mark it as version 2, I can resend
the series as version 2 and point to the old patches as version 1.

Best regards,
Ariana




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI
  2026-07-24 12:30     ` Ariana.Lazar
@ 2026-07-24 12:44       ` Conor Dooley
  0 siblings, 0 replies; 6+ messages in thread
From: Conor Dooley @ 2026-07-24 12:44 UTC (permalink / raw)
  To: Ariana.Lazar
  Cc: conor, dlechner, nuno.sa, linux-iio, devicetree, robh, jic23,
	andy, krzk+dt, linux-kernel, conor+dt

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

On Fri, Jul 24, 2026 at 12:30:31PM +0000, Ariana.Lazar@microchip.com wrote:
> Hi Conor,
> 
> > This is actually a v2, right? This was submitted before using a bit
> > of a
> > different approach IIRC?
> 
> This is the first version combining both the refactoring and the SPI
> support into a single series. Originally I have sent the refactoring
> patches separately and in order to avoid confusion (cannot do a diff
> with old patches) and to be tracked easily I have decided to make this
> series as version 1.
> 
> If you think it is a good idea to mark it as version 2, I can resend
> the series as version 2 and point to the old patches as version 1.

Yeah, if you submit something and then do a big rework or consolidation
of series you should treat it as being a new version of whichever you
consider to be a new version rather than a new series.

Cheers,
Conor.

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* 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   ` [PATCH 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules Joshua Crofts
@ 2026-07-26  0:14   ` Jonathan Cameron
  1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2026-07-26  0:14 UTC (permalink / raw)
  To: Ariana Lazar
  Cc: 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:

> Prepare the driver for the bus-specific code refactoring into

by refactoring into..

> separate files. The renamed file will contain the common DAC functionality
> shared by the MCP47FxBy1/2/4/8 I2C and MCP48FxBy1/2/4/8 SPI drivers. The
> MCP47FEB02 driver was refactored into two modules: mcp47feb02-core.c
> and mcp47feb02-i2c.c in order to prepare the support for SPI
> MCP48FxBy1/2/4/8 DAC family on top of the current implementation.

Rewrap to a consistent line length.  People tend to use either 72 or 75 chars
for commit messages.

My main comment in the following is that a lot of stuff gets moved into
the header and I think the vast majority of it can stay in the core.c
file reducing it's scope and generally keeping things a little simpler.

Jonathan


> 
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---
>  MAINTAINERS                                        |   4 +-
>  drivers/iio/dac/Kconfig                            |  10 +-
>  drivers/iio/dac/Makefile                           |   3 +-
>  .../iio/dac/{mcp47feb02.c => mcp47feb02-core.c}    | 414 +--------------------
>  drivers/iio/dac/mcp47feb02-i2c.c                   | 145 ++++++++
>  drivers/iio/dac/mcp47feb02.h                       | 153 ++++++++
>  6 files changed, 325 insertions(+), 404 deletions(-)

> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02-core.c
> similarity index 64%
> rename from drivers/iio/dac/mcp47feb02.c
> rename to drivers/iio/dac/mcp47feb02-core.c
> index a823c2a673a26d70e5829cb587034da435af0451..6c86fa40e6eb2f1f602cac4c75265b6af7000c72 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02-core.c
> @@ -2,7 +2,7 @@
>  /*
>   * IIO driver for MCP47FEB02 Multi-Channel DAC with I2C interface
>   *
> - * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
> + * Copyright (C) 2026 Microchip Technology Inc. and its subsidiaries

I'd go with 2025-2026 

>   *
>   * Author: Ariana Lazar <ariana.lazar@microchip.com>
>   *



> 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

> 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>
> +#include <linux/device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Register addresses must be left shifted with 3 positions in order to append command mask */

Maybe it becomes obvious later, but for this patch at least, why
are we moving the register defines into a header?  They are only used
from the core driver so can we not leave them there?

Aim to have as little as possible in the shared header.
That also applies to the enums and most of the structures that follow.


> +#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 */
> +#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
> +
> +/* Macro used for generating chip features structures */
> +#define MCP47FEB02_CHIP_INFO(_name, _channels, _res, _vref1, _eeprom) \
> +static const struct mcp47feb02_features _name##_chip_features = { \
> +	.name = #_name, \
> +	.phys_channels = _channels, \
> +	.resolution = _res, \
> +	.have_ext_vref1 = _vref1, \
> +	.have_eeprom = _eeprom, \
> +}
> +
> +enum mcp47feb02_vref_mode {
> +	MCP47FEB02_VREF_VDD = 0,
> +	MCP47FEB02_INTERNAL_BAND_GAP = 1,
> +	MCP47FEB02_EXTERNAL_VREF_UNBUFFERED = 2,
> +	MCP47FEB02_EXTERNAL_VREF_BUFFERED = 3,
> +};
> +
> +enum mcp47feb02_scale {
> +	MCP47FEB02_SCALE_VDD = 0,
> +	MCP47FEB02_SCALE_GAIN_X1 = 1,
> +	MCP47FEB02_SCALE_GAIN_X2 = 2,
> +};
> +
> +enum mcp47feb02_gain_bit_mode {
> +	MCP47FEB02_GAIN_BIT_X1 = 0,
> +	MCP47FEB02_GAIN_BIT_X2 = 1,
> +};
> +
> +extern const char * const mcp47feb02_powerdown_modes[];
> +
> +/**
> + * struct mcp47feb02_features - chip specific data
> + * @name: device name
> + * @phys_channels: number of hardware channels
> + * @resolution: DAC resolution
> + * @have_ext_vref1: does the hardware have an the second external voltage reference?
> + * @have_eeprom: does the hardware have an internal eeprom?
> + */
> +struct mcp47feb02_features {
> +	const char *name;
> +	unsigned int phys_channels;
> +	unsigned int resolution;
> +	bool have_ext_vref1;
> +	bool have_eeprom;
> +};
I think this is one of the few structures that does want to be in this header.

> +
> +/**
> + * struct mcp47feb02_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
> + */
> +struct mcp47feb02_channel_data {
> +	u8 ref_mode;
> +	bool use_2x_gain;
> +	bool powerdown;
> +	u8 powerdown_mode;
> +	u16 dac_data;
> +};
> +
> +/**
> + * struct mcp47feb02_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 mcp47feb02_data {
> +	struct mcp47feb02_channel_data chdata[MCP47FEB02_MAX_CH];
> +	struct mutex lock; /* prevents concurrent reads/writes to driver's state members */
> +	const struct mcp47feb02_features *chip_features;
> +	int scale_1[2 * MCP47FEB02_MAX_SCALES_CH];
> +	int scale[2 * MCP47FEB02_MAX_SCALES_CH];
> +	unsigned long active_channels_mask;
> +	struct regmap *regmap;
> +	const char *labels[MCP47FEB02_MAX_CH];
> +	u16 phys_channels;
> +	bool vref1_buffered;
> +	bool vref_buffered;
> +	bool use_vref1;
> +	bool use_vref;
> +};

Even this looks superficially like it could stay in the -core.c file.

> +
> +extern const struct regmap_config mcp47feb02_regmap_config;
> +extern const struct regmap_config mcp47fvb02_regmap_config;
> +
> +/* Properties shared by I2C and SPI families */
> +int mcp47feb02_common_probe(const struct mcp47feb02_features *chip_features, struct regmap *regmap);
> +
> +extern const struct dev_pm_ops mcp47feb02_pm_ops;
> +
> +#endif /* __DRIVERS_IIO_DAC_MCP47FEB02_H__ */
> +
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-07-26  0:15 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20260723-mcp47feb02_refactor-v1-0-ee59e63672bc@microchip.com>
     [not found] ` <20260723-mcp47feb02_refactor-v1-2-ee59e63672bc@microchip.com>
2026-07-23 16:38   ` [PATCH 2/3] dt-bindings: iio: dac: add support for MCP48FEB02 SPI Conor Dooley
2026-07-24 12:30     ` Ariana.Lazar
2026-07-24 12:44       ` Conor Dooley
     [not found] ` <20260723-mcp47feb02_refactor-v1-3-ee59e63672bc@microchip.com>
2026-07-23 21:02   ` [PATCH 3/3] iio: dac: add support for Microchip MCP48FEB02 Joshua Crofts
     [not found] ` <20260723-mcp47feb02_refactor-v1-1-ee59e63672bc@microchip.com>
2026-07-23 21:18   ` [PATCH 1/3] iio: dac: mcp47feb02: refactor MCP47FEB02 I2C driver into two modules Joshua Crofts
2026-07-26  0:14   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox