[PATCH 1/2] dt-bindings: iio: dac: add MCP4821

[PATCH 1/2] dt-bindings: iio: dac: add MCP4821

[Anshul Dalal]

Adds support for MCP48xx series of DACs.

Datasheet:
  [MCP48x1] https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
  [MCP48x2] https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf

Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
 .../bindings/iio/dac/microchip,mcp4821.yaml   | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml

diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
new file mode 100644
index 000000000000..904de15300bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4821.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip MCP4821 and similar DACs
+
+description: |
+  Supports MCP48x1 (single channel) and MCP48x2 (dual channel) series of DACs.
+  Device supports simplex communication over SPI in Mode 0,1 and Mode 1,1.
+
+  +---------+--------------+-------------+
+  | Device  |  Resolution  |   Channels  |
+  |---------|--------------|-------------|
+  | MCP4801 |     8-bit    |      1      |
+  | MCP4811 |    10-bit    |      1      |
+  | MCP4821 |    12-bit    |      1      |
+  | MCP4802 |     8-bit    |      2      |
+  | MCP4812 |    10-bit    |      2      |
+  | MCP4822 |    12-bit    |      2      |
+  +---------+--------------+-------------+
+
+  Datasheet:
+    MCP48x1: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
+    MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
+
+maintainers:
+  - Anshul Dalal <anshulusr@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - microchip,mcp4801
+      - microchip,mcp4811
+      - microchip,mcp4821
+      - microchip,mcp4802
+      - microchip,mcp4812
+      - microchip,mcp4822
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dac@0 {
+            compatible = "microchip,mcp4821";
+            reg = <0>;
+            vdd-supply = <&vdd_regulator>;
+        };
+    };
-- 
2.42.1

[PATCH 2/2] iio: dac: driver for MCP4821

[Anshul Dalal]

Adds driver for the MCP48xx series of DACs.

Device uses a simplex SPI channel.
To set the value of an output channel, a 16-bit
data of following format must be written:

Bit field | Description
15 [MSB]  | Channel selection bit
            0 -> Channel A
            1 -> Channel B
13        | Output Gain Selection bit
            0 -> 2x Gain (Vref = 4.096V)
            1 -> 1x Gain (Vref = 2.048V)
12        | Output Shutdown Control bit
            0 -> Shutdown the selected channel
            1 -> Active mode operation
11-0 [LSB]| DAC Input Data bits
            Value's big endian representation is
            taken as input for the selected DAC
            channel.
            For devices with a resolution of less
            than 12-bits, only the x most
            significant bits are considered where
            x is the resolution of the device.
Reference: Page#22 [MCP48x2 Datasheet]

Supported devices:
  +---------+--------------+-------------+
  | Device  |  Resolution  |   Channels  |
  |---------|--------------|-------------|
  | MCP4801 |     8-bit    |      1      |
  | MCP4811 |    10-bit    |      1      |
  | MCP4821 |    12-bit    |      1      |
  | MCP4802 |     8-bit    |      2      |
  | MCP4812 |    10-bit    |      2      |
  | MCP4822 |    12-bit    |      2      |
  +---------+--------------+-------------+

Devices tested:
  MCP4821 [12-bit single channel]
  MCP4802 [8-bit dual channel]

Tested on Raspberry Pi Zero 2W

Datasheet:
  [MCP48x1] https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
  [MCP48x2] https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf

Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
 MAINTAINERS               |   7 ++
 drivers/iio/dac/Kconfig   |  10 ++
 drivers/iio/dac/Makefile  |   1 +
 drivers/iio/dac/mcp4821.c | 207 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 225 insertions(+)
 create mode 100644 drivers/iio/dac/mcp4821.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..8d9274c33c6e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13029,6 +13029,13 @@ F:	Documentation/ABI/testing/sysfs-bus-iio-potentiometer-mcp4531
 F:	drivers/iio/potentiometer/mcp4018.c
 F:	drivers/iio/potentiometer/mcp4531.c
 
+MCP4821 DAC DRIVER
+M:	Anshul Dalal <anshulusr@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
+F:	drivers/iio/dac/mcp4821.c
+
 MCR20A IEEE-802.15.4 RADIO DRIVER
 M:	Stefan Schmidt <stefan@datenfreihafen.org>
 L:	linux-wpan@vger.kernel.org
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 93b8be183de6..f5adc364de30 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -400,6 +400,16 @@ config MCP4728
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4728.
 
