Add MAX14001/MAX14002 support

Notes:
Since v10, I have not used exactly the same approach as Kim did in v9, nor the same approach as in my v1. Instead, I merged both implementations, and this v11 is quite different from both. Therefore, I have dropped the review by Krzysztof Kozlowski. I am not very familiar with the kernel’s review process, should I add it back? Should I list your names as Reviewed-by? Thanks.

The MAX14001 and MAX14002 both have the COUT output pin and the FAULT output pin, and work the same. I have decided to declare them as interrupts because I think some action should be done when they are hit. However, the implementation of these features is not present in the v11 driver code, as it was not in v9. But I plan to submit it in the next steps.

 .../bindings/iio/adc/adi,max14001.yaml        | 87 +++++++++++++++++++
 MAINTAINERS                                   |  8 ++
 2 files changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +48 -0

new file mode 100644
index 000000000000..c61119b16cf5
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023-2025 Analog Devices Inc.
+# Copyright 2023 Kim Seer Paller
+# Copyright 2025 Marilene Andrade Garcia
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX14001-MAX14002 ADC
+
+maintainers:
+  - Kim Seer Paller <[email protected]>
+  - Marilene Andrade Garcia <[email protected]>
+
+description: |
+    Single channel 10 bit ADC with SPI interface.
+    Datasheet can be found here
+      https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    oneOf:
+      - const: adi,max14002
+      - items:
+          - const: adi,max14001
+          - const: adi,max14002
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 5000000
+
+  vdd-supply:
+    description:
+      Isolated DC-DC power supply input voltage.
+
+  vddl-supply:
+    description:
+      Logic power supply.
+
+  refin-supply:
+    description:
+      ADC voltage reference supply.
+

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +14 -0

+  interrupt-names:
+    minItems: 1
+    items:
+      - const: cout
+      - const: fault
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddl-supply
+
+unevaluatedProperties: false
+

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +5 -0

+        vdd-supply = <&vdd>;
+        vddl-supply = <&vddl>;
+      };
+    };
+...

Quoted 6 lines · Marilene Andrade Garcia

The MAX14001 and MAX14002 both have the COUT output pin and the FAULT output pin, and work the same. I have decided to declare them as interrupts because I think some action should be done when they are hit. However, the implementation of these features is not present in the v11 driver code, as it was not in v9. But I plan to submit it in the next steps.

The devicetree bindings should be as complete as possible and not care if the driver doesn't use everything. So adding them now is the right thing to do.

Quoted 9 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +9 -0

+  interrupts:
+    minItems: 1
+    items:
+      - description: |
+          Asserts high when ADC readings exceed the upper threshold and low
+          when below the lower threshold. Must be connected to the COUT pin.
+      - description: |
+          Alert output that asserts low during a number of different error
+          conditions. The interrupt source must be attached to FAULT pin.

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml line 57

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +9 -0

+  interrupts:
+    minItems: 1
+    items:
+      - description: |
+          Asserts high when ADC readings exceed the upper threshold and low
+          when below the lower threshold. Must be connected to the COUT pin.
+      - description: |
+          Alert output that asserts low during a number of different error
+          conditions. The interrupt source must be attached to FAULT pin.

These descriptions read wrong to me. They __are__ the COUT and FAULT pins, given what David responded to above, not something that can be connected to these pins (if they were, they would be represented as -gpios rather than interrupts most likely). Unless you mean that these pins can have some other use and are only available on the COUT/FAULT pins when some register value is set - but even in that case saying "must be" doesn't fit since the interrupt property could be used to configure the device accordingly.

...

Quoted 11 lines

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml line 57

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +9 -0

+  interrupts:
+    minItems: 1
+    items:
+      - description: |
+          Asserts high when ADC readings exceed the upper threshold and low
+          when below the lower threshold. Must be connected to the COUT pin.
+      - description: |
+          Alert output that asserts low during a number of different error
+          conditions. The interrupt source must be attached to FAULT pin.

Quoted 8 lines

These descriptions read wrong to me. They __are__ the COUT and FAULT pins, given what David responded to above, not something that can be connected to these pins (if they were, they would be represented as -gpios rather than interrupts most likely). Unless you mean that these pins can have some other use and are only available on the COUT/FAULT pins when some register value is set - but even in that case saying "must be" doesn't fit since the interrupt property could be used to configure the device accordingly.

COUT and FAULT are just two pins on the ADC chip that can be used to generate interrupts. Would a description like the one below sound better?

interrupts:
minItems: 1
items:
- description: |
cout: Comparator output signal that asserts high when ADC readings exceed the upper threshold and low when readings fall below the lower threshold.

- description: |
fault: When fault reporting is enabled, the FAULT pin is asserted low whenever one of the monitored fault conditions occurs.

Best regards,
Marcelo

Quoted 24 lines · Marcelo Schmitt

...

Quoted 11 lines

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml line 57

Quoted 10 lines

Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml new file +9 -0

+  interrupts:
+    minItems: 1
+    items:
+      - description: |
+          Asserts high when ADC readings exceed the upper threshold and low
+          when below the lower threshold. Must be connected to the COUT pin.
+      - description: |
+          Alert output that asserts low during a number of different error
+          conditions. The interrupt source must be attached to FAULT pin.

Quoted 8 lines

These descriptions read wrong to me. They __are__ the COUT and FAULT pins, given what David responded to above, not something that can be connected to these pins (if they were, they would be represented as -gpios rather than interrupts most likely). Unless you mean that these pins can have some other use and are only available on the COUT/FAULT pins when some register value is set - but even in that case saying "must be" doesn't fit since the interrupt property could be used to configure the device accordingly.

COUT and FAULT are just two pins on the ADC chip that can be used to generate interrupts. Would a description like the one below sound better?

Quoted 7 lines

interrupts:
minItems: 1
items:
- description: |
cout: Comparator output signal that asserts high when ADC readings exceed the upper threshold and low when readings fall below the lower threshold.

