[PATCH v11 0/3] Add MAX14001/MAX14002 support

[PATCH v11 0/3] Add MAX14001/MAX14002 support

[Marilene Andrade Garcia]

Hello maintainers,

Thank you for reviewing v10, for your suggestions, and for answering
my questions.

I believe I’ve addressed most of the requested code changes. There was only
one that I haven’t fixed, I’ve included the reasons in the patch message.

I’d also like to use this cover letter to address some of the remaining
questions from previous cover letters.

Regarding regmap: thank you, David, for your response. I’ve implemented it,
and the code seems to be working fine. I tested it on the Raspberry Pi
modified kernel version rpi-6.12 with Raspberry Pi 5 hardware, using the 
MAX14001PMB evaluation board.

As for in_Y_mean_raw, the issue is that I don’t have the file
/sys/bus/iio/devices/iio:device0/in_0_mean_raw; instead, I have
/sys/bus/iio/devices/iio:device0/in_voltage0_mean_raw. I was thinking of 
adding in_voltageY_mean_raw to the documentation, so I am submitting a
patch with this change in the current patch set.

Thank you also for the explanations about the extra analog frontend
circuitry. I plan to study this further and send a dedicated patch to cover
it in the future.

Thank you Jonathan for the two possible solutions to set the number of ADC
readings used in the mean calculation. I’ll study both approaches and send
a dedicated patch to implement one of them in the next steps.

I intend to continue sending patches to implement all the features of the
MAX14001/MAX14002. Since I mostly work on weekends, I’ll be submitting
patches at a low frequency, but consistently.

Thank you for your time.
Best regards,
Marilene Andrade Garcia


Marilene Andrade Garcia (3):
  dt-bindings: iio: adc: add max14001
  iio: adc: max14001: New driver
  iio: ABI: Add voltage mean raw attribute

 Documentation/ABI/testing/sysfs-bus-iio       |   1 +
 .../bindings/iio/adc/adi,max14001.yaml        |  87 +++++
 MAINTAINERS                                   |   9 +
 drivers/iio/adc/Kconfig                       |  10 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/max14001.c                    | 356 ++++++++++++++++++
 6 files changed, 464 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
 create mode 100644 drivers/iio/adc/max14001.c


base-commit: 671b9b6d7f4fe17a174c410397e72253877ca64e
-- 
2.34.1

[PATCH v11 1/3] dt-bindings: iio: adc: add max14001

[Marilene Andrade Garcia]

Add device-tree documentation for MAX14001/MAX14002 ADCs.
The MAX14001/MAX14002 are isolated, single-channel analog-to-digital
converters with programmable voltage comparators and inrush current
control optimized for configurable binary input applications.

They share the same features, but in the MAX14001 the inrush trigger
threshold, current magnitude, and current duration are all programmable,
whereas in the MAX14002 these parameters are fixed.

Co-developed-by: Kim Seer Paller <kimseer.paller@analog.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
---

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:
- Changed the name to refin-supply.
- Added interrupt-names property.
- Added minItems in the interrupt property and shortened the descriptions.
- Added the fallback in the compatible property.

Change I was not able to do:
- Add the spi-lsb-first required property, even though I totally agree that
it needs to be used. However, the SPI controller that I am using does not
support SPI_LSB_FIRST, and this was leading to errors. Therefore, I suggest
keeping it without the property for now and using bitrev16 in the driver
code. As soon as I finish working on this driver, I intend to submit
patches to the SPI kernel code to handle bit reverse operation when the SPI
controller does not support it. Once that is integrated into the kernel, I
will update the driver code accordingly; I have left a TODO message in the
ADC driver code about it.

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

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
new file mode 100644
index 000000000000..c61119b16cf5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
@@ -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 <kimseer.paller@analog.com>
+  - Marilene Andrade Garcia <marilene.agarcia@gmail.com>
+
+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.
+
+  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.
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - const: cout
+      - const: fault
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+  - vddl-supply
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      max14001: adc@0 {
+        compatible = "adi,max14001", "adi,max14002";
+        reg = <0>;
+        spi-max-frequency = <5000000>;
+        vdd-supply = <&vdd>;
+        vddl-supply = <&vddl>;
+      };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index d53a536288ca..0bae420caa63 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14991,6 +14991,14 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/max9860.txt
 F:	sound/soc/codecs/max9860.*
 