+config MCP4821
+	tristate "MCP4801/11/21/02/12/22 DAC driver"
+	depends on SPI
+	help
+	  Say yes here to build the driver for the Microchip MCP4801
+	  MCP4811, MCP4821, MCP4802, MCP4812 and MCP4822 DAC devices.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp4821.
+
 config MCP4922
 	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
 	depends on SPI
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 5b2bac900d5a..55bf89739d14 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_MAX5522) += max5522.o
 obj-$(CONFIG_MAX5821) += max5821.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
 obj-$(CONFIG_MCP4728) += mcp4728.o
+obj-$(CONFIG_MCP4821) += mcp4821.o
 obj-$(CONFIG_MCP4922) += mcp4922.o
 obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
 obj-$(CONFIG_STM32_DAC) += stm32-dac.o
diff --git a/drivers/iio/dac/mcp4821.c b/drivers/iio/dac/mcp4821.c
new file mode 100644
index 000000000000..d08acf6b8993
--- /dev/null
+++ b/drivers/iio/dac/mcp4821.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Microchip MCP4801, MCP4811, MCP4821, MCP4802, MCP4812 and MCP4822
+ *
+ * Based on the work of:
+ *	Michael Welling (MCP4922 Driver)
+ *
+ * Datasheet:
+ *	MCP48x1: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
+ *	MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
+ *
+ * TODO:
+ *	- Configurable gain
+ *	- Regulator control
+ */
+
+#include <asm/unaligned.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi.h>
+
+#define MCP4821_ACTIVE_MODE BIT(12)
+#define MCP4802_SECOND_CHAN BIT(15)
+#define MCP4821_CHAN_NUM    1
+
+/* DAC uses an internal Voltage reference of 4.096V at a gain of 2x */
+#define MCP4821_2X_GAIN_VREF_MV 4096
+
+enum mcp4821_supported_drvice_ids {
+	ID_MCP4801,
+	ID_MCP4811,
+	ID_MCP4821,
+	ID_MCP4802,
+	ID_MCP4812,
+	ID_MCP4822,
+};
+
+static int mcp4821_chan_count(enum mcp4821_supported_drvice_ids device_id)
+{
+	switch (device_id) {
+	case ID_MCP4801:
+	case ID_MCP4811:
+	case ID_MCP4821:
+		return 1;
+	default:
+		return 2;
+	}
+}
+
+struct mcp4821_state {
+	struct spi_device *spi;
+	struct mutex lock;
+	u16 dac_value[2];
+};
+
+#define MCP4821_CHAN(channel_id, resolution)				\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.output = 1,						\
+		.indexed = 1,						\
+		.channel = (channel_id),				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+		.scan_type = {						\
+			.sign = 'u',					\
+			.realbits = (resolution),			\
+			.storagebits = 16,				\
+			.shift = 12 - (resolution),			\
+		},							\
+	}
+
+static const struct iio_chan_spec mcp4821_channels[6][2] = {
+	[ID_MCP4801] = { MCP4821_CHAN(0, 8), {} },
+	[ID_MCP4811] = { MCP4821_CHAN(0, 10), {} },
+	[ID_MCP4821] = { MCP4821_CHAN(0, 12), {} },
+	[ID_MCP4802] = { MCP4821_CHAN(0, 8), MCP4821_CHAN(1, 8) },
+	[ID_MCP4812] = { MCP4821_CHAN(0, 10), MCP4821_CHAN(1, 10) },
+	[ID_MCP4822] = { MCP4821_CHAN(0, 12), MCP4821_CHAN(1, 12) },
+};
+
+static int mcp4821_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	struct mcp4821_state *state;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		state = iio_priv(indio_dev);
+		mutex_lock(&state->lock);
+		*val = state->dac_value[chan->channel];
+		mutex_unlock(&state->lock);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = MCP4821_2X_GAIN_VREF_MV;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp4821_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct mcp4821_state *state;
+	__be16 write_val;
+	u8 write_buffer[2];
+	int ret;
+	bool is_value_valid = val >= 0 && val < BIT(chan->scan_type.realbits) &&
+			      val2 == 0;
+	if (mask == IIO_CHAN_INFO_RAW && is_value_valid) {
+		state = iio_priv(indio_dev);
+
+		write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
+		if (chan->channel)
+			write_val |= MCP4802_SECOND_CHAN;
+		put_unaligned_be16(write_val, write_buffer);
+		ret = spi_write(state->spi, write_buffer, sizeof(write_buffer));
+		if (ret) {
+			dev_err(&state->spi->dev,
+				"Failed to write to device: %d", ret);
+			return ret;
+		}
+
+		mutex_lock(&state->lock);
+		state->dac_value[chan->channel] = val;
+		mutex_unlock(&state->lock);
+		return 0;
+	} else {
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info mcp4821_info = {
+	.read_raw = &mcp4821_read_raw,
+	.write_raw = &mcp4821_write_raw,
+};
+
+static int mcp4821_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp4821_state *state;
+	const struct spi_device_id *id;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (indio_dev == NULL) {
+		dev_err(&spi->dev, "Failed to allocate iio device\n");
+		return -ENOMEM;
+	}
+
+	state = iio_priv(indio_dev);
+	spi_set_drvdata(spi, indio_dev);
+	id = spi_get_device_id(spi);
+
+	state->spi = spi;
+	mutex_init(&state->lock);
+
+	indio_dev->name = id->name;
+	indio_dev->info = &mcp4821_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mcp4821_channels[id->driver_data];
+	indio_dev->num_channels = mcp4821_chan_count(id->driver_data);
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id mcp4821_id_table[] = {
+	{ "mcp4801", ID_MCP4801},
+	{ "mcp4811", ID_MCP4811},
+	{ "mcp4821", ID_MCP4821},
+	{ "mcp4802", ID_MCP4802},
+	{ "mcp4812", ID_MCP4812},
+	{ "mcp4822", ID_MCP4822},
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(spi, mcp4821_id_table);
+
+static const struct of_device_id mcp4821_of_table[] = {
+	{ .compatible = "microchip,mcp4801"},
+	{ .compatible = "microchip,mcp4811"},
+	{ .compatible = "microchip,mcp4821"},
+	{ .compatible = "microchip,mcp4802"},
+	{ .compatible = "microchip,mcp4812"},
+	{ .compatible = "microchip,mcp4822"},
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mcp4821_of_table);
+
+static struct spi_driver mcp4821_driver = {
+	.driver = {
+		.name = "mcp4821",
+		.of_match_table = mcp4821_of_table,
+	},
+	.probe = mcp4821_probe,
+	.id_table = mcp4821_id_table,
+};
+
+module_spi_driver(mcp4821_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Microchip MCP4821 DAC Driver");
+MODULE_LICENSE("GPL");
-- 
2.42.1

Re: [PATCH 2/2] iio: dac: driver for MCP4821

[kernel test robot]

Hi Anshul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.7-rc1 next-20231117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Anshul-Dalal/iio-dac-driver-for-MCP4821/20231117-153451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20231117073040.685860-2-anshulusr%40gmail.com
patch subject: [PATCH 2/2] iio: dac: driver for MCP4821
config: i386-randconfig-061-20231119 (https://download.01.org/0day-ci/archive/20231119/202311190944.LgtvolpG-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231119/202311190944.LgtvolpG-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311190944.LgtvolpG-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/dac/mcp4821.c:120:27: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted __be16 [usertype] write_val @@     got unsigned long @@
   drivers/iio/dac/mcp4821.c:120:27: sparse:     expected restricted __be16 [usertype] write_val
   drivers/iio/dac/mcp4821.c:120:27: sparse:     got unsigned long
>> drivers/iio/dac/mcp4821.c:122:35: sparse: sparse: invalid assignment: |=
>> drivers/iio/dac/mcp4821.c:122:35: sparse:    left side has type restricted __be16
>> drivers/iio/dac/mcp4821.c:122:35: sparse:    right side has type unsigned long
>> drivers/iio/dac/mcp4821.c:123:36: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected unsigned short [usertype] val @@     got restricted __be16 [usertype] write_val @@
   drivers/iio/dac/mcp4821.c:123:36: sparse:     expected unsigned short [usertype] val
   drivers/iio/dac/mcp4821.c:123:36: sparse:     got restricted __be16 [usertype] write_val
   drivers/iio/dac/mcp4821.c: note: in included file (through include/linux/rculist.h, include/linux/pid.h, include/linux/sched.h, ...):
   include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true

vim +120 drivers/iio/dac/mcp4821.c

   106	
   107	static int mcp4821_write_raw(struct iio_dev *indio_dev,
   108				     struct iio_chan_spec const *chan, int val,
   109				     int val2, long mask)
   110	{
   111		struct mcp4821_state *state;
   112		__be16 write_val;
   113		u8 write_buffer[2];
   114		int ret;
   115		bool is_value_valid = val >= 0 && val < BIT(chan->scan_type.realbits) &&
   116				      val2 == 0;
   117		if (mask == IIO_CHAN_INFO_RAW && is_value_valid) {
   118			state = iio_priv(indio_dev);
   119	
 > 120			write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
   121			if (chan->channel)
 > 122				write_val |= MCP4802_SECOND_CHAN;
 > 123			put_unaligned_be16(write_val, write_buffer);
   124			ret = spi_write(state->spi, write_buffer, sizeof(write_buffer));
   125			if (ret) {
   126				dev_err(&state->spi->dev,
   127					"Failed to write to device: %d", ret);
   128				return ret;
   129			}
   130	
   131			mutex_lock(&state->lock);
   132			state->dac_value[chan->channel] = val;
   133			mutex_unlock(&state->lock);
   134			return 0;
   135		} else {
   136			return -EINVAL;
   137		}
   138	}
   139	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] iio: dac: driver for MCP4821

[Jonathan Cameron]

On Fri, 17 Nov 2023 13:00:38 +0530
Anshul Dalal <anshulusr@gmail.com> wrote:

Hi Anshul,

Comments inline.

> Adds driver for the MCP48xx series of DACs.
> 
> Device uses a simplex SPI channel.
> To set the value of an output channel, a 16-bit
> data of following format must be written:

Wrap anything that isn't specifically formatted at 75 chars not this short.  
I could fix that whilst applying but you are going to be doing a v2 anyway
so you can tidy it up :)

> 
> Bit field | Description
> 15 [MSB]  | Channel selection bit
>             0 -> Channel A
>             1 -> Channel B
> 13        | Output Gain Selection bit
>             0 -> 2x Gain (Vref = 4.096V)
>             1 -> 1x Gain (Vref = 2.048V)
> 12        | Output Shutdown Control bit
>             0 -> Shutdown the selected channel
>             1 -> Active mode operation
> 11-0 [LSB]| DAC Input Data bits
>             Value's big endian representation is
>             taken as input for the selected DAC
>             channel.
>             For devices with a resolution of less
>             than 12-bits, only the x most
>             significant bits are considered where
>             x is the resolution of the device.
> Reference: Page#22 [MCP48x2 Datasheet]
> 
> Supported devices:
>   +---------+--------------+-------------+
>   | Device  |  Resolution  |   Channels  |
>   |---------|--------------|-------------|
>   | MCP4801 |     8-bit    |      1      |
>   | MCP4811 |    10-bit    |      1      |
>   | MCP4821 |    12-bit    |      1      |
>   | MCP4802 |     8-bit    |      2      |
>   | MCP4812 |    10-bit    |      2      |
>   | MCP4822 |    12-bit    |      2      |
>   +---------+--------------+-------------+
> 
> Devices tested:
>   MCP4821 [12-bit single channel]
>   MCP4802 [8-bit dual channel]
> 
> Tested on Raspberry Pi Zero 2W
> 
> Datasheet:
>   [MCP48x1] https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
>   [MCP48x2] https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
So Datasheet is part of the tag block so no line break between this
and sign off.  The way to format this is to add two entries and apply comments
after them.  Don't worry about long lines for this.

> 

Datasheet: https//.... #MCP48x1
Datasheet: https//.... #MCP48x2
Signed-off-by: ...

> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
...

> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 93b8be183de6..f5adc364de30 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -400,6 +400,16 @@ config MCP4728
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4728.
>  
> +config MCP4821
> +	tristate "MCP4801/11/21/02/12/22 DAC driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build the driver for the Microchip MCP4801
> +	  MCP4811, MCP4821, MCP4802, MCP4812 and MCP4822 DAC devices.

Numeric order her as well please.

> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mcp4821.
> +
>  config MCP4922
>  	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
>  	depends on SPI

...

> diff --git a/drivers/iio/dac/mcp4821.c b/drivers/iio/dac/mcp4821.c
> new file mode 100644
> index 000000000000..d08acf6b8993
> --- /dev/null
> +++ b/drivers/iio/dac/mcp4821.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Microchip MCP4801, MCP4811, MCP4821, MCP4802, MCP4812 and MCP4822
> + *
> + * Based on the work of:
> + *	Michael Welling (MCP4922 Driver)
> + *
> + * Datasheet:
> + *	MCP48x1: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
> + *	MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
> + *
> + * TODO:
> + *	- Configurable gain
> + *	- Regulator control
> + */
> +
> +#include <asm/unaligned.h>
So in IIO at least header ordering is normally (each block alphabetical)

linux/*.h

linux/iio/*.h (it's an IIO driver so sometimes good to separate these out)

asm/*.h (these are effectively even more specific so we put them in their own block.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +
> +#define MCP4821_ACTIVE_MODE BIT(12)
> +#define MCP4802_SECOND_CHAN BIT(15)
> +#define MCP4821_CHAN_NUM    1
Not used so drop this.

> +
> +/* DAC uses an internal Voltage reference of 4.096V at a gain of 2x */
> +#define MCP4821_2X_GAIN_VREF_MV 4096
> +
> +enum mcp4821_supported_drvice_ids {
> +	ID_MCP4801,
> +	ID_MCP4811,
> +	ID_MCP4821,
> +	ID_MCP4802,

Numeric order preferred throughout if we have long lists like this.

> +	ID_MCP4812,
> +	ID_MCP4822,
> +};
> +
> +static int mcp4821_chan_count(enum mcp4821_supported_drvice_ids device_id)
> +{
> +	switch (device_id) {
> +	case ID_MCP4801:
> +	case ID_MCP4811:
> +	case ID_MCP4821:
> +		return 1;
> +	default:

I'd rather see this explicit for each device even though it's a few more lines
of code.

> +		return 2;
> +	}
> +}
> +
> +struct mcp4821_state {
> +	struct spi_device *spi;
> +	struct mutex lock;

All locks need to have a comment describing the scope of data they are protecting.

> +	u16 dac_value[2];
> +};
> +
> +#define MCP4821_CHAN(channel_id, resolution)				\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.output = 1,						\
> +		.indexed = 1,						\
> +		.channel = (channel_id),				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = (resolution),			\
> +			.storagebits = 16,				\
> +			.shift = 12 - (resolution),			\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec mcp4821_channels[6][2] = {
> +	[ID_MCP4801] = { MCP4821_CHAN(0, 8), {} },

C will fill in the second entry with zeros without needing a {} and
as we don't care about it anyway, just don't bother adding the {} placeholder,
making it a bit neater.

> +	[ID_MCP4811] = { MCP4821_CHAN(0, 10), {} },
> +	[ID_MCP4821] = { MCP4821_CHAN(0, 12), {} },
> +	[ID_MCP4802] = { MCP4821_CHAN(0, 8), MCP4821_CHAN(1, 8) },
> +	[ID_MCP4812] = { MCP4821_CHAN(0, 10), MCP4821_CHAN(1, 10) },
> +	[ID_MCP4822] = { MCP4821_CHAN(0, 12), MCP4821_CHAN(1, 12) },
> +};
> +
> +static int mcp4821_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct mcp4821_state *state;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		state = iio_priv(indio_dev);
> +		mutex_lock(&state->lock);

Can use the new auto cleanup stuff for this to shorten things a tiny bit.

		scoped_guard(mutex, &state->lock)
			*val = state->dac_channel[chan->channel];

Obviously very minor advantage here so I don't mind either way.

> +		*val = state->dac_value[chan->channel];
> +		mutex_unlock(&state->lock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = MCP4821_2X_GAIN_VREF_MV;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp4821_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct mcp4821_state *state;
> +	__be16 write_val;
> +	u8 write_buffer[2];

type of these two is backwards.  you are build the value in write_val, so that should
just be a u16, then converting it to be16 so write_buffer should be __be16

> +	int ret;
> +	bool is_value_valid = val >= 0 && val < BIT(chan->scan_type.realbits) &&
> +			      val2 == 0;
> +	if (mask == IIO_CHAN_INFO_RAW && is_value_valid) {
> +		state = iio_priv(indio_dev);
> +
> +		write_val = MCP4821_ACTIVE_MODE | val << chan->scan_type.shift;
> +		if (chan->channel)
> +			write_val |= MCP4802_SECOND_CHAN;
> +		put_unaligned_be16(write_val, write_buffer);



> +		ret = spi_write(state->spi, write_buffer, sizeof(write_buffer));
> +		if (ret) {
> +			dev_err(&state->spi->dev,
> +				"Failed to write to device: %d", ret);
> +			return ret;
> +		}
> +
> +		mutex_lock(&state->lock);

As above if you want to.


> +		state->dac_value[chan->channel] = val;
> +		mutex_unlock(&state->lock);
> +		return 0;
> +	} else {
> +		return -EINVAL;
Flip this and separate the unrelated conditions.
	if (mask != IIO_CHAN_INFO_RAW)
		return -EINVAL;

	if (!is_value_valid)
		return -EINVAL;

	...

reduces unnecessary indent and generally puts the errors 'out of line' which
is common kernel approach and generally 'what we expect to see' when reviewing.


> +	}
> +}
> +
> +static const struct iio_info mcp4821_info = {
> +	.read_raw = &mcp4821_read_raw,
> +	.write_raw = &mcp4821_write_raw,
> +};
> +
> +static int mcp4821_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp4821_state *state;
> +	const struct spi_device_id *id;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (indio_dev == NULL) {
> +		dev_err(&spi->dev, "Failed to allocate iio device\n");
> +		return -ENOMEM;
		return dev_err_probe(&spi->dev, -ENOMEM,
				     "failed to allocate IIO device\n");

However that's very unlikely to ever happen, so you could just not bother
with the message.

> +	}
> +
> +	state = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);

Why?

> +	id = spi_get_device_id(spi);
For dt bindings this relies on the fallback of breaking up the compatible.
That's fragile, so add the data to the of_table as well + use pointers
not enum values so that you can then use spi_get_device_match_data()
which handles other firmware types as well and falls back to what you
have here.

> +
> +	state->spi = spi;
> +	mutex_init(&state->lock);
> +
> +	indio_dev->name = id->name;

Also fragile (very weird things happen if you end up with fallback DT compatibles
in the future).  So embed a string in a device specific structure as described
below.

> +	indio_dev->info = &mcp4821_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp4821_channels[id->driver_data];
> +	indio_dev->num_channels = mcp4821_chan_count(id->driver_data);
Combine channels and chan_count as static data in a
struct mcp4821_info {
	struct iio_chan_spec channels];
	int num_channels;
	const char *name;
}
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct spi_device_id mcp4821_id_table[] = {
> +	{ "mcp4801", ID_MCP4801},
use &mcp4821_data[ID_MCP4801] etc in here.. (with cast as needed, lots of
examples in tree...
> +	{ "mcp4811", ID_MCP4811},
> +	{ "mcp4821", ID_MCP4821},
> +	{ "mcp4802", ID_MCP4802},
> +	{ "mcp4812", ID_MCP4812},
> +	{ "mcp4822", ID_MCP4822},
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, mcp4821_id_table);
> +
> +static const struct of_device_id mcp4821_of_table[] = {
> +	{ .compatible = "microchip,mcp4801"},
and provide the data here as well. 
> +	{ .compatible = "microchip,mcp4811"},
> +	{ .compatible = "microchip,mcp4821"},
...

Thanks,

Jonathan

Re: [PATCH 1/2] dt-bindings: iio: dac: add MCP4821

[Conor Dooley]

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

On Fri, Nov 17, 2023 at 01:00:37PM +0530, Anshul Dalal wrote:
> Adds support for MCP48xx series of DACs.
> 
> Datasheet:
>   [MCP48x1] https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
>   [MCP48x2] https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
> 
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  .../bindings/iio/dac/microchip,mcp4821.yaml   | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
> new file mode 100644
> index 000000000000..904de15300bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4821.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP4821 and similar DACs
> +
> +description: |
> +  Supports MCP48x1 (single channel) and MCP48x2 (dual channel) series of DACs.
> +  Device supports simplex communication over SPI in Mode 0,1 and Mode 1,1.
> +
> +  +---------+--------------+-------------+
> +  | Device  |  Resolution  |   Channels  |
> +  |---------|--------------|-------------|
> +  | MCP4801 |     8-bit    |      1      |
> +  | MCP4811 |    10-bit    |      1      |
> +  | MCP4821 |    12-bit    |      1      |
> +  | MCP4802 |     8-bit    |      2      |
> +  | MCP4812 |    10-bit    |      2      |
> +  | MCP4822 |    12-bit    |      2      |
> +  +---------+--------------+-------------+
> +
> +  Datasheet:
> +    MCP48x1: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
> +    MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
> +
> +maintainers:
> +  - Anshul Dalal <anshulusr@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp4801
> +      - microchip,mcp4811
> +      - microchip,mcp4821
> +      - microchip,mcp4802
> +      - microchip,mcp4812
> +      - microchip,mcp4822
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        dac@0 {
> +            compatible = "microchip,mcp4821";
> +            reg = <0>;
> +            vdd-supply = <&vdd_regulator>;
> +        };
> +    };
> -- 
> 2.42.1
> 

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

Re: [PATCH 1/2] dt-bindings: iio: dac: add MCP4821

[Jonathan Cameron]

On Fri, 17 Nov 2023 13:00:37 +0530
Anshul Dalal <anshulusr@gmail.com> wrote:

> Adds support for MCP48xx series of DACs.
> 
> Datasheet:
>   [MCP48x1] https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
>   [MCP48x2] https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
> 
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
Hi Anshul,

Usually we mark vdd-supply as required given I guess device doesn't work
without a supply. Obviously we don't actually have to provide it in a binding
if the supply is always on and we are fine with a stub regulator being
provided by the regulator subsystem.

There was some discussion about this a while back and conclusion was
mark them required in bindings anyway.  We haven't yet updated this in all
the older IIO bindings and it's a minor thing, but given the build warning
on patch 2 you are going around again so might as well tidy that up!

Jonathan


> ---
>  .../bindings/iio/dac/microchip,mcp4821.yaml   | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
> new file mode 100644
> index 000000000000..904de15300bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/microchip,mcp4821.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/microchip,mcp4821.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip MCP4821 and similar DACs
> +
> +description: |
> +  Supports MCP48x1 (single channel) and MCP48x2 (dual channel) series of DACs.
> +  Device supports simplex communication over SPI in Mode 0,1 and Mode 1,1.
> +
> +  +---------+--------------+-------------+
> +  | Device  |  Resolution  |   Channels  |
> +  |---------|--------------|-------------|
> +  | MCP4801 |     8-bit    |      1      |
> +  | MCP4811 |    10-bit    |      1      |
> +  | MCP4821 |    12-bit    |      1      |
> +  | MCP4802 |     8-bit    |      2      |
> +  | MCP4812 |    10-bit    |      2      |
> +  | MCP4822 |    12-bit    |      2      |
> +  +---------+--------------+-------------+
> +
> +  Datasheet:
> +    MCP48x1: https://ww1.microchip.com/downloads/en/DeviceDoc/22244B.pdf
> +    MCP48x2: https://ww1.microchip.com/downloads/en/DeviceDoc/20002249B.pdf
> +
> +maintainers:
> +  - Anshul Dalal <anshulusr@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - microchip,mcp4801
> +      - microchip,mcp4811
> +      - microchip,mcp4821
> +      - microchip,mcp4802
> +      - microchip,mcp4812
> +      - microchip,mcp4822

Whilst I understand the reasoning of keeping these grouped by number of channels,
I'd still rather see them in numeric order here and probably also in the table above.
Given that grouping by resolution rather than channels would also be a valid choice,
I don't see a strong reason to keep them out of order.

Also, manufacturers often get creative with numbering (when they run of out of digits
for example - maybe they'll do a 16 channel variant one day and then be stuck) so
trying to group things is often a loosing game long term!

Jonathan


>