I think you should mention the pin name here, like you did below. "asserts high on the COUT pin" or w/e.

Quoted 6 lines

- description: |
fault: When fault reporting is enabled, the FAULT pin is asserted low whenever one of the monitored fault conditions occurs.

Best regards,
Marcelo

The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers more features, like a binary comparator, a filtered reading that can provide the average of the last 2, 4, or 8 ADC readings, and an inrush comparator that triggers the inrush current. There is also a fault feature that can diagnose seven possible fault conditions. And an option to select an external or internal ADC voltage reference.

MAX14001/MAX14002 features implemented so far:
- Raw ADC reading.
- Filtered ADC average reading with the default configuration.
- MV fault disable.
- Selection of external or internal ADC voltage reference, depending on whether it is declared in the device tree.

drivers/iio/adc/Makefile +2 -0

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

drivers/iio/adc/Makefile +13 -0

+        * and SPI transfer.
+        */
+       struct mutex lock;
+       /*
+        * The following buffers will be bit-reversed during device
+        * communication, because the device transmits and receives data
+        * LSB-first.
+        * DMA (thus cache coherency maintenance) requires the transfer
+        * buffers to live in their own cache lines.
+        */
+       __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
+       __be16 spi_rx_buffer;
+};

drivers/iio/adc/Makefile +8 -0

+        * in order to meet the device communication requirements.
+        */
+       st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
+                                                FIELD_PREP(MAX14001_MASK_WR, 1) |
+                                                FIELD_PREP(MAX14001_MASK_DATA, val)));
+
+       return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
+}

drivers/iio/adc/Makefile +17 -0

+       indio_dev->name = st->chip_info->name;
+       indio_dev->info = &max14001_info;
+       indio_dev->channels = max14001_channel;
+       indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
+       indio_dev->modes = INDIO_DIRECT_MODE;
+
+       st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
+       if (IS_ERR(st->regmap))
+               return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
+
+       ret = devm_mutex_init(dev, &st->lock);
+       if (ret)
+               return ret;
+
+       ret = devm_regulator_get_enable(dev, "vdd");
+       if (ret)
+               return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");

drivers/iio/adc/Makefile +12 -0

+                */
+               ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
+               if (ret)
+                       return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
+       }
+
+       ret = max14001_disable_mv_fault(st);
+       if (ret)
+               return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
+
+       return devm_iio_device_register(dev, indio_dev);
+}

The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers more features, like a binary comparator, a filtered reading that can provide the average of the last 2, 4, or 8 ADC readings, and an inrush comparator that triggers the inrush current. There is also a fault feature that can diagnose seven possible fault conditions. And an option to select an external or internal ADC voltage reference.

MAX14001/MAX14002 features implemented so far:
- Raw ADC reading.
- Filtered ADC average reading with the default configuration.
- MV fault disable.
- Selection of external or internal ADC voltage reference, depending on whether it is declared in the device tree.

Co-developed-by Kim Seer Paller <[email protected]>

Signed-off-by Kim Seer Paller <[email protected]>

Signed-off-by Marilene Andrade Garcia <[email protected]>

---

I have addressed almost all of the comments, thank you very much for the review. I would like to highlight some of them:

Changes since v10:
- Dropped the kernel.h include
- Add the cleanup.h, mutex.h, regmap.h and units.h includes
- Renamed the reg_addr variable name to reg
- Renamed the reg_data variable name to val
- Added the regmap implementation
- Used scoped_guard()
- Refactored the get refin voltage code
- Replace max14001_chip_model with data structures separated
- Added debugfs_reg_access

Change I was not able to do:
- I could not remove bitrev16 because I am using an SPI controller that does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using the spi-lsb-first devicetree property for now, since this driver currently works for both types of controllers: those that support it and those that do not. I left a TODO comment to address this issue as soon as the SPI kernel code starts handling the bit-reverse operation for controllers that do not have this support. Once I finish my work on this driver, if the SPI code still does not include this handling, I can submit patches to add it.

 MAINTAINERS                |   1 +
 drivers/iio/adc/Kconfig    |  10 ++
 drivers/iio/adc/Makefile   |   1 +
 drivers/iio/adc/max14001.c | 356 +++++++++++++++++++++++++++++++++++++
 4 files changed, 368 insertions(+)
 create mode 100644 drivers/iio/adc/max14001.c

MAINTAINERS +1 -0

index 0bae420caa63..a9cf93ba8b21 100644
@@ -14998,6 +14998,7 @@ L:	[email protected]
 S:	Maintained
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
+F:	drivers/iio/adc/max14001.c
 
 MAXBOTIX ULTRASONIC RANGER IIO DRIVER
 M:	Andreas Klinger <[email protected]>

drivers/iio/adc/Kconfig +0 -0

index 237fa2061329..a1f2afce60ad 100644
@@ -991,6 +991,16 @@ config MAX11410
 	  To compile this driver as a module, choose M here: the module will be
 	  called max11410.
 

drivers/iio/adc/Makefile +0 -0

 obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX34408) += max34408.o

drivers/iio/adc/max14001.c new file +59 -0

new file mode 100644
index 000000000000..2ade57970064
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+/*
+ * Analog Devices MAX14001/MAX14002 ADC driver
+ *
+ * Copyright (C) 2023-2025 Analog Devices Inc.
+ * Copyright (C) 2023 Kim Seer Paller <[email protected]>
+ * Copyright (c) 2025 Marilene Andrade Garcia <[email protected]>
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
+ */
+
+/*
+ * TODO:
+ * Replace bitrev16 with SPI_LSB_FIRST once the SPI kernel code supports handling
+ * SPI controllers that lack LSB-first support.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitrev.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+/* MAX14001 Registers Address */
+#define MAX14001_REG_ADC		0x00
+#define MAX14001_REG_FADC		0x01
+#define MAX14001_REG_FLAGS		0x02
+#define MAX14001_REG_FLTEN		0x03
+#define MAX14001_REG_THL		0x04
+#define MAX14001_REG_THU		0x05
+#define MAX14001_REG_INRR		0x06
+#define MAX14001_REG_INRT		0x07
+#define MAX14001_REG_INRP		0x08
+#define MAX14001_REG_CFG		0x09
+#define MAX14001_REG_ENBL		0x0A
+#define MAX14001_REG_ACT		0x0B
+#define MAX14001_REG_WEN		0x0C
+
+#define MAX14001_REG_VERIFICATION(x)	((x) + 0x10)
+
+#define MAX14001_REG_CFG_BIT_EXRF	BIT(5)
+
+#define MAX14001_REG_WEN_VALUE_WRITE	0x294
+
+#define MAX14001_MASK_ADDR		GENMASK(15, 11)
+#define MAX14001_MASK_WR		BIT(10)