+MAX14001/MAX14002 IIO ADC DRIVER
+M:	Kim Seer Paller <kimseer.paller@analog.com>
+M:	Marilene Andrade Garcia <marilene.agarcia@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
+
 MAXBOTIX ULTRASONIC RANGER IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org
-- 
2.34.1

[PATCH v11 2/3] iio: adc: max14001: New driver

[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 <kimseer.paller@analog.com>
Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
---

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 0bae420caa63..a9cf93ba8b21 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14998,6 +14998,7 @@ L:	linux-iio@vger.kernel.org
 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 <ak@it-klinger.de>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 237fa2061329..a1f2afce60ad 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -991,6 +991,16 @@ config MAX11410
 	  To compile this driver as a module, choose M here: the module will be
 	  called max11410.
 
+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.
+
 config MAX1241
 	tristate "Maxim max1241 ADC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 611c16430621..9c4ceb527db7 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -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
 obj-$(CONFIG_MAX1241) += max1241.o
 obj-$(CONFIG_MAX1363) += max1363.o
 obj-$(CONFIG_MAX34408) += max34408.o
diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
new file mode 100644
index 000000000000..2ade57970064
--- /dev/null
+++ b/drivers/iio/adc/max14001.c
@@ -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 <kimseer.paller@analog.com>
+ * Copyright (c) 2025 Marilene Andrade Garcia <marilene.agarcia@gmail.com>
+ *
+ * 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)
+#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.
+	 */
+	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;
+};
+
+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[] = {
+		{
+			.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;
+
+	/*
+	 * 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)));
+
+	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)));
+
+	return 0;
+}
+
+static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
+{
+	/*
+	 * 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)));
+
+	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);
+	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;
+	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;
+
+		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,
+};
+
+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) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int max14001_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	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->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");
+
+	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);
+
+	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);
+		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);
+}
+
+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 <kimseer.paller@analog.com>");
+MODULE_AUTHOR("Marilene Andrade Garcia <marilene.agarcia@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH v11 3/3] iio: ABI: Add voltage mean raw attribute

[Marilene Andrade Garcia]

Add the missing in_voltageY_mean_raw attribute for the average of voltage
measurements.

Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
---

When I use _mean_raw (IIO_CHAN_INFO_AVERAGE_RAW), I get a file called 
in_voltageY_mean_raw, so I added it to the documentation. 
Thanks.

 Documentation/ABI/testing/sysfs-bus-iio | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 89b4740dcfa1..6dd67bd4e73d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -422,6 +422,7 @@ Description:
 		Scaled humidity measurement in milli percent.
 
 What:		/sys/bus/iio/devices/iio:deviceX/in_Y_mean_raw
+What:		/sys/bus/iio/devices/iio:deviceX/in_voltageY_mean_raw
 KernelVersion:	3.5
 Contact:	linux-iio@vger.kernel.org
 Description:
-- 
2.34.1

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[Andy Shevchenko]

On Tue, Sep 16, 2025 at 1:16 AM Marilene Andrade Garcia
<marilene.agarcia@gmail.com> wrote:
>
> 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.

This version looks almost good to me, a few nit-picks below.

...

> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -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
>  obj-$(CONFIG_MAX1241) += max1241.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MAX34408) += max34408.o

Please, keep it ordered.


...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>

> +#include <linux/byteorder/generic.h>

This is wrong, should be asm/byteorder.h going after linux/* but
before linux/iio/* ones...

> +#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>

...here

asm/byteorder.h

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

...

> +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,

Lock to protect...

> +        * 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;
> +};

...

> +static struct max14001_chip_info max14001_chip_info = {
> +       .name = "max14001",
> +};
> +
> +static struct max14001_chip_info max14002_chip_info = {
> +       .name = "max14002",
> +};

These can be moved closer to their first user (ID table?).

...

> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
> +{
> +       /*
> +        * Prepare SPI transmit buffer 16 bit-value big-endian format and
> +        * reverses bit order to align with the LSB-first input on SDI port

reverse

> +        * 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));
> +}

...

> +static int max14001_probe(struct spi_device *spi)
> +{
> +       struct device *dev = &spi->dev;
> +       struct iio_dev *indio_dev;
> +       struct max14001_state *st;
> +       int ret, ext_vrefin = 0;

bool use_ext_vrefin = false;

> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       st = iio_priv(indio_dev);
> +       st->spi = spi;
> +       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");

Remove this check, it will be almost always a dead code. The developer
implementing a new device support won't be able to test the driver
anyway with properly given chip_info.

> +       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");
> +
> +       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)

if (ret == -ENODEV) ?
It would be interesting out of curiosity to see bloat-o-meter output
for the original check and my proposal. I would choose one which gives
less code, but in case of equality, I would rather go with a more
explicit (my proposal) check.


> +               ret = 1250000;
> +       else
> +               ext_vrefin = 1;

use_ext_vrefin = true;

> +       st->vref_mV = ret / (MICRO / MILLI);
> +
> +       if (ext_vrefin) {

if (use_ext_vrefin) {

> +               /*
> +                * Configure the MAX14001/MAX14002 to use an external voltage reference source
> +                * by setting the bit 5 of the configuration register

Missing period.

> +                */
> +               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);
> +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 1/3] dt-bindings: iio: adc: add max14001