drivers/iio/adc/max14001.c new file +18 -0

+};
+
+struct max14001_chip_info {
+	const char *name;
+};
+
+static struct max14001_chip_info max14001_chip_info = {
+	.name = "max14001",
+};
+
+static struct max14001_chip_info max14002_chip_info = {
+	.name = "max14002",
+};
+
+static int max14001_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct max14001_state *st = context;
+	struct spi_transfer xfers[] = {

drivers/iio/adc/max14001.c new file +76 -0

+
+	return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
+}
+
+static int max14001_write_single_reg(void *context, unsigned int reg, unsigned int val)
+{
+	struct max14001_state *st = context;
+	int ret;
+
+	/* Enable writing to the SPI register */
+	ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
+	if (ret)
+		return ret;
+
+	/* Writing data into SPI register */
+	ret = max14001_write(st, reg, val);
+	if (ret)
+		return ret;
+
+	/* Disable writing to the SPI register */
+	return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_write_verification_reg(struct max14001_state *st, unsigned int reg)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return max14001_write(st, MAX14001_REG_VERIFICATION(reg), val);
+}
+
+static int max14001_disable_mv_fault(struct max14001_state *st)
+{
+	unsigned int reg;
+	int ret;
+
+	/* Enable writing to the SPI registers */
+	ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
+	if (ret)
+		return ret;
+
+	/*
+	 * Reads all registers and writes the values to their appropriate
+	 * verification registers to clear the Memory Validation fault.
+	 */
+	for (reg = MAX14001_REG_FLTEN; reg <= MAX14001_REG_ENBL; reg++) {
+		ret = max14001_write_verification_reg(st, reg);
+		if (ret)
+			return ret;
+	}
+
+	/* Disable writing to the SPI registers */
+	return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_debugfs_reg_access(struct iio_dev *indio_dev,
+				       unsigned int reg, unsigned int writeval,
+				       unsigned int *readval)
+{
+	struct max14001_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static int max14001_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct max14001_state *st = iio_priv(indio_dev);

drivers/iio/adc/max14001.c new file +10 -0

+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		scoped_guard(mutex, &st->lock)
+			ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mV;
+		*val2 = 10;

drivers/iio/adc/max14001.c new file +7 -0

+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int max14001_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;

drivers/iio/adc/max14001.c new file +25 -0

+	st->chip_info = spi_get_device_match_data(spi);
+	if (!st->chip_info)
+		return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
+
+	indio_dev->name = st->chip_info->name;
+	indio_dev->info = &max14001_info;
+	indio_dev->channels = max14001_channel;
+	indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
+
+	ret = devm_regulator_get_enable(dev, "vddl");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");

drivers/iio/adc/max14001.c new file +35 -0

+
+	ret = max14001_disable_mv_fault(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct spi_device_id max14001_id_table[] = {
+	{ "max14001", (kernel_ulong_t)&max14001_chip_info },
+	{ "max14002", (kernel_ulong_t)&max14002_chip_info },
+	{ }
+};
+
+static const struct of_device_id max14001_of_match[] = {
+	{ .compatible = "adi,max14001", .data = &max14001_chip_info },
+	{ .compatible = "adi,max14002", .data = &max14002_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max14001_of_match);
+
+static struct spi_driver max14001_driver = {
+	.driver = {
+		.name = "max14001",
+		.of_match_table = max14001_of_match,
+	},
+	.probe = max14001_probe,
+	.id_table = max14001_id_table,
+};
+module_spi_driver(max14001_driver);
+
+MODULE_AUTHOR("Kim Seer Paller <[email protected]>");
+MODULE_AUTHOR("Marilene Andrade Garcia <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
+MODULE_LICENSE("GPL");

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

drivers/iio/adc/max14001.c line 259

Quoted 7 lines

drivers/iio/adc/max14001.c new file +7 -0

+static const struct iio_chan_spec max14001_channel[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_AVERAGE_RAW) |

I think there was some discussion about this last time [1] that it would actually make sense to have the average as a separate channel.

There are a few reasons for this. If we add buffered read support later, we could only read the average in a buffered read if it is a separate channel. And the average channel should have an extra control that is either the -3 dB low pass filter point (using existing ABI) or the averaging window size (potentially new ABI).

If we don't care about ever reading the filtered and not filtered value at the same time, we could just have a single channel and only ever read from the FADC register and ignore the ADC register.

In either case, we would not need IIO_CHAN_INFO_AVERAGE_RAW.

[1]: https://lore.kernel.org/linux-iio/[email protected]/

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

Quoted 9 lines

Change I was not able to do:
- I could not remove bitrev16 because I am using an SPI controller that does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using the spi-lsb-first devicetree property for now, since this driver currently works for both types of controllers: those that support it and those that do not. I left a TODO comment to address this issue as soon as the SPI kernel code starts handling the bit-reverse operation for controllers that do not have this support. Once I finish my work on this driver, if the SPI code still does not include this handling, I can submit patches to add it.

I looked more at what it would take to implement this in the SPI core code and found that it would actually be quite difficult to do in a generic way because there are so many edge/corner/n-dim cases. We can't change tx_buf data in-place because it might be const data that is in some memory area that can't be modified. And things would get complicated if different transfers pointed to the same buffer memory addresses anyway. So we would basically have to allocate new memory for all buffers, copy all tx data to that new memory, reverse all of the tx bits, and update all the pointers in the transfer structs. Then when the message was finished, we would have to reverse all of the rx bits, copy all of the rx buffers back to the original buffers and put all the buffer pointers back the way they were. But this could write over some of the original tx data if tx_buf and rx_buf point to the same original buffer, which would break things if a peripheral driver expected the tx data to persist.

And we can't do this during the SPI optimize
step because that currently allows the tx_buf data values to be modified after optimization.

So perhaps it is best to just handle it in the peripheral driver. It will be much more efficent that way anyway.

However, we still do want to handle SPI_LSB_FIRST now so that people with hardware support can be more efficient and we don't want things to break if someone puts spi-lsb-first in the devicetree.

drivers/iio/adc/max14001.c line 311

Quoted 5 lines

drivers/iio/adc/max14001.c new file +5 -0

+	if (ret < 0)
+		ret = 1250000;
+	else
+		ext_vrefin = 1;
+	st->vref_mV = ret / (MICRO / MILLI);

Just a style choice here, but in other drivers with similar handling we wrote it like this to avoid the extra if statement:

	if (ret < 0 && ret != -ENODEV)
		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");

	ext_vrefin = ret != -ENODEV;
	st->vref_mV = ext_vrefin ? ret / 1000 : 1250;

Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions on this.

Or we could drop ext_vrefin and have:

	if (ret < 0 && ret != -ENODEV)
		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");

	if (ret != -ENODEV) {
		st->vref_mV = ret / 1000;

		/* regmap set bits goes here. */
		... 
	} else {
		st->vref_mV = 1250;
	}

drivers/iio/adc/max14001.c line 320

Quoted 1 line

drivers/iio/adc/max14001.c new file +1 -0

+			return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");

These lines are getting very long. We try to wrap to 80 characters as much as we can in the IIO subsystem.

Quoted 1 line · David Lechner

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

Quoted 10 lines

Quoted 9 lines

Change I was not able to do:
- I could not remove bitrev16 because I am using an SPI controller that does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using the spi-lsb-first devicetree property for now, since this driver currently works for both types of controllers: those that support it and those that do not. I left a TODO comment to address this issue as soon as the SPI kernel code starts handling the bit-reverse operation for controllers that do not have this support. Once I finish my work on this driver, if the SPI code still does not include this handling, I can submit patches to add it.

Quoted 14 lines

I looked more at what it would take to implement this in the SPI core code and found that it would actually be quite difficult to do in a generic way because there are so many edge/corner/n-dim cases. We can't change tx_buf data in-place because it might be const data that is in some memory area that can't be modified. And things would get complicated if different transfers pointed to the same buffer memory addresses anyway. So we would basically have to allocate new memory for all buffers, copy all tx data to that new memory, reverse all of the tx bits, and update all the pointers in the transfer structs. Then when the message was finished, we would have to reverse all of the rx bits, copy all of the rx buffers back to the original buffers and put all the buffer pointers back the way they were. But this could write over some of the original tx data if tx_buf and rx_buf point to the same original buffer, which would break things if a peripheral driver expected the tx data to persist.

And what's the problem here? We do the same with bounce-buffers in case of DMA/IOMMU (okay, without actual data modification, but it's possible on-the-fly).

Quoted 3 lines

And we can't do this during the SPI optimize
step because that currently allows the tx_buf data values to be modified after optimization.

This I don't know, so perhaps it's indeed a showstopper.

Quoted 6 lines

So perhaps it is best to just handle it in the peripheral driver. It will be much more efficent that way anyway.

However, we still do want to handle SPI_LSB_FIRST now so that people with hardware support can be more efficient and we don't want things to break if someone puts spi-lsb-first in the devicetree.

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · David Lechner

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

Quoted 25 lines

Quoted 24 lines

Quoted 9 lines

Change I was not able to do:
- I could not remove bitrev16 because I am using an SPI controller that does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using the spi-lsb-first devicetree property for now, since this driver currently works for both types of controllers: those that support it and those that do not. I left a TODO comment to address this issue as soon as the SPI kernel code starts handling the bit-reverse operation for controllers that do not have this support. Once I finish my work on this driver, if the SPI code still does not include this handling, I can submit patches to add it.

I looked more at what it would take to implement this in the SPI core code and found that it would actually be quite difficult to do in a generic way because there are so many edge/corner/n-dim cases. We can't change tx_buf data in-place because it might be const data that is in some memory area that can't be modified. And things would get complicated if different transfers pointed to the same buffer memory addresses anyway. So we would basically have to allocate new memory for all buffers, copy all tx data to that new memory, reverse all of the tx bits, and update all the pointers in the transfer structs. Then when the message was finished, we would have to reverse all of the rx bits, copy all of the rx buffers back to the original buffers and put all the buffer pointers back the way they were. But this could write over some of the original tx data if tx_buf and rx_buf point to the same original buffer, which would break things if a peripheral driver expected the tx data to persist.

Quoted 3 lines

And what's the problem here? We do the same with bounce-buffers in case of DMA/IOMMU (okay, without actual data modification, but it's possible on-the-fly).

Actually, can this be done on a regmap level instead? We have a lot of custom regmap IO accessors, bulk accessor that does something to a data can be also implemented.

Quoted 12 lines

Quoted 3 lines

And we can't do this during the SPI optimize
step because that currently allows the tx_buf data values to be modified after optimization.

This I don't know, so perhaps it's indeed a showstopper.

Quoted 6 lines

So perhaps it is best to just handle it in the peripheral driver. It will be much more efficent that way anyway.

However, we still do want to handle SPI_LSB_FIRST now so that people with hardware support can be more efficient and we don't want things to break if someone puts spi-lsb-first in the devicetree.

Quoted 1 line · David Lechner

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

Quoted 8 lines

drivers/iio/adc/max14001.c line 311

Quoted 5 lines

drivers/iio/adc/max14001.c new file +5 -0

+	if (ret < 0)
+		ret = 1250000;
+	else
+		ext_vrefin = 1;
+	st->vref_mV = ret / (MICRO / MILLI);

Just a style choice here, but in other drivers with similar handling we wrote it like this to avoid the extra if statement:

I didn't get this. You move from clear if to not-so-clear ternary. How is the proposed code better?

Quoted 7 lines

	if (ret < 0 && ret != -ENODEV)
		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");

	ext_vrefin = ret != -ENODEV;
	st->vref_mV = ext_vrefin ? ret / 1000 : 1250;

Quoted 4 lines

Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions on this.

Or we could drop ext_vrefin and have:

It goes back and force. Can we keep the code as it's in this version?

Quoted 12 lines

	if (ret < 0 && ret != -ENODEV)
		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");

	if (ret != -ENODEV) {
		st->vref_mV = ret / 1000;

		/* regmap set bits goes here. */
		... 
	} else {
		st->vref_mV = 1250;
	}

Quoted 34 lines · Marilene Andrade Garcia

The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers more features, like a binary comparator, a filtered reading that can provide the average of the last 2, 4, or 8 ADC readings, and an inrush comparator that triggers the inrush current. There is also a fault feature that can diagnose seven possible fault conditions. And an option to select an external or internal ADC voltage reference.

MAX14001/MAX14002 features implemented so far:
- Raw ADC reading.
- Filtered ADC average reading with the default configuration.
- MV fault disable.
- Selection of external or internal ADC voltage reference, depending on whether it is declared in the device tree.

Co-developed-by Kim Seer Paller <[email protected]>

Signed-off-by Kim Seer Paller <[email protected]>

Signed-off-by Marilene Andrade Garcia <[email protected]>

---

I have addressed almost all of the comments, thank you very much for the review. I would like to highlight some of them:

Changes since v10:
- Dropped the kernel.h include
- Add the cleanup.h, mutex.h, regmap.h and units.h includes
- Renamed the reg_addr variable name to reg
- Renamed the reg_data variable name to val
- Added the regmap implementation
- Used scoped_guard()
- Refactored the get refin voltage code
- Replace max14001_chip_model with data structures separated
- Added debugfs_reg_access

Quoted 9 lines

Change I was not able to do:
- I could not remove bitrev16 because I am using an SPI controller that does not support SPI_LSB_FIRST. So I suggest keeping bitrev16 and not using the spi-lsb-first devicetree property for now, since this driver currently works for both types of controllers: those that support it and those that do not. I left a TODO comment to address this issue as soon as the SPI kernel code starts handling the bit-reverse operation for controllers that do not have this support. Once I finish my work on this driver, if the SPI code still does not include this handling, I can submit patches to add it.

I looked more at what it would take to implement this in the SPI core code and found that it would actually be quite difficult to do in a generic way because there are so many edge/corner/n-dim cases. We can't change tx_buf data in-place because it might be const data that is in some memory area that can't be modified. And things would get complicated if different transfers pointed to the same buffer memory addresses anyway. So we would basically have to allocate new memory for all buffers, copy all tx data to that new memory, reverse all of the tx bits, and update all the pointers in the transfer structs. Then when the message was finished, we would have to reverse all of the rx bits, copy all of the rx buffers back to the original buffers and put all the buffer pointers back the way they were. But this could write over some of the original tx data if tx_buf and rx_buf point to the same original buffer, which would break things if a peripheral driver expected the tx data to persist. And we can't do this during the SPI optimize step because that currently allows the tx_buf data values to be modified after optimization.

So perhaps it is best to just handle it in the peripheral driver. It will be much more efficent that way anyway.

However, we still do want to handle SPI_LSB_FIRST now so that people with hardware support can be more efficient and we don't want things to break if someone puts spi-lsb-first in the devicetree.

I made some comments below on how we could handle this.

Quoted 28 lines

  MAINTAINERS                |   1 +
  drivers/iio/adc/Kconfig    |  10 ++
  drivers/iio/adc/Makefile   |   1 +
  drivers/iio/adc/max14001.c | 356 +++++++++++++++++++++++++++++++++++++
  4 files changed, 368 insertions(+)

create mode 100644 drivers/iio/adc/max14001.c

MAINTAINERS +1 -0

index 0bae420caa63..a9cf93ba8b21 100644
@@ -14998,6 +14998,7 @@ L:	[email protected]
  S:	Maintained
  W:	https://ez.analog.com/linux-software-drivers
  F:	Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
+F:	drivers/iio/adc/max14001.c
 
  MAXBOTIX ULTRASONIC RANGER IIO DRIVER
  M:	Andreas Klinger <[email protected]>

drivers/iio/adc/Kconfig +0 -0

index 237fa2061329..a1f2afce60ad 100644
@@ -991,6 +991,16 @@ config MAX11410
  	  To compile this driver as a module, choose M here: the module will be
  	  called max11410.
 

drivers/iio/adc/Kconfig config MAX11410 · line 1003

Quoted 10 lines

drivers/iio/adc/Kconfig +10 -0

+config MAX14001
+	tristate "Analog Devices MAX14001/MAX14002 ADC driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Analog Devices MAX14001/MAX14002
+	  Configurable, Isolated 10-bit ADCs for Multi-Range Binary Inputs.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called max14001.
+

Shouldn't MAX14... be after MAX12... for alphabetical order?

drivers/iio/adc/Makefile obj-$(CONFIG_MAX11100) += max11100.o · line 90

Quoted 12 lines

drivers/iio/adc/Kconfig +0 -0

  config MAX1241
  	tristate "Maxim max1241 ADC driver"
  	depends on SPI_MASTER

drivers/iio/adc/Makefile +1 -0

index 611c16430621..9c4ceb527db7 100644
@@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
  obj-$(CONFIG_MAX1118) += max1118.o
  obj-$(CONFIG_MAX11205) += max11205.o
  obj-$(CONFIG_MAX11410) += max11410.o
+obj-$(CONFIG_MAX14001) += max14001.o

Same here.

Quoted 68 lines

drivers/iio/adc/Makefile +0 -0

  obj-$(CONFIG_MAX1241) += max1241.o
  obj-$(CONFIG_MAX1363) += max1363.o
  obj-$(CONFIG_MAX34408) += max34408.o

drivers/iio/adc/max14001.c new file +59 -0

new file mode 100644
index 000000000000..2ade57970064
@@ -0,0 +1,356 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+/*
+ * Analog Devices MAX14001/MAX14002 ADC driver
+ *
+ * Copyright (C) 2023-2025 Analog Devices Inc.
+ * Copyright (C) 2023 Kim Seer Paller <[email protected]>
+ * Copyright (c) 2025 Marilene Andrade Garcia <[email protected]>
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
+ */
+
+/*
+ * TODO:
+ * Replace bitrev16 with SPI_LSB_FIRST once the SPI kernel code supports handling
+ * SPI controllers that lack LSB-first support.
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bitrev.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+/* MAX14001 Registers Address */
+#define MAX14001_REG_ADC		0x00
+#define MAX14001_REG_FADC		0x01
+#define MAX14001_REG_FLAGS		0x02
+#define MAX14001_REG_FLTEN		0x03
+#define MAX14001_REG_THL		0x04
+#define MAX14001_REG_THU		0x05
+#define MAX14001_REG_INRR		0x06
+#define MAX14001_REG_INRT		0x07
+#define MAX14001_REG_INRP		0x08
+#define MAX14001_REG_CFG		0x09
+#define MAX14001_REG_ENBL		0x0A
+#define MAX14001_REG_ACT		0x0B
+#define MAX14001_REG_WEN		0x0C
+
+#define MAX14001_REG_VERIFICATION(x)	((x) + 0x10)
+
+#define MAX14001_REG_CFG_BIT_EXRF	BIT(5)
+
+#define MAX14001_REG_WEN_VALUE_WRITE	0x294
+
+#define MAX14001_MASK_ADDR		GENMASK(15, 11)
+#define MAX14001_MASK_WR		BIT(10)

drivers/iio/adc/max14001.c line 69

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+#define MAX14001_MASK_DATA		GENMASK(9, 0)
+
+struct max14001_state {
+	const struct max14001_chip_info *chip_info;
+	struct spi_device *spi;
+	struct regmap *regmap;
+	int vref_mV;
+	/*
+	 * lock protect against multiple concurrent accesses, RMW sequence,
+	 * and SPI transfer.

regmap has it's own locking. Currently, this is only protecteing a couple of reads. So eaither we don't need this because the lock in regmap is good enough or we have missed some places where locking would be needed.

In other words, we probably don't need this lock until later, so could drop it for now.

drivers/iio/adc/max14001.c line 71

Quoted 2 lines

drivers/iio/adc/max14001.c new file +2 -0

+	 */
+	struct mutex lock;

	bool spi_hw_has_lsb_first;

drivers/iio/adc/max14001.c line 80

Quoted 9 lines

drivers/iio/adc/max14001.c new file +9 -0

+	/*
+	 * The following buffers will be bit-reversed during device
+	 * communication, because the device transmits and receives data
+	 * LSB-first.
+	 * DMA (thus cache coherency maintenance) requires the transfer
+	 * buffers to live in their own cache lines.
+	 */
+	__be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
+	__be16 spi_rx_buffer;

These would no longer be strictly big-endian, so we could just make them u16 with a note in the comments.

Quoted 19 lines

drivers/iio/adc/max14001.c new file +18 -0

+};
+
+struct max14001_chip_info {
+	const char *name;
+};
+
+static struct max14001_chip_info max14001_chip_info = {
+	.name = "max14001",
+};
+
+static struct max14001_chip_info max14002_chip_info = {
+	.name = "max14002",
+};
+
+static int max14001_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct max14001_state *st = context;
+	struct spi_transfer xfers[] = {

drivers/iio/adc/max14001.c line 108

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+		{
+			.tx_buf = &st->spi_tx_buffer,
+			.len = sizeof(st->spi_tx_buffer),
+			.cs_change = 1,
+		}, {
+			.rx_buf = &st->spi_rx_buffer,
+			.len = sizeof(st->spi_rx_buffer),
+		},
+	};
+	int ret;

	u16 addr, data;

drivers/iio/adc/max14001.c line 115

Quoted 7 lines

drivers/iio/adc/max14001.c new file +7 -0

+
+	/*
+	 * Prepare SPI transmit buffer 16 bit-value big-endian format and
+	 * reverses bit order to align with the LSB-first input on SDI port
+	 * in order to meet the device communication requirements.
+	 */
+	st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg)));

	addr = FIELD_PREP(MAX14001_MASK_ADDR, reg);

	if (st->spi_hw_has_lsb_first)
		st->spi_tx_buffer = cpu_to_le16(addr);
	else
		st->spi_tx_buffer = cpu_to_be16(bitrev16(addr));	
	> +