[David Lechner]

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

...

> 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.

I think you made the right judgement call here. The changes are significant
enough to deserve another look. And you did the right thing by explaining why
you dropped the review tags.

> 
> 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.

> 
> 
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> new file mode 100644
> index 000000000000..c61119b16cf5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
> @@ -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 <kimseer.paller@analog.com>
> +  - Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> +
> +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.
> +
> +  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.
> +

I don't think the `|` is needed here. It is only needed when formatting
should be preserved, e.g. you have paragraphs or a list. Or when there
are certain characters like `:` in the text. But neither are the case here.

> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: cout
> +      - const: fault
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - vddl-supply
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      max14001: adc@0 {
> +        compatible = "adi,max14001", "adi,max14002";
> +        reg = <0>;
> +        spi-max-frequency = <5000000>;

I would add:

           spi-lsb-first;

to this example so that people know it is a possibility.

I don't think we need to list it in the properties though since
it is included by $ref: /schemas/spi/spi-peripheral-props.yaml#.

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

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[David Lechner]

On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
> 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 <kimseer.paller@analog.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> ---
> 
> 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.

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.

> 
>  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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0bae420caa63..a9cf93ba8b21 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14998,6 +14998,7 @@ L:	linux-iio@vger.kernel.org
>  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 <ak@it-klinger.de>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 237fa2061329..a1f2afce60ad 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -991,6 +991,16 @@ config MAX11410
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max11410.
>  
> +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?

>  config MAX1241
>  	tristate "Maxim max1241 ADC driver"
>  	depends on SPI_MASTER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 611c16430621..9c4ceb527db7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -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.

>  obj-$(CONFIG_MAX1241) += max1241.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MAX34408) += max34408.o
> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000000..2ade57970064
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c
> @@ -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 <kimseer.paller@analog.com>
> + * Copyright (c) 2025 Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> + *
> + * 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)
> +#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.

> +	 */
> +	struct mutex lock;

	bool spi_hw_has_lsb_first;

> +	/*
> +	 * 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.

> +};
> +
> +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[] = {
> +		{
> +			.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;

> +
> +	/*
> +	 * 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));	
	> +
> +	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);

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

	u16 addr;

> +	/*
> +	 * 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));	

> +
> +	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);
> +	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;
	}

> +	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;
> +
> +		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.
	> +};
> +
> +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/20250903171105.00003dcd@huawei.com/

> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +static int max14001_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	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;

> +	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");
> +
> +	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;
	}

> +
> +	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().

> +		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.

> +
> +	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 <kimseer.paller@analog.com>");
> +MODULE_AUTHOR("Marilene Andrade Garcia <marilene.agarcia@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[David Lechner]

On 9/16/25 1:04 PM, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

>> +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/20250903171105.00003dcd@huawei.com/
> 
I thought about this some more while I was eating lunch and
I think I like the second option the best. We keep things simple
with just one channel for now. And if anyone ever does need to
read both filtered and unfiltered at the same time, we can add
it, but if that never happens, then we won't have wasted time
implementing it.

Re: [PATCH v11 1/3] dt-bindings: iio: adc: add max14001

[Conor Dooley]

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

On Tue, Sep 16, 2025 at 11:40:47AM -0500, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

> > 
> > 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.

> > +  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.

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

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[Andy Shevchenko]

On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

> > 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 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).

> 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.

> 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.

...

> > +	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?

> 	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:

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

> 	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;
> 	}

...

> > +			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.

Side note: checkpatch.pl almost never complained (okay, something like 15y+
ago) on long string literals at the end of statements. For the code lines
I fully support the wrapping.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[Andy Shevchenko]

On Wed, Sep 17, 2025 at 11:10:42AM +0300, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
> > On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...

> > > 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 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.

> > 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.
> 
> > 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.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[David Lechner]

On 9/17/25 3:10 AM, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
> 

...

>>> +	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?

I can't say one is better than the other. What I suggested is just
how we've done it other similar other drivers.

> 
>> 	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:
> 
> It goes back and force. Can we keep the code as it's in this version?

Sure. Existing code is good enough for me. (And in that case, I
agree that renaming to `use_ext_vrefin` is an improvement.)

> 
>> 	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;
>> 	}
> 

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[David Lechner]

On 9/17/25 3:12 AM, Andy Shevchenko wrote:
> On Wed, Sep 17, 2025 at 11:10:42AM +0300, Andy Shevchenko wrote:
>> On Tue, Sep 16, 2025 at 01:04:41PM -0500, David Lechner wrote:
>>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
> 
> ...
> 
>>>> 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 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).

OK, maybe not as much problem as I thought. Just rather inefficient.
I might have another look. We could perhaps allocate the buffers
during the spi_optimize phase and only swap bits on each transfer to
make it as efficient as possible.

> 
> 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.
> 

Currently, if you have a peripheral that has the SPI_LSB_FIRST flag connected
to a controller that does not have that flag, the SPI core code will refuse to
make a SPI device for the peripheral. So to make anything work at all, the
core SPI code is going to have to be made aware one way or another.

>>> 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.
>>
>>> 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.
> 
> 

Re: [PATCH v11 1/3] dt-bindings: iio: adc: add max14001

[Marcelo Schmitt]

...
> 
> > > +  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.

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

Re: [PATCH v11 1/3] dt-bindings: iio: adc: add max14001

[Conor Dooley]

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

On Sat, Sep 20, 2025 at 06:44:52PM -0300, Marcelo Schmitt wrote:
> ...
> > 
> > > > +  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.
> 
> 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.

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

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

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

Re: [PATCH v11 1/3] dt-bindings: iio: adc: add max14001

[Marilene Andrade Garcia]

On 21/09/2025 18:22, Conor Dooley wrote:
> On Sat, Sep 20, 2025 at 06:44:52PM -0300, Marcelo Schmitt wrote:
>> ...
>>>
>>>>> +  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.
>>
>> 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.
> 
> I think you should mention the pin name here, like you did below.
> "asserts high on the COUT pin" or w/e.
> 
>>        - description: |
>>            fault: When fault reporting is enabled, the FAULT pin is asserted low
>>            whenever one of the monitored fault conditions occurs.
>>
>> Best regards,
>> Marcelo
Ok, I will change that for v12. Thanks.

Best Regards,
Marilene

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[Marilene Andrade Garcia]

On 16/09/2025 15:04, David Lechner wrote:
> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:
>> 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 <kimseer.paller@analog.com>
>> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
>> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> ---
>>
>> 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.
> 
> 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.
> 
>>
>>   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
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0bae420caa63..a9cf93ba8b21 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -14998,6 +14998,7 @@ L:	linux-iio@vger.kernel.org
>>   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 <ak@it-klinger.de>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 237fa2061329..a1f2afce60ad 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -991,6 +991,16 @@ config MAX11410
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called max11410.
>>   
>> +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?
> 
>>   config MAX1241
>>   	tristate "Maxim max1241 ADC driver"
>>   	depends on SPI_MASTER
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 611c16430621..9c4ceb527db7 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -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.
> 
>>   obj-$(CONFIG_MAX1241) += max1241.o
>>   obj-$(CONFIG_MAX1363) += max1363.o
>>   obj-$(CONFIG_MAX34408) += max34408.o
>> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
>> new file mode 100644
>> index 000000000000..2ade57970064
>> --- /dev/null
>> +++ b/drivers/iio/adc/max14001.c
>> @@ -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 <kimseer.paller@analog.com>
>> + * Copyright (c) 2025 Marilene Andrade Garcia <marilene.agarcia@gmail.com>
>> + *
>> + * 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)
>> +#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.
> 
>> +	 */
>> +	struct mutex lock;
> 
> 	bool spi_hw_has_lsb_first;
> 
>> +	/*
>> +	 * 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.

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?

> 
>> +};
>> +
>> +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[] = {
>> +		{
>> +			.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;
> 
>> +
>> +	/*
>> +	 * 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));	
> 	> +
>> +	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);
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
>> +{
> 
> 	u16 addr;
> 
>> +	/*
>> +	 * 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));	
> 
>> +
>> +	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);
>> +	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;
> 	}
> 

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.

>> +	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;
>> +
>> +		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.
> 	> +};
>> +
>> +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/20250903171105.00003dcd@huawei.com/
> 
>> +				      BIT(IIO_CHAN_INFO_SCALE),
>> +	},
>> +};
>> +
>> +static int max14001_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	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;
> 
>> +	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");
>> +
>> +	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;
> 	}
> 
>> +
>> +	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().
> 
>> +		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.
> 
>> +
>> +	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 <kimseer.paller@analog.com>");
>> +MODULE_AUTHOR("Marilene Andrade Garcia <marilene.agarcia@gmail.com>");
>> +MODULE_DESCRIPTION("Analog Devices MAX14001/MAX14002 ADCs driver");
>> +MODULE_LICENSE("GPL");
> 

Best Regards,
Marilene

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[David Lechner]

On 9/22/25 7:56 PM, Marilene Andrade Garcia wrote:
> On 16/09/2025 15:04, David Lechner wrote:
>> 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.

>>> +    /*
>>> +     * 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.
> 
> 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;

>>
>> 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;
>>     }
>>
> 
> 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.

Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[Marilene Andrade Garcia]

On 23/09/2025 11:27, David Lechner wrote:
> On 9/22/25 7:56 PM, Marilene Andrade Garcia wrote:
>> On 16/09/2025 15:04, David Lechner wrote:
>>> 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.
> 
>>>> +    /*
>>>> +     * 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.
>>
>> 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;
> 
>>>
>>> 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;
>>>      }
>>>
>>
>> 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.
> 
Ok, Thank you for the answers.

Best Regards,
Marilene