drivers/iio/adc/max14001.c line 125

Quoted 9 lines

drivers/iio/adc/max14001.c new file +9 -0

+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	/*
+	 * Convert received 16-bit value from big-endian to cpu-endian format
+	 * and reverses bit order.
+	 */
+	*val = FIELD_GET(MAX14001_MASK_DATA, bitrev16(be16_to_cpu(st->spi_rx_buffer)));

	if (st->spi_hw_has_lsb_first)
		data = le16_to_cpu(st->spi_rx_buffer);
	else
		data = bitrev16(be16_to_cpu(st->spi_rx_buffer));

	val = FIELD_GET(MAX14001_MASK_DATA, data);

drivers/iio/adc/max14001.c line 131

Quoted 6 lines

drivers/iio/adc/max14001.c new file +6 -0

+
+	return 0;
+}
+
+static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
+{

	u16 addr;

drivers/iio/adc/max14001.c line 139

Quoted 8 lines

drivers/iio/adc/max14001.c new file +8 -0

+	/*
+	 * Prepare SPI transmit buffer 16 bit-value big-endian format and
+	 * reverses bit order to align with the LSB-first input on SDI port
+	 * in order to meet the device communication requirements.
+	 */
+	st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
+						 FIELD_PREP(MAX14001_MASK_WR, 1) |
+						 FIELD_PREP(MAX14001_MASK_DATA, val)));

	addr = FIELD_PREP(MAX14001_MASK_ADDR, reg) |
	       FIELD_PREP(MAX14001_MASK_WR, 1) |
	       FIELD_PREP(MAX14001_MASK_DATA, val);

	if (st->spi_hw_has_lsb_first)
		st->spi_tx_buffer = cpu_to_le16(addr);
	else
		st->spi_tx_buffer = cpu_to_be16(bitrev16(addr));	

Quoted 76 lines

drivers/iio/adc/max14001.c new file +76 -0

+
+	return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
+}
+
+static int max14001_write_single_reg(void *context, unsigned int reg, unsigned int val)
+{
+	struct max14001_state *st = context;
+	int ret;
+
+	/* Enable writing to the SPI register */
+	ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
+	if (ret)
+		return ret;
+
+	/* Writing data into SPI register */
+	ret = max14001_write(st, reg, val);
+	if (ret)
+		return ret;
+
+	/* Disable writing to the SPI register */
+	return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_write_verification_reg(struct max14001_state *st, unsigned int reg)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return max14001_write(st, MAX14001_REG_VERIFICATION(reg), val);
+}
+
+static int max14001_disable_mv_fault(struct max14001_state *st)
+{
+	unsigned int reg;
+	int ret;
+
+	/* Enable writing to the SPI registers */
+	ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
+	if (ret)
+		return ret;
+
+	/*
+	 * Reads all registers and writes the values to their appropriate
+	 * verification registers to clear the Memory Validation fault.
+	 */
+	for (reg = MAX14001_REG_FLTEN; reg <= MAX14001_REG_ENBL; reg++) {
+		ret = max14001_write_verification_reg(st, reg);
+		if (ret)
+			return ret;
+	}
+
+	/* Disable writing to the SPI registers */
+	return max14001_write(st, MAX14001_REG_WEN, 0);
+}
+
+static int max14001_debugfs_reg_access(struct iio_dev *indio_dev,
+				       unsigned int reg, unsigned int writeval,
+				       unsigned int *readval)
+{
+	struct max14001_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static int max14001_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct max14001_state *st = iio_priv(indio_dev);

drivers/iio/adc/max14001.c line 225

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		scoped_guard(mutex, &st->lock)
+			ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;

The scoped_guard() looks a bit odd with the extra indent. I would write it like this instead:

	case IIO_CHAN_INFO_RAW: {
		guard(mutex)(&st->lock);

		ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
		if (ret)
			return ret;

		return IIO_VAL_INT;
	}

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+	case IIO_CHAN_INFO_AVERAGE_RAW:
+		scoped_guard(mutex, &st->lock)
+			ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = st->vref_mV;
+		*val2 = 10;

drivers/iio/adc/max14001.c line 245

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct regmap_config max14001_regmap_config = {
+	.reg_read = max14001_read,
+	.reg_write = max14001_write_single_reg,

	.max_register = ?;

Setting max_register is useful.

	> +};

drivers/iio/adc/max14001.c line 259

Quoted 13 lines

drivers/iio/adc/max14001.c new file +13 -0

+
+static const struct iio_info max14001_info = {
+	.read_raw = max14001_read_raw,
+	.debugfs_reg_access = max14001_debugfs_reg_access,
+};
+
+static const struct iio_chan_spec max14001_channel[] = {
+	{
+		.type = IIO_VOLTAGE,
+		.indexed = 1,
+		.channel = 0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_AVERAGE_RAW) |

I think there was some discussion about this last time [1] that it would actually make sense to have the average as a separate channel.

There are a few reasons for this. If we add buffered read support later, we could only read the average in a buffered read if it is a separate channel. And the average channel should have an extra control that is either the -3 dB low pass filter point (using existing ABI) or the averaging window size (potentially new ABI).

If we don't care about ever reading the filtered and not filtered value at the same time, we could just have a single channel and only ever read from the FADC register and ignore the ADC register.

In either case, we would not need IIO_CHAN_INFO_AVERAGE_RAW.

[1]: https://lore.kernel.org/linux-iio/[email protected]/

Quoted 7 lines

drivers/iio/adc/max14001.c new file +7 -0

+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int max14001_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;

drivers/iio/adc/max14001.c line 276

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+	struct iio_dev *indio_dev;
+	struct max14001_state *st;
+	int ret, ext_vrefin = 0;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;

	st->spi_hw_has_lsb_first = spi->mode & SPI_LSB_FIRST;

Quoted 25 lines

drivers/iio/adc/max14001.c new file +25 -0

+	st->chip_info = spi_get_device_match_data(spi);
+	if (!st->chip_info)
+		return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
+
+	indio_dev->name = st->chip_info->name;
+	indio_dev->info = &max14001_info;
+	indio_dev->channels = max14001_channel;
+	indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
+
+	ret = devm_mutex_init(dev, &st->lock);
+	if (ret)
+		return ret;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
+
+	ret = devm_regulator_get_enable(dev, "vddl");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");

drivers/iio/adc/max14001.c line 311

Quoted 10 lines

drivers/iio/adc/max14001.c new file +10 -0

+
+	ret = devm_regulator_get_enable_read_voltage(dev, "refin");
+	if (ret < 0 && ret != -ENODEV)
+		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
+
+	if (ret < 0)
+		ret = 1250000;
+	else
+		ext_vrefin = 1;
+	st->vref_mV = ret / (MICRO / MILLI);

Just a style choice here, but in other drivers with similar handling we wrote it like this to avoid the extra if statement:

	if (ret < 0 && ret != -ENODEV)
		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");

	ext_vrefin = ret != -ENODEV;
	st->vref_mV = ext_vrefin ? ret / 1000 : 1250;

Keeping (MICRO / MILLI) instead of 1000 is fine too. There are varying opinions on this.

Or we could drop ext_vrefin and have:

	if (ret < 0 && ret != -ENODEV)
		return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");

	if (ret != -ENODEV) {
		st->vref_mV = ret / 1000;

		/* regmap set bits goes here. */
		...
	} else {
		st->vref_mV = 1250;
	}

drivers/iio/adc/max14001.c line 318

Quoted 7 lines

drivers/iio/adc/max14001.c new file +7 -0

+
+	if (ext_vrefin) {
+		/*
+		 * Configure the MAX14001/MAX14002 to use an external voltage reference source
+		 * by setting the bit 5 of the configuration register
+		 */
+		ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);

Can be simplifed with regmap_set_bits().

drivers/iio/adc/max14001.c line 321

Quoted 3 lines

drivers/iio/adc/max14001.c new file +3 -0

+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
+	}

These lines are getting very long. We try to wrap to 80 characters as much as we can in the IIO subsystem.

Quoted 27 lines

drivers/iio/adc/max14001.c new file +27 -0

+
+	ret = max14001_disable_mv_fault(st);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct spi_device_id max14001_id_table[] = {
+	{ "max14001", (kernel_ulong_t)&max14001_chip_info },
+	{ "max14002", (kernel_ulong_t)&max14002_chip_info },
+	{ }
+};
+
+static const struct of_device_id max14001_of_match[] = {
+	{ .compatible = "adi,max14001", .data = &max14001_chip_info },
+	{ .compatible = "adi,max14002", .data = &max14002_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max14001_of_match);
+
+static struct spi_driver max14001_driver = {
+	.driver = {
+		.name = "max14001",
+		.of_match_table = max14001_of_match,
+	},
+	.probe = max14001_probe,

drivers/iio/adc/max14001.c line 356

Quoted 8 lines

drivers/iio/adc/max14001.c new file +8 -0

+	.id_table = max14001_id_table,
+};
+module_spi_driver(max14001_driver);
+
+MODULE_AUTHOR("Kim Seer Paller <[email protected]>");
+MODULE_AUTHOR("Marilene Andrade Garcia <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
+MODULE_LICENSE("GPL");

Quoted 1 line · David Lechner

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

Quoted 10 lines

Quoted 9 lines

+    /*
+     * The following buffers will be bit-reversed during device +     * communication, because the device transmits and receives data +     * LSB-first.
+     * DMA (thus cache coherency maintenance) requires the transfer +     * buffers to live in their own cache lines.
+     */
+    __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
+    __be16 spi_rx_buffer;

Quoted 2 lines

These would no longer be strictly big-endian, so we could just make them u16 with a note in the comments.

Hello David, I have some doubts that I would like to clarify before sending v12. Since I am not able to test the case using SPI_LSB_FIRST, I noticed that you suggest saving the data as __le in this case. Just out of curiosity, if I use SPI_LSB_FIRST, would saving the data as __be lead to errors?

Quoted 13 lines

The scoped_guard() looks a bit odd with the extra indent. I would write it like this instead:

    case IIO_CHAN_INFO_RAW: {
        guard(mutex)(&st->lock);

        ret = regmap_read(st->regmap, MAX14001_REG_ADC, val); if (ret)
            return ret;

Quoted 3 lines

        return IIO_VAL_INT;
    }

Ok, thank you. But since I removed the mutex, as it was mentioned in the first comments, I should not use the guard, right? At least not for now.

Quoted 2 lines · Marilene Andrade Garcia

Quoted 1 line · David Lechner

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

In general, please trim out extra stuff like I've done here when you reply. It makes it easier to find the important parts. I hope I didn't miss any of your questions.

Quoted 10 lines

Quoted 10 lines

Quoted 9 lines

+    /*
+     * The following buffers will be bit-reversed during device +     * communication, because the device transmits and receives data +     * LSB-first.
+     * DMA (thus cache coherency maintenance) requires the transfer +     * buffers to live in their own cache lines.
+     */
+    __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
+    __be16 spi_rx_buffer;

Quoted 4 lines

Quoted 2 lines

These would no longer be strictly big-endian, so we could just make them u16 with a note in the comments.

Hello David, I have some doubts that I would like to clarify before sending v12. Since I am not able to test the case using SPI_LSB_FIRST, I noticed that you suggest saving the data as __le in this case. Just out of curiosity, if I use SPI_LSB_FIRST, would saving the data as __be lead to errors?

My thinking is that since we are sending things out 1 byte at a time, in order for the least significant bit of 16 bits to be sent first, the least significant byte has to be sent first. So will little-endian, the byte containing the least significant bit of the 16 bits will be first in memory.

__be is just a naming convention and doesn't actually cause any bytes to be swapped in memory. It just lets readers of the code know that the value stored there has to be handled carefully because it may not be cpu-endian. It would be confusing to readers to store a little-endian value in a __be16 variable, but technically, no, it would not cause any errors.

This is why I suggested to make it u16. It is still wrong but it is equally wrong in both cases. If you still want to use __be16 though, you could make a union instead.

union {
	__be16 be;
	__le16 le;
} spi_tx_buffer;
union {
	__be16 be;
	__le16 le;
} spi_rx_buffer;

Quoted 13 lines

Quoted 13 lines

The scoped_guard() looks a bit odd with the extra indent. I would write it like this instead:

    case IIO_CHAN_INFO_RAW: {
        guard(mutex)(&st->lock);

        ret = regmap_read(st->regmap, MAX14001_REG_ADC, val); if (ret)
            return ret;

Quoted 6 lines

Quoted 3 lines

        return IIO_VAL_INT;
    }

Ok, thank you. But since I removed the mutex, as it was mentioned in the first comments, I should not use the guard, right? At least not for now.

Correct. The regmap_read() has something similar internally